[8.0 Regression] Fix handling of `__builtin_constant_p` inside template arguments, enumerators, case statements, and the enable_if attribute.

[EricWF]

Summary

The following code is accepted by Clang 7 and prior but rejected by the upcoming 8 release and in trunk [1]

// error {{never produces a constant expression}}
void foo(const char* s) __attribute__((enable_if(__builtin_constant_p(*s) == false, "trap"))) {}
void test() { foo("abc"); }

Prior to Clang 8, the call to __builtin_constant_p was a constant expression returning false. Currently, it's not a valid constant expression.

The bug is caused because we failed to set InConstantContext when attempting to evaluate unevaluated constant expressions.

[1] https://godbolt.org/z/ksAjmq

[hans]

I think I'll have to defer to Richard for reviewing this, since I'm not familiar with the code. Since it's a regression we probably want to merge it to clang 8. Can you give any guidance to how bad a regression this is, e.g. does it break some important piece of code?

[EricWF]

In D59038#1421310, @hans wrote:

I think I'll have to defer to Richard for reviewing this, since I'm not familiar with the code. Since it's a regression we probably want to merge it to clang 8. Can you give any guidance to how bad a regression this is, e.g. does it break some important piece of code?

It breaks absl::StrFormat [1][2]. And it breaks libstdc++'s std::string_view inside unevaluated constant expressions [3].

This regression is a problem for Abseil.

[1] https://godbolt.org/z/Q_J2JP
[2] https://github.com/abseil/abseil-cpp/issues/271
[3 https://godbolt.org/z/Fg1sqM]

[rsmith]

LGTM, and I think this is safe enough to take for Clang 8.

[hans]

In D59038#1422890, @rsmith wrote:

LGTM, and I think this is safe enough to take for Clang 8.

Do you think the severity is high enough to spin another release candidate?

My concern is that since this didn't show up in testing until now, which I guess is quite a while after it regressed, maybe it's not important enough to delay the already late release? Also it sounds like it isn't completely fixed yet..

[EricWF]

• Fix regressions inside template parameters, case statements, and enumerators.

[EricWF]

In D59038#1422905, @hans wrote:

In D59038#1422890, @rsmith wrote:

LGTM, and I think this is safe enough to take for Clang 8.

Do you think the severity is high enough to spin another release candidate?

Yes. Very much so. It also effect template arguments, case statements, enumerators, and probably more.

My concern is that since this didn't show up in testing until now, which I guess is quite a while after it regressed, maybe it's not important enough to delay the already late release?

The common case for hitting this bug requires using libstdc++ 7.0 or newer and C++17. This may be relatively rare for our release testers, who are probably either using libc++, targeting the system libstdc++, or not turning on C++17.
As libstdc++ 7.0 becomes more common and as more users move to C++17 people are going to run into this a lot.

Also it sounds like it isn't completely fixed yet..

It is now.

[EricWF]

Add more tests.

[EricWF]

• Correct tests for unscoped enums.

[rsmith]

In D59038#1422905, @hans wrote:

In D59038#1422890, @rsmith wrote:

LGTM, and I think this is safe enough to take for Clang 8.

Do you think the severity is high enough to spin another release candidate?

Given that the regression breaks both libstdc++ >=7 and abseil, I'm inclined to say yes. (Either that or we should aim to release a fixed 7.0.1 as soon as feasible after 7).

lib/AST/ExprConstant.cpp

bool Expr::EvaluateAsLValue(EvalResult &Result, const ASTContext &Ctx) const {
   LV.moveInto(Result.Val);
   return true;
 }
 
 bool Expr::EvaluateAsConstantExpr(EvalResult &Result, ConstExprUsage Usage,
                                   const ASTContext &Ctx) const {
   EvalInfo::EvaluationMode EM = EvalInfo::EM_ConstantExpression;
   EvalInfo Info(Ctx, Result, EM);
+  Info.InConstantContext = true;
   if (!::Evaluate(Result.Val, Info, this))
     return false;
 
   return CheckConstantExpression(Info, getExprLoc(), getType(), Result.Val,
                                  Usage);
 }
 
 bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,

 }
 
 bool Expr::EvaluateWithSubstitution(APValue &Value, ASTContext &Ctx,
                                     const FunctionDecl *Callee,
                                     ArrayRef<const Expr*> Args,
                                     const Expr *This) const {
   Expr::EvalStatus Status;
   EvalInfo Info(Ctx, Status, EvalInfo::EM_ConstantExpressionUnevaluated);
+  Info.InConstantContext = true;
 
   LValue ThisVal;
   const LValue *ThisPtr = nullptr;
   if (This) {
 #ifndef NDEBUG
     auto *MD = dyn_cast<CXXMethodDecl>(Callee);
     assert(MD && "Don't provide `this` for non-methods.");
     assert(!MD->isStatic() && "Don't provide `this` for static methods.");
bool Expr::isPotentialConstantExprUnevaluated(Expr *E,
                                               const FunctionDecl *FD,
                                               SmallVectorImpl<
                                                 PartialDiagnosticAt> &Diags) {
   Expr::EvalStatus Status;
   Status.Diag = &Diags;
 
   EvalInfo Info(FD->getASTContext(), Status,
                 EvalInfo::EM_PotentialConstantExpressionUnevaluated);
+  Info.InConstantContext = true;
 
   // Fabricate a call stack frame to give the arguments a plausible cover story.
   ArrayRef<const Expr*> Args;
   ArgVector ArgValues(0);
   bool Success = EvaluateArgs(Args, ArgValues, Info);
   (void)Success;
   assert(Success &&
          "Failed to set up arguments for potential constant evaluation");

test/SemaCXX/constant-expression-cxx1y.cpp

 static_assert(e2.x != e2.y, "");
 
 } // namespace IndirectFields
 
 constexpr bool indirect_builtin_constant_p(const char *__s) {
   return __builtin_constant_p(*__s);
 }
 constexpr bool n = indirect_builtin_constant_p("a");
+
+__attribute__((enable_if(indirect_builtin_constant_p("a") == n, "OK")))
+int test_in_enable_if() { return 0; }
+int n2 = test_in_enable_if();
+
+template <bool n = indirect_builtin_constant_p("a")>
+int test_in_template_param() { return 0; }
+int n3 = test_in_template_param();
+
+void test_in_case(int n) {
+  switch (n) {
+    case indirect_builtin_constant_p("abc"):
+    break;
+  }
+}
+enum InEnum1 {
+  ONE = indirect_builtin_constant_p("abc")
+};
+enum InEnum2 : int {
+  TWO = indirect_builtin_constant_p("abc")
+};
+enum class InEnum3 {
+  THREE = indirect_builtin_constant_p("abc")
+};

test/SemaCXX/enable_if.cpp

template <typename T> struct is_same<T, T> {
     enum { value = 1 };
   };
 
   void foo(int a) __attribute__((enable_if(a, "")));
   void foo(float a) __attribute__((enable_if(1, "")));
 
   static_assert(is_same<__typeof__(foo)*, decltype(&foo)>::value, "");
 }
+
+namespace InConstantContext {
+void foo(const char *s) __attribute__((enable_if(((void)__builtin_constant_p(*s), true), "trap"))) {}
+
+void test() {
+  InConstantContext::foo("abc");
+}
+} // namespace InConstantContext