[IR] Introduce captures attribute #116990
Conversation
nikic
requested review from
preames,
arichardson,
jdoerfert,
dtcxzyw and
efriedma-quic
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesThis introduces the This initial patch only introduces the IR/bitcode support for the attribute and its in-memory representation as Based on the RFC feedback, I've used a syntax similar to the I've added some pretty extensive documentation to LangRef on the semantics. One non-obvious bit here is that using ptrtoint will not result in a "return-only" capture, even if the ptrtoint result is only used in the return value. Without this requirement we wouldn't be able to continue ordinary capture analysis on the return value. Patch is 27.09 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/116990.diff 19 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 9f4c90ba82a419..df8599a818b6fc 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -1379,6 +1379,36 @@ Currently, only the following parameter attributes are defined:
function, returning a pointer to allocated storage disjoint from the
storage for any other object accessible to the caller.
+``captures(...)``
+ This attributes restrict the ways in which the callee may capture the
+ pointer. This is not a valid attribute for return values. This attribute
+ applies only to the particular copy of the pointer passed in this argument.
+
+ The arguments of ``captures`` is a list of captured pointer components,
+ which may be ``none``, or a combination of:
+
+ - ``address``: The integral address of the pointer.
+ - ``provenance``: The ability to access the pointer for both read and write
+ after the function returns.
+ - ``read_provenance``: The ability to access the pointer only for reads
+ after the function returns.
+
+ Additionally, it is possible to specify that the pointer is captured via
+ the return value only, by using ``caputres(ret: ...)``.
+
+ The `pointer capture section <pointercapture>` discusses these semantics
+ in more detail.
+
+ Some examples of how to use the attribute:
+
+ - ``captures(none)``: Pointer not captured.
+ - ``captures(address, provenance)``: Equivalent to omitting the attribute.
+ - ``captures(address)``: Address may be captured, but not provenance.
+ - ``captures(address, read_provenance)``: Both address and provenance
+ captured, but only for read-only access.
+ - ``captures(ret: address, provenance)``: Pointer captured through return
+ value only.
+
.. _nocapture:
``nocapture``
@@ -3318,10 +3348,91 @@ Pointer Capture
---------------
Given a function call and a pointer that is passed as an argument or stored in
-the memory before the call, a pointer is *captured* by the call if it makes a
-copy of any part of the pointer that outlives the call.
-To be precise, a pointer is captured if one or more of the following conditions
-hold:
+memory before the call, the call may capture two components of the pointer:
+
+ * The address of the pointer, which is its integral value. This also includes
+ parts of the address or any information about the address, including the
+ fact that it does not equal one specific value.
+ * The provenance of the pointer, which is the ability to perform memory
+ accesses through the pointer, in the sense of the :ref:`pointer aliasing
+ rules <pointeraliasing>`. We further distinguish whether only read acceses
+ are allowed, or both reads and writes.
+
+For example, the following function captures the address of ``%a``, because
+it is compared to a pointer, leaking information about the identitiy of the
+pointer:
+
+.. code-block:: llvm
+
+ @glb = global i8 0
+
+ define i1 @f(ptr %a) {
+ %c = icmp eq ptr %a, @glb
+ ret i1 %c
+ }
+
+The function does not capture the provenance of the pointer, because the
+``icmp`` instruction only operates on the pointer address. The following
+function captures both the address and provenance of the pointer, as both
+may be read from ``@glb`` after the function returns:
+
+.. code-block:: llvm
+
+ @glb = global ptr null
+
+ define void @f(ptr %a) {
+ store ptr %a, ptr @glb
+ ret void
+ }
+
+The following function captures *neither* the address nor the provenance of
+the pointer:
+
+.. code-block:: llvm
+
+ define i32 @f(ptr %a) {
+ %v = load i32, ptr %a
+ ret i32
+ }
+
+While address capture includes uses of the address within the body of the
+function, provenance capture refers exclusively to the ability to perform
+accesses *after* the function returns. Memory accesses within the function
+itself are not considered pointer captures.
+
+We can further say that the capture only occurs through a specific location.
+In the following example, the pointer (both address and provenance) is captured
+through the return value only:
+
+.. code-block:: llvm
+
+ define ptr @f(ptr %a) {
+ %gep = getelementptr i8, ptr %a, i64 4
+ ret ptr %gep
+ }
+
+However, we always consider direct inspection of the pointer address
+(e.g. using ``ptrtoint``) to be location-independent. The following example
+is *not* considered a return-only capture, even though the ``ptrtoint``
+ultimately only contribues to the return value:
+
+.. code-block:: llvm
+
+ @lookup = constant [4 x i8] [i8 0, i8 1, i8 2, i8 3]
+
+ define ptr @f(ptr %a) {
+ %a.addr = ptrtoint ptr %a to i64
+ %mask = and i64 %a.addr, 3
+ %gep = getelementptr i8, ptr @lookup, i64 %mask
+ ret ptr %gep
+ }
+
+This definition is chosen to allow capture analysis to continue with the return
+value in the usual fashion.
+
+The following describes possible ways to capture a pointer in more detail,
+where unqualified uses of the word "capture" refer to capturing both address
+and provenance.
1. The call stores any bit of the pointer carrying information into a place,
and the stored bits can be read from the place by the caller after this call
@@ -3360,13 +3471,14 @@ hold:
@lock = global i1 true
define void @f(ptr %a) {
- store ptr %a, ptr* @glb
+ store ptr %a, ptr @glb
store atomic i1 false, ptr @lock release ; %a is captured because another thread can safely read @glb
store ptr null, ptr @glb
ret void
}
-3. The call's behavior depends on any bit of the pointer carrying information.
+3. The call's behavior depends on any bit of the pointer carrying information
+ (address capture only).
.. code-block:: llvm
@@ -3374,7 +3486,7 @@ hold:
define void @f(ptr %a) {
%c = icmp eq ptr %a, @glb
- br i1 %c, label %BB_EXIT, label %BB_CONTINUE ; escapes %a
+ br i1 %c, label %BB_EXIT, label %BB_CONTINUE ; captures address of %a only
BB_EXIT:
call void @exit()
unreachable
@@ -3382,8 +3494,7 @@ hold:
ret void
}
-4. The pointer is used in a volatile access as its address.
-
+4. The pointer is used as the pointer operand of a volatile access.
.. _volatile:
diff --git a/llvm/include/llvm/AsmParser/LLParser.h b/llvm/include/llvm/AsmParser/LLParser.h
index 1ef8b8ffc39660..bc95a57da3c2ae 100644
--- a/llvm/include/llvm/AsmParser/LLParser.h
+++ b/llvm/include/llvm/AsmParser/LLParser.h
@@ -376,6 +376,7 @@ namespace llvm {
bool inAttrGrp, LocTy &BuiltinLoc);
bool parseRangeAttr(AttrBuilder &B);
bool parseInitializesAttr(AttrBuilder &B);
+ bool parseCapturesAttr(AttrBuilder &B);
bool parseRequiredTypeAttr(AttrBuilder &B, lltok::Kind AttrToken,
Attribute::AttrKind AttrKind);
diff --git a/llvm/include/llvm/AsmParser/LLToken.h b/llvm/include/llvm/AsmParser/LLToken.h
index 178c911120b4ce..48b8aa0158c660 100644
--- a/llvm/include/llvm/AsmParser/LLToken.h
+++ b/llvm/include/llvm/AsmParser/LLToken.h
@@ -207,6 +207,11 @@ enum Kind {
kw_inaccessiblememonly,
kw_inaccessiblemem_or_argmemonly,
+ // Captures attribute:
+ kw_address,
+ kw_provenance,
+ kw_read_provenance,
+
// nofpclass attribute:
kw_all,
kw_nan,
diff --git a/llvm/include/llvm/Bitcode/LLVMBitCodes.h b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
index a0fb32f67e3858..4a7af55fce871a 100644
--- a/llvm/include/llvm/Bitcode/LLVMBitCodes.h
+++ b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
@@ -783,6 +783,7 @@ enum AttributeKindCodes {
ATTR_KIND_CORO_ELIDE_SAFE = 98,
ATTR_KIND_NO_EXT = 99,
ATTR_KIND_NO_DIVERGENCE_SOURCE = 100,
+ ATTR_KIND_CAPTURES = 101,
};
enum ComdatSelectionKindCodes {
diff --git a/llvm/include/llvm/IR/Attributes.h b/llvm/include/llvm/IR/Attributes.h
index 2755ced404dddb..7612e553fe32e6 100644
--- a/llvm/include/llvm/IR/Attributes.h
+++ b/llvm/include/llvm/IR/Attributes.h
@@ -284,6 +284,9 @@ class Attribute {
/// Returns memory effects.
MemoryEffects getMemoryEffects() const;
+ /// Returns information from captures attribute.
+ CaptureInfo getCaptureInfo() const;
+
/// Return the FPClassTest for nofpclass
FPClassTest getNoFPClass() const;
@@ -436,6 +439,7 @@ class AttributeSet {
UWTableKind getUWTableKind() const;
AllocFnKind getAllocKind() const;
MemoryEffects getMemoryEffects() const;
+ CaptureInfo getCaptureInfo() const;
FPClassTest getNoFPClass() const;
std::string getAsString(bool InAttrGrp = false) const;
@@ -1260,6 +1264,9 @@ class AttrBuilder {
/// Add memory effect attribute.
AttrBuilder &addMemoryAttr(MemoryEffects ME);
+ /// Add captures attribute.
+ AttrBuilder &addCapturesAttr(CaptureInfo CI);
+
// Add nofpclass attribute
AttrBuilder &addNoFPClassAttr(FPClassTest NoFPClassMask);
diff --git a/llvm/include/llvm/IR/Attributes.td b/llvm/include/llvm/IR/Attributes.td
index 49f4527bde66e7..e6e9846c412a7f 100644
--- a/llvm/include/llvm/IR/Attributes.td
+++ b/llvm/include/llvm/IR/Attributes.td
@@ -183,6 +183,9 @@ def NoCallback : EnumAttr<"nocallback", IntersectAnd, [FnAttr]>;
/// Function creates no aliases of pointer.
def NoCapture : EnumAttr<"nocapture", IntersectAnd, [ParamAttr]>;
+/// Specify how the pointer may be captured.
+def Captures : IntAttr<"captures", IntersectCustom, [ParamAttr]>;
+
/// Function is not a source of divergence.
def NoDivergenceSource : EnumAttr<"nodivergencesource", IntersectAnd, [FnAttr]>;
diff --git a/llvm/include/llvm/Support/ModRef.h b/llvm/include/llvm/Support/ModRef.h
index 5a9d80c87ae27a..d610aa5eaac6b5 100644
--- a/llvm/include/llvm/Support/ModRef.h
+++ b/llvm/include/llvm/Support/ModRef.h
@@ -273,6 +273,93 @@ raw_ostream &operator<<(raw_ostream &OS, MemoryEffects RMRB);
// Legacy alias.
using FunctionModRefBehavior = MemoryEffects;
+/// Components of the pointer that may be captured.
+enum class CaptureComponents : uint8_t {
+ None = 0,
+ Address = (1 << 0),
+ ReadProvenance = (1 << 1),
+ Provenance = (1 << 2) | ReadProvenance,
+ All = Address | Provenance,
+ LLVM_MARK_AS_BITMASK_ENUM(Provenance),
+};
+
+inline bool capturesNothing(CaptureComponents CC) {
+ return CC == CaptureComponents::None;
+}
+
+inline bool capturesAnything(CaptureComponents CC) {
+ return CC != CaptureComponents::None;
+}
+
+inline bool capturesAddress(CaptureComponents CC) {
+ return (CC & CaptureComponents::Address) != CaptureComponents::None;
+}
+
+inline bool capturesReadProvenanceOnly(CaptureComponents CC) {
+ return (CC & CaptureComponents::Provenance) ==
+ CaptureComponents::ReadProvenance;
+}
+
+inline bool capturesFullProvenance(CaptureComponents CC) {
+ return (CC & CaptureComponents::Provenance) == CaptureComponents::Provenance;
+}
+
+raw_ostream &operator<<(raw_ostream &OS, CaptureComponents CC);
+
+/// Represents which components of the pointer may be captured and whether
+/// the capture is via the return value only. This represents the captures(...)
+/// attribute in IR.
+///
+/// For more information on the precise semantics see LangRef.
+class CaptureInfo {
+ CaptureComponents Components;
+ bool ReturnOnly;
+
+public:
+ CaptureInfo(CaptureComponents Components, bool ReturnOnly = false)
+ : Components(Components),
+ ReturnOnly(capturesAnything(Components) && ReturnOnly) {}
+
+ /// Create CaptureInfo that may capture all components of the pointer.
+ static CaptureInfo all() { return CaptureInfo(CaptureComponents::All); }
+
+ /// Get the potentially captured components of the pointer.
+ operator CaptureComponents() const { return Components; }
+
+ /// Whether the pointer is captured through the return value only.
+ bool isReturnOnly() const { return ReturnOnly; }
+
+ bool operator==(CaptureInfo Other) const {
+ return Components == Other.Components && ReturnOnly == Other.ReturnOnly;
+ }
+
+ bool operator!=(CaptureInfo Other) const { return !(*this == Other); }
+
+ /// Compute union of CaptureInfos.
+ CaptureInfo operator|(CaptureInfo Other) const {
+ return CaptureInfo(Components | Other.Components,
+ ReturnOnly && Other.ReturnOnly);
+ }
+
+ /// Compute intersection of CaptureInfos.
+ CaptureInfo operator&(CaptureInfo Other) const {
+ return CaptureInfo(Components & Other.Components,
+ ReturnOnly || Other.ReturnOnly);
+ }
+
+ static CaptureInfo createFromIntValue(uint32_t Data) {
+ return CaptureInfo(CaptureComponents(Data >> 1), Data & 1);
+ }
+
+ /// Convert CaptureInfo into an encoded integer value (used by captures
+ /// attribute).
+ uint32_t toIntValue() const {
+ return (uint32_t(Components) << 1) | ReturnOnly;
+ }
+};
+
+raw_ostream &operator<<(raw_ostream &OS, CaptureInfo Info);
+
} // namespace llvm
#endif
diff --git a/llvm/lib/AsmParser/LLLexer.cpp b/llvm/lib/AsmParser/LLLexer.cpp
index 1b8e033134f51b..7b6be79723b96b 100644
--- a/llvm/lib/AsmParser/LLLexer.cpp
+++ b/llvm/lib/AsmParser/LLLexer.cpp
@@ -704,6 +704,9 @@ lltok::Kind LLLexer::LexIdentifier() {
KEYWORD(argmemonly);
KEYWORD(inaccessiblememonly);
KEYWORD(inaccessiblemem_or_argmemonly);
+ KEYWORD(address);
+ KEYWORD(provenance);
+ KEYWORD(read_provenance);
// nofpclass attribute
KEYWORD(all);
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index b8a8df71d4de21..880fe543810570 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -1644,6 +1644,8 @@ bool LLParser::parseEnumAttribute(Attribute::AttrKind Attr, AttrBuilder &B,
return parseRangeAttr(B);
case Attribute::Initializes:
return parseInitializesAttr(B);
+ case Attribute::Captures:
+ return parseCapturesAttr(B);
default:
B.addAttribute(Attr);
Lex.Lex();
@@ -3165,6 +3167,53 @@ bool LLParser::parseInitializesAttr(AttrBuilder &B) {
return false;
}
+bool LLParser::parseCapturesAttr(AttrBuilder &B) {
+ CaptureComponents CC = CaptureComponents::None;
+ bool ReturnOnly = false;
+
+ // We use syntax like captures(ret: address, provenance), so the colon
+ // should not be interpreted as a label terminator.
+ Lex.setIgnoreColonInIdentifiers(true);
+ auto _ = make_scope_exit([&] { Lex.setIgnoreColonInIdentifiers(false); });
+
+ Lex.Lex();
+ if (parseToken(lltok::lparen, "expected '('"))
+ return true;
+
+ if (EatIfPresent(lltok::kw_ret)) {
+ if (parseToken(lltok::colon, "expected ':'"))
+ return true;
+
+ ReturnOnly = true;
+ }
+
+ if (EatIfPresent(lltok::kw_none)) {
+ if (parseToken(lltok::rparen, "expected ')'"))
+ return true;
+ } else {
+ while (true) {
+ if (EatIfPresent(lltok::kw_address))
+ CC |= CaptureComponents::Address;
+ else if (EatIfPresent(lltok::kw_provenance))
+ CC |= CaptureComponents::Provenance;
+ else if (EatIfPresent(lltok::kw_read_provenance))
+ CC |= CaptureComponents::ReadProvenance;
+ else
+ return tokError(
+ "expected one of 'address', 'provenance' or 'read_provenance'");
+
+ if (EatIfPresent(lltok::rparen))
+ break;
+
+ if (parseToken(lltok::comma, "expected ',' or ')'"))
+ return true;
+ }
+ }
+
+ B.addCapturesAttr(CaptureInfo(CC, ReturnOnly));
+ return false;
+}
+
/// parseOptionalOperandBundles
/// ::= /*empty*/
/// ::= '[' OperandBundle [, OperandBundle ]* ']'
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 3e6abacac27261..e74422cfe90fb5 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -2215,6 +2215,8 @@ static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
return Attribute::CoroElideSafe;
case bitc::ATTR_KIND_NO_EXT:
return Attribute::NoExt;
+ case bitc::ATTR_KIND_CAPTURES:
+ return Attribute::Captures;
}
}
@@ -2354,6 +2356,8 @@ Error BitcodeReader::parseAttributeGroupBlock() {
B.addAllocKindAttr(static_cast<AllocFnKind>(Record[++i]));
else if (Kind == Attribute::Memory)
B.addMemoryAttr(MemoryEffects::createFromIntValue(Record[++i]));
+ else if (Kind == Attribute::Captures)
+ B.addCapturesAttr(CaptureInfo::createFromIntValue(Record[++i]));
else if (Kind == Attribute::NoFPClass)
B.addNoFPClassAttr(
static_cast<FPClassTest>(Record[++i] & fcAllFlags));
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index 24a4c2e8303d5a..83a5f753ee5685 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -902,6 +902,8 @@ static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
return bitc::ATTR_KIND_INITIALIZES;
case Attribute::NoExt:
return bitc::ATTR_KIND_NO_EXT;
+ case Attribute::Captures:
+ return bitc::ATTR_KIND_CAPTURES;
case Attribute::EndAttrKinds:
llvm_unreachable("Can not encode end-attribute kinds marker.");
case Attribute::None:
diff --git a/llvm/lib/IR/AttributeImpl.h b/llvm/lib/IR/AttributeImpl.h
index 82c501dcafcb7f..59cc489ade40de 100644
--- a/llvm/lib/IR/AttributeImpl.h
+++ b/llvm/lib/IR/AttributeImpl.h
@@ -346,6 +346,7 @@ class AttributeSetNode final
UWTableKind getUWTableKind() const;
AllocFnKind getAllocKind() const;
MemoryEffects getMemoryEffects() const;
+ CaptureInfo getCaptureInfo() const;
FPClassTest getNoFPClass() const;
std::string getAsString(bool InAttrGrp) const;
Type *getAttributeType(Attribute::AttrKind Kind) const;
diff --git a/llvm/lib/IR/Attributes.cpp b/llvm/lib/IR/Attributes.cpp
index e9daa01b899e8f..052998698321a6 100644
--- a/llvm/lib/IR/Attributes.cpp
+++ b/llvm/lib/IR/Attributes.cpp
@@ -487,6 +487,12 @@ MemoryEffects Attribute::getMemoryEffects() const {
return MemoryEffects::createFromIntValue(pImpl->getValueAsInt());
}
+CaptureInfo Attribute::getCaptureInfo() const {
+ assert(hasAttribute(Attribute::Captures) &&
+ "Can only call getCaptureInfo() on captures attribute");
+ return CaptureInfo::createFromIntValue(pImpl->getValueAsInt());
+}
+
FPClassTest Attribute::getNoFPClass() const {
assert(hasAttribute(Attribute::NoFPClass) &&
"Can only call getNoFPClass() on nofpclass attribute");
@@ -647,6 +653,13 @@ std::string Attribute::getAsString(bool InAttrGrp) const {
return Result;
}
+ if (hasAttribute(Attribute::Captures)) {
+ std::string Result;
+ raw_string_ostream OS(Result);
+ OS << getCaptureInfo();
+ return Result;
+ }
+
if (hasAttribute(Attribute::NoFPClass)) {
std::string Result = "nofpclass";
raw_string_ostream OS(Result);
@@ -1050,6 +1063,10 @@ AttributeSet::intersectWith(LLVMContext &C, AttributeSet Other) const {
Intersected.addMemoryAttr(Attr0.getMemoryEffects() |
Attr1.getMemoryEffects());
break;
+ case Attribute::Captures:
+ Intersected.addCapturesAttr(Attr0.getCaptureInfo() |
+ Attr1.getCaptureInfo());
+ break;
case Attribute::NoFPClass:
Intersected.addNoFPClassAttr(Attr0.getNoFPClass() &
Attr1.getNoFPClass());
@@ -1170,6 +1187,10 @@ MemoryEffects AttributeSet::getMemoryEffects() const {
return SetNode ? SetNode->getMemoryEffects() : MemoryEffects::unknown();
}
+CaptureInfo AttributeSet::getCaptureInfo() const {
+ return SetNode ? SetNode->getCaptureInfo() : CaptureInfo::all();
+}
+
FPClassTest AttributeSet::getNoFPClass() const {
return SetNode ? SetNode->getNoFPClass() : fcNone;
}
@@ -1358,6 +1379,12 @@ MemoryEffects AttributeSetNode::getMemoryEffects() const {
return MemoryEffects::unknown();
}
+CaptureInfo AttributeSetNode::getCaptureInfo() const {
+ if (auto A = findEnumAttribute(Attribute::Captures))
+ return A->getCaptureInfo();
+ return CaptureInfo::all();
+}
+
FPClassTest AttributeSetNode::getNoFPClass() const {
if (auto A = findEnumAttribute(Attribute::NoFPClass))
return A->getNoFPClass();
@@ -2190,6 +2217,10 @@ AttrBuilder &AttrBuilder::addMemoryAttr(MemoryEffects ME) {
return addRawIntAttr(Attribute::Memory, ME.toIntValue());
}
+AttrBuilder &AttrBuilder::addCapturesAttr(CaptureInfo CI) {
+ return addRawIntAttr(Attribute::Captures, CI.toIntValue());
+}
+
AttrBuilder &AttrBuilder::addNoFPClassAttr(FPClassTest Mask) {...
[truncated]
|
|
Do we want to say something about null pointers? Not sure which way you want to go, but we currently have some special handling for null in capture tracking. Do we need to say anything about fences? (ref #64188) |
This is a good question. Generally speaking, comparison with null still counts as an address capture, just like any other comparison. This is because The caveat here is that is that we have two special cases:
So maybe this could be generalized like this for the purpose of LangRef?
We probably should, but I don't want to do that as part of this change, as this would also require implementation changes that I'm not sure how to do. My understand here is that (assuming this will not get fixed on the level of the memory model) fences only affect the notion of "captures before" by saying that if there is a fence (possibly behind a call) and the pointer is later captured, then the capture already occurs at the point of the fence. This is not something that really integrates into current CaptureTracking, which is based on following def-use chains, so we never see the fence. |
| } | ||
|
|
||
| However, we always consider direct inspection of the pointer address | ||
| (e.g. using ``ptrtoint``) to be location-independent. The following example |
There was a problem hiding this comment.
Does this mean ptrtoint unconditionally captures provenance? I'd like to be able to avoid this for CHERI where it would only be an address capture.
But I guess we avoid this by making sure to always use our local llvm.cheri.cap.address.get intrinsic instead of ptrtoint (although ptrtoint is more helpful for optimizations/known bits).
There was a problem hiding this comment.
Currently LLVM considers ptrtoint to always capture provenance, but I do plan to introduce a variant of it that only captures the address. This is also needed to better model strict provenance in Rust, and should also be helpful to represent pointer differences (though those might benefit from a separate intrinsic, really).
|
I'm not quite sure about the statement that icmp doesn't capture provenance... I mean, take something like the following: Your phrasing implies this code is UB. Which... I'm not sure whether it's what you want, but it's a change to the underlying semantics. (LLVM optimizations currently convert icmp of a ptrtoint to an icmp of the underlying pointer.) In the interest of making sure there aren't any surprises here, maybe we should say that this is a provenance capture? (If you're planning to introduce a non-provenance-capturing ptrtoint, it can be used to express a non-capturing icmp.) I think it makes sense to consider a null-comparison involving dereferenceable_or_null to be non-capturing, but I think we need to tighten the phrasing on dereferenceable_or_null to actually make that true. There isn't really anything stopping someone from just doing an out-of-bounds gep where the result happens to be null... at which point, you've captured the address. I think we need some statement indicating the "null" in dereferenceable_or_null has to be a true null, not the result of an out-of-bounds gep. |
I think everyone would agree that this variant is UB, right? (Assuming The problem in your example isn't so much that we convert icmp of ptrtoint into icmp of pointers, but that we then DCE the ptrtoints, losing the provenance exposure side effect. This is the same class of problem as #33896, and all the related issues. I really wouldn't want to specify icmp as a provenance capturing operation. I think nowadays we're pretty clear on icmp being an address-only, provenance-unaware operation (thus also changes like #82458) and going back on that would muddy the waters quite a bit, not to mention all the consequences of making icmps a non-pure operation. Of course, we'd just ignore all those consequences anyway, but I'd rather move us towards the correct direction... I think if we find evidence that this is a problem in practice (rather than just in adversarially constructed inttoptr examples, where we already have ten other ways in which LLVM can miscompile them...) I'd consider temporarily working around this on the implementation side only, while still specifying the correct semantics in LangRef.
Good point. I think the relevant distinction here would be that the "null" needs to not have provenance. But then we'd have a case where adding provenance to a pointer would introduce additional UB, losing provenance monotonicity, which is likely a bad idea. I'm not sure how to specify this cleanly while retaining the existing special case. A possible alternative here would be to instead split off an extra |
So your position is that transforming
Propagating an additional bit is more complicated, unfortunately, but it might be the best solution. A lot of transforms can probably ignore the null capture (for example, if the pointer in question is produced by an inbounds gep). Another alternative is to introduce an inbounds bit on icmp. |
Basically, yes. (I don't think the icmp transform itself is invalid, but rather the DCE of the unused ptrtoint instruction, but the details aren't really relevant. If we need a bandaid for this, my intuition is that just not doing that icmp transform wouldn't be a particularly large loss.)
Right. Anything AA-related is only going to care about provenance, and the transforms that care about address capture usually do some kind of allocation replacement (like e.g. call slot optimization and a few related ones) where we know in advance that we're replacing a non-null pointer with another non-null pointer, so null-pointer-only capture can be ignored. |
Just out of curiosity, do we need another version of icmp ptr that captures the provenance? I think the spec of C/C++/Rust may guarantee the provenance capturing of pointer comparisons: https://godbolt.org/z/5zbTfPa3x |
I'm not really sure what your godbolt example is supposed to show (I don't see any relation to provenance capture?) but to answer your question: Rust definitely does not capture provenance in comparisons. The current C/C++ standards do not specify provenance semantics, but relevant papers have converged on a pnvi-ae style model, where pointer comparisons are not considered to be address-exposed operations. I just double checked that this is still the case in the latest WG14 paper on the topic (N3271 for TS6010). |
|
I've updated this to support |
|
Yes, that's correct, because icmp only compares the pointer address. See #82458 for the relevant GVN change (we haven't propagated that one to some other places yet, like select folding). Though I will note that the transform GCC does in your particular example is still correct (https://alive2.llvm.org/ce/z/EgWrbA): If |
| - ``captures(address_is_null)``: Only captures whether the address is null. | ||
| - ``captures(address, read_provenance)``: Both address and provenance | ||
| captured, but only for read-only access. | ||
| - ``captures(ret: address, provenance)``: Pointer captured through return |
There was a problem hiding this comment.
If I'm understanding correctly, it's not possible to have something like captures(address_is_null, ret: address, provenance)? Would something like that make sense?
There was a problem hiding this comment.
Yes, that's currently not supported. This is just to reduce complexity because I don't think it would buy us much in practice right now. But I could change this to be more memory-like and track the captured components independently for the "return" and "other" locations.
There was a problem hiding this comment.
I pushed a change to track locations separately, so captures(address_is_null, ret: address, provenance) is now supported.
|
ping |
|
ping :) |
| LLVM_MARK_AS_BITMASK_ENUM(Provenance), | ||
| }; | ||
|
|
||
| inline bool capturesNothing(CaptureComponents CC) { |
There was a problem hiding this comment.
I'm not sure what the following commits look like but maybe it would it be better for IDE code completion to have a struct wrapper with these as member functions?
There was a problem hiding this comment.
I haven't worked on the following commits yet, so I expect the API here to change a good bit once I get around to the inference + CaptureTracking changes.
I could wrap this in a struct, but then I'd have to reimplement the functionality that BitMaskEnum provides :(
nikic
force-pushed
the
captures
branch
2 times, most recently
from
54abc42 to
c93f4fa
Compare
| ;--- none-before.ll | ||
|
|
||
| ; CHECK-NONE-BEFORE: <stdin>:[[@LINE+1]]:38: error: cannot use 'none' with other component | ||
| define void @test(ptr captures(none, address) %p) { |
There was a problem hiding this comment.
Not sure if this is already covered elsewhere, but should we also have some tests with the attribute applied on non-pointer types?
There was a problem hiding this comment.
Good point, I forgot to add this attribute to typeIncompatible. Done now and added a test.
|
New year's ping :) |
|
Ping :) |
llvm/docs/LangRef.rst
Outdated
|
|
||
| - ``address``: The integral address of the pointer. | ||
| - ``address_is_null`` (subset of ``address``): Whether the address is null. | ||
| - ``provenance``: The ability to access the pointer for both read and write |
There was a problem hiding this comment.
Just curious, if we already have a separate read_provenance, would it make sense to have write_provenance instead of just provenance?
There was a problem hiding this comment.
My thinking here was that LLVM doesn't really have a notion of write-only memory. Memory can be read-only or read-write, but not write-only. Our writeonly attribute, after recent clarifications, allows reads as long as they are unobservable. So I think write_provenance always has to be combined with read_provenance, which is why I went with plain provenance including both.
There was a problem hiding this comment.
Sounds good to me, we can always adjust if needed
This introduces the `captures` attribute as described in: https://discourse.llvm.org/t/rfc-improvements-to-capture-tracking/81420 This initial patch only introduces the IR/bitcode support for the attribute and its in-memory representation as `CaptureInfo`. This will be followed by a patch to remove (and upgrade) the `nocapture` attribute, and then by actual inference/analysis support. Based on the RFC feedback, I've used a syntax similar to the `memory` attribute, though the only "location" that can be specified right now is `ret`. I've added some pretty extensive documentation to LangRef on the semantics. One non-obvious bit here is that using ptrtoint will not result in a "return-only" capture, even if the ptrtoint result is only used in the return value. Without this requirement we wouldn't be able to continue ordinary capture analysis on the return value.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/11883 Here is the relevant piece of the build log for the reference |
This introduces the `captures` attribute as described in: https://discourse.llvm.org/t/rfc-improvements-to-capture-tracking/81420 This initial patch only introduces the IR/bitcode support for the attribute and its in-memory representation as `CaptureInfo`. This will be followed by a patch to upgrade and remove the `nocapture` attribute, and then by actual inference/analysis support. Based on the RFC feedback, I've used a syntax similar to the `memory` attribute, though the only "location" that can be specified is `ret`. I've added some pretty extensive documentation to LangRef on the semantics. One non-obvious bit here is that using ptrtoint will not result in a "return-only" capture, even if the ptrtoint result is only used in the return value. Without this requirement we wouldn't be able to continue ordinary capture analysis on the return value.
This PR removes the old `nocapture` attribute, replacing it with the new `captures` attribute introduced in #116990. This change is intended to be essentially NFC, replacing existing uses of `nocapture` with `captures(none)` without adding any new analysis capabilities. Making use of non-`none` values is left for a followup. Some notes: * `nocapture` will be upgraded to `captures(none)` by the bitcode reader. * `nocapture` will also be upgraded by the textual IR reader. This is to make it easier to use old IR files and somewhat reduce the test churn in this PR. * Helper APIs like `doesNotCapture()` will check for `captures(none)`. * MLIR import will convert `captures(none)` into an `llvm.nocapture` attribute. The representation in the LLVM IR dialect should be updated separately.
This PR removes the old `nocapture` attribute, replacing it with the new `captures` attribute introduced in llvm#116990. This change is intended to be essentially NFC, replacing existing uses of `nocapture` with `captures(none)` without adding any new analysis capabilities. Making use of non-`none` values is left for a followup. Some notes: * `nocapture` will be upgraded to `captures(none)` by the bitcode reader. * `nocapture` will also be upgraded by the textual IR reader. This is to make it easier to use old IR files and somewhat reduce the test churn in this PR. * Helper APIs like `doesNotCapture()` will check for `captures(none)`. * MLIR import will convert `captures(none)` into an `llvm.nocapture` attribute. The representation in the LLVM IR dialect should be updated separately.
This PR removes the old `nocapture` attribute, replacing it with the new `captures` attribute introduced in llvm#116990. This change is intended to be essentially NFC, replacing existing uses of `nocapture` with `captures(none)` without adding any new analysis capabilities. Making use of non-`none` values is left for a followup. Some notes: * `nocapture` will be upgraded to `captures(none)` by the bitcode reader. * `nocapture` will also be upgraded by the textual IR reader. This is to make it easier to use old IR files and somewhat reduce the test churn in this PR. * Helper APIs like `doesNotCapture()` will check for `captures(none)`. * MLIR import will convert `captures(none)` into an `llvm.nocapture` attribute. The representation in the LLVM IR dialect should be updated separately.
This introduces the
capturesattribute as described in: https://discourse.llvm.org/t/rfc-improvements-to-capture-tracking/81420This initial patch only introduces the IR/bitcode support for the attribute and its in-memory representation as
CaptureInfo. This will be followed by a patch to upgrade and remove thenocaptureattribute, and then by actual inference/analysis support.Based on the RFC feedback, I've used a syntax similar to the
memoryattribute, though the only "location" that can be specified isret.I've added some pretty extensive documentation to LangRef on the semantics. One non-obvious bit here is that using ptrtoint will not result in a "return-only" capture, even if the ptrtoint result is only used in the return value. Without this requirement we wouldn't be able to continue ordinary capture analysis on the return value.