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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Hi Akira, thanks for working on this!



================
Comment at: lib/AST/ExprConstant.cpp:1165-1173
+  auto LB = Temporaries.lower_bound(Key);
+
+  // If an element with key Key is found, reset the value and return it. This
+  // can happen if Key is part of a default argument expression.
+  if (LB != Temporaries.end() && LB->first == Key)
+    return LB->second = APValue();
+
----------------
I think that the problem is more subtle than this. This static assert errors (previously clang would assert) when it really it should be fine.
```
constexpr const int &x(const int &p = 0) { return p; }
static_assert(&x() != &x());
```
Because default arguments are allocated on the caller side, both the calls to `x()` call createTemporary for the same MaterializeTemporaryExpr in the same CallStackFrame, when really that MTE should correspond to two distinct values. This patch just hides that underlying problem by recycling the value created during the first call during the second call.

Maybe we could have a fancier key that incorporates a node on the caller side, such as the CXXDefaultArgExpr as well at the MTE, and store that fancy key in APValue::LValueBases? That would allow us generate distinct values for these MTEs, and also remember what expression originated it. What do you think about that?

There is small discussion about this problem here: https://bugs.llvm.org/show_bug.cgi?id=33140


https://reviews.llvm.org/D42776