Skip to content

Commit 7e3735d

Browse files
committed
Reapply [CaptureTracking][FunctionAttrs] Add support for CaptureInfo (#125880)
Relative to the previous attempt, this adjusts isEscapeSource() to not treat calls with captures(ret: address, provenance) or similar arguments as escape sources. This addresses the miscompile reported at: #125880 (comment) The implementation uses a helper function on CallBase to make this check a bit more efficient (e.g. by skipping the byval checks) as checking attributes on all arguments if fairly expensive. ------ This extends CaptureTracking to support inferring non-trivial CaptureInfos. The focus of this patch is to only support FunctionAttrs, other users of CaptureTracking will be updated in followups. The key API changes here are: * DetermineUseCaptureKind() now returns a UseCaptureInfo where the UseCC component specifies what is captured at that Use and the ResultCC component specifies what may be captured via the return value of the User. Usually only one or the other will be used (corresponding to previous MAY_CAPTURE or PASSTHROUGH results), but both may be set for call captures. * The CaptureTracking::captures() extension point is passed this UseCaptureInfo as well and then can decide what to do with it by returning an Action, which is one of: Stop: stop traversal. ContinueIgnoringReturn: continue traversal but don't follow the instruction return value. Continue: continue traversal and follow the instruction return value if it has additional CaptureComponents. For now, this patch retains the (unsound) special logic for comparison of null with a dereferenceable pointer. I'd like to switch key code to take advantage of address/address_is_null before dropping it. This PR mainly intends to introduce necessary API changes and basic inference support, there are various possible improvements marked with TODOs.
1 parent 39ec9de commit 7e3735d

28 files changed

+539
-241
lines changed

Diff for: clang/test/CodeGen/allow-ubsan-check.c

+3-3
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ int div(int x, int y) {
8686
}
8787

8888
// CHECK-LABEL: define dso_local i32 @null(
89-
// CHECK-SAME: ptr noundef readonly [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
89+
// CHECK-SAME: ptr noundef readonly captures(address_is_null) [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
9090
// CHECK-NEXT: [[ENTRY:.*:]]
9191
// CHECK-NEXT: [[TMP0:%.*]] = icmp eq ptr [[X]], null, !nosanitize [[META2]]
9292
//
@@ -102,7 +102,7 @@ int div(int x, int y) {
102102
// CHECK-NEXT: ret i32 [[TMP2]]
103103
//
104104
// TR-LABEL: define dso_local i32 @null(
105-
// TR-SAME: ptr noundef readonly [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
105+
// TR-SAME: ptr noundef readonly captures(address_is_null) [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
106106
// TR-NEXT: [[ENTRY:.*:]]
107107
// TR-NEXT: [[TMP0:%.*]] = icmp eq ptr [[X]], null, !nosanitize [[META2]]
108108
// TR-NEXT: [[TMP1:%.*]] = tail call i1 @llvm.allow.ubsan.check(i8 29), !nosanitize [[META2]]
@@ -116,7 +116,7 @@ int div(int x, int y) {
116116
// TR-NEXT: ret i32 [[TMP2]]
117117
//
118118
// REC-LABEL: define dso_local i32 @null(
119-
// REC-SAME: ptr noundef readonly [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
119+
// REC-SAME: ptr noundef readonly captures(address_is_null) [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
120120
// REC-NEXT: [[ENTRY:.*:]]
121121
// REC-NEXT: [[TMP0:%.*]] = icmp eq ptr [[X]], null, !nosanitize [[META2]]
122122
// REC-NEXT: [[TMP1:%.*]] = tail call i1 @llvm.allow.ubsan.check(i8 29), !nosanitize [[META2]]

Diff for: clang/test/CodeGenCXX/RelativeVTablesABI/dynamic-cast.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O3 -o - -emit-llvm | FileCheck %s
55

6-
// CHECK: define{{.*}} ptr @_Z6upcastP1B(ptr noundef readnone returned %b) local_unnamed_addr
6+
// CHECK: define{{.*}} ptr @_Z6upcastP1B(ptr noundef readnone returned captures(ret: address, provenance) %b) local_unnamed_addr
77
// CHECK-NEXT: entry:
88
// CHECK-NEXT: ret ptr %b
99
// CHECK-NEXT: }
@@ -22,12 +22,12 @@
2222

2323
// CHECK: declare ptr @__dynamic_cast(ptr, ptr, ptr, i64) local_unnamed_addr
2424

25-
// CHECK: define{{.*}} ptr @_Z8selfcastP1B(ptr noundef readnone returned %b) local_unnamed_addr
25+
// CHECK: define{{.*}} ptr @_Z8selfcastP1B(ptr noundef readnone returned captures(ret: address, provenance) %b) local_unnamed_addr
2626
// CHECK-NEXT: entry
2727
// CHECK-NEXT: ret ptr %b
2828
// CHECK-NEXT: }
2929

30-
// CHECK: define{{.*}} ptr @_Z9void_castP1B(ptr noundef readonly %b) local_unnamed_addr
30+
// CHECK: define{{.*}} ptr @_Z9void_castP1B(ptr noundef readonly captures(address_is_null, ret: address, provenance) %b) local_unnamed_addr
3131
// CHECK-NEXT: entry:
3232
// CHECK-NEXT: [[isnull:%[0-9]+]] = icmp eq ptr %b, null
3333
// CHECK-NEXT: br i1 [[isnull]], label %[[dynamic_cast_end:[a-z0-9._]+]], label %[[dynamic_cast_notnull:[a-z0-9._]+]]

Diff for: clang/test/CodeGenCXX/RelativeVTablesABI/type-info.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
// CHECK-NEXT: ret ptr @_ZTS1A
2525
// CHECK-NEXT: }
2626

27-
// CHECK: define{{.*}} i1 @_Z5equalP1A(ptr noundef readonly %a) local_unnamed_addr
27+
// CHECK: define{{.*}} i1 @_Z5equalP1A(ptr noundef readonly captures(address_is_null) %a) local_unnamed_addr
2828
// CHECK-NEXT: entry:
2929
// CHECK-NEXT: [[isnull:%[0-9]+]] = icmp eq ptr %a, null
3030
// CHECK-NEXT: br i1 [[isnull]], label %[[bad_typeid:[a-z0-9._]+]], label %[[end:[a-z0-9.+]+]]

Diff for: clang/test/CodeGenOpenCL/amdgcn-buffer-rsrc-type.cl

+2-2
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ __amdgpu_buffer_rsrc_t getBuffer(void *p) {
2222
}
2323

2424
// CHECK-LABEL: define {{[^@]+}}@consumeBufferPtr
25-
// CHECK-SAME: (ptr addrspace(5) noundef readonly [[P:%.*]]) local_unnamed_addr #[[ATTR0]] {
25+
// CHECK-SAME: (ptr addrspace(5) noundef readonly captures(address) [[P:%.*]]) local_unnamed_addr #[[ATTR0]] {
2626
// CHECK-NEXT: entry:
2727
// CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq ptr addrspace(5) [[P]], addrspacecast (ptr null to ptr addrspace(5))
2828
// CHECK-NEXT: br i1 [[TOBOOL_NOT]], label [[IF_END:%.*]], label [[IF_THEN:%.*]]
@@ -39,7 +39,7 @@ void consumeBufferPtr(__amdgpu_buffer_rsrc_t *p) {
3939
}
4040

4141
// CHECK-LABEL: define {{[^@]+}}@test
42-
// CHECK-SAME: (ptr addrspace(5) noundef readonly [[A:%.*]]) local_unnamed_addr #[[ATTR0]] {
42+
// CHECK-SAME: (ptr addrspace(5) noundef readonly captures(address) [[A:%.*]]) local_unnamed_addr #[[ATTR0]] {
4343
// CHECK-NEXT: entry:
4444
// CHECK-NEXT: [[TMP0:%.*]] = load i32, ptr addrspace(5) [[A]], align 16, !tbaa [[TBAA8:![0-9]+]]
4545
// CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i32 [[TMP0]], 0

Diff for: clang/test/CodeGenOpenCL/as_type.cl

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ int3 f8(char16 x) {
6767
return __builtin_astype(x, int3);
6868
}
6969

70-
//CHECK: define{{.*}} spir_func noundef ptr addrspace(1) @addr_cast(ptr noundef readnone %[[x:.*]])
70+
//CHECK: define{{.*}} spir_func noundef ptr addrspace(1) @addr_cast(ptr noundef readnone captures(ret: address, provenance) %[[x:.*]])
7171
//CHECK: %[[cast:.*]] ={{.*}} addrspacecast ptr %[[x]] to ptr addrspace(1)
7272
//CHECK: ret ptr addrspace(1) %[[cast]]
7373
global int* addr_cast(int *x) {

Diff for: llvm/include/llvm/Analysis/CaptureTracking.h

+53-15
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,13 @@
1414
#define LLVM_ANALYSIS_CAPTURETRACKING_H
1515

1616
#include "llvm/ADT/DenseMap.h"
17+
#include "llvm/Support/ModRef.h"
1718

1819
namespace llvm {
1920

2021
class Value;
2122
class Use;
23+
class CaptureInfo;
2224
class DataLayout;
2325
class Instruction;
2426
class DominatorTree;
@@ -77,10 +79,47 @@ namespace llvm {
7779
const DominatorTree &DT,
7880
unsigned MaxUsesToExplore = 0);
7981

82+
/// Capture information for a specific Use.
83+
struct UseCaptureInfo {
84+
/// Components captured by this use.
85+
CaptureComponents UseCC;
86+
/// Components captured by the return value of the user of this Use.
87+
CaptureComponents ResultCC;
88+
89+
UseCaptureInfo(CaptureComponents UseCC,
90+
CaptureComponents ResultCC = CaptureComponents::None)
91+
: UseCC(UseCC), ResultCC(ResultCC) {}
92+
93+
static UseCaptureInfo passthrough() {
94+
return UseCaptureInfo(CaptureComponents::None, CaptureComponents::All);
95+
}
96+
97+
bool isPassthrough() const {
98+
return capturesNothing(UseCC) && capturesAnything(ResultCC);
99+
}
100+
101+
operator CaptureComponents() const { return UseCC | ResultCC; }
102+
};
103+
80104
/// This callback is used in conjunction with PointerMayBeCaptured. In
81105
/// addition to the interface here, you'll need to provide your own getters
82106
/// to see whether anything was captured.
83107
struct CaptureTracker {
108+
/// Action returned from captures().
109+
enum Action {
110+
/// Stop the traversal.
111+
Stop,
112+
/// Continue traversal, and also follow the return value of the user if
113+
/// it has additional capture components (that is, if it has capture
114+
/// components in Ret that are not part of Other).
115+
Continue,
116+
/// Continue traversal, but do not follow the return value of the user,
117+
/// even if it has additional capture components. Should only be used if
118+
/// captures() has already taken the potential return captures into
119+
/// account.
120+
ContinueIgnoringReturn,
121+
};
122+
84123
virtual ~CaptureTracker();
85124

86125
/// tooManyUses - The depth of traversal has breached a limit. There may be
@@ -94,32 +133,31 @@ namespace llvm {
94133
/// U->getUser() is always an Instruction.
95134
virtual bool shouldExplore(const Use *U);
96135

97-
/// captured - Information about the pointer was captured by the user of
98-
/// use U. Return true to stop the traversal or false to continue looking
99-
/// for more capturing instructions.
100-
virtual bool captured(const Use *U) = 0;
136+
/// Use U directly captures CI.UseCC and additionally CI.ResultCC
137+
/// through the return value of the user of U.
138+
///
139+
/// Return one of Stop, Continue or ContinueIgnoringReturn to control
140+
/// further traversal.
141+
virtual Action captured(const Use *U, UseCaptureInfo CI) = 0;
101142

102143
/// isDereferenceableOrNull - Overload to allow clients with additional
103144
/// knowledge about pointer dereferenceability to provide it and thereby
104145
/// avoid conservative responses when a pointer is compared to null.
105146
virtual bool isDereferenceableOrNull(Value *O, const DataLayout &DL);
106147
};
107148

108-
/// Types of use capture kinds, see \p DetermineUseCaptureKind.
109-
enum class UseCaptureKind {
110-
NO_CAPTURE,
111-
MAY_CAPTURE,
112-
PASSTHROUGH,
113-
};
114-
115149
/// Determine what kind of capture behaviour \p U may exhibit.
116150
///
117-
/// A use can be no-capture, a use can potentially capture, or a use can be
118-
/// passthrough such that the uses of the user or \p U should be inspected.
151+
/// The returned UseCaptureInfo contains the components captured directly
152+
/// by the use (UseCC) and the components captured through the return value
153+
/// of the user (ResultCC).
154+
///
155+
/// \p Base is the starting value of the capture analysis, which is
156+
/// relevant for address_is_null captures.
119157
/// The \p IsDereferenceableOrNull callback is used to rule out capturing for
120158
/// certain comparisons.
121-
UseCaptureKind
122-
DetermineUseCaptureKind(const Use &U,
159+
UseCaptureInfo
160+
DetermineUseCaptureKind(const Use &U, const Value *Base,
123161
llvm::function_ref<bool(Value *, const DataLayout &)>
124162
IsDereferenceableOrNull);
125163

Diff for: llvm/include/llvm/IR/InstrTypes.h

+5
Original file line numberDiff line numberDiff line change
@@ -1692,6 +1692,11 @@ class CallBase : public Instruction {
16921692
return capturesNothing(getCaptureInfo(OpNo));
16931693
}
16941694

1695+
/// Returns whether the call has an argument that has an attribute like
1696+
/// captures(ret: address, provenance), where the return capture components
1697+
/// are not a subset of the other capture components.
1698+
bool hasArgumentWithAdditionalReturnCaptureComponents() const;
1699+
16951700
/// Determine whether this argument is passed by value.
16961701
bool isByValArgument(unsigned ArgNo) const {
16971702
return paramHasAttr(ArgNo, Attribute::ByVal);

Diff for: llvm/include/llvm/Support/ModRef.h

+13
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,10 @@ inline bool capturesFullProvenance(CaptureComponents CC) {
326326
return (CC & CaptureComponents::Provenance) == CaptureComponents::Provenance;
327327
}
328328

329+
inline bool capturesAll(CaptureComponents CC) {
330+
return CC == CaptureComponents::All;
331+
}
332+
329333
raw_ostream &operator<<(raw_ostream &OS, CaptureComponents CC);
330334

331335
/// Represents which components of the pointer may be captured in which
@@ -350,6 +354,15 @@ class CaptureInfo {
350354
/// Create CaptureInfo that may capture all components of the pointer.
351355
static CaptureInfo all() { return CaptureInfo(CaptureComponents::All); }
352356

357+
/// Create CaptureInfo that may only capture via the return value.
358+
static CaptureInfo
359+
retOnly(CaptureComponents RetComponents = CaptureComponents::All) {
360+
return CaptureInfo(CaptureComponents::None, RetComponents);
361+
}
362+
363+
/// Whether the pointer is only captured via the return value.
364+
bool isRetOnly() const { return capturesNothing(OtherComponents); }
365+
353366
/// Get components potentially captured by the return value.
354367
CaptureComponents getRetComponents() const { return RetComponents; }
355368

Diff for: llvm/lib/Analysis/AliasAnalysis.cpp

+9-3
Original file line numberDiff line numberDiff line change
@@ -835,9 +835,15 @@ bool llvm::isBaseOfObject(const Value *V) {
835835
}
836836

837837
bool llvm::isEscapeSource(const Value *V) {
838-
if (auto *CB = dyn_cast<CallBase>(V))
839-
return !isIntrinsicReturningPointerAliasingArgumentWithoutCapturing(CB,
840-
true);
838+
if (auto *CB = dyn_cast<CallBase>(V)) {
839+
if (isIntrinsicReturningPointerAliasingArgumentWithoutCapturing(CB, true))
840+
return false;
841+
842+
// The return value of a function with a captures(ret: address, provenance)
843+
// attribute is not necessarily an escape source. The return value may
844+
// alias with a non-escaping object.
845+
return !CB->hasArgumentWithAdditionalReturnCaptureComponents();
846+
}
841847

842848
// The load case works because isNonEscapingLocalObject considers all
843849
// stores to be escapes (it passes true for the StoreCaptures argument

0 commit comments

Comments
 (0)