<div dir="ltr">Thanks, fixed in r371557.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 9 Sep 2019 at 19:12, Michael Spencer via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div><div dir="ltr" class="gmail-m_-7432941496926789363gmail_signature">On Fri, Apr 26, 2019 at 7:56 PM Richard Smith via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: rsmith<br>
Date: Fri Apr 26 19:58:17 2019<br>
New Revision: 359367<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=359367&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=359367&view=rev</a><br>
Log:<br>
Reinstate r359059, reverted in r359361, with a fix to properly prevent<br>
us emitting the operand of __builtin_constant_p if it has side-effects.<br>
<br>
Original commit message:<br>
<br>
Fix interactions between __builtin_constant_p and constexpr to match<br>
current trunk GCC.<br>
<br>
GCC permits information from outside the operand of<br>
__builtin_constant_p (but in the same constant evaluation context) to be<br>
used within that operand; clang now does so too. A few other minor<br>
deviations from GCC's behavior showed up in my testing and are also<br>
fixed (matching GCC):<br>
* Clang now supports nullptr_t as the argument type for<br>
__builtin_constant_p<br>
* Clang now returns true from __builtin_constant_p if called with a<br>
null pointer<br>
* Clang now returns true from __builtin_constant_p if called with an<br>
integer cast to pointer type<br>
<br>
Added:<br>
cfe/trunk/test/SemaCXX/builtin-constant-p.cpp<br>
Modified:<br>
cfe/trunk/lib/AST/ExprConstant.cpp<br>
cfe/trunk/lib/CodeGen/CGBuiltin.cpp<br>
cfe/trunk/lib/Sema/SemaChecking.cpp<br>
cfe/trunk/test/CodeGen/builtin-constant-p.c<br>
cfe/trunk/test/SemaCXX/enable_if.cpp<br>
<br>
Modified: cfe/trunk/lib/AST/ExprConstant.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=359367&r1=359366&r2=359367&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=359367&r1=359366&r2=359367&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)<br>
+++ cfe/trunk/lib/AST/ExprConstant.cpp Fri Apr 26 19:58:17 2019<br>
@@ -7801,19 +7801,33 @@ EvaluateBuiltinClassifyType(const CallEx<br>
}<br>
<br>
/// EvaluateBuiltinConstantPForLValue - Determine the result of<br>
-/// __builtin_constant_p when applied to the given lvalue.<br>
+/// __builtin_constant_p when applied to the given pointer.<br>
///<br>
-/// An lvalue is only "constant" if it is a pointer or reference to the first<br>
-/// character of a string literal.<br>
-template<typename LValue><br>
-static bool EvaluateBuiltinConstantPForLValue(const LValue &LV) {<br>
- const Expr *E = LV.getLValueBase().template dyn_cast<const Expr*>();<br>
- return E && isa<StringLiteral>(E) && LV.getLValueOffset().isZero();<br>
+/// A pointer is only "constant" if it is null (or a pointer cast to integer)<br>
+/// or it points to the first character of a string literal.<br>
+static bool EvaluateBuiltinConstantPForLValue(const APValue &LV) {<br>
+ APValue::LValueBase Base = LV.getLValueBase();<br>
+ if (Base.isNull()) {<br>
+ // A null base is acceptable.<br>
+ return true;<br>
+ } else if (const Expr *E = Base.dyn_cast<const Expr *>()) {<br>
+ if (!isa<StringLiteral>(E))<br>
+ return false;<br>
+ return LV.getLValueOffset().isZero();<br>
+ } else {<br>
+ // Any other base is not constant enough for GCC.<br>
+ return false;<br>
+ }<br>
}<br>
<br>
/// EvaluateBuiltinConstantP - Evaluate __builtin_constant_p as similarly to<br>
/// GCC as we can manage.<br>
-static bool EvaluateBuiltinConstantP(ASTContext &Ctx, const Expr *Arg) {<br>
+static bool EvaluateBuiltinConstantP(EvalInfo &Info, const Expr *Arg) {<br>
+ // Constant-folding is always enabled for the operand of __builtin_constant_p<br>
+ // (even when the enclosing evaluation context otherwise requires a strict<br>
+ // language-specific constant expression).<br>
+ FoldConstant Fold(Info, true);<br>
+<br>
QualType ArgType = Arg->getType();<br>
<br>
// __builtin_constant_p always has one operand. The rules which gcc follows<br>
@@ -7821,34 +7835,30 @@ static bool EvaluateBuiltinConstantP(AST<br>
//<br>
// - If the operand is of integral, floating, complex or enumeration type,<br>
// and can be folded to a known value of that type, it returns 1.<br>
- // - If the operand and can be folded to a pointer to the first character<br>
- // of a string literal (or such a pointer cast to an integral type), it<br>
- // returns 1.<br>
+ // - If the operand can be folded to a pointer to the first character<br>
+ // of a string literal (or such a pointer cast to an integral type)<br>
+ // or to a null pointer or an integer cast to a pointer, it returns 1.<br>
//<br>
// Otherwise, it returns 0.<br>
//<br>
// FIXME: GCC also intends to return 1 for literals of aggregate types, but<br>
- // its support for this does not currently work.<br>
- if (ArgType->isIntegralOrEnumerationType()) {<br>
- Expr::EvalResult Result;<br>
- if (!Arg->EvaluateAsRValue(Result, Ctx) || Result.HasSideEffects)<br>
+ // its support for this did not work prior to GCC 9 and is not yet well<br>
+ // understood.<br>
+ if (ArgType->isIntegralOrEnumerationType() || ArgType->isFloatingType() ||<br>
+ ArgType->isAnyComplexType() || ArgType->isPointerType() ||<br>
+ ArgType->isNullPtrType()) {<br>
+ APValue V;<br>
+ if (!::EvaluateAsRValue(Info, Arg, V)) {<br>
+ Fold.keepDiagnostics();<br>
return false;<br>
+ }<br>
<br>
- APValue &V = Result.Val;<br>
- if (V.getKind() == APValue::Int)<br>
- return true;<br>
+ // For a pointer (possibly cast to integer), there are special rules.<br>
if (V.getKind() == APValue::LValue)<br>
return EvaluateBuiltinConstantPForLValue(V);<br>
- } else if (ArgType->isFloatingType() || ArgType->isAnyComplexType()) {<br>
- return Arg->isEvaluatable(Ctx);<br>
- } else if (ArgType->isPointerType() || Arg->isGLValue()) {<br>
- LValue LV;<br>
- Expr::EvalStatus Status;<br>
- EvalInfo Info(Ctx, Status, EvalInfo::EM_ConstantFold);<br>
- if ((Arg->isGLValue() ? EvaluateLValue(Arg, LV, Info)<br>
- : EvaluatePointer(Arg, LV, Info)) &&<br>
- !Status.HasSideEffects)<br>
- return EvaluateBuiltinConstantPForLValue(LV);<br>
+<br>
+ // Otherwise, any constant value is good enough.<br>
+ return V.getKind() != APValue::Uninitialized;<br>
}<br>
<br>
// Anything else isn't considered to be sufficiently constant.<br>
@@ -8258,18 +8268,15 @@ bool IntExprEvaluator::VisitBuiltinCallE<br>
}<br>
<br>
case Builtin::BI__builtin_constant_p: {<br>
- auto Arg = E->getArg(0);<br>
- if (EvaluateBuiltinConstantP(Info.Ctx, Arg))<br>
+ const Expr *Arg = E->getArg(0);<br>
+ if (EvaluateBuiltinConstantP(Info, Arg))<br>
return Success(true, E);<br>
- auto ArgTy = Arg->IgnoreImplicit()->getType();<br>
- if (!Info.InConstantContext && !Arg->HasSideEffects(Info.Ctx) &&<br>
- !ArgTy->isAggregateType() && !ArgTy->isPointerType()) {<br>
- // We can delay calculation of __builtin_constant_p until after<br>
- // inlining. Note: This diagnostic won't be shown to the user.<br>
+ else if (Info.InConstantContext)<br>
+ return Success(false, E);<br>
+ else {<br>
Info.FFDiag(E, diag::note_invalid_subexpr_in_const_expr);<br>
return false;<br>
}<br>
- return Success(false, E);<br>
}<br>
<br>
case Builtin::BI__builtin_is_constant_evaluated:<br>
<br>
Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=359367&r1=359366&r2=359367&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=359367&r1=359366&r2=359367&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Fri Apr 26 19:58:17 2019<br>
@@ -2027,8 +2027,17 @@ RValue CodeGenFunction::EmitBuiltinExpr(<br>
<br>
const Expr *Arg = E->getArg(0);<br>
QualType ArgType = Arg->getType();<br>
- if (!hasScalarEvaluationKind(ArgType) || ArgType->isFunctionType())<br>
- // We can only reason about scalar types.<br>
+ // FIXME: The allowance for Obj-C pointers and block pointers is historical<br>
+ // and likely a mistake.<br>
+ if (!ArgType->isIntegralOrEnumerationType() && !ArgType->isFloatingType() &&<br>
+ !ArgType->isObjCObjectPointerType() && !ArgType->isBlockPointerType())<br>
+ // Per the GCC documentation, only numeric constants are recognized after<br>
+ // inlining.<br>
+ return RValue::get(ConstantInt::get(ResultType, 0));<br>
+<br>
+ if (Arg->HasSideEffects(getContext()))<br>
+ // The argument is unevaluated, so be conservative if it might have<br>
+ // side-effects.<br>
return RValue::get(ConstantInt::get(ResultType, 0));<br>
<br>
Value *ArgValue = EmitScalarExpr(Arg);<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaChecking.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=359367&r1=359366&r2=359367&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=359367&r1=359366&r2=359367&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Apr 26 19:58:17 2019<br>
@@ -1199,10 +1199,14 @@ Sema::CheckBuiltinFunctionCall(FunctionD<br>
if (checkArgCount(*this, TheCall, 1)) return true;<br>
TheCall->setType(Context.IntTy);<br>
break;<br>
- case Builtin::BI__builtin_constant_p:<br>
+ case Builtin::BI__builtin_constant_p: {<br>
if (checkArgCount(*this, TheCall, 1)) return true;<br>
+ ExprResult Arg = DefaultFunctionArrayLvalueConversion(TheCall->getArg(0));<br>
+ if (Arg.isInvalid()) return true;<br>
+ TheCall->setArg(0, Arg.get());<br>
TheCall->setType(Context.IntTy);<br>
break;<br>
+ }<br>
case Builtin::BI__builtin_launder:<br>
return SemaBuiltinLaunder(*this, TheCall);<br>
case Builtin::BI__sync_fetch_and_add:<br>
<br>
Modified: cfe/trunk/test/CodeGen/builtin-constant-p.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/builtin-constant-p.c?rev=359367&r1=359366&r2=359367&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/builtin-constant-p.c?rev=359367&r1=359366&r2=359367&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGen/builtin-constant-p.c (original)<br>
+++ cfe/trunk/test/CodeGen/builtin-constant-p.c Fri Apr 26 19:58:17 2019<br>
@@ -177,3 +177,11 @@ void test17() {<br>
// CHECK: call void asm sideeffect "", {{.*}}(i32 -1) <br>
__asm__ __volatile__("" :: "n"( (__builtin_constant_p(test17_v) || 0) ? 1 : -1));<br>
}<br>
+<br>
+int test18_f();<br>
+// CHECK: define void @test18<br>
+// CHECK-NOT: call {{.*}}test18_f<br>
+void test18() {<br>
+ int a, b;<br>
+ (void)__builtin_constant_p((a = b, test18_f()));<br>
+}<br>
<br>
Added: cfe/trunk/test/SemaCXX/builtin-constant-p.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/builtin-constant-p.cpp?rev=359367&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/builtin-constant-p.cpp?rev=359367&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/builtin-constant-p.cpp (added)<br>
+++ cfe/trunk/test/SemaCXX/builtin-constant-p.cpp Fri Apr 26 19:58:17 2019<br>
@@ -0,0 +1,61 @@<br>
+// RUN: %clang_cc1 -std=c++17 -verify %s<br>
+<br>
+using intptr_t = __INTPTR_TYPE__;<br>
+<br>
+// Test interaction of constexpr and __builtin_constant_p.<br>
+<br>
+template<typename T> constexpr bool bcp(T t) {<br>
+ return __builtin_constant_p(t);<br>
+}<br>
+template<typename T> constexpr bool bcp_fold(T t) {<br>
+ return __builtin_constant_p(((void)(intptr_t)&t, t));<br>
+}<br>
+<br>
+constexpr intptr_t ensure_fold_is_generally_not_enabled = // expected-error {{constant expression}}<br>
+ (intptr_t)&ensure_fold_is_generally_not_enabled; // expected-note {{cast}}<br>
+<br>
+constexpr intptr_t ptr_to_int(const void *p) {<br>
+ return __builtin_constant_p(1) ? (intptr_t)p : (intptr_t)p;<br>
+}<br>
+<br>
+constexpr int *int_to_ptr(intptr_t n) {<br>
+ return __builtin_constant_p(1) ? (int*)n : (int*)n;<br>
+}<br>
+<br>
+int x;<br>
+<br>
+// Integer and floating point constants encountered during constant expression<br>
+// evaluation are considered constant. So is nullptr_t.<br>
+static_assert(bcp(1));<br>
+static_assert(bcp_fold(1));<br>
+static_assert(bcp(1.0));<br>
+static_assert(bcp_fold(1.0));<br>
+static_assert(bcp(nullptr));<br>
+static_assert(bcp_fold(nullptr));<br>
+<br>
+// Pointers to the start of strings are considered constant.<br>
+static_assert(bcp("foo"));<br>
+static_assert(bcp_fold("foo"));<br>
+<br>
+// Null pointers are considered constant.<br>
+static_assert(bcp<int*>(nullptr));<br>
+static_assert(bcp_fold<int*>(nullptr));<br>
+static_assert(bcp<const char*>(nullptr));<br>
+static_assert(bcp_fold<const char*>(nullptr));<br>
+<br>
+// Other pointers are not.<br>
+static_assert(!bcp(&x));<br>
+static_assert(!bcp_fold(&x));<br>
+<br>
+// Pointers cast to integers follow the rules for pointers.<br>
+static_assert(bcp(ptr_to_int("foo")));<br>
+static_assert(bcp_fold(ptr_to_int("foo")));<br>
+static_assert(!bcp(ptr_to_int(&x)));<br>
+static_assert(!bcp_fold(ptr_to_int(&x)));<br>
+<br>
+// Integers cast to pointers follow the integer rules.<br>
+static_assert(bcp(int_to_ptr(0)));<br>
+static_assert(bcp_fold(int_to_ptr(0)));<br>
+static_assert(bcp(int_to_ptr(123))); // GCC rejects these due to not recognizing<br>
+static_assert(bcp_fold(int_to_ptr(123))); // the bcp conditional in 'int_to_ptr' ...<br>
+static_assert(__builtin_constant_p((int*)123)); // ... but GCC accepts this<br>
<br>
Modified: cfe/trunk/test/SemaCXX/enable_if.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/enable_if.cpp?rev=359367&r1=359366&r2=359367&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/enable_if.cpp?rev=359367&r1=359366&r2=359367&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/enable_if.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/enable_if.cpp Fri Apr 26 19:58:17 2019<br>
@@ -522,3 +522,14 @@ void test() {<br>
InConstantContext::foo("abc");<br>
}<br>
} // namespace InConstantContext<br>
+<br>
+namespace StringLiteralDetector {<br>
+ void need_string_literal(const char *p) __attribute__((enable_if(__builtin_constant_p(p), "argument is not a string literal"))); // expected-note 2{{not a string literal}}<br>
+ void test(const char *unknown) {<br>
+ need_string_literal("foo");<br>
+ need_string_literal(unknown); // expected-error {{no matching function}}<br>
+ constexpr char str[] = "bar";<br>
+ need_string_literal(str); // expected-error {{no matching function}}<br>
+ }<br>
+}<br>
+<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a></blockquote><div><br></div><div>I bisected a failure to what I believe to be this commit which broke the following C testcase:</div><div><br></div><div>extern int a;<br><br>int i[] = {<br> __builtin_constant_p (a && 0) ? a && 0 : -1<br>};<br></div><div><br></div><div>In this specific context `__builtin_constant_p (a && 0)` resolves to true, but the `a && 0` in the true branch of the conditional is not treated as a constant. If you make it not an array clang behaves properly.</div><div><br></div><div>- Michael Spencer</div></div></div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>