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

[Pierre-vh]

Summary

Fix an edge case ExprConstant.cpp's EvaluateWithSubstitution when called by CheckEnableIf

The assertion in CallStackFrame::getTemporary
could fail during evaluation of nested calls to a function
using enable_if when the second argument was a
value-dependent expression.

This caused a temporary to be created for the second
argument with a given version during the
evaluation of the inner call, but we bailed out
when evaluating the second argument of the
outer call due to the expression being value-dependent.
After bailing out, we tried to clean up the argument's value slot but it
caused an assertion to trigger in getTemporary as
a temporary for the second argument existed, but only for the inner call and not the outer call.

See the test case for a more complete description of the issue.

[Pierre-vh]

Missing newline at EOF

[Pierre-vh]

I think the right choice is to remove the assert as it's an invariant that can be broken in some cases, so I don't feel like the assert is worth it anymore.
Alternatively I could add something to bypass the assert in that specific call to getParamSlot for Value-Dependent expressions, it's a more targeted fix in case you think the assertion must be left in.

[shafik]

If I am reading the code correctly it looks like if the call to (*I)->isValueDependent() is true then the temporary will not be created and therefore we should not be attempting to access the slot.

If this is the case then maybe the checking in EvaluateWithSubstitution(...) needs to be more carefully done?

I am not familiar with this code but I don't know if you analysis provides a convincing case the assert should be removed.

[Pierre-vh]

In D139713#3989071, @shafik wrote:

If I am reading the code correctly it looks like if the call to (*I)->isValueDependent() is true then the temporary will not be created and therefore we should not be attempting to access the slot.

If this is the case then maybe the checking in EvaluateWithSubstitution(...) needs to be more carefully done?

I am not familiar with this code but I don't know if you analysis provides a convincing case the assert should be removed.

Indeed, this only happens when isValueDependent returns true.

I am also not familiar with the code, so I just decided to propose a quick fix to get the discussion started; I certainly don't mind changing the nature of the fix if we agree it should be fixed differently.
For instance, we could also make the "getParam" call faillible by adding some "tryGetParam" variant that doesn't have the assert, or by passing some optional boolean to indicate it's acceptable to have the key present in the map with a different version.

My initial reasoning was that if the assert can be broken by legitimate code, then it shouldn't be there

[shafik]

In D139713#3989709, @Pierre-vh wrote:

In D139713#3989071, @shafik wrote:

If I am reading the code correctly it looks like if the call to (*I)->isValueDependent() is true then the temporary will not be created and therefore we should not be attempting to access the slot.

If this is the case then maybe the checking in EvaluateWithSubstitution(...) needs to be more carefully done?

I am not familiar with this code but I don't know if you analysis provides a convincing case the assert should be removed.

Indeed, this only happens when isValueDependent returns true.

I am also not familiar with the code, so I just decided to propose a quick fix to get the discussion started; I certainly don't mind changing the nature of the fix if we agree it should be fixed differently.
For instance, we could also make the "getParam" call faillible by adding some "tryGetParam" variant that doesn't have the assert, or by passing some optional boolean to indicate it's acceptable to have the key present in the map with a different version.

My initial reasoning was that if the assert can be broken by legitimate code, then it shouldn't be there

It looks like the original commit that added getTemporary(...) which was 4e2698ca9e49e that this invariant held but the later commit which added getParamSlot(...) which is f7f2e4261a98b broke the invariant.

So I think that looks like an error and any fix should maintain the invariant unless strongly motivated reasoning can be put forward to remove it but I am not totally sure.

CC @rsmith @ahatanak who are the authors of the two commits mentioned above.

[Pierre-vh]

Put the assert back in, use alternative fix

[ahatanak]

I read the patches and review comments in https://reviews.llvm.org/D42776, but I don't remember why I added that assert. Maybe I was just trying to ensure the version number passed to getTemporary was the one that was used to create the temporary. For example, if a temporary was created using version X and later retrieved using version Y because of some bug in constant evaluation, the assertion would fail.

But the assertion is wrong, so maybe it should just be removed.

[Pierre-vh]

Remove assert
Please review @ahatanak

[ahatanak]

LGTM

clang/lib/AST/ExprConstant.cpp

public:
     ~CallStackFrame();
 
     // Return the temporary for Key whose version number is Version.
     APValue *getTemporary(const void *Key, unsigned Version) {
       MapKeyTy KV(Key, Version);
       auto LB = Temporaries.lower_bound(KV);
       if (LB != Temporaries.end() && LB->first == KV)
         return &LB->second;
-      // Pair (Key,Version) wasn't found in the map. Check that no elements
-      // in the map have 'Key' as their key.
-      assert((LB == Temporaries.end() || LB->first.first != Key) &&
-             (LB == Temporaries.begin() || std::prev(LB)->first.first != Key) &&
-             "Element with key 'Key' found in map");
       return nullptr;
     }
 
     // Return the current temporary for Key in the map.
     APValue *getCurrentTemporary(const void *Key) {
       auto UB = Temporaries.upper_bound(MapKeyTy(Key, UINT_MAX));
       if (UB != Temporaries.begin() && std::prev(UB)->first.first == Key)
         return &std::prev(UB)->second;

clang/test/SemaCXX/enable_if-nested-call-with-valuedependent-param.cpp

+// 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());
+  }
+};