[PATCH] D139713: [Sema] Fix crash when evaluating nested call with value-dependent arg

[Pierre van Houtryve via Phabricator via cfe-commits]

Pierre-vh updated this revision to Diff 482384.
Pierre-vh added a comment.

Put the assert back in, use alternative fix


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139713/new/

https://reviews.llvm.org/D139713

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/enable_if-nested-call-with-valuedependent-param.cpp


Index: clang/test/SemaCXX/enable_if-nested-call-with-valuedependent-param.cpp
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/enable_if-nested-call-with-valuedependent-param.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -fsyntax-only %s -std=c++14
+
+// Checks that Clang doesn't crash/assert on the nested call to "kaboom"
+// in "bar()".
+//
+// This is an interesting test case for `ExprConstant.cpp`'s `CallStackFrame`
+// because it triggers the following chain of events:
+// 0. `CheckEnableIf` calls `EvaluateWithSubstitution`.
+//  1. The outer call to "kaboom" gets evaluated.
+//   2. The expr for "a" gets evaluated, it has a version X;
+//      a temporary with the key (a, X) is created.
+//     3. The inner call to "kaboom" gets evaluated.
+//       4. The expr for "a" gets evaluated, it has a version Y;
+//          a temporary with the key (a, Y) is created.
+//       5. The expr for "b" gets evaluated, it has a version Y;
+//          a temporary with the key (b, Y) is created.
+//   6. `EvaluateWithSubstitution` looks at "b" but cannot evaluate it
+//      because it's value-dependent (due to the call to "f.foo()").
+//
+// When `EvaluateWithSubstitution` bails out while evaluating the outer
+// call, it attempts to fetch "b"'s param slot to clean it up.
+//
+// This used to cause an assertion failure in `getTemporary` because
+// a temporary with the key "(b, Y)" (created at step 4) existed but
+// not one for "(b, X)", which is what it was trying to fetch.
+
+template<typename T>
+__attribute__((enable_if(true, "")))
+T kaboom(T a, T b) {
+  return b;
+}
+
+struct A {
+  double foo();
+};
+
+template <int>
+struct B {
+  A &f;
+
+  void bar() {
+    kaboom(kaboom(0.0, 1.0), f.foo());
+  }
+};
Index: clang/lib/AST/ExprConstant.cpp
===================================================================
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -16059,9 +16059,13 @@
     if ((*I)->isValueDependent() ||
         !EvaluateCallArg(PVD, *I, Call, Info) ||
         Info.EvalStatus.HasSideEffects) {
-      // If evaluation fails, throw away the argument entirely.
-      if (APValue *Slot = Info.getParamSlot(Call, PVD))
-        *Slot = APValue();
+      // If evaluation fails, throw away the argument entirely unless I is
+      // value-dependent. In those cases, the condition above will short-circuit
+      // before calling `EvaluateCallArg` and no param slot is created.
+      if (!(*I)->isValueDependent()) {
+        if (APValue *Slot = Info.getParamSlot(Call, PVD))
+          *Slot = APValue();
+      }
     }
 
     // Ignore any side-effects from a failed evaluation. This is safe because


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D139713.482384.patch
Type: text/x-patch
Size: 2723 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221213/bd5df667/attachment.bin>