r359367 - Reinstate r359059, reverted in r359361, with a fix to properly prevent

Michael Spencer via cfe-commits cfe-commits at lists.llvm.org
Mon Sep 9 19:12:06 PDT 2019


On Fri, Apr 26, 2019 at 7:56 PM Richard Smith via cfe-commits <
cfe-commits at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190909/9046ead9/attachment-0001.html>


More information about the cfe-commits mailing list