From 3206effeb4a77c82255ebb7e375b3917fa01dc70 Mon Sep 17 00:00:00 2001 From: Camsyn Date: Mon, 24 Mar 2025 17:05:00 +0800 Subject: [PATCH] [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 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(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(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)