[rtsan] Handle attributed IR function declarations

[davidtrevelyan]

Fixes #169377.

Previously, the RealtimeSanitizer pass only handled attributed function definitions in IR, and we have recently found that attributed function declarations caused it to crash. To fix the issue, we must check whether the IR function is empty before attempting to do any manipulation of its instructions.

This PR:

• Adds checks for whether IR Functions are empty() in each relevant at the top-level RTSan pass routine
• Removes the utility function rtsanPreservedCFGAnalyses from the pass, whose result was unused and which would otherwise have complicated the fix

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (davidtrevelyan)

Changes

Addresses #169377.

Previously, the RealtimeSanitizer pass only handled attributed function definitions in IR, and attributed function declarations caused it to crash. To fix the issue, we must check whether the IR function is empty before attempting to do any manipulation of its instructions.

This PR:

• Adds checks for whether IR Functions are empty() in each relevant RTSan pass routine
• Removes the utility function rtsanPreservedCFGAnalyses from the pass, whose result was unused and which would otherwise have complicated the fix

---

Full diff: https://github.com/llvm/llvm-project/pull/169577.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/RealtimeSanitizer.cpp (+7-9)
• (added) llvm/test/Instrumentation/RealtimeSanitizer/rtsan_attrib_declare.ll (+12)

diff --git a/llvm/lib/Transforms/Instrumentation/RealtimeSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/RealtimeSanitizer.cpp
index 5ef6ffb58a7c1..adb0e518c6074 100644
--- a/llvm/lib/Transforms/Instrumentation/RealtimeSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/RealtimeSanitizer.cpp
@@ -61,23 +61,21 @@ static void insertCallAtAllFunctionExitPoints(Function &Fn,
       insertCallBeforeInstruction(Fn, I, InsertFnName, FunctionArgs);
 }
 
-static PreservedAnalyses rtsanPreservedCFGAnalyses() {
-  PreservedAnalyses PA;
-  PA.preserveSet<CFGAnalyses>();
-  return PA;
-}
+static void runSanitizeRealtime(Function &Fn) {
+  if (Fn.empty())
+    return;
 
-static PreservedAnalyses runSanitizeRealtime(Function &Fn) {
   insertCallAtFunctionEntryPoint(Fn, "__rtsan_realtime_enter", {});
   insertCallAtAllFunctionExitPoints(Fn, "__rtsan_realtime_exit", {});
-  return rtsanPreservedCFGAnalyses();
 }
 
-static PreservedAnalyses runSanitizeRealtimeBlocking(Function &Fn) {
+static void runSanitizeRealtimeBlocking(Function &Fn) {
+  if (Fn.empty())
+    return;
+
   IRBuilder<> Builder(&Fn.front().front());
   Value *Name = Builder.CreateGlobalString(demangle(Fn.getName()));
   insertCallAtFunctionEntryPoint(Fn, "__rtsan_notify_blocking_call", {Name});
-  return rtsanPreservedCFGAnalyses();
 }
 
 PreservedAnalyses RealtimeSanitizerPass::run(Module &M,
diff --git a/llvm/test/Instrumentation/RealtimeSanitizer/rtsan_attrib_declare.ll b/llvm/test/Instrumentation/RealtimeSanitizer/rtsan_attrib_declare.ll
new file mode 100644
index 0000000000000..a5336b9e9e491
--- /dev/null
+++ b/llvm/test/Instrumentation/RealtimeSanitizer/rtsan_attrib_declare.ll
@@ -0,0 +1,12 @@
+; RUN: opt < %s -passes='rtsan' -S | FileCheck %s
+
+declare void @declared_realtime_function() sanitize_realtime #0
+
+declare void @declared_blocking_function() sanitize_realtime_blocking #0
+
+; RealtimeSanitizer pass should ignore attributed functions that are just declarations
+; CHECK: declared_realtime_function
+; CHECK-EMPTY:
+; CHECK: declared_blocking_function
+; CHECK-EMPTY:
+

[cjappl]

Seems reasonable enough, just have some questions before I approve

[cjappl]

Is not returning these preserved analyses going to bite us elsewhere? I remember most of the sanitizer transform passes (or transform passes generally) returning these.

[davidtrevelyan]

Perhaps! But the issue is we're already not returning these preserved analyses - they're sadly discarded at the moment. I would advocate for this NFC because it makes the current behaviour explicit - even if it's not correct. I bundled this change in with this PR because it made the implementation of the checking a bit easier, but I'd be happy to submit this in a separate preparatory PR first - if that's easier to follow - just let me know.

[cjappl]

Is there precedence in other sanitizer passes for doing this? (Curious how they solved this problem)

[davidtrevelyan]

Thanks for the good idea. I hadn't looked before drafting this PR, but it appears like ASan solves it a similar way at one level higher - at the top level of the pass's run method:

llvm-project/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

Line 1318 in f5742c4

if (F.empty())

ThreadSanitizer has a bool check for whether it should sanitizeFunction, which returns false if there are no calls in it. MemorySanitizer does the same thing as AddressSanitizer with a continue at the top level if the function is .empty().

If you prefer the ASan/MSan semantics I'm very happy to switch to it 👍

[inkreasing]

Looks good! doesn't crash anymore and the rtsan tests i added to rustc also still pass.

[zwuis]

Addresses #169377.

You can use this format.

[cjappl]

Approved as written! I lean slightly to breaking the NFC into another commit but not strongly enough to block based on it

Thanks for investigating the other sanitizers. Looks like there is good precedence for this type of check

[davidtrevelyan]

Approved as written! I lean slightly to breaking the NFC into another commit but not strongly enough to block based on it

Thanks for investigating the other sanitizers. Looks like there is good precedence for this type of check

@cjappl agreed on breaking the NFC into another commit. I've amended this branch to only do enough to fix the bug, adding just the check for the empty function, and we'll tackle the preserved analyses in a separate PR. I think it warrants a little more thought. Thanks for the suggestion!

[cjappl]

Great! Thanks

[davidtrevelyan]

/cherry-pick 5d4c441

[llvmbot]

/pull-request #170641