Use is.constant intrinsic for __builtin_constant_p

[void]

Summary

A __builtin_constant_p may end up with a constant after inlining. Use
the is.constant intrinsic if it's a variable that's in a context where it may
resolve to a constant, e.g., an argument to a function after inlining.

See: https://reviews.llvm.org/D4276

[void]

This is related to D4276, which I believe has been deleted. However, I believe that a modified version of that patch will work.

The idea of this patch is *not* to be fully compatible with gcc's implementation, but to expand clang's implementation of __builtin_constant_p without breaking existing code.

This has been tested by successfully building (and running) the Linux kernel for both ARM and X86.

[nickdesaulniers]

lib/Sema/SemaExpr.cpp

15604

../tools/clang/lib/Sema/SemaExpr.cpp:15619:13: warning: private field 'S' is not used [-Wunused-private-field]

Sema &S;
      ^

[void]

Removed unused field.

[nickdesaulniers]

lib/Sema/SemaExpr.cpp

15621–15623

std::for_each?

[nickdesaulniers]

lib/Sema/SemaExpr.cpp

15621–15623

Sorry, posted that comment as you uploaded the next version. This should be highlighting L15618 to L15620.

[void]

lib/Sema/SemaExpr.cpp

15621–15623

That's no used anywhere else in the source code. Maybe there's another mechanism that they use?

[nickdesaulniers]

lib/Sema/SemaExpr.cpp

15621–15623

Oh, looks like it was only added to C++17; I think Clang+LLVM use a lower language version. Did C++ stl really not have a way to apply the same function over an iterator until C++17?

[nickdesaulniers]

lib/Sema/SemaExpr.cpp

15621–15623

At least a range for would make this more concise:

for (auto& II : *E)
  Visit(II);

[rsmith]

Rather than adding a CanDelayEvaluation flag to call expressions, I think it would be substantially better to introduce a new Expr wrapper node for expressions that are required to be constant expressions. (That'd eventually also give us a place to cache the evaluated value of such an expression, where today we recompute it each time it's needed.) Essentially, you would add a ConstExpr node (or similar), teach IgnoreImplicit and friends to step over it, and add it in the places where we semantically require an expression to be a constant expression. This has various benefits: there are other upcoming language features (in C++20) that require this knowledge and don't necessarily have a CallExpr to tie it to, and it gives us a place to stash an evaluated value, and it gives IR generation a simple way to detect expressions that it can constant-evaluate rather than emitting.

When the evaluator steps into such an expression, it can track that it's done so, and ensure that it always produces a constant value for any __builtin_constant_p calls in that scope.

include/clang/AST/Expr.h

2290

It's not reasonable to make all CallExprs sizeof(void*) bytes larger for this. If we really need this, you can track it it in the CallExprBits on the base class. (But you'll also need to extend at least the Serialization and ASTImporter code to handle it, and probably TreeTransform too.)

But I don't think you need this.

lib/AST/ExprConstant.cpp

8162

Hmm, OK. Per https://bugs.llvm.org/show_bug.cgi?id=4898#c38, the correct check would be whether the reason we found the expression to be non-constant was that it tried to read the value of a local variable or function parameter. Eg, for:

void f(int x, int y) {
  bool k = __builtin_constant_p(3 + x < 5 * y);

... we should defer the __builtin_constant_p until after optimization. (And we should never do this if the expression has side-effects.) But given that your goal is merely to improve the status quo, not to exactly match GCC, this may be sufficient.

You should at least check that the variable is non-volatile, though. (More generally, check that Arg doesn't have side-effects.)

8162–8164

Your canDelayEvaluation check does not appear to cover several of the cases we'd need to check for here. Eg:

void f(int n) {
  enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error because a's value is non-constant

8164

It's not correct to return false here without producing a diagnostic explaining why the evaluation is non-constant. You can use Info.FFDiag() to produce a default "invalid subexpression" diagnostic, which is probably sufficient given that we won't actually show the diagnostic to the user in most cases.

lib/Sema/SemaExpr.cpp

5694

Why are you marking this? This isn't (necessarily) a context where a constant expression is required. (In C, the overall initializer for a global would be, but a local-scope compound literal would not.)

Also, this should be in the Build function, not in the ActOn function.

5799

Likewise here, I don't see the justification for this.

[nickdesaulniers]

test/Sema/builtins.c

125

such as

Otherwise it's possible to read this as the non-constants being resolved to a file scoped array.

[void]

In D52854#1255755, @rsmith wrote:

Essentially, you would add a ConstExpr node (or similar), teach IgnoreImplicit and friends to step over it

I've been trying to do this to no avail. Here's the code I was working with:

//===----------------------------------------------------------------------===//
// Wrapper Expressions.
//===----------------------------------------------------------------------===//

// ConstExpr is a wrapper class indicating that the expression must be constant.
// This is useful for something like __builtin_constant_p(), which in some
// contexts is required to be constant before generating LLVM IR.
template <typename T>
class ConstExpr : public Expr {
  T *E;

 public:
  ConstExpr(Expr *E) : E(E) {}

  T *getSubExpr() { return E; }
  const T *getSubExpr() const { return E; }

  static bool classof(const Stmt *T) {
    return T->getStmtClass() == ConstExprClass;
  }
};

I did it this way because otherwise I would have to forward all calls to individual expression, which is ugly and made me want to cry. The issue is getting the ConstExprClass variable. Those variables are automatically generated via macro magick, but those macros aren't set up to handle templates.

Do I need to mangle the macros to support templates, or is there another way to achieve this?

lib/AST/ExprConstant.cpp

8162

I had the side effect check in there before. I removed it for some reason. I'll re-add it.

lib/Sema/SemaExpr.cpp

5694

The name may be misleading. It's looking to see if it should mark whether it can delay evaluation or not. I'll change its name to be clearer.

5799

I can't find an equivalent "Build..." for InitList expressions. Could you point me to it?

[void]

@rsmith I've been implementing your suggestion, but it's quickly becoming ridiculous. Just to implement a ConstExpr wrapper class, I need to touch ~23 files, virtually all changes being boilerplate code to forward the action to the wrapped expression. And this is before I add the code in this patch that implements the feature. While it would be nice to use LLVM's type system to determine if an expression is expected to be constant, it appears that doing that is much worse than adding the information to the bits field you mentioned.

[efriedma]

I don't think that was the approach @rsmith was suggesting; you don't need to wrap every possible kind of expression. Rather, at the point where the expression is required to be constant, add a single expression which wraps the entire expression tree to indicate that. So for "constexpr int c = 10+2;", the expression tree for the initializer is something like ConstExpr(BinaryOperator(IntegerLiteral, IntegerLiteral)).

[void]

No, I understood that. But the issue is that you then need to specify this new expression class in over 23 different files: various macros, switch statements, etc.

[void]

Here's a WIP patch to show what I'm talking about.

example.diff16 KBDownload

[void]

I updated the patch that implements putting the "can delay evaluation" bit into the CallExprBitfields. PTAL.

include/clang/AST/Expr.h

 /// results in a function call. For example, CXXOperatorCallExpr is
 /// a subclass for overloaded operator calls that use operator syntax, e.g.,
 /// "str1 + str2" to resolve to a function call.
 class CallExpr : public Expr {
   enum { FN=0, PREARGS_START=1 };
   Stmt **SubExprs;
   unsigned NumArgs;
   SourceLocation RParenLoc;
 
   void updateDependenciesFromArg(Expr *Arg);
 
 protected:
   // These versions of the constructor are for derived classes.
   CallExpr(const ASTContext &C, StmtClass SC, Expr *fn,
            ArrayRef<Expr *> preargs, ArrayRef<Expr *> args, QualType t,
            ExprValueKind VK, SourceLocation rparenloc);
   CallExpr(const ASTContext &C, StmtClass SC, Expr *fn, ArrayRef<Expr *> args,
public:
   /// getBuiltinCallee - If this is a call to a builtin, return the builtin ID
   /// of the callee. If not, return 0.
   unsigned getBuiltinCallee() const;
 
   /// Returns \c true if this is a call to a builtin which does not
   /// evaluate side-effects within its arguments.
   bool isUnevaluatedBuiltinCall(const ASTContext &Ctx) const;
 
+  // Used by __builtin_constant_p() to indicate that its evaluation can be
+  // delayed until after inlining.
+  void setCanDelayEvaluation(bool V) { CallExprBits.CanDelayEvaluation = V; }
+  bool getCanDelayEvaluation() const { return CallExprBits.CanDelayEvaluation; }
+
   /// getCallReturnType - Get the return type of the call expr. This is not
   /// always the type of the expr itself, if the return type is a reference
   /// type.
   QualType getCallReturnType(const ASTContext &Ctx) const;
 
   SourceLocation getRParenLoc() const { return RParenLoc; }
   void setRParenLoc(SourceLocation L) { RParenLoc = L; }
 

include/clang/AST/Stmt.h

protected:
   };
 
   class CallExprBitfields {
     friend class CallExpr;
 
     unsigned : NumExprBits;
 
     unsigned NumPreArgs : 1;
+    unsigned CanDelayEvaluation : 1;
   };
 
   class ExprWithCleanupsBitfields {
     friend class ASTStmtReader; // deserialization
     friend class ExprWithCleanups;
 
     unsigned : NumExprBits;
 

include/clang/Sema/Sema.h

public:
   QualType getCapturedDeclRefType(VarDecl *Var, SourceLocation Loc);
 
   /// Mark all of the declarations referenced within a particular AST node as
   /// referenced. Used when template instantiation instantiates a non-dependent
   /// type -- entities referenced by the type are now referenced.
   void MarkDeclarationsReferencedInType(SourceLocation Loc, QualType T);
   void MarkDeclarationsReferencedInExpr(Expr *E,
                                         bool SkipLocalVariables = false);
+  void MaybeMarkBuiltinConstantPCannotDelayEvaluation(Expr *E);
 
   /// Try to recover by turning the given expression into a
   /// call.  Returns true if recovery was attempted or an error was
   /// emitted; this may also leave the ExprResult invalid.
   bool tryToRecoverWithCall(ExprResult &E, const PartialDiagnostic &PD,
                             bool ForceComplain = false,
                             bool (*IsPlausibleResult)(QualType) = nullptr);
 

lib/AST/Expr.cpp

for (unsigned i = 0; i != NumPreArgs; ++i) {
     SubExprs[i+PREARGS_START] = preargs[i];
   }
   for (unsigned i = 0; i != args.size(); ++i) {
     updateDependenciesFromArg(args[i]);
     SubExprs[i+PREARGS_START+NumPreArgs] = args[i];
   }
 
   CallExprBits.NumPreArgs = NumPreArgs;
+  CallExprBits.CanDelayEvaluation = true;
   RParenLoc = rparenloc;
 }
 
 CallExpr::CallExpr(const ASTContext &C, StmtClass SC, Expr *fn,
                    ArrayRef<Expr *> args, QualType t, ExprValueKind VK,
                    SourceLocation rparenloc)
     : CallExpr(C, SC, fn, ArrayRef<Expr *>(), args, t, VK, rparenloc) {}
 

lib/AST/ExprConstant.cpp

bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
 
   case Builtin::BI__builtin_clz:
   case Builtin::BI__builtin_clzl:
   case Builtin::BI__builtin_clzll:
   case Builtin::BI__builtin_clzs: {
     APSInt Val;
     if (!EvaluateInteger(E->getArg(0), Val, Info))
       return false;
     if (!Val)
       return Error(E);
 
     return Success(Val.countLeadingZeros(), E);
   }
 
-  case Builtin::BI__builtin_constant_p:
-    return Success(EvaluateBuiltinConstantP(Info.Ctx, E->getArg(0)), E);
+  case Builtin::BI__builtin_constant_p: {
+    auto Arg = E->getArg(0);
+    if (EvaluateBuiltinConstantP(Info.Ctx, Arg))
+      return Success(true, E);
+    Arg = Arg->IgnoreParenCasts();
+    if (isa<DeclRefExpr>(Arg->IgnoreParenCasts()) &&
+        !Arg->HasSideEffects(Info.Ctx) && E->getCanDelayEvaluation()) {
+      // Note: This diagnostic won't be shown to the user.
+      Info.FFDiag(E, diag::note_invalid_subexpr_in_const_expr);
+      return false;
+    }
+    return Success(false, E);
+  }
 
   case Builtin::BI__builtin_ctz:
   case Builtin::BI__builtin_ctzl:
   case Builtin::BI__builtin_ctzll:
   case Builtin::BI__builtin_ctzs: {
     APSInt Val;
     if (!EvaluateInteger(E->getArg(0), Val, Info))
       return false;

lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin_rotateleft64:
     return emitRotate(E, false);
 
   case Builtin::BI__builtin_rotateright8:
   case Builtin::BI__builtin_rotateright16:
   case Builtin::BI__builtin_rotateright32:
   case Builtin::BI__builtin_rotateright64:
     return emitRotate(E, true);
 
+  case Builtin::BI__builtin_constant_p:
+    if (CGM.getCodeGenOpts().OptimizationLevel == 0 ||
+        hasAggregateEvaluationKind(E->getArg(0)->getType()))
+      return RValue::get(ConstantInt::get(Int32Ty, 0));
+    return RValue::get(emitUnaryBuiltin(*this, E, Intrinsic::is_constant));
+
   case Builtin::BI__builtin_object_size: {
     unsigned Type =
         E->getArg(1)->EvaluateKnownConstInt(getContext()).getZExtValue();
     auto *ResType = cast<llvm::IntegerType>(ConvertType(E->getType()));
 
     // We pass this builtin onto the optimizer so that it can figure out the
     // object size in more complex cases.
     return RValue::get(emitBuiltinObjectSize(E->getArg(0), Type, ResType,

lib/Parse/ParseDecl.cpp

while (Tok.isNot(tok::r_brace)) {
     SourceLocation EqualLoc;
     ExprResult AssignedVal;
     EnumAvailabilityDiags.emplace_back(*this);
 
     if (TryConsumeToken(tok::equal, EqualLoc)) {
       AssignedVal = ParseConstantExpression();
       if (AssignedVal.isInvalid())
         SkipUntil(tok::comma, tok::r_brace, StopBeforeMatch);
+      else
+        Actions.MaybeMarkBuiltinConstantPCannotDelayEvaluation(
+            AssignedVal.get());
     }
 
     // Install the enumerator constant into EnumDecl.
     Decl *EnumConstDecl = Actions.ActOnEnumConstant(
         getCurScope(), EnumDecl, LastEnumConstDecl, IdentLoc, Ident, attrs,
         EqualLoc, AssignedVal.get());
     EnumAvailabilityDiags.back().done();
 
if (NumElements.isInvalid()) {
     // If the expression was invalid, skip it.
     SkipUntil(tok::r_square, StopAtSemi);
     return;
   }
 
   T.consumeClose();
 
   MaybeParseCXX11Attributes(DS.getAttributes());
+  if (NumElements.get() && !Actions.CurContext->isFunctionOrMethod())
+    Actions.MaybeMarkBuiltinConstantPCannotDelayEvaluation(NumElements.get());
 
   // Remember that we parsed a array type, and remember its features.
   D.AddTypeInfo(
       DeclaratorChunk::getArray(DS.getTypeQualifiers(), StaticLoc.isValid(),
                                 isStar, NumElements.get(), T.getOpenLocation(),
                                 T.getCloseLocation()),
       std::move(DS.getAttributes()), T.getCloseLocation());
 }

lib/Parse/ParseInit.cpp

ExprResult Parser::ParseBraceInitializer() {
   /// initializer.
   ExprVector InitExprs;
 
   if (Tok.is(tok::r_brace)) {
     // Empty initializers are a C++ feature and a GNU extension to C.
     if (!getLangOpts().CPlusPlus)
       Diag(LBraceLoc, diag::ext_gnu_empty_initializer);
     // Match the '}'.
-    return Actions.ActOnInitList(LBraceLoc, None, ConsumeBrace());
+    auto Init = Actions.ActOnInitList(LBraceLoc, None, ConsumeBrace());
+    Actions.MaybeMarkBuiltinConstantPCannotDelayEvaluation(Init.get());
+    return Init;
   }
 
   // Enter an appropriate expression evaluation context for an initializer list.
   EnterExpressionEvaluationContext EnterContext(
       Actions, EnterExpressionEvaluationContext::InitList);
 
   bool InitExprsOk = true;
 
while (1) {
     ConsumeToken();
 
     // Handle trailing comma.
     if (Tok.is(tok::r_brace)) break;
   }
 
   bool closed = !T.consumeClose();
 
-  if (InitExprsOk && closed)
-    return Actions.ActOnInitList(LBraceLoc, InitExprs,
+  if (InitExprsOk && closed) {
+    auto Init = Actions.ActOnInitList(LBraceLoc, InitExprs,
                                        T.getCloseLocation());
+    Actions.MaybeMarkBuiltinConstantPCannotDelayEvaluation(Init.get());
+    return Init;
+  }
 
   return ExprError(); // an error occurred.
 }
 
 
 // Return true if a comma (or closing brace) is necessary after the
 // __if_exists/if_not_exists statement.
 bool Parser::ParseMicrosoftIfExistsBraceInitializer(ExprVector &InitExprs,

lib/Sema/SemaExpr.cpp

retry:
   } else {
     if (CheckOtherCall(TheCall, Proto))
       return ExprError();
   }
 
   return MaybeBindToTemporary(TheCall);
 }
 
 ExprResult
 Sema::ActOnCompoundLiteral(SourceLocation LParenLoc, ParsedType Ty,
                            SourceLocation RParenLoc, Expr *InitExpr) {
   assert(Ty && "ActOnCompoundLiteral(): missing type");
   assert(InitExpr && "ActOnCompoundLiteral(): missing expression");
 
   TypeSourceInfo *TInfo;
   QualType literalType = GetTypeFromParser(Ty, &TInfo);
   if (!TInfo)
Sema::ActOnInitList(SourceLocation LBraceLoc, MultiExprArg InitArgList,
   for (unsigned I = 0, E = InitArgList.size(); I != E; ++I) {
     if (InitArgList[I]->getType()->isNonOverloadPlaceholderType()) {
       ExprResult result = CheckPlaceholderExpr(InitArgList[I]);
 
       // Ignore failures; dropping the entire initializer list because
       // of one failure would be terrible for indexing/etc.
       if (result.isInvalid()) continue;
 
       InitArgList[I] = result.get();
     }
   }
 
   // Semantic analysis for initializers is done by ActOnDeclarator() and
   // CheckInitializer() - it requires knowledge of the object being initialized.
 
   InitListExpr *E = new (Context) InitListExpr(Context, LBraceLoc, InitArgList,
                                                RBraceLoc);
public:
     }
   };
 }
 
 /// Mark any declarations that appear within this expression or any
 /// potentially-evaluated subexpressions as "referenced".
 ///
 /// \param SkipLocalVariables If true, don't mark local variables as
 /// 'referenced'.
 void Sema::MarkDeclarationsReferencedInExpr(Expr *E,
                                             bool SkipLocalVariables) {
   EvaluatedExprMarker(*this, SkipLocalVariables).Visit(E);
 }
 
+namespace {
+  // Helper class that marks  all calls to __builtin_constant_p that cannot be
+  // evaluated after inlining.
+  //
+  // GCC honors __builtin_constant_p() after inlining. Clang performs inlining
+  // too late to do that in the front-end. Therefore, clang converts bcp calls
+  // that can't be evaluated to a constant to an intrinsic so that the
+  // middle-end can determine whether or not it's constant after inlining. But
+  // this won't work for initializer lists and other constructs. For those, we
+  // need to be conservative in our analysis. So if we cannot determine that a
+  // value is constant before LLVM IR generation, then we resolve the bcp call
+  // to "false".
+  struct BuiltinConstantPCannotDelayEvaluation :
+    public EvaluatedExprVisitor<BuiltinConstantPCannotDelayEvaluation> {
+      typedef EvaluatedExprVisitor<BuiltinConstantPCannotDelayEvaluation>
+          Inherited;
+
+      BuiltinConstantPCannotDelayEvaluation(Sema &S)
+          : Inherited(S.Context) { }
+
+      void VisitCallExpr(CallExpr *E) {
+        auto DC = E->getDirectCallee();
+        if (!DC || DC->getBuiltinID() != Builtin::BI__builtin_constant_p)
+          return;
+        E->setCanDelayEvaluation(false);
+      }
+
+      void VisitInitListExpr(InitListExpr *E) {
+        for (auto Elem : *E)
+          Visit(Elem);
+      }
+  };
+}
+
+// Mark all calls to __builtin_constant_p that cannot have their evaluation
+// delayed. I.e., we won't generate an llvm.is.constant() intrinsic if the front
+// end is unable to ensure the argument is constant. E.g., when a
+// __builtin_constant_p() is used in an InitListExpr or subscript of a global
+// VarDecl.
+void Sema::MaybeMarkBuiltinConstantPCannotDelayEvaluation(Expr *E) {
+  BuiltinConstantPCannotDelayEvaluation(*this).Visit(E);
+}
+
 /// Emit a diagnostic that describes an effect on the run-time behavior
 /// of the program being compiled.
 ///
 /// This routine emits the given diagnostic when the code currently being
 /// type-checked is "potentially evaluated", meaning that there is a
 /// possibility that the code will actually be executable. Code in sizeof()
 /// expressions, code used only during overload resolution, etc., are not
 /// potentially evaluated. This routine will suppress such diagnostics or,

lib/Serialization/ASTReaderStmt.cpp

void ASTStmtReader::VisitOMPArraySectionExpr(OMPArraySectionExpr *E) {
   E->setLength(Record.readSubExpr());
   E->setColonLoc(ReadSourceLocation());
   E->setRBracketLoc(ReadSourceLocation());
 }
 
 void ASTStmtReader::VisitCallExpr(CallExpr *E) {
   VisitExpr(E);
   E->setNumArgs(Record.getContext(), Record.readInt());
+  E->setCanDelayEvaluation(Record.readInt());
   E->setRParenLoc(ReadSourceLocation());
   E->setCallee(Record.readSubExpr());
   for (unsigned I = 0, N = E->getNumArgs(); I != N; ++I)
     E->setArg(I, Record.readSubExpr());
 }
 
 void ASTStmtReader::VisitCXXMemberCallExpr(CXXMemberCallExpr *E) {
   VisitCallExpr(E);

lib/Serialization/ASTWriterStmt.cpp

void ASTStmtWriter::VisitOMPArraySectionExpr(OMPArraySectionExpr *E) {
   Record.AddSourceLocation(E->getColonLoc());
   Record.AddSourceLocation(E->getRBracketLoc());
   Code = serialization::EXPR_OMP_ARRAY_SECTION;
 }
 
 void ASTStmtWriter::VisitCallExpr(CallExpr *E) {
   VisitExpr(E);
   Record.push_back(E->getNumArgs());
+  Record.push_back(E->getCanDelayEvaluation());
   Record.AddSourceLocation(E->getRParenLoc());
   Record.AddStmt(E->getCallee());
   for (CallExpr::arg_iterator Arg = E->arg_begin(), ArgEnd = E->arg_end();
        Arg != ArgEnd; ++Arg)
     Record.AddStmt(*Arg);
   Code = serialization::EXPR_CALL;
 }
 

test/Analysis/builtin-functions.cpp

void g(int i) {
   }
 }
 
 void test_constant_p() {
   int i = 1;
   const int j = 2;
   constexpr int k = 3;
   clang_analyzer_eval(__builtin_constant_p(42) == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(j) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(j + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(" ") == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 0); // expected-warning {{FALSE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning {{TRUE}}
 }

test/Sema/builtins.c

 }
 
 // PR7885
 int test16() {
   return __builtin_constant_p() + // expected-error{{too few arguments}}
          __builtin_constant_p(1, 2); // expected-error {{too many arguments}}
 }
 
+// __builtin_constant_p cannot resolve non-constants such as a file scoped array.
+int expr;
+char y[__builtin_constant_p(expr) ? -1 : 1]; // no warning, the builtin is false.
+
+// no warning, the builtin is false.
+struct foo { int a; };
+struct foo x = (struct foo) { __builtin_constant_p(42) ? 37 : 927 };
+
 const int test17_n = 0;
 const char test17_c[] = {1, 2, 3, 0};
 const char test17_d[] = {1, 2, 3, 4};
 typedef int __attribute__((vector_size(16))) IntVector;
 struct Aggregate { int n; char c; };
 enum Enum { EnumValue1, EnumValue2 };
 
 typedef __typeof(sizeof(int)) size_t;
#define F(...) ASSERT(!__builtin_constant_p(__VA_ARGS__))
   F((IntVector){0, 1, 2, 3});
 
   // Ensure that a technique used in glibc is handled correctly.
 #define OPT(...) (__builtin_constant_p(__VA_ARGS__) && strlen(__VA_ARGS__) < 4)
   // FIXME: These are incorrectly treated as ICEs because strlen is treated as
   // a builtin.
   ASSERT(OPT("abc"));
   ASSERT(!OPT("abcd"));
+
   // In these cases, the strlen is non-constant, but the __builtin_constant_p
-  // is 0: the array size is not an ICE but is foldable.
-  ASSERT(!OPT(test17_c));        // expected-warning {{folded}}
+  // is 0: the array size is not an ICE.
+  ASSERT(!OPT(test17_c));        // no warning expected
+  ASSERT(!OPT((char*)test17_c)); // no warning expected
+  ASSERT(!OPT(test17_d));        // no warning expected
+  ASSERT(!OPT((char*)test17_d)); // no warning expected
+
+  // These are foldable.
   ASSERT(!OPT(&test17_c[0]));    // expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_c)); // expected-warning {{folded}}
-  ASSERT(!OPT(test17_d));        // expected-warning {{folded}}
   ASSERT(!OPT(&test17_d[0]));    // expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_d)); // expected-warning {{folded}}
 
 #undef OPT
 #undef T
 #undef F
 }
 
 void test18() {
   char src[1024];

 #define my_memcpy(x,y,z) __builtin___memcpy_chk(x,y,z, __builtin_object_size(x,0))
 
 void test23() {
   char src[1024];
   char buf[10];
   memcpy(buf, src, 11); // expected-warning{{'memcpy' will always overflow; destination buffer has size 10, but size argument is 11}}
   my_memcpy(buf, src, 11); // expected-warning{{'__builtin___memcpy_chk' will always overflow; destination buffer has size 10, but size argument is 11}}
 }
+
+size_t test24(int a) {
+  char x[__builtin_constant_p(a) ? -1 : 1]; // no warning expected
+  return strlen(x);
+}
+
+__attribute__((always_inline))
+size_t test25(int a) {
+  char x[__builtin_constant_p(a) ? 1 : -1]; // no warning expected
+  return strlen(x);
+}
+
+size_t test26() {
+  return test25(2);
+}
+
+void f(int n) {
+  enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error because a's value is non-constant
+}