[compiler-rt] [llvm] [TSan, SanitizerBinaryMetadata] Analyze the capture status for `alloca` rather than arbitrary `Addr` (PR #132756)
via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 24 08:10:58 PDT 2025
https://github.com/Camsyn created https://github.com/llvm/llvm-project/pull/132756
This PR is based on my last PR #132752, but addressing a different issue.
This commit addresses the limitation in `PointerMayBeCaptured` analysis when dealing with derived pointers (e.g. arr+1) as described in issue #132739.
The current implementation of `PointerMayBeCaptured` may miss captures of the underlying `alloca` when analyzing derived pointers, leading to some FNs in TSan, as follows:
```c
void *Thread(void *a) {
((int*)a)[1] = 43;
return 0;
}
int main() {
int Arr[2] = {41, 42};
pthread_t t;
pthread_create(&t, 0, Thread, &Arr[0]);
// Missed instrumentation here due to the FN of PointerMayBeCaptured
Arr[1] = 43;
barrier_wait(&barrier);
pthread_join(t, 0);
}
```
Refer to this [godbolt page](https://godbolt.org/z/n67GrxdcE) to get the compilation result of TSan.
Even when `PointerMayBeCaptured` working correctly, it should backtrack to the original `alloca` firstly during analysis, causing redundancy to the outer's `findAllocaForValue`.
```cpp
const AllocaInst *AI = findAllocaForValue(Addr);
// Instead of Addr, we should check whether its base pointer is captured.
if (AI && !PointerMayBeCaptured(Addr, true)) ...
```
Key changes:
Directly analyze the capture status of the underlying `alloca` instead of derived pointers to ensure accurate capture detection
```cpp
const AllocaInst *AI = findAllocaForValue(Addr);
// Instead of Addr, we should check whether its base pointer is captured.
if (AI && !PointerMayBeCaptured(Addr, AI)) ...
```
>From 07a991ff71acba5ca97771bf37de9eb6c7b34759 Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Mon, 24 Mar 2025 17:05:00 +0800
Subject: [PATCH 1/2] [TSan, SanitizerBinaryMetadata] Improve instrument for
derived pointers via phis/selects
ThreadSanitizer.cpp and SanitizerBinaryMetadata.cpp previously used
`getUnderlyingObject` to check if pointers originate from stack objects.
However,
`getUnderlyingObject()` by default only looks through linear chains, not
selects/phis. In particular, this means that we miss cases involving
involving pointer induction variables.
This patch replaces `getUnderlyingObject` with `findAllocaForValue`
which:
1. Properly tracks through PHINodes and select operations
2. Directly identifies if a pointer comes from a `AllocaInst`
Performance impact:
- Compilation: Moderate cost increase due to wider value tracing, but...
- Runtime: Significant wins for code with pointer induction variables
derived from stack allocas, especially for loop-heavy code, as
instrumentation can now be safely omitted.
---
.../SanitizerBinaryMetadata.cpp | 4 +--
.../Instrumentation/ThreadSanitizer.cpp | 4 +--
.../ThreadSanitizer/capture.ll | 31 +++++++++++++++++++
3 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
index c5932f0d65ee1..4a7eb9bccb860 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
@@ -393,8 +393,8 @@ bool maybeSharedMutable(const Value *Addr) {
if (!Addr)
return true;
- if (isa<AllocaInst>(getUnderlyingObject(Addr)) &&
- !PointerMayBeCaptured(Addr, /*ReturnCaptures=*/true))
+ const AllocaInst *AI = findAllocaForValue(Addr);
+ if (AI && !PointerMayBeCaptured(Addr, /*ReturnCaptures=*/true))
return false; // Object is on stack but does not escape.
Addr = Addr->stripInBoundsOffsets();
diff --git a/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
index 2b403b695c1d2..baa176939e507 100644
--- a/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
@@ -448,8 +448,8 @@ void ThreadSanitizer::chooseInstructionsToInstrument(
}
}
- if (isa<AllocaInst>(getUnderlyingObject(Addr)) &&
- !PointerMayBeCaptured(Addr, /*ReturnCaptures=*/true)) {
+ const AllocaInst *AI = findAllocaForValue(Addr);
+ if (AI && !PointerMayBeCaptured(Addr, /*ReturnCaptures=*/true)) {
// The variable is addressable but not captured, so it cannot be
// referenced from a different thread and participate in a data race
// (see llvm/Analysis/CaptureTracking.h for details).
diff --git a/llvm/test/Instrumentation/ThreadSanitizer/capture.ll b/llvm/test/Instrumentation/ThreadSanitizer/capture.ll
index 8edf310df9823..9cd5d77f4753e 100644
--- a/llvm/test/Instrumentation/ThreadSanitizer/capture.ll
+++ b/llvm/test/Instrumentation/ThreadSanitizer/capture.ll
@@ -88,4 +88,35 @@ entry:
; CHECK: __tsan_write
; CHECK: ret void
+define void @notcaptured3(i1 %cond) nounwind uwtable sanitize_thread {
+entry:
+ %stkobj = alloca [2 x i32], align 8
+ %derived = getelementptr inbounds i32, ptr %stkobj, i64 1
+ %ptr = select i1 %cond, ptr %derived, ptr %stkobj
+ store i32 42, ptr %ptr, align 4
+ ret void
+}
+; CHECK-LABEL: define void @notcaptured3
+; CHECK-NOT: call void @__tsan_write4(ptr %ptr)
+; CHECK: ret void
+define void @notcaptured4() nounwind uwtable sanitize_thread {
+entry:
+ %stkobj = alloca [10 x i8], align 1
+ br label %loop
+
+exit:
+ ret void
+
+loop:
+ %count = phi i32 [ 0, %entry ], [ %addone, %loop ]
+ %derived = phi ptr [ %stkobj, %entry ], [ %ptraddone, %loop ]
+ store i32 %count, ptr %derived, align 4
+ %ptraddone = getelementptr inbounds i32, ptr %derived, i64 1
+ %addone = add nuw nsw i32 %count, 1
+ %eq10 = icmp eq i32 %addone, 10
+ br i1 %eq10, label %exit, label %loop
+}
+; CHECK-LABEL: define void @notcaptured4
+; CHECK: ret void
+; CHECK-NOT: call void @__tsan_write4(ptr %derived)
>From 1b94efe9843beaa3a4348c37e27dd2d10558e8c1 Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Mon, 24 Mar 2025 21:53:53 +0800
Subject: [PATCH 2/2] [TSan, SanitizerBinaryMetadata] Analyze the capture
status for `alloca` rather than arbitrary `Addr`
This commit addresses the limitation in `PointerMayBeCaptured` analysis
when dealing with derived pointers (e.g. arr+1) as described in
issue #132739.
The current implementation may miss captures of the underlying alloca
when analyzing derived pointers. Even when working correctly,
backtracking to the original alloca during analysis causes redundancy
to the outer's `findAllocaForValue`.
Key changes:
Directly analyze the capture status of the underlying alloca instead
of derived pointers to ensure accurate capture detection
This patch fixes some FNs of TSan, referring to the appending
testcases for more details.
---
compiler-rt/test/tsan/stack_race3.cpp | 22 +++++++++++++++++++
.../SanitizerBinaryMetadata.cpp | 2 +-
.../Instrumentation/ThreadSanitizer.cpp | 3 ++-
3 files changed, 25 insertions(+), 2 deletions(-)
create mode 100644 compiler-rt/test/tsan/stack_race3.cpp
diff --git a/compiler-rt/test/tsan/stack_race3.cpp b/compiler-rt/test/tsan/stack_race3.cpp
new file mode 100644
index 0000000000000..5078d81f3cc58
--- /dev/null
+++ b/compiler-rt/test/tsan/stack_race3.cpp
@@ -0,0 +1,22 @@
+// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
+#include "test.h"
+
+void *Thread(void *a) {
+ barrier_wait(&barrier);
+ ((int*)a)[1] = 43;
+ return 0;
+}
+
+int main() {
+ barrier_init(&barrier, 2);
+ int Arr[2] = {41, 42};
+ pthread_t t;
+ pthread_create(&t, 0, Thread, &Arr[0]);
+ Arr[1] = 43;
+ barrier_wait(&barrier);
+ pthread_join(t, 0);
+}
+
+// CHECK: WARNING: ThreadSanitizer: data race
+// CHECK: Location is stack of main thread.
+
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
index 4a7eb9bccb860..f22a87421319b 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
@@ -394,7 +394,7 @@ bool maybeSharedMutable(const Value *Addr) {
return true;
const AllocaInst *AI = findAllocaForValue(Addr);
- if (AI && !PointerMayBeCaptured(Addr, /*ReturnCaptures=*/true))
+ if (AI && !PointerMayBeCaptured(AI, /*ReturnCaptures=*/true))
return false; // Object is on stack but does not escape.
Addr = Addr->stripInBoundsOffsets();
diff --git a/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
index baa176939e507..7c5fc40d51b78 100644
--- a/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
@@ -449,7 +449,8 @@ void ThreadSanitizer::chooseInstructionsToInstrument(
}
const AllocaInst *AI = findAllocaForValue(Addr);
- if (AI && !PointerMayBeCaptured(Addr, /*ReturnCaptures=*/true)) {
+ // Instead of Addr, we should check whether its base pointer is captured.
+ if (AI && !PointerMayBeCaptured(AI, /*ReturnCaptures=*/true)) {
// The variable is addressable but not captured, so it cannot be
// referenced from a different thread and participate in a data race
// (see llvm/Analysis/CaptureTracking.h for details).
More information about the llvm-commits
mailing list