-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[llvm][rtsan] Add transform pass for sanitize_realtime_unsafe #109543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ddca9d5
144c637
8cc5872
e3302a4
1a7b7e3
47754d4
0e656bb
384d92f
bc0f8ed
b4db726
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,49 +15,77 @@ | |
|
||
#include "llvm/IR/Analysis.h" | ||
#include "llvm/IR/IRBuilder.h" | ||
#include "llvm/IR/InstIterator.h" | ||
#include "llvm/IR/Module.h" | ||
|
||
#include "llvm/Demangle/Demangle.h" | ||
#include "llvm/Transforms/Instrumentation/RealtimeSanitizer.h" | ||
|
||
using namespace llvm; | ||
|
||
static SmallVector<Type *> getArgTypes(ArrayRef<Value *> FunctionArgs) { | ||
SmallVector<Type *> Types; | ||
for (Value *Arg : FunctionArgs) | ||
Types.push_back(Arg->getType()); | ||
return Types; | ||
} | ||
|
||
static void insertCallBeforeInstruction(Function &Fn, Instruction &Instruction, | ||
const char *FunctionName) { | ||
const char *FunctionName, | ||
ArrayRef<Value *> FunctionArgs) { | ||
LLVMContext &Context = Fn.getContext(); | ||
FunctionType *FuncType = FunctionType::get(Type::getVoidTy(Context), false); | ||
FunctionType *FuncType = FunctionType::get(Type::getVoidTy(Context), | ||
getArgTypes(FunctionArgs), false); | ||
FunctionCallee Func = | ||
Fn.getParent()->getOrInsertFunction(FunctionName, FuncType); | ||
IRBuilder<> Builder{&Instruction}; | ||
Builder.CreateCall(Func, {}); | ||
Builder.CreateCall(Func, FunctionArgs); | ||
} | ||
|
||
static void insertCallAtFunctionEntryPoint(Function &Fn, | ||
const char *InsertFnName) { | ||
|
||
insertCallBeforeInstruction(Fn, Fn.front().front(), InsertFnName); | ||
const char *InsertFnName, | ||
ArrayRef<Value *> FunctionArgs) { | ||
insertCallBeforeInstruction(Fn, Fn.front().front(), InsertFnName, | ||
FunctionArgs); | ||
} | ||
|
||
static void insertCallAtAllFunctionExitPoints(Function &Fn, | ||
const char *InsertFnName) { | ||
for (auto &BB : Fn) | ||
for (auto &I : BB) | ||
if (isa<ReturnInst>(&I)) | ||
insertCallBeforeInstruction(Fn, I, InsertFnName); | ||
const char *InsertFnName, | ||
ArrayRef<Value *> FunctionArgs) { | ||
for (auto &I : instructions(Fn)) | ||
if (isa<ReturnInst>(&I)) | ||
insertCallBeforeInstruction(Fn, I, InsertFnName, FunctionArgs); | ||
} | ||
|
||
static PreservedAnalyses rtsanPreservedCFGAnalyses() { | ||
PreservedAnalyses PA; | ||
PA.preserveSet<CFGAnalyses>(); | ||
return PA; | ||
} | ||
cjappl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
static PreservedAnalyses runSanitizeRealtime(Function &Fn) { | ||
insertCallAtFunctionEntryPoint(Fn, "__rtsan_realtime_enter", {}); | ||
insertCallAtAllFunctionExitPoints(Fn, "__rtsan_realtime_exit", {}); | ||
return rtsanPreservedCFGAnalyses(); | ||
} | ||
|
||
static PreservedAnalyses runSanitizeRealtimeUnsafe(Function &Fn) { | ||
IRBuilder<> Builder(&Fn.front().front()); | ||
Value *Name = Builder.CreateGlobalString(demangle(Fn.getName())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly out of interest, why do we de-mangle here and not in the reporting? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a really great question. In all honesty I didn't originally think about the possibility of de-mangling during the report stage. After thinking about it further, I have an idea on which one will probably be less work overall, but I'd need advice on whether the distribution of work is appropriate. Here's how I see the work involved:
My very naive intuition is that demangling in the pass is preferable, but very happy to move it to the reporting if I have my priorities a bit upside down. Thanks in advance for any suggestions here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you are missing the downside of binary size, because demangled name takes up more space, for demangling in the pass. I am not sure if demangling will cause any noticable overhead over unwinding and symbolizing the stack frame. Taking a step back, why do we pass this name and not use the PC and symbolize that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for this question and prompting us to look into this in more detail. I had indeed not considered the issue of binary size, so went away and thought about it a bit harder with @cjappl. There's lots to think about here, especially in terms of how we expect the sanitizer to be used. We anticipate that the sanitizer users will only be marking a handful of functions they are worried about with the We tried the PC approach in a branch and confirmed it works, but we were sad to lose the function name for printing diagnostics in release builds, and also to have to do the extra run-time work to unwind the stack. Being able to run the sanitizer with release builds is one of our design goals - we'd like to achieve as-near-normal performance as possible, so that users can meaningfully QA their real-time systems with RealtimeSanitizer enabled. Given:
... our moderate preference is currently to keep the approach how we have it outlined in this PR. However, this is of course caveated with the appreciation that we're very new here and inexperienced with the codebase, so I'm very keen to take as much advice as possible on this. Many thanks for your help so far - keen to hear your thoughts! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Confirming that this also meant that the whole stack trace symbolization was gone, but that was okay.
I don't understand. To symbolize a PC, we don't need to unwind the stack. From the runtime CLs I saw go buy, it seems like you unwind the stack on error regardless though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes - thanks - that was indeed my intended meaning.
That's correct, my mistake. We can discount the runtime cost of unwinding the stack and just concentrate on the functionality/binary size trade-off. After some more thought, we also realised that the symbolised PC approach would make suppression of these errors difficult to implement, because we would no longer have a function name to match against a suppression list. For this reason, we're leaning more strongly towards the existing implmentation and taking the small hit on binary size. I noticed that you approved this PR last night, so I guess you're happy enough with it as it is - please let me know if you have any further concerns. Thanks again for the review 👍 |
||
insertCallAtFunctionEntryPoint(Fn, "__rtsan_notify_blocking_call", {Name}); | ||
return rtsanPreservedCFGAnalyses(); | ||
} | ||
|
||
RealtimeSanitizerPass::RealtimeSanitizerPass( | ||
const RealtimeSanitizerOptions &Options) {} | ||
|
||
PreservedAnalyses RealtimeSanitizerPass::run(Function &F, | ||
PreservedAnalyses RealtimeSanitizerPass::run(Function &Fn, | ||
AnalysisManager<Function> &AM) { | ||
if (F.hasFnAttribute(Attribute::SanitizeRealtime)) { | ||
insertCallAtFunctionEntryPoint(F, "__rtsan_realtime_enter"); | ||
insertCallAtAllFunctionExitPoints(F, "__rtsan_realtime_exit"); | ||
|
||
PreservedAnalyses PA; | ||
PA.preserveSet<CFGAnalyses>(); | ||
return PA; | ||
} | ||
if (Fn.hasFnAttribute(Attribute::SanitizeRealtime)) | ||
return runSanitizeRealtime(Fn); | ||
|
||
if (Fn.hasFnAttribute(Attribute::SanitizeRealtimeUnsafe)) | ||
return runSanitizeRealtimeUnsafe(Fn); | ||
|
||
return PreservedAnalyses::all(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals all --version 5 | ||
; RUN: opt < %s -passes=rtsan -S | FileCheck %s | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use update_test_checks.py, unless there's some reason this is not possible here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome, great tool. Thanks for showing me this script - I've updated this test in 0775459 |
||
|
||
; RealtimeSanitizer pass should create the demangled function name as a global string and, | ||
; at the function entrypoint, pass it as an argument to the rtsan notify method | ||
|
||
;. | ||
; CHECK: @[[GLOB0:[0-9]+]] = private unnamed_addr constant [20 x i8] c"blocking_function()\00", align 1 | ||
;. | ||
define void @_Z17blocking_functionv() #0 { | ||
; CHECK-LABEL: define void @_Z17blocking_functionv( | ||
; CHECK-SAME: ) #[[ATTR0:[0-9]+]] { | ||
; CHECK-NEXT: call void @__rtsan_notify_blocking_call(ptr @[[GLOB0]]) | ||
; CHECK-NEXT: ret void | ||
; | ||
ret void | ||
} | ||
|
||
define noundef i32 @main() #2 { | ||
; CHECK-LABEL: define noundef i32 @main() { | ||
; CHECK-NEXT: call void @_Z17blocking_functionv() | ||
; CHECK-NEXT: ret i32 0 | ||
; | ||
call void @_Z17blocking_functionv() #4 | ||
ret i32 0 | ||
} | ||
|
||
attributes #0 = { mustprogress noinline sanitize_realtime_unsafe optnone ssp uwtable(sync) } | ||
cjappl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
;. | ||
; CHECK: attributes #[[ATTR0]] = { mustprogress noinline optnone sanitize_realtime_unsafe ssp uwtable(sync) } | ||
;. |
Uh oh!
There was an error while loading. Please reload this page.