Skip to content

Commit 59b26ab

Browse files
authored
[TSan, SanitizerBinaryMetadata] Analyze the capture status for alloca rather than arbitrary Addr (#132756)
This PR is based on my last PR #132752 (the first commit of this PR), 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: ```cpp 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(AI, true)) ... ```
1 parent be04497 commit 59b26ab

File tree

4 files changed

+39
-2
lines changed

4 files changed

+39
-2
lines changed

compiler-rt/test/tsan/stack_race3.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
2+
#include "test.h"
3+
4+
void *Thread(void *a) {
5+
barrier_wait(&barrier);
6+
((int *)a)[1] = 43;
7+
return 0;
8+
}
9+
10+
int main() {
11+
barrier_init(&barrier, 2);
12+
int Arr[2] = {41, 42};
13+
pthread_t t;
14+
pthread_create(&t, 0, Thread, &Arr[0]);
15+
Arr[1] = 43;
16+
barrier_wait(&barrier);
17+
pthread_join(t, 0);
18+
}
19+
20+
// CHECK: WARNING: ThreadSanitizer: data race
21+
// CHECK: Location is stack of main thread.

llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -393,7 +393,7 @@ bool maybeSharedMutable(const Value *Addr) {
393393
return true;
394394

395395
const AllocaInst *AI = findAllocaForValue(Addr);
396-
if (AI && !PointerMayBeCaptured(Addr, /*ReturnCaptures=*/true))
396+
if (AI && !PointerMayBeCaptured(AI, /*ReturnCaptures=*/true))
397397
return false; // Object is on stack but does not escape.
398398

399399
Addr = Addr->stripInBoundsOffsets();

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -449,7 +449,8 @@ void ThreadSanitizer::chooseInstructionsToInstrument(
449449
}
450450

451451
const AllocaInst *AI = findAllocaForValue(Addr);
452-
if (AI && !PointerMayBeCaptured(Addr, /*ReturnCaptures=*/true)) {
452+
// Instead of Addr, we should check whether its base pointer is captured.
453+
if (AI && !PointerMayBeCaptured(AI, /*ReturnCaptures=*/true)) {
453454
// The variable is addressable but not captured, so it cannot be
454455
// referenced from a different thread and participate in a data race
455456
// (see llvm/Analysis/CaptureTracking.h for details).

llvm/test/Instrumentation/ThreadSanitizer/capture.ll

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,21 @@ entry:
4747
; CHECK: __tsan_write
4848
; CHECK: ret void
4949

50+
define void @captured3() nounwind uwtable sanitize_thread {
51+
entry:
52+
%stkobj = alloca [2 x i32], align 8
53+
; escapes due to store into global
54+
store ptr %stkobj, ptr @sink, align 8
55+
; derived is captured as its base object is captured
56+
%derived = getelementptr inbounds i32, ptr %stkobj, i64 1
57+
store i32 42, ptr %derived, align 4
58+
ret void
59+
}
60+
; CHECK-LABEL: define void @captured3
61+
; CHECK: __tsan_write
62+
; CHECK: __tsan_write
63+
; CHECK: ret void
64+
5065
define void @notcaptured0() nounwind uwtable sanitize_thread {
5166
entry:
5267
%ptr = alloca i32, align 4

0 commit comments

Comments
 (0)