[TSan, SanitizerBinaryMetadata] Analyze the capture status for `alloca` rather than arbitrary `Addr` #132756

Camsyn · 2025-03-24T15:10:50Z

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:

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 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.

    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

    const AllocaInst *AI = findAllocaForValue(Addr);
    // Instead of Addr, we should check whether its base pointer is captured.
    if (AI && !PointerMayBeCaptured(AI, true)) ...

github-actions · 2025-03-24T15:11:10Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-03-24T15:11:46Z

@llvm/pr-subscribers-llvm-transforms

Author: None (Camsyn)

Changes

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:

void *Thread(void *a) {
  ((int*)a)[1] = 43;
  return 0;
}

int main() {
  int Arr[2] = {41, 42};
  pthread_t t;
  pthread_create(&amp;t, 0, Thread, &amp;Arr[0]);
  // Missed instrumentation here due to the FN of PointerMayBeCaptured
  Arr[1] = 43;
  barrier_wait(&amp;barrier);
  pthread_join(t, 0);
}

Refer to this godbolt page 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.

    const AllocaInst *AI = findAllocaForValue(Addr);
    // Instead of Addr, we should check whether its base pointer is captured.
    if (AI &amp;&amp; !PointerMayBeCaptured(Addr, true)) ...

Key changes:
Directly analyze the capture status of the underlying alloca instead of derived pointers to ensure accurate capture detection

    const AllocaInst *AI = findAllocaForValue(Addr);
    // Instead of Addr, we should check whether its base pointer is captured.
    if (AI &amp;&amp; !PointerMayBeCaptured(Addr, AI)) ...

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

4 Files Affected:

(added) compiler-rt/test/tsan/stack_race3.cpp (+22)
(modified) llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp (+2-2)
(modified) llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp (+3-2)
(modified) llvm/test/Instrumentation/ThreadSanitizer/capture.ll (+31)

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 c5932f0d65ee1..f22a87421319b 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(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 2b403b695c1d2..7c5fc40d51b78 100644
--- a/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
@@ -448,8 +448,9 @@ void ThreadSanitizer::chooseInstructionsToInstrument(
       }
     }
 
-    if (isa<AllocaInst>(getUnderlyingObject(Addr)) &&
-        !PointerMayBeCaptured(Addr, /*ReturnCaptures=*/true)) {
+    const AllocaInst *AI = findAllocaForValue(Addr);
+    // 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).
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)

llvmbot · 2025-03-24T15:11:48Z

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

Author: None (Camsyn)

Changes

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:

void *Thread(void *a) {
  ((int*)a)[1] = 43;
  return 0;
}

int main() {
  int Arr[2] = {41, 42};
  pthread_t t;
  pthread_create(&amp;t, 0, Thread, &amp;Arr[0]);
  // Missed instrumentation here due to the FN of PointerMayBeCaptured
  Arr[1] = 43;
  barrier_wait(&amp;barrier);
  pthread_join(t, 0);
}

Refer to this godbolt page 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.

    const AllocaInst *AI = findAllocaForValue(Addr);
    // Instead of Addr, we should check whether its base pointer is captured.
    if (AI &amp;&amp; !PointerMayBeCaptured(Addr, true)) ...

Key changes:
Directly analyze the capture status of the underlying alloca instead of derived pointers to ensure accurate capture detection

    const AllocaInst *AI = findAllocaForValue(Addr);
    // Instead of Addr, we should check whether its base pointer is captured.
    if (AI &amp;&amp; !PointerMayBeCaptured(Addr, AI)) ...

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

4 Files Affected:

(added) compiler-rt/test/tsan/stack_race3.cpp (+22)
(modified) llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp (+2-2)
(modified) llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp (+3-2)
(modified) llvm/test/Instrumentation/ThreadSanitizer/capture.ll (+31)

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 c5932f0d65ee1..f22a87421319b 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(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 2b403b695c1d2..7c5fc40d51b78 100644
--- a/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
@@ -448,8 +448,9 @@ void ThreadSanitizer::chooseInstructionsToInstrument(
       }
     }
 
-    if (isa<AllocaInst>(getUnderlyingObject(Addr)) &&
-        !PointerMayBeCaptured(Addr, /*ReturnCaptures=*/true)) {
+    const AllocaInst *AI = findAllocaForValue(Addr);
+    // 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).
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)

github-actions · 2025-03-25T02:22:57Z

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

melver

I think your description is wrong:

 const AllocaInst *AI = findAllocaForValue(Addr);
 // Instead of Addr, we should check whether its base pointer is captured.
if (AI && !PointerMayBeCaptured(Addr, AI)) ...

Shouldn't it say "if (AI && !PointerMayBeCaptured(AI, true))" ?

llvm/test/Instrumentation/ThreadSanitizer/capture.ll

Camsyn · 2025-03-25T07:57:23Z

I think your description is wrong:
 const AllocaInst *AI = findAllocaForValue(Addr);
 // Instead of Addr, we should check whether its base pointer is captured.
if (AI && !PointerMayBeCaptured(Addr, AI)) ...
Shouldn't it say "if (AI && !PointerMayBeCaptured(AI, true))" ?

Ohhh, it is a literal mistake in the PR description, but the relevant modification is correct.
Fixed.

melver · 2025-03-25T09:40:39Z

This looks good. But we should commit the other PR first, and rebase this one. I leave it to you when you want to submit the other PR or if you want to await other reviewers' feedback too.

…a` 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 llvm#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.

Camsyn · 2025-04-24T07:29:49Z

As the prerequisite PR #132752 has been merged, I rebased this PR to the main branch. I think it's okay to merge now.

Just an FYI - another NFC PR #132744 relevant to this PR has been merged, which highlights PointerMayBeCaptured usage scenarios in the comments (violated by TSan/SanitizerBinaryMetadata before this PR).

Friendly ping :) @melver

nikic

LGTM

github-actions · 2025-04-24T08:48:32Z

@Camsyn Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

…a` rather than arbitrary `Addr` (llvm#132756) This PR is based on my last PR llvm#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 llvm#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)) ... ```

llvmbot added compiler-rt compiler-rt:tsan Thread sanitizer compiler-rt:sanitizer llvm:transforms labels Mar 24, 2025

dtcxzyw requested review from nikic, melver and vitalybuka March 25, 2025 02:19

Camsyn force-pushed the fix-tsan-capture branch from 1b94efe to 08b4006 Compare March 25, 2025 06:11

melver requested changes Mar 25, 2025

View reviewed changes

llvm/test/Instrumentation/ThreadSanitizer/capture.ll Outdated Show resolved Hide resolved

melver mentioned this pull request Mar 25, 2025

[TSan, SanitizerBinaryMetadata] Improve instrument for derived pointers via phis/selects #132752

Merged

melver self-requested a review March 25, 2025 09:40

Camsyn force-pushed the fix-tsan-capture branch from 914b617 to 1a3518a Compare April 24, 2025 07:14

nikic approved these changes Apr 24, 2025

View reviewed changes

melver approved these changes Apr 24, 2025

View reviewed changes

melver merged commit 59b26ab into llvm:main Apr 24, 2025
11 checks passed

Camsyn deleted the fix-tsan-capture branch April 24, 2025 10:26

Camsyn mentioned this pull request Apr 26, 2025

Request Commit Access For Camsyn #137457

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TSan, SanitizerBinaryMetadata] Analyze the capture status for `alloca` rather than arbitrary `Addr` #132756

[TSan, SanitizerBinaryMetadata] Analyze the capture status for `alloca` rather than arbitrary `Addr` #132756

Camsyn commented Mar 24, 2025 •

edited

Loading

github-actions bot commented Mar 24, 2025

llvmbot commented Mar 24, 2025

llvmbot commented Mar 24, 2025

github-actions bot commented Mar 25, 2025 •

edited

Loading

melver left a comment

Camsyn commented Mar 25, 2025 •

edited

Loading

melver commented Mar 25, 2025

Camsyn commented Apr 24, 2025

nikic left a comment

github-actions bot commented Apr 24, 2025

[TSan, SanitizerBinaryMetadata] Analyze the capture status for alloca rather than arbitrary Addr #132756

[TSan, SanitizerBinaryMetadata] Analyze the capture status for alloca rather than arbitrary Addr #132756

Conversation

Camsyn commented Mar 24, 2025 • edited Loading

github-actions bot commented Mar 24, 2025

llvmbot commented Mar 24, 2025

llvmbot commented Mar 24, 2025

github-actions bot commented Mar 25, 2025 • edited Loading

melver left a comment

Choose a reason for hiding this comment

Camsyn commented Mar 25, 2025 • edited Loading

melver commented Mar 25, 2025

Camsyn commented Apr 24, 2025

nikic left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 24, 2025

[TSan, SanitizerBinaryMetadata] Analyze the capture status for `alloca` rather than arbitrary `Addr` #132756

[TSan, SanitizerBinaryMetadata] Analyze the capture status for `alloca` rather than arbitrary `Addr` #132756

Camsyn commented Mar 24, 2025 •

edited

Loading

github-actions bot commented Mar 25, 2025 •

edited

Loading

Camsyn commented Mar 25, 2025 •

edited

Loading