[clang] implement current direction of CWG2765 for string literal comparisons in constant evaluation #109208
Conversation
Track the identity of each string literal object produced by evaluation with a global version number. Accept comparisons between literals of the same version, and between literals of different versions that cannot possibly be placed in overlapping storage. Treat the remaining comparisons as non-constant.
produce different string literals each time.
llvmbot
added
clang
zygoloid
changed the title
|
@llvm/pr-subscribers-clang Author: Richard Smith (zygoloid) ChangesTrack the identity of each string literal object produced by evaluation Full diff: https://github.com/llvm/llvm-project/pull/109208.diff 8 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index b65a1f7dff5bc1..6170bcd4f15ae3 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -324,6 +324,13 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// This is lazily created. This is intentionally not serialized.
mutable llvm::StringMap<StringLiteral *> StringLiteralCache;
+ /// The next string literal "version" to allocate during constant evaluation.
+ /// This is used to distinguish between repeated evaluations of the same
+ /// string literal.
+ ///
+ /// TODO: Ensure version numbers don't collide when deserialized.
+ unsigned NextStringLiteralVersion = 0;
+
/// MD5 hash of CUID. It is calculated when first used and cached by this
/// data member.
mutable std::string CUIDHash;
@@ -3278,6 +3285,10 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// PredefinedExpr to cache evaluated results.
StringLiteral *getPredefinedStringLiteralFromCache(StringRef Key) const;
+ /// Return the next version number to be used for a string literal evaluated
+ /// as part of constant evaluation.
+ unsigned getNextStringLiteralVersion() { return NextStringLiteralVersion++; }
+
/// Return a declaration for the global GUID object representing the given
/// GUID value.
MSGuidDecl *getMSGuidDecl(MSGuidDeclParts Parts) const;
diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index 21a307d1e89878..76e693f6b4a6ca 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -96,6 +96,8 @@ def note_constexpr_pointer_constant_comparison : Note<
"at runtime">;
def note_constexpr_literal_comparison : Note<
"comparison of addresses of literals has unspecified value">;
+def note_constexpr_opaque_call_comparison : Note<
+ "comparison against opaque constant has unspecified value">;
def note_constexpr_pointer_weak_comparison : Note<
"comparison against address of weak declaration '%0' can only be performed "
"at runtime">;
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 6387e375dda79c..d9384a7c125a82 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -54,8 +54,10 @@
#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/TargetInfo.h"
#include "llvm/ADT/APFixedPoint.h"
+#include "llvm/ADT/Sequence.h"
#include "llvm/ADT/SmallBitVector.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Casting.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/SaveAndRestore.h"
#include "llvm/Support/SipHash.h"
@@ -2061,8 +2063,8 @@ static bool EvaluateIgnoredValue(EvalInfo &Info, const Expr *E) {
return true;
}
-/// Should this call expression be treated as a no-op?
-static bool IsNoOpCall(const CallExpr *E) {
+/// Should this call expression be treated as forming an opaque constant?
+static bool IsOpaqueConstantCall(const CallExpr *E) {
unsigned Builtin = E->getBuiltinCallee();
return (Builtin == Builtin::BI__builtin___CFStringMakeConstantString ||
Builtin == Builtin::BI__builtin___NSStringMakeConstantString ||
@@ -2070,6 +2072,12 @@ static bool IsNoOpCall(const CallExpr *E) {
Builtin == Builtin::BI__builtin_function_start);
}
+static bool IsOpaqueConstantCall(const LValue &LVal) {
+ auto *BaseExpr =
+ llvm::dyn_cast_or_null<CallExpr>(LVal.Base.dyn_cast<const Expr *>());
+ return BaseExpr && IsOpaqueConstantCall(BaseExpr);
+}
+
static bool IsGlobalLValue(APValue::LValueBase B) {
// C++11 [expr.const]p3 An address constant expression is a prvalue core
// constant expression of pointer type that evaluates to...
@@ -2115,7 +2123,7 @@ static bool IsGlobalLValue(APValue::LValueBase B) {
case Expr::ObjCBoxedExprClass:
return cast<ObjCBoxedExpr>(E)->isExpressibleAsConstantInitializer();
case Expr::CallExprClass:
- return IsNoOpCall(cast<CallExpr>(E));
+ return IsOpaqueConstantCall(cast<CallExpr>(E));
// For GCC compatibility, &&label has static storage duration.
case Expr::AddrLabelExprClass:
return true;
@@ -2142,11 +2150,81 @@ static const ValueDecl *GetLValueBaseDecl(const LValue &LVal) {
return LVal.Base.dyn_cast<const ValueDecl*>();
}
-static bool IsLiteralLValue(const LValue &Value) {
- if (Value.getLValueCallIndex())
+// Information about an LValueBase that is some kind of string.
+struct LValueBaseString {
+ std::string ObjCEncodeStorage;
+ StringRef Bytes;
+ int CharWidth;
+};
+
+// Gets the lvalue base of LVal as a string.
+static bool GetLValueBaseAsString(const EvalInfo &Info, const LValue &LVal,
+ LValueBaseString &AsString) {
+ const auto *BaseExpr = LVal.Base.dyn_cast<const Expr *>();
+ if (!BaseExpr)
+ return false;
+
+ // For ObjCEncodeExpr, we need to compute and store the string.
+ if (const auto *EE = dyn_cast<ObjCEncodeExpr>(BaseExpr)) {
+ Info.Ctx.getObjCEncodingForType(EE->getEncodedType(),
+ AsString.ObjCEncodeStorage);
+ AsString.Bytes = AsString.ObjCEncodeStorage;
+ AsString.CharWidth = 1;
+ return true;
+ }
+
+ // Otherwise, we have a StringLiteral.
+ const auto *Lit = dyn_cast<StringLiteral>(BaseExpr);
+ if (const auto *PE = dyn_cast<PredefinedExpr>(BaseExpr))
+ Lit = PE->getFunctionName();
+
+ if (!Lit)
+ return false;
+
+ AsString.Bytes = Lit->getBytes();
+ AsString.CharWidth = Lit->getCharByteWidth();
+ return true;
+}
+
+// Determine whether two string literals potentially overlap. This will be the
+// case if they agree on the values of all the bytes on the overlapping region
+// between them.
+static bool ArePotentiallyOverlappingStringLiterals(const EvalInfo &Info,
+ const LValue &LHS,
+ const LValue &RHS) {
+ LValueBaseString LHSString, RHSString;
+ if (!GetLValueBaseAsString(Info, LHS, LHSString) ||
+ !GetLValueBaseAsString(Info, RHS, RHSString))
return false;
- const Expr *E = Value.Base.dyn_cast<const Expr*>();
- return E && !isa<MaterializeTemporaryExpr>(E);
+
+ // This is the byte offset to the location of the first character of LHS
+ // within RHS. We don't need to look at the characters of one string that
+ // would appear before the start of the other string if they were merged.
+ CharUnits Offset = RHS.Offset - LHS.Offset;
+ if (Offset.isPositive()) {
+ RHSString.Bytes = RHSString.Bytes.drop_front(Offset.getQuantity());
+ } else if (Offset.isNegative()) {
+ LHSString.Bytes = LHSString.Bytes.drop_front(-Offset.getQuantity());
+ }
+
+ bool LHSIsLonger = LHSString.Bytes.size() > RHSString.Bytes.size();
+ StringRef Longer = LHSIsLonger ? LHSString.Bytes : RHSString.Bytes;
+ StringRef Shorter = LHSIsLonger ? RHSString.Bytes : LHSString.Bytes;
+ int ShorterCharWidth = (LHSIsLonger ? RHSString : LHSString).CharWidth;
+
+ // The null terminator isn't included in the string data, so check for it
+ // manually. If the longer string doesn't have a null terminator where the
+ // shorter string ends, they aren't potentially overlapping.
+ for (int nullByte : llvm::seq(ShorterCharWidth)) {
+ if (Shorter.size() + nullByte >= Longer.size())
+ break;
+ if (Longer[Shorter.size() + nullByte])
+ return false;
+ }
+
+ // Otherwise, they're potentially overlapping if and only if the overlapping
+ // region is the same.
+ return Shorter == Longer.take_front(Shorter.size());
}
static bool IsWeakLValue(const LValue &Value) {
@@ -8573,7 +8651,10 @@ class LValueExprEvaluator
bool VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *E);
bool VisitCompoundLiteralExpr(const CompoundLiteralExpr *E);
bool VisitMemberExpr(const MemberExpr *E);
- bool VisitStringLiteral(const StringLiteral *E) { return Success(E); }
+ bool VisitStringLiteral(const StringLiteral *E) {
+ uint64_t Version = Info.getASTContext().getNextStringLiteralVersion();
+ return Success(APValue::LValueBase(E, 0, static_cast<unsigned>(Version)));
+ }
bool VisitObjCEncodeExpr(const ObjCEncodeExpr *E) { return Success(E); }
bool VisitCXXTypeidExpr(const CXXTypeidExpr *E);
bool VisitCXXUuidofExpr(const CXXUuidofExpr *E);
@@ -9639,7 +9720,7 @@ static bool isOneByteCharacterType(QualType T) {
bool PointerExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
unsigned BuiltinOp) {
- if (IsNoOpCall(E))
+ if (IsOpaqueConstantCall(E))
return Success(E);
switch (BuiltinOp) {
@@ -13889,13 +13970,21 @@ EvaluateComparisonBinaryOperator(EvalInfo &Info, const BinaryOperator *E,
(!RHSValue.Base && !RHSValue.Offset.isZero()))
return DiagComparison(diag::note_constexpr_pointer_constant_comparison,
!RHSValue.Base);
- // It's implementation-defined whether distinct literals will have
- // distinct addresses. In clang, the result of such a comparison is
- // unspecified, so it is not a constant expression. However, we do know
- // that the address of a literal will be non-null.
- if ((IsLiteralLValue(LHSValue) || IsLiteralLValue(RHSValue)) &&
- LHSValue.Base && RHSValue.Base)
+ // C++2c [intro.object]/10:
+ // Two objects [...] may have the same address if [...] they are both
+ // potentially non-unique objects.
+ // C++2c [intro.object]/9:
+ // An object is potentially non-unique if it is a string literal object,
+ // the backing array of an initializer list, or a subobject thereof.
+ //
+ // This makes the comparison result unspecified, so it's not a constant
+ // expression.
+ //
+ // TODO: Do we need to handle the initializer list case here?
+ if (ArePotentiallyOverlappingStringLiterals(Info, LHSValue, RHSValue))
return DiagComparison(diag::note_constexpr_literal_comparison);
+ if (IsOpaqueConstantCall(LHSValue) || IsOpaqueConstantCall(RHSValue))
+ return DiagComparison(diag::note_constexpr_opaque_call_comparison);
// We can't tell whether weak symbols will end up pointing to the same
// object.
if (IsWeakLValue(LHSValue) || IsWeakLValue(RHSValue))
diff --git a/clang/test/AST/ByteCode/builtin-functions.cpp b/clang/test/AST/ByteCode/builtin-functions.cpp
index 9fd5eae67a21f6..3de0baaade50d0 100644
--- a/clang/test/AST/ByteCode/builtin-functions.cpp
+++ b/clang/test/AST/ByteCode/builtin-functions.cpp
@@ -966,7 +966,8 @@ namespace shufflevector {
namespace FunctionStart {
void a(void) {}
static_assert(__builtin_function_start(a) == a, ""); // both-error {{not an integral constant expression}} \
- // both-note {{comparison of addresses of literals has unspecified value}}
+ // ref-note {{comparison against opaque constant has unspecified value}} \
+ // expected-note {{comparison of addresses of literals has unspecified value}}
}
namespace BuiltinInImplicitCtor {
diff --git a/clang/test/AST/ByteCode/cxx20.cpp b/clang/test/AST/ByteCode/cxx20.cpp
index 9bbc3dbe0073d3..f2a87bae55b13e 100644
--- a/clang/test/AST/ByteCode/cxx20.cpp
+++ b/clang/test/AST/ByteCode/cxx20.cpp
@@ -99,7 +99,7 @@ constexpr int f() {
static_assert(f());
#endif
-/// Distinct literals have disctinct addresses.
+/// Distinct literals have distinct addresses.
/// see https://github.com/llvm/llvm-project/issues/58754
constexpr auto foo(const char *p) { return p; }
constexpr auto p1 = "test1";
@@ -108,22 +108,16 @@ constexpr auto p2 = "test2";
constexpr bool b1 = foo(p1) == foo(p1);
static_assert(b1);
-constexpr bool b2 = foo(p1) == foo(p2); // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{comparison of addresses of literals}} \
- // ref-note {{declared here}}
-static_assert(!b2); // ref-error {{not an integral constant expression}} \
- // ref-note {{not a constant expression}}
+constexpr bool b2 = foo(p1) == foo(p2);
+static_assert(!b2);
constexpr auto name1() { return "name1"; }
constexpr auto name2() { return "name2"; }
-constexpr auto b3 = name1() == name1();
-static_assert(b3);
-constexpr auto b4 = name1() == name2(); // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{has unspecified value}} \
- // ref-note {{declared here}}
-static_assert(!b4); // ref-error {{not an integral constant expression}} \
- // ref-note {{not a constant expression}}
+constexpr auto b3 = name1() == name1(); // ref-error {{must be initialized by a constant expression}} \
+ // ref-note {{comparison of addresses of literals}}
+constexpr auto b4 = name1() == name2();
+static_assert(!b4);
namespace UninitializedFields {
class A {
diff --git a/clang/test/SemaCXX/builtins.cpp b/clang/test/SemaCXX/builtins.cpp
index f47ed3a1f7ebfc..7df405f0662a15 100644
--- a/clang/test/SemaCXX/builtins.cpp
+++ b/clang/test/SemaCXX/builtins.cpp
@@ -47,7 +47,7 @@ void a(void) {}
int n;
void *p = __builtin_function_start(n); // expected-error {{argument must be a function}}
static_assert(__builtin_function_start(a) == a, ""); // expected-error {{static assertion expression is not an integral constant expression}}
-// expected-note@-1 {{comparison of addresses of literals has unspecified value}}
+// expected-note@-1 {{comparison against opaque constant has unspecified value}}
} // namespace function_start
void no_ms_builtins() {
diff --git a/clang/test/SemaCXX/constant-expression-cxx11.cpp b/clang/test/SemaCXX/constant-expression-cxx11.cpp
index 44ef540f41fa8c..3d56c3e351f9bd 100644
--- a/clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -2,6 +2,10 @@
// RUN: %clang_cc1 -std=c++20 -isystem %S/Inputs -fsyntax-only -verify=expected,cxx11_20,cxx20_23,pre-cxx23 -triple x86_64-linux -Wno-string-plus-int -Wno-pointer-arith -Wno-zero-length-array -Wno-c99-designator -fcxx-exceptions -pedantic %s -Wno-comment -Wno-tautological-pointer-compare -Wno-bool-conversion
// RUN: %clang_cc1 -std=c++11 -isystem %S/Inputs -fsyntax-only -verify=expected,cxx11_20,cxx11,pre-cxx23 -triple x86_64-linux -Wno-string-plus-int -Wno-pointer-arith -Wno-zero-length-array -Wno-c99-designator -fcxx-exceptions -pedantic %s -Wno-comment -Wno-tautological-pointer-compare -Wno-bool-conversion
+// This macro forces its argument to be constant-folded, even if it's not
+// otherwise a constant expression.
+#define fold(x) (__builtin_constant_p(x) ? (x) : (x))
+
namespace StaticAssertFoldTest {
int x;
@@ -358,11 +362,31 @@ struct Str {
extern char externalvar[];
constexpr bool constaddress = (void *)externalvar == (void *)0x4000UL; // expected-error {{must be initialized by a constant expression}} expected-note {{reinterpret_cast}}
-constexpr bool litaddress = "foo" == "foo"; // expected-error {{must be initialized by a constant expression}}
-// expected-note@-1 {{comparison of addresses of literals has unspecified value}}
-// cxx20_23-warning@-2 {{comparison between two arrays is deprecated}}
static_assert(0 != "foo", "");
+// OK: These string literals cannot possibly overlap.
+static_assert(+"foo" != +"bar", "");
+static_assert("xfoo" + 1 != "yfoo" + 1, "");
+static_assert(+"foot" != +"foo", "");
+static_assert(+"foo\0bar" != +"foo\0baz", "");
+
+// These can't overlap because the null terminator for UTF-16 is two bytes wide.
+static_assert(fold((const char*)u"A" != (const char*)"\0A\0x"), "");
+static_assert(fold((const char*)u"A" != (const char*)"A\0\0x"), "");
+
+// These strings may overlap, and so the result of the comparison is unknown.
+constexpr bool may_overlap_1 = +"foo" == +"foo"; // expected-error {{}} expected-note {{addresses of literals}}
+constexpr bool may_overlap_2 = +"foo" == +"foo\0bar"; // expected-error {{}} expected-note {{addresses of literals}}
+constexpr bool may_overlap_3 = +"foo" == "bar\0foo" + 4; // expected-error {{}} expected-note {{addresses of literals}}
+constexpr bool may_overlap_4 = "xfoo" + 1 == "xfoo" + 1; // expected-error {{}} expected-note {{addresses of literals}}
+
+// These may overlap even though they have different encodings.
+// One of these two comparisons is non-constant, but due to endianness we don't
+// know which one.
+constexpr bool may_overlap_different_encoding[] =
+ {fold((const char*)u"A" != (const char*)"xA\0\0\0x" + 1), fold((const char*)u"A" != (const char*)"x\0A\0\0x" + 1)};
+ // expected-error@-2 {{}} expected-note@-1 {{addresses of literals}}
+
}
namespace MaterializeTemporary {
@@ -1543,16 +1567,10 @@ namespace MutableMembers {
namespace Fold {
- // This macro forces its argument to be constant-folded, even if it's not
- // otherwise a constant expression.
- #define fold(x) (__builtin_constant_p(x) ? (x) : (x))
-
constexpr int n = (long)(char*)123; // expected-error {{constant expression}} expected-note {{reinterpret_cast}}
constexpr int m = fold((long)(char*)123); // ok
static_assert(m == 123, "");
- #undef fold
-
}
namespace DR1454 {
diff --git a/clang/test/SemaCXX/constant-expression-cxx14.cpp b/clang/test/SemaCXX/constant-expression-cxx14.cpp
index 70ab5dcd357c1c..01326aed8be63d 100644
--- a/clang/test/SemaCXX/constant-expression-cxx14.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx14.cpp
@@ -1306,3 +1306,18 @@ constexpr int field(int a) {
static_assert(field(3), ""); // expected-error {{constant expression}} \
// expected-note {{in call to 'field(3)'}}
}
+
+namespace literal_comparison {
+
+constexpr bool different_in_loop(bool b = false) {
+ if (b) return false;
+
+ const char *p[2] = {};
+ for (const char *&r : p)
+ r = "hello";
+ return p[0] == p[1]; // expected-note {{addresses of literals}}
+}
+constexpr bool check = different_in_loop();
+ // expected-error@-1 {{}} expected-note@-1 {{in call}}
+
+}
\ No newline at end of file
|
shafik
requested review from
cor3ntin,
tbaederr,
AaronBallman,
erichkeane and
Endilll
zygoloid
changed the title
| constexpr bool different_in_loop(bool b = false) { | ||
| if (b) return false; |
There was a problem hiding this comment.
What's the purpose of this 'b' parameter?
There was a problem hiding this comment.
Just to suppress the "constexpr function never produces a constant expression" diagnostic.
| @@ -96,6 +96,8 @@ def note_constexpr_pointer_constant_comparison : Note< | |||
| "at runtime">; | |||
| def note_constexpr_literal_comparison : Note< | |||
| "comparison of addresses of literals has unspecified value">; | |||
| def note_constexpr_opaque_call_comparison : Note< | |||
| "comparison against opaque constant has unspecified value">; | |||
There was a problem hiding this comment.
I can't imagine a normal person knows what's meant by "opaque constant"
There was a problem hiding this comment.
Yeah, opaque constant is pretty opaque, but I'm struggling to think of a better way to phrase this. It only triggers when calling a builtin which forms an address constant.
Mayyyybbbe: comparison against an anonymous object with a constant address has unspecified value
? (or maybe 'unnamed' instead of 'anonymous')
There was a problem hiding this comment.
It's not necessarily anonymous, or an object. For example, we get one of these constants from __builtin_start_address(function), where the returned address is often the address of the function (though in some cases it might be a different code address -- and we don't know).
Added the actual pointer value to the diagnostic:
note: comparison against opaque constant address '&__builtin_function_start(a)' has unspecified value
Hopefully that makes it a bit clearer what the problem is :)
| @@ -96,6 +96,8 @@ def note_constexpr_pointer_constant_comparison : Note< | |||
| "at runtime">; | |||
| def note_constexpr_literal_comparison : Note< | |||
| "comparison of addresses of literals has unspecified value">; | |||
| def note_constexpr_opaque_call_comparison : Note< | |||
| "comparison against opaque constant has unspecified value">; | |||
There was a problem hiding this comment.
Yeah, opaque constant is pretty opaque, but I'm struggling to think of a better way to phrase this. It only triggers when calling a builtin which forms an address constant.
Mayyyybbbe: comparison against an anonymous object with a constant address has unspecified value
? (or maybe 'unnamed' instead of 'anonymous')
| return (Builtin == Builtin::BI__builtin___CFStringMakeConstantString || | ||
| Builtin == Builtin::BI__builtin___NSStringMakeConstantString || | ||
| Builtin == Builtin::BI__builtin_ptrauth_sign_constant || |
There was a problem hiding this comment.
Can we get test coverage for these cases?
clang/include/clang/AST/ASTContext.h
Outdated
| /// This is used to distinguish between repeated evaluations of the same | ||
| /// string literal. | ||
| /// | ||
| /// TODO: Ensure version numbers don't collide when deserialized. |
There was a problem hiding this comment.
Should we add a test case involving modules to make sure that behavior is reasonable?
clang/include/clang/AST/ASTContext.h
Outdated
| /// This is used to distinguish between repeated evaluations of the same | ||
| /// string literal. | ||
| /// | ||
| /// TODO: Ensure version numbers don't collide when deserialized. |
There was a problem hiding this comment.
This seems troublesome. The modules case here is somewhat concerning to me. I wonder if we could figure out some way to encode/serialize the "how we got here" via source information to provide the number? I realize I'm being a little hand-wavy, but perhaps back to the 'line + col that caused this evaluation'?
There was a problem hiding this comment.
It's not too hard to contrive a situation where there's an observable misbehavior. For example:
export module A;
export constexpr const char *f() { return "hello"; }export module B;
import A;
export constexpr const char *p = f();export module C;
import A;
export constexpr const char *q = f();import B;
import C;
// Should be non-constant, but these both have version 0 and
// the same `StringLiteral`, so presumably passes.
static_assert(p == q);
// Might return false at runtime in practice.
bool g() { return p == q; }It's a somewhat low-probability misbehavior at least -- you need the version numbers to collide across modules. (They can also collide with this patch after unsigned overflow, but I don't think we need to worry about that.)
Maybe this has too high a risk of collisions still, but we could initialize this counter to a hash of the current module name instead of 0. :)
If we want to do this properly, we could track the greatest version used in each module, and offset the versions in each module by the sum of the versions of earlier-loaded modules (and likewise offset the version for the current file by the sum across all modules). Alternatively we could try to recompute versions at import time, building a map of {module ID, imported version} -> new version. But it's not quite that simple, because given
export module A;
export constexpr const char *p = "foo";export module B;
import A;
export constexpr const char *q = p;we need the constant value of p imported from A to be the same as the constant value of q imported from B.
... So in the former case we would need to do proper remapping not just adding an offset, and in the latter case we'd need to track which module ID the imported version originally came from. Both options seem expensive.
Maybe we can think of a better way? (Initializing the counter to a hash is sounding a bit better now, huh?)
There was a problem hiding this comment.
Maybe we can think of a better way? (Initializing the counter to a hash is sounding a bit better now, huh?)
Thats probably at least as good as my line/column/file info suggestion, but based on the above/thinking further, I think anything besides "start at the same number for all" is perfectly acceptable.
Co-authored-by: Timm Baeder <[email protected]>
Co-authored-by: Aaron Ballman <[email protected]>
Tweak diagnostic to avoid suggesting this is a rule from a language standard.
clang/include/clang/AST/ASTContext.h
Outdated
| /// This is used to distinguish between repeated evaluations of the same | ||
| /// string literal. | ||
| /// | ||
| /// TODO: Ensure version numbers don't collide when deserialized. |
There was a problem hiding this comment.
It's not too hard to contrive a situation where there's an observable misbehavior. For example:
export module A;
export constexpr const char *f() { return "hello"; }export module B;
import A;
export constexpr const char *p = f();export module C;
import A;
export constexpr const char *q = f();import B;
import C;
// Should be non-constant, but these both have version 0 and
// the same `StringLiteral`, so presumably passes.
static_assert(p == q);
// Might return false at runtime in practice.
bool g() { return p == q; }It's a somewhat low-probability misbehavior at least -- you need the version numbers to collide across modules. (They can also collide with this patch after unsigned overflow, but I don't think we need to worry about that.)
Maybe this has too high a risk of collisions still, but we could initialize this counter to a hash of the current module name instead of 0. :)
If we want to do this properly, we could track the greatest version used in each module, and offset the versions in each module by the sum of the versions of earlier-loaded modules (and likewise offset the version for the current file by the sum across all modules). Alternatively we could try to recompute versions at import time, building a map of {module ID, imported version} -> new version. But it's not quite that simple, because given
export module A;
export constexpr const char *p = "foo";export module B;
import A;
export constexpr const char *q = p;we need the constant value of p imported from A to be the same as the constant value of q imported from B.
... So in the former case we would need to do proper remapping not just adding an offset, and in the latter case we'd need to track which module ID the imported version originally came from. Both options seem expensive.
Maybe we can think of a better way? (Initializing the counter to a hash is sounding a bit better now, huh?)
| @@ -96,6 +96,8 @@ def note_constexpr_pointer_constant_comparison : Note< | |||
| "at runtime">; | |||
| def note_constexpr_literal_comparison : Note< | |||
| "comparison of addresses of literals has unspecified value">; | |||
| def note_constexpr_opaque_call_comparison : Note< | |||
| "comparison against opaque constant has unspecified value">; | |||
There was a problem hiding this comment.
It's not necessarily anonymous, or an object. For example, we get one of these constants from __builtin_start_address(function), where the returned address is often the address of the function (though in some cases it might be a different code address -- and we don't know).
Added the actual pointer value to the diagnostic:
note: comparison against opaque constant address '&__builtin_function_start(a)' has unspecified value
Hopefully that makes it a bit clearer what the problem is :)
| return (Builtin == Builtin::BI__builtin___CFStringMakeConstantString || | ||
| Builtin == Builtin::BI__builtin___NSStringMakeConstantString || | ||
| Builtin == Builtin::BI__builtin_ptrauth_sign_constant || |
clang/docs/ReleaseNotes.rst
Outdated
| - During constant evaluation, comparisons between different evaluations of the | ||
| same string literal are now correctly treated as non-constant, and comparisons | ||
| between string literals that cannot possibly overlap in memory are now treated | ||
| as constant. |
There was a problem hiding this comment.
Could you mention the CWG issue here as well?
There was a problem hiding this comment.
I'm happy once SOME level of solution for the index-starting-value is done (NextStringLiteralVersion=rand() 🤡). I think the avoided collisions is worth a touch of effort, but not much more.
I think there is some concern from others about opaque that should probably be settled on, but I'm happy with whatever all of you come up with, even if it is opaque.
Um. So. I added a test for this before I started working on a fix, and ... the test passes. It turns out that the existing implementation already works: while we do have some support for importing computed constant values, we always recompute constants locally before using them as inputs to further local constant evaluations, so we produce a local numbering even for what appear to be imported constants. We would need to do something different here if we ever change that, but we now have test coverage for it, so we should notice.
I hope I've addressed that by including the value of the pointer in the diagnostic. |
clang/lib/AST/ExprConstant.cpp
Outdated
| @@ -8573,7 +8661,10 @@ class LValueExprEvaluator | |||
| bool VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *E); | |||
| bool VisitCompoundLiteralExpr(const CompoundLiteralExpr *E); | |||
| bool VisitMemberExpr(const MemberExpr *E); | |||
| bool VisitStringLiteral(const StringLiteral *E) { return Success(E); } | |||
| bool VisitStringLiteral(const StringLiteral *E) { | |||
| uint64_t Version = Info.getASTContext().getNextStringLiteralVersion(); | |||
There was a problem hiding this comment.
getNextStringLiteralVersion() returns unsigned, not uint64_t?
There was a problem hiding this comment.
Fixed. (Sorry, just some junk left behind from a previous approach.)
| constexpr auto b3 = name1() == name1(); // ref-error {{must be initialized by a constant expression}} \ | ||
| // ref-note {{comparison of addresses of literals}} | ||
| constexpr auto b4 = name1() == name2(); | ||
| static_assert(!b4); |
There was a problem hiding this comment.
Just from looking at these few lines, I don't understand why b3 warns but b4 doesn't. They both compare the address of literals.
The bytecode interpreter simply creates global variables for string literals, so b3 here is simply true, like it was in the current interpreter before. Is this behavior wrong now?
There was a problem hiding this comment.
The C++ model in general is that each evaluation of a string literal expression can produce a distinct object, and that these objects can fully or partially overlap each other in memory when they have suitable values; that's the model that this PR is implementing.
b3 is non-constant because each call to name1() can produce a distinct value, so the comparison result is unspecified. b4 is constant and false under this PR because the values returned by name1() and name2() cannot possibly be the same -- those two string literal evaluations can't produce the same value because the strings have different contents.
|
I worry about string literals in vague-linkage entities, because the "version" of the string literal which is referred to from an inline-function/variable at runtime may not actually be the same version of the string literal seen in the current compilation -- at runtime we may in fact be using a different version of the symbol from another TU. So, I think even a comparison of a string literal address with "the same version" cannot be guaranteed to be consistent with runtime. Of course, this is already an issue, even without constexpr evaluation -- we make the same assumptions in the optimizer. So, maybe it's fine to continue ignoring this problem? |
Co-authored-by: Timm Baeder <[email protected]>
I think string literals in inline functions shouldn't be a problem by themselves -- each evaluation of the string literal is allowed to produce a different object, and each constant evaluation will produce a different version. But I think there is a problem in general for string literals that appear in the evaluated value of a vague linkage constant. For example: constexpr inline const char *s = "foo";... is required by the standard to have the same value across all translation units, but we currently don't ensure that's the case. I think that's a separable issue from this PR, but it's clearly related -- though I think probably the right answer here is for the ABI to assign a mangling to the string literal in this case. See itanium-cxx-abi/cxx-abi#78 and #57957. |
Yep. I brought it up mainly in case consideration of that set of issues might necessitate design changes here. It sounds like that's not the case though, so I'm happy for it to not be a blocker for this PR. |
|
I believe all the feedback has been addressed, and plan to merge in 24 hours if there are no further comments. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/6480 Here is the relevant piece of the build log for the reference |
|
Hi, We are seeing build error on fmtlib project after this patch is landed. The error message from the clang is: Full build log can be seen from: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8735688601552461713/+/u/build/ninja/stdout We are still looking into it to understand if it is a compiler bug or an issue with fmtlib's implementation. |
|
That backtrace doesn't match the fmtlib code available from their official repository. Where can I find the code you're building? |
|
I found this: https://fuchsia.googlesource.com/third_party/github.com/fmtlib/fmt/+/refs/heads/upstream/main/include/fmt/base.h#2664 Note that this is passing So yes, this fmtlib code is wrong -- it's incorrectly assuming that a string literal will always evaluate to the same value, which is not true at runtime or (after this Clang change) at compile time. The correct thing to do would be what is done a few lines below in the next constructor: convert the |
|
I've filed a fmtlib bug: fmtlib/fmt#4177 |
…parisons in constant evaluation (llvm#109208) Track the identity of each string literal object produced by evaluation with a global version number. Accept comparisons between literals of the same version, and between literals of different versions that cannot possibly be placed in overlapping storage. Treat the remaining comparisons as non-constant. --------- Co-authored-by: Timm Baeder <[email protected]> Co-authored-by: Aaron Ballman <[email protected]>
Track the identity of each string literal object produced by evaluation
with a global version number. Accept comparisons between literals of the
same version, and between literals of different versions that cannot
possibly be placed in overlapping storage. Treat the remaining
comparisons as non-constant.