Fix PR28366: Teach the const-expression evaluator to be more fault tolerant with non-const enclosing local variables, or otherwise fold them if const.

[faisalv]

Summary

Fix the following Bug:
https://llvm.org/bugs/show_bug.cgi?id=28366

This patch teaches the constant expression evaluator to search the frame stack for a local variable (instead of assuming a local variable is on the current frame).

This has the following benefits:

• if the local variable from an enclosing scope was an error during parsing/instantiation - it remains an error (instead of a compiler crash) during constant expression evaluation
• a local variable that is a constant expression from an enclosing scope is now evaluatable

Additionally, fix SemaTemplateInstantiateDecl so that when it instantiates the enable_if attribute it uses the instantiated declaration (instead of the template) when evaluating the condition (so that the instantiated param's decl context is correctly found). This was done to fix the following regression:

template <typename T> T templatedBar(T m) attribute((enable_if(m > 0, ""))) { return T(); }

[rsmith]

lib/AST/ExprConstant.cpp

4791–4802

This doesn't seem right to me: in the case where the variable is not in the expected frame, there seems to be no reason to expect it would be in a caller's frame rather than in some unrelated place, nor to special-case that situation. (And this does the wrong thing for constexpr lambdas, where the captured variable need not be in the *innermost* call frame for the function in which the variable was declared.)

Instead, how about changing the VD->hasLocalStorage() && ... condition below to also check that the DeclContext of the VarDecl is the callee of the current call, and leave Frame null otherwise.

[faisalv]

Updated the patch per Richard's direction (which helped clarify my thinking about this): Check only the current call frame - if the VarDecl is contained within the current callee - only then does it make sense to dig into the stack-variables of the frame. No other scenario really makes sense (even for lambdas, we care about the parent decl-contexts (static binding), not variables within the call-frame stack (run-time binding).

Thanks Richard.

[rsmith]

lib/AST/ExprConstant.cpp

4795–4797

This is talking too much about the what and not enough about the why. "We expect to find a value for a local variable in the current frame, unless the variable was actually declared in a lexically-enclosing context." or something like that?

4798–4800

This can be specified more simply as:

if (Info.CurrentCall->Callee &&
    Info.CurrentCall->Callee->Equals(VD->getDeclContext()))

lib/Sema/SemaTemplateInstantiateDecl.cpp

190

Do we have existing test coverage for this (tests that fail with the ExprConstant change if we don't fix this at the same time)?

test/SemaCXX/constant-expression-cxx11.cpp

2091

Maybe also static_assert that we get the correct value here?

[faisalv]

Address Richard's comments.

[faisalv]

Address Richard's comments.

lib/Sema/SemaTemplateInstantiateDecl.cpp

190

Yes we do. That's how I realized it needed to be fixed - it's enable_if.cpp in SemaCXX that has the following test:
template <typename T> T templatedBar(T m) attribute((enable_if(m > 0, ""))) { return T(); }

[rsmith]

LGTM, thanks!

test/SemaCXX/constant-expression-cxx11.cpp

2091

I would use 'c' + 'd' here, just in case we get the same incorrect value evaluating one of these in both places. (That is, if d incorrectly evaluated to 0 both here and on line 2088, this assert would still pass.)

[Eugene.Zelenko]

Looks like patch was not committed.

[faisalv]

Fixed by commit: r286748
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161107/176865.html
Thanks for the review!

lib/AST/ExprConstant.cpp

   return Error(E);
 }
 
+
 bool LValueExprEvaluator::VisitVarDecl(const Expr *E, const VarDecl *VD) {
   CallStackFrame *Frame = nullptr;
-  if (VD->hasLocalStorage() && Info.CurrentCall->Index > 1)
-    Frame = Info.CurrentCall;
+  if (VD->hasLocalStorage() && Info.CurrentCall->Index > 1) {
+    // Only if a local variable was declared in the function currently being
+    // evaluated, do we expect to be able to find its value in the current
+    // frame. (Otherwise it was likely declared in an enclosing context and
+    // could either have a valid evaluatable value (for e.g. a constexpr
+    // variable) or be ill-formed (and trigger an appropriate evaluation
+    // diagnostic)).
+    if (Info.CurrentCall->Callee &&
+        Info.CurrentCall->Callee->Equals(VD->getDeclContext())) {
+      Frame = Info.CurrentCall;
+    }
+  }
 
   if (!VD->getType()->isReferenceType()) {
     if (Frame) {

lib/Sema/SemaTemplateInstantiateDecl.cpp

 
   SmallVector<PartialDiagnosticAt, 8> Diags;
   if (A->getCond()->isValueDependent() && !Cond->isValueDependent() &&
-      !Expr::isPotentialConstantExprUnevaluated(Cond, cast<FunctionDecl>(Tmpl),
+      !Expr::isPotentialConstantExprUnevaluated(Cond, cast<FunctionDecl>(New),
                                                 Diags)) {
     S.Diag(A->getLocation(), diag::err_enable_if_never_constant_expr);
     for (int I = 0, N = Diags.size(); I != N; ++I)

test/SemaCXX/constant-expression-cxx11.cpp

   constexpr Z z(1);
   static_assert(z.w == 1 && z.x == 2 && z.y == 3 && z.z == 4, "");
 }
+
+
+namespace PR28366 {
+namespace ns1 {
+
+void f(char c) { //expected-note2{{declared here}}
+  struct X {
+    static constexpr char f() { //expected-error{{never produces a constant expression}}
+      return c; //expected-error{{reference to local}} expected-note{{non-const variable}}
+    }
+  };
+  int I = X::f();
+}
+
+void g() {
+  const int c = 'c';
+  static const int d = 'd';
+  struct X {
+    static constexpr int f() {
+      return c + d;
+    }
+  };
+  static_assert(X::f() == c + d,"");
+}
+
+
+} // end ns1
+
+} //end ns PR28366
+