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