[rtsan][test] Prevent test check from being optimized out in LTO builds

[ilovepi]

In LTO builds, some test checks can be optimized away, since the compiler can
see through the memory accesses after inlining across TUs. This causes
the existing death tests to fail, since the functions are completely
optimized out and things like copying a lambda will no longer occur and
trigger the sanitizer.

To prevent that, we can use an empty inline assembly block to tell the
compiler that memory is modified, and prevent it from doing that.

[llvmbot]

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

Author: Paul Kirth (ilovepi)

Changes

In LTO builds, some test checks can be optimized away, since the compiler can
see through the memory accesses after inlining across TUs. This causes
the existing death tests to fail, since the functions are completely
optimized out and things like copying a lambda will no longer occur and
trigger the sanitizer.

To prevent that, we can use an empty inline assembly block to tell the
compiler that memory is modified, and prevent it from doing that.

---

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

1 Files Affected:

• (modified) compiler-rt/lib/rtsan/tests/rtsan_test_functional.cpp (+12-1)

diff --git a/compiler-rt/lib/rtsan/tests/rtsan_test_functional.cpp b/compiler-rt/lib/rtsan/tests/rtsan_test_functional.cpp
index ef11b71f167e1b..40c4f3b129129d 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_functional.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_functional.cpp
@@ -145,13 +145,24 @@ TEST(TestRtsan, LaunchingAThreadDiesWhenRealtime) {
 
 namespace {
 void InvokeStdFunction(std::function<void()> &&function) { function(); }
+
+template <typename T>
+void HideMemoryFromCompiler(T* memory) {
+  // Pass the pointer to an empty assembly block as an input, and inform
+  // the compiler that memory is read to and possibly modified. This should not
+  // be architecture specific, since the asm block is empty.
+  __asm__ __volatile__("" ::"r"(memory) : "memory");
+}
 } // namespace
 
 TEST(TestRtsan, CopyingALambdaWithLargeCaptureDiesWhenRealtime) {
   std::array<float, 16> lots_of_data;
   auto LargeLambda = [lots_of_data]() mutable {
-    // Stop everything getting optimised out
     lots_of_data[3] = 0.25f;
+    // In LTO builds, this lambda can be optimized away, since the compiler can
+    // see through the memory accesses after inlining across TUs. Ensure it can
+    // no longer reason about the memory access, so that won't happen.
+    HideMemoryFromCompiler(&lots_of_data[3]);
     EXPECT_EQ(16u, lots_of_data.size());
     EXPECT_EQ(0.25f, lots_of_data[3]);
   };

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ilovepi]

For context this is required to land #121819, since the copy large lambda test will fail otherwise, due to being completely optimized out when building the Runtimes w/ LTO enabled.

[ilovepi]

@cjappl I guess there's a question here about what's being sanitized: the source program vs the optimized binary. So of the goal of RTsan is to find source level issues, then perhaps it should prevent inlining or other types of optimizations on instrumented functions. If we only care about what makes it into the binary, then the current approach is probably fine, but these and other tests may need to use tricks like this more liberally. We should also probably consider adding a buildbot config that tests LTO'd runtimes once the libunwind CMake patches land.

[cjappl]

@cjappl I guess there's a question here about what's being sanitized: the source program vs the optimized binary. So of the goal of RTsan is to find source level issues, then perhaps it should prevent inlining or other types of optimizations on instrumented functions. If we only care about what makes it into the binary, then the current approach is probably fine, but these and other tests may need to use tricks like this more liberally. We should also probably consider adding a buildbot config that tests LTO'd runtimes once the libunwind CMake patches land.

CC @davidtrevelyan to make sure I'm correctly speaking for us.

IMO we are sanitizing the optimized binary. If a malloc exists in source, but then is optimized out, that binary should be safe to run and should not crash. The sister approach (performance constraints attributes) shines on the source side of things, so I think we should lean in to our strength of testing the final product.

[cjappl]

This code LGTM - I would wait until you hear confirmation from @davidtrevelyan that he agrees with my previous comment.

I'll ping him personally as well to ensure you're not blocked for long

[ilovepi]

@cjappl I guess there's a question here about what's being sanitized: the source program vs the optimized binary. So of the goal of RTsan is to find source level issues, then perhaps it should prevent inlining or other types of optimizations on instrumented functions. If we only care about what makes it into the binary, then the current approach is probably fine, but these and other tests may need to use tricks like this more liberally. We should also probably consider adding a buildbot config that tests LTO'd runtimes once the libunwind CMake patches land.

CC @davidtrevelyan to make sure I'm correctly speaking for us.

IMO we are sanitizing the optimized binary. If a malloc exists in source, but then is optimized out, that binary should be safe to run and should not crash. The sister approach (performance constraints attributes) shines on the source side of things, so I think we should lean in to our strength of testing the final product.

SGTM. I just wanted to make sure we're not going to make things harder for ourselves later, or silently start missing things we should be catching.

[cjappl]

I think this is good to merge when you need to!

[davidtrevelyan]

Thanks for the patch @ilovepi - and thanks @cjappl for the review, which sounds good to me too. I agree that we should be sanitizing the optimized binary, not the source code. I'm happy for this to be merged as soon as you're ready 👍