[PATCH] D42776: [Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thank you, this looks like a great direction.

As noted, there are a bunch of other cases that we should cover with this approach. I'm really happy about the number of related bugs we get to fix with this change.



================
Comment at: lib/AST/ExprConstant.cpp:3186-3187
   // object under construction.
-  if (Info.isEvaluatingConstructor(LVal.getLValueBase(), LVal.CallIndex)) {
+  if (Info.isEvaluatingConstructor(LVal.getLValueBase(),
+                                   LVal.getLValueCallIndex())) {
     BaseType = Info.Ctx.getCanonicalType(BaseType);
----------------
This should take the version into account.


================
Comment at: lib/AST/ExprConstant.cpp:5236
     if (Frame) {
-      Result.set(VD, Frame->Index);
+      Result.set({VD, Frame->Index});
       return true;
----------------
Hmm. We should be versioning local variables as well. Currently we'll accept invalid code such as:

```
constexpr int f() {
  int *p = nullptr;
  for (int k = 0; k != 2; ++k) {
    int local_var = 0;
    if (k == 0)
      p = &local_var;
    else
      return *p;
  }
}
static_assert(f() == 0);
```


================
Comment at: lib/AST/ExprConstant.cpp:5275-5278
+    unsigned Version = Info.CurrentCall->getMTEVersion();
     Value = &Info.CurrentCall->
-        createTemporary(E, E->getStorageDuration() == SD_Automatic);
-    Result.set(E, Info.CurrentCall->Index);
+        createTemporary(E, E->getStorageDuration() == SD_Automatic, Version);
+    Result.set({E, Info.CurrentCall->Index, Version});
----------------
Can you combine these, so we have a single function to create a temporary and produce both an `LValue` denoting it and an `APValue*` to hold its evaluated value?


================
Comment at: lib/AST/ExprConstant.cpp:5772
     } else {
-      Result.set(SubExpr, Info.CurrentCall->Index);
+      Result.set({SubExpr, Info.CurrentCall->Index});
       if (!EvaluateInPlace(Info.CurrentCall->createTemporary(SubExpr, false),
----------------
This should create a versioned temporary object.


================
Comment at: lib/AST/ExprConstant.cpp:6540
   bool VisitConstructExpr(const Expr *E) {
-    Result.set(E, Info.CurrentCall->Index);
+    Result.set({E, Info.CurrentCall->Index});
     return EvaluateInPlace(Info.CurrentCall->createTemporary(E, false),
----------------
This should create a versioned temporary object.


================
Comment at: lib/AST/ExprConstant.cpp:8031
+         (A.getLValueCallIndex() == B.getLValueCallIndex() &&
+          A.getLValueVersion() == B.getLValueVersion());
 }
----------------
You already checked this above. It'd make sense to check the call index and version in the same place here, but we should only need one check for each :)


================
Comment at: lib/AST/ExprConstant.cpp:9974-9997
+    LV.set({E, Info.CurrentCall->Index});
     APValue &Value = Info.CurrentCall->createTemporary(E, false);
     if (!EvaluateArray(E, LV, Value, Info))
       return false;
     Result = Value;
   } else if (T->isRecordType()) {
     LValue LV;
----------------
These temporaries should all be versioned.


https://reviews.llvm.org/D42776