Pierre-vh created this revision.
Pierre-vh added reviewers: ahatanak, rsmith.
Herald added a project: All.
Pierre-vh requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
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());
+ }
+};
\ No newline at end of file
Index: clang/lib/AST/ExprConstant.cpp
===================================================================
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -593,11 +593,6 @@
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;
}
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());
+ }
+};
\ No newline at end of file
Index: clang/lib/AST/ExprConstant.cpp
===================================================================
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -593,11 +593,6 @@
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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
