[llvm] [CaptureTracking][NFC] Clarify usage expectations in PointerMayBeCaptured comments (PR #132744)
via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 14 05:48:01 PDT 2025
https://github.com/Camsyn updated https://github.com/llvm/llvm-project/pull/132744
>From 87c0937b9cc29c312a379a72c07aea84a8dcbdf0 Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Mon, 24 Mar 2025 15:07:04 +0800
Subject: [PATCH 1/6] [CaptureTracking] Supports analysis for dervied pointers
Fixes issue #132739.
This fixes a missed capture detection scenario where only the base
pointer is captured.
Previously, PointerMayBeCaptured only performed forward def-use
analysis on the given pointer, which worked for base pointers
(Arguments/Allocas/Calls) but failed to detect captures of derived
pointers when their base pointers were captured.
The fix: For the input pointer, first trace back to its base pointer
using `getUnderlyingObject`, then perform capture analysis on the base.
This ensures proper handling of derived pointers with bases captured.
Performance considerations:
- Most existing callers already pass base pointers (the common case), so
the added backtracing has negligible overhead
- The few callers (ThreadSanitizer.cpp/SanitizerBinaryMetadata.cpp)
passing arbitrary pointers are sanitizer-related components where
compilation time is less critical compared to runtime overhead
This addresses false negatives in capture detection while maintaining
reasonable compilation efficiency.
---
llvm/lib/Analysis/CaptureTracking.cpp | 4 +++
.../Analysis/CaptureTrackingTest.cpp | 28 +++++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/llvm/lib/Analysis/CaptureTracking.cpp b/llvm/lib/Analysis/CaptureTracking.cpp
index aa53a27537567..0d598cd245464 100644
--- a/llvm/lib/Analysis/CaptureTracking.cpp
+++ b/llvm/lib/Analysis/CaptureTracking.cpp
@@ -424,6 +424,10 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker,
if (MaxUsesToExplore == 0)
MaxUsesToExplore = DefaultMaxUsesToExplore;
+ // When analyzing a derived pointer, we need to analyze its underlying
+ // object to determine whether it is captured.
+ // E.g., `ptr + 1` is captured if `ptr` is captured.
+ V = getUnderlyingObjectAggressive(V);
SmallVector<const Use *, 20> Worklist;
Worklist.reserve(getDefaultMaxUsesToExploreForCaptureTracking());
SmallSet<const Use *, 20> Visited;
diff --git a/llvm/unittests/Analysis/CaptureTrackingTest.cpp b/llvm/unittests/Analysis/CaptureTrackingTest.cpp
index ea3f21efc014c..f35fc0a151470 100644
--- a/llvm/unittests/Analysis/CaptureTrackingTest.cpp
+++ b/llvm/unittests/Analysis/CaptureTrackingTest.cpp
@@ -132,3 +132,31 @@ TEST(CaptureTracking, MultipleUsesInSameInstruction) {
EXPECT_EQ(ICmp, CT.Captures[6]->getUser());
EXPECT_EQ(1u, CT.Captures[6]->getOperandNo());
}
+
+TEST(CaptureTracking, DerivedPointerIfBasePointerCaptured) {
+ StringRef Assembly = R"(
+ declare void @bar(ptr)
+
+ define void @test() {
+ %stkobj = alloca [2 x i32]
+ %derived = getelementptr inbounds [2 x i32], ptr %stkobj, i64 0, i64 1
+ store i32 1, ptr %derived
+ call void @bar(ptr %stkobj)
+ ret void
+ }
+ )";
+
+ LLVMContext Context;
+ SMDiagnostic Error;
+ auto M = parseAssemblyString(Assembly, Error, Context);
+ ASSERT_TRUE(M) << "Bad assembly?";
+
+ Function *F = M->getFunction("test");
+ BasicBlock *BB = &F->getEntryBlock();
+ Instruction *StackObj = &*BB->begin();
+ Instruction *DerviedPtr = StackObj->getNextNode();
+
+ // The base object and its derived pointer are both captured.
+ EXPECT_TRUE(PointerMayBeCaptured(StackObj, true));
+ EXPECT_TRUE(PointerMayBeCaptured(DerviedPtr, true));
+}
>From cd64fa32a925de801c77864e71f49796f5193c87 Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Tue, 25 Mar 2025 14:31:00 +0800
Subject: [PATCH 2/6] Tackle some format issues adhering the suggestions of the
reviewer
---
llvm/unittests/Analysis/CaptureTrackingTest.cpp | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/llvm/unittests/Analysis/CaptureTrackingTest.cpp b/llvm/unittests/Analysis/CaptureTrackingTest.cpp
index f35fc0a151470..d5e3ebb454b2b 100644
--- a/llvm/unittests/Analysis/CaptureTrackingTest.cpp
+++ b/llvm/unittests/Analysis/CaptureTrackingTest.cpp
@@ -138,10 +138,10 @@ TEST(CaptureTracking, DerivedPointerIfBasePointerCaptured) {
declare void @bar(ptr)
define void @test() {
- %stkobj = alloca [2 x i32]
- %derived = getelementptr inbounds [2 x i32], ptr %stkobj, i64 0, i64 1
+ %stack_obj = alloca [2 x i32]
+ %derived = getelementptr inbounds [2 x i32], ptr %stack_obj, i64 0, i64 1
store i32 1, ptr %derived
- call void @bar(ptr %stkobj)
+ call void @bar(ptr %stack_obj)
ret void
}
)";
@@ -154,9 +154,9 @@ TEST(CaptureTracking, DerivedPointerIfBasePointerCaptured) {
Function *F = M->getFunction("test");
BasicBlock *BB = &F->getEntryBlock();
Instruction *StackObj = &*BB->begin();
- Instruction *DerviedPtr = StackObj->getNextNode();
+ Instruction *DerivedPtr = StackObj->getNextNode();
// The base object and its derived pointer are both captured.
EXPECT_TRUE(PointerMayBeCaptured(StackObj, true));
- EXPECT_TRUE(PointerMayBeCaptured(DerviedPtr, true));
+ EXPECT_TRUE(PointerMayBeCaptured(DerivedPtr, true));
}
>From 5ba4919202d740e0d108ac82aedd0891e2f0cf7c Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Sat, 12 Apr 2025 16:10:18 +0800
Subject: [PATCH 3/6] Revert "Tackle some format issues adhering the
suggestions of the reviewer"
This reverts commit cd64fa32a925de801c77864e71f49796f5193c87.
---
llvm/unittests/Analysis/CaptureTrackingTest.cpp | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/llvm/unittests/Analysis/CaptureTrackingTest.cpp b/llvm/unittests/Analysis/CaptureTrackingTest.cpp
index d5e3ebb454b2b..f35fc0a151470 100644
--- a/llvm/unittests/Analysis/CaptureTrackingTest.cpp
+++ b/llvm/unittests/Analysis/CaptureTrackingTest.cpp
@@ -138,10 +138,10 @@ TEST(CaptureTracking, DerivedPointerIfBasePointerCaptured) {
declare void @bar(ptr)
define void @test() {
- %stack_obj = alloca [2 x i32]
- %derived = getelementptr inbounds [2 x i32], ptr %stack_obj, i64 0, i64 1
+ %stkobj = alloca [2 x i32]
+ %derived = getelementptr inbounds [2 x i32], ptr %stkobj, i64 0, i64 1
store i32 1, ptr %derived
- call void @bar(ptr %stack_obj)
+ call void @bar(ptr %stkobj)
ret void
}
)";
@@ -154,9 +154,9 @@ TEST(CaptureTracking, DerivedPointerIfBasePointerCaptured) {
Function *F = M->getFunction("test");
BasicBlock *BB = &F->getEntryBlock();
Instruction *StackObj = &*BB->begin();
- Instruction *DerivedPtr = StackObj->getNextNode();
+ Instruction *DerviedPtr = StackObj->getNextNode();
// The base object and its derived pointer are both captured.
EXPECT_TRUE(PointerMayBeCaptured(StackObj, true));
- EXPECT_TRUE(PointerMayBeCaptured(DerivedPtr, true));
+ EXPECT_TRUE(PointerMayBeCaptured(DerviedPtr, true));
}
>From 087e9e4539b37fa77bca8e801dff7503d75365b8 Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Sat, 12 Apr 2025 16:10:37 +0800
Subject: [PATCH 4/6] Revert "[CaptureTracking] Supports analysis for dervied
pointers"
This reverts commit 87c0937b9cc29c312a379a72c07aea84a8dcbdf0.
---
llvm/lib/Analysis/CaptureTracking.cpp | 4 ---
.../Analysis/CaptureTrackingTest.cpp | 28 -------------------
2 files changed, 32 deletions(-)
diff --git a/llvm/lib/Analysis/CaptureTracking.cpp b/llvm/lib/Analysis/CaptureTracking.cpp
index 0d598cd245464..aa53a27537567 100644
--- a/llvm/lib/Analysis/CaptureTracking.cpp
+++ b/llvm/lib/Analysis/CaptureTracking.cpp
@@ -424,10 +424,6 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker,
if (MaxUsesToExplore == 0)
MaxUsesToExplore = DefaultMaxUsesToExplore;
- // When analyzing a derived pointer, we need to analyze its underlying
- // object to determine whether it is captured.
- // E.g., `ptr + 1` is captured if `ptr` is captured.
- V = getUnderlyingObjectAggressive(V);
SmallVector<const Use *, 20> Worklist;
Worklist.reserve(getDefaultMaxUsesToExploreForCaptureTracking());
SmallSet<const Use *, 20> Visited;
diff --git a/llvm/unittests/Analysis/CaptureTrackingTest.cpp b/llvm/unittests/Analysis/CaptureTrackingTest.cpp
index f35fc0a151470..ea3f21efc014c 100644
--- a/llvm/unittests/Analysis/CaptureTrackingTest.cpp
+++ b/llvm/unittests/Analysis/CaptureTrackingTest.cpp
@@ -132,31 +132,3 @@ TEST(CaptureTracking, MultipleUsesInSameInstruction) {
EXPECT_EQ(ICmp, CT.Captures[6]->getUser());
EXPECT_EQ(1u, CT.Captures[6]->getOperandNo());
}
-
-TEST(CaptureTracking, DerivedPointerIfBasePointerCaptured) {
- StringRef Assembly = R"(
- declare void @bar(ptr)
-
- define void @test() {
- %stkobj = alloca [2 x i32]
- %derived = getelementptr inbounds [2 x i32], ptr %stkobj, i64 0, i64 1
- store i32 1, ptr %derived
- call void @bar(ptr %stkobj)
- ret void
- }
- )";
-
- LLVMContext Context;
- SMDiagnostic Error;
- auto M = parseAssemblyString(Assembly, Error, Context);
- ASSERT_TRUE(M) << "Bad assembly?";
-
- Function *F = M->getFunction("test");
- BasicBlock *BB = &F->getEntryBlock();
- Instruction *StackObj = &*BB->begin();
- Instruction *DerviedPtr = StackObj->getNextNode();
-
- // The base object and its derived pointer are both captured.
- EXPECT_TRUE(PointerMayBeCaptured(StackObj, true));
- EXPECT_TRUE(PointerMayBeCaptured(DerviedPtr, true));
-}
>From 3959e067f3aa2fc1ceab8aec44aede62fb3900ad Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Sat, 12 Apr 2025 16:14:43 +0800
Subject: [PATCH 5/6] [CaptureTracking][NFC] Clarify usage expectations in
PointerMayBeCaptured comments
The comments for PointerMayBeCaptured and related APIs have been
updated to explicitly state that these functions assume the input is an
function-local object. It is the caller's responsibility to ensure this
precondition is met. This clarification aligns with recent discussions
and feedback from code reviews, aiming to prevent misuse and potential
analysis errors.
---
llvm/include/llvm/Analysis/CaptureTracking.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/llvm/include/llvm/Analysis/CaptureTracking.h b/llvm/include/llvm/Analysis/CaptureTracking.h
index 8c54851571cff..a15142ccfaae2 100644
--- a/llvm/include/llvm/Analysis/CaptureTracking.h
+++ b/llvm/include/llvm/Analysis/CaptureTracking.h
@@ -41,6 +41,8 @@ namespace llvm {
/// MaxUsesToExplore specifies how many uses the analysis should explore for
/// one value before giving up due too "too many uses". If MaxUsesToExplore
/// is zero, a default value is assumed.
+ /// This assumes the pointer is to a function-local object. The caller is
+ /// responsible for ensuring this.
bool PointerMayBeCaptured(const Value *V, bool ReturnCaptures,
unsigned MaxUsesToExplore = 0);
@@ -48,6 +50,8 @@ namespace llvm {
/// components that are part of \p Mask. Once \p StopFn on the accumulated
/// components returns true, the traversal is aborted early. By default, this
/// happens when *any* of the components in \p Mask are captured.
+ /// This assumes the pointer is to a function-local object. The caller is
+ /// responsible for ensuring this.
CaptureComponents PointerMayBeCaptured(
const Value *V, bool ReturnCaptures, CaptureComponents Mask,
function_ref<bool(CaptureComponents)> StopFn = capturesAnything,
@@ -64,6 +68,8 @@ namespace llvm {
/// MaxUsesToExplore specifies how many uses the analysis should explore for
/// one value before giving up due too "too many uses". If MaxUsesToExplore
/// is zero, a default value is assumed.
+ /// This assumes the pointer is to a function-local object. The caller is
+ /// responsible for ensuring this.
bool PointerMayBeCapturedBefore(const Value *V, bool ReturnCaptures,
const Instruction *I, const DominatorTree *DT,
bool IncludeI = false,
@@ -184,6 +190,8 @@ namespace llvm {
/// MaxUsesToExplore specifies how many uses the analysis should explore for
/// one value before giving up due too "too many uses". If MaxUsesToExplore
/// is zero, a default value is assumed.
+ /// This assumes the pointer is to a function-local object. The caller is
+ /// responsible for ensuring this.
void PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker,
unsigned MaxUsesToExplore = 0);
>From 1d8292b4c2baae4233272d2d0b89ce250885938a Mon Sep 17 00:00:00 2001
From: Camsyn <camsyn at foxmail.com>
Date: Mon, 14 Apr 2025 20:47:40 +0800
Subject: [PATCH 6/6] Refine the comment aadhering to the reviewer
---
llvm/include/llvm/Analysis/CaptureTracking.h | 23 +++++++++++++-------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/llvm/include/llvm/Analysis/CaptureTracking.h b/llvm/include/llvm/Analysis/CaptureTracking.h
index a15142ccfaae2..bd8d2bb568876 100644
--- a/llvm/include/llvm/Analysis/CaptureTracking.h
+++ b/llvm/include/llvm/Analysis/CaptureTracking.h
@@ -41,8 +41,9 @@ namespace llvm {
/// MaxUsesToExplore specifies how many uses the analysis should explore for
/// one value before giving up due too "too many uses". If MaxUsesToExplore
/// is zero, a default value is assumed.
- /// This assumes the pointer is to a function-local object. The caller is
- /// responsible for ensuring this.
+ /// This function only considers captures of the passed value via its def-use
+ /// chain, without considering captures of values it may be based on, or
+ /// implicit captures such as for external globals.
bool PointerMayBeCaptured(const Value *V, bool ReturnCaptures,
unsigned MaxUsesToExplore = 0);
@@ -50,8 +51,9 @@ namespace llvm {
/// components that are part of \p Mask. Once \p StopFn on the accumulated
/// components returns true, the traversal is aborted early. By default, this
/// happens when *any* of the components in \p Mask are captured.
- /// This assumes the pointer is to a function-local object. The caller is
- /// responsible for ensuring this.
+ /// This function only considers captures of the passed value via its def-use
+ /// chain, without considering captures of values it may be based on, or
+ /// implicit captures such as for external globals.
CaptureComponents PointerMayBeCaptured(
const Value *V, bool ReturnCaptures, CaptureComponents Mask,
function_ref<bool(CaptureComponents)> StopFn = capturesAnything,
@@ -68,8 +70,9 @@ namespace llvm {
/// MaxUsesToExplore specifies how many uses the analysis should explore for
/// one value before giving up due too "too many uses". If MaxUsesToExplore
/// is zero, a default value is assumed.
- /// This assumes the pointer is to a function-local object. The caller is
- /// responsible for ensuring this.
+ /// This function only considers captures of the passed value via its def-use
+ /// chain, without considering captures of values it may be based on, or
+ /// implicit captures such as for external globals.
bool PointerMayBeCapturedBefore(const Value *V, bool ReturnCaptures,
const Instruction *I, const DominatorTree *DT,
bool IncludeI = false,
@@ -81,6 +84,9 @@ namespace llvm {
/// on the accumulated components returns true, the traversal is aborted
/// early. By default, this happens when *any* of the components in \p Mask
/// are captured.
+ /// This function only considers captures of the passed value via its def-use
+ /// chain, without considering captures of values it may be based on, or
+ /// implicit captures such as for external globals.
CaptureComponents PointerMayBeCapturedBefore(
const Value *V, bool ReturnCaptures, const Instruction *I,
const DominatorTree *DT, bool IncludeI, CaptureComponents Mask,
@@ -190,8 +196,9 @@ namespace llvm {
/// MaxUsesToExplore specifies how many uses the analysis should explore for
/// one value before giving up due too "too many uses". If MaxUsesToExplore
/// is zero, a default value is assumed.
- /// This assumes the pointer is to a function-local object. The caller is
- /// responsible for ensuring this.
+ /// This function only considers captures of the passed value via its def-use
+ /// chain, without considering captures of values it may be based on, or
+ /// implicit captures such as for external globals.
void PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker,
unsigned MaxUsesToExplore = 0);
More information about the llvm-commits
mailing list