[Clang] Instantiate the correct lambda call operator #110446
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: None (Sirraide) ChangesSee issue #110401. This is a very tentative fix for the following issue: when a lambda’s class type is merged across modules (e.g. because it is defined in a template in the GMF of some module This causes issues in situations where the call operator is in a template and accesses declarations in the surrounding context: when those declarations are instantated, a mapping is introduced from the original node in the template to that of the instantiation. If such an instantiation happens in My tentative fix for this is to first search our own methods for a call operator in This fixes #110401. Full diff: https://github.com/llvm/llvm-project/pull/110446.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 14907e7db18de3..17a0c45d98c5c8 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -426,6 +426,7 @@ Bug Fixes to C++ Support
- Fixed an assertion failure in debug mode, and potential crashes in release mode, when
diagnosing a failed cast caused indirectly by a failed implicit conversion to the type of the constructor parameter.
- Fixed an assertion failure by adjusting integral to boolean vector conversions (#GH108326)
+- Clang now instantiates the correct lambda call operator when a lambda's class type is merged across modules. (#GH110401)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 01143391edab40..af9c644337c84d 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -1631,9 +1631,32 @@ static bool allLookupResultsAreTheSame(const DeclContext::lookup_result &R) {
static NamedDecl* getLambdaCallOperatorHelper(const CXXRecordDecl &RD) {
if (!RD.isLambda()) return nullptr;
DeclarationName Name =
- RD.getASTContext().DeclarationNames.getCXXOperatorName(OO_Call);
- DeclContext::lookup_result Calls = RD.lookup(Name);
+ RD.getASTContext().DeclarationNames.getCXXOperatorName(OO_Call);
+
+ // If we have multiple call operators, we might be in a situation
+ // where we merged this lambda with one from another module; in that
+ // case, return our method (instead of that of the other lambda).
+ //
+ // This avoids situations where, given two modules A and B, if we
+ // try to instantiate A's call operator in a function in B, anything
+ // in the call operator that relies on local decls in the surrounding
+ // function will crash because it tries to find A's decls, but we only
+ // instantiated B's:
+ //
+ // template <typename>
+ // void f() {
+ // using T = int; // We only instantiate B's version of this.
+ // auto L = [](T) { }; // But A's call operator wants A's here.
+ // }
+ //
+ // To mitigate this, search our own decls first.
+ // FIXME: This feels like a hack; is there a better way of doing this?
+ for (CXXMethodDecl *M : RD.methods())
+ if (M->getDeclName() == Name)
+ return M;
+ // Otherwise, do a full lookup to find external decls too.
+ DeclContext::lookup_result Calls = RD.lookup(Name);
assert(!Calls.empty() && "Missing lambda call operator!");
assert(allLookupResultsAreTheSame(Calls) &&
"More than one lambda call operator!");
diff --git a/clang/test/Modules/gh110401.cppm b/clang/test/Modules/gh110401.cppm
new file mode 100644
index 00000000000000..6b335eb5ba9d55
--- /dev/null
+++ b/clang/test/Modules/gh110401.cppm
@@ -0,0 +1,44 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux -emit-module-interface %t/a.cppm -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux -emit-module-interface -fprebuilt-module-path=%t %t/b.cppm -o %t/B.pcm
+
+// Just check that this doesn't crash.
+
+//--- a.cppm
+module;
+
+template <typename _Visitor>
+void __do_visit(_Visitor &&__visitor) {
+ using _V0 = int;
+ [](_V0 __v) -> _V0 { return __v; } (1);
+}
+
+export module A;
+
+void g() {
+ struct Visitor { };
+ __do_visit(Visitor());
+}
+
+//--- b.cppm
+module;
+
+template <typename _Visitor>
+void __do_visit(_Visitor &&__visitor) {
+ using _V0 = int;
+
+ // Check that we instantiate this lambda's call operator in 'f' below
+ // instead of the one in 'a.cppm' here; otherwise, we won't find a
+ // corresponding instantiation of the using declaration above.
+ [](_V0 __v) -> _V0 { return __v; } (1);
+}
+
+export module B;
+import A;
+
+void f() {
+ __do_visit(1);
+}
|
There was a problem hiding this comment.
Thanks for the analysis!
Further more, if we want to fix such issue better, may be we need to refactor the current lookup method into a version that understands modules, then we can do better lookup with modules. This is helpful for #90154. But this is not required here.
See issue #110401.
This is a fix for the following issue: when a lambda’s class type is merged across modules (e.g. because it is defined in a template in the GMF of some module
A, and some other moduleBboth importsAand has the same template in its GMF), thengetLambdaCallOperator()might return the wrong operator (e.g. while compilingB, the lambda’s class type would be the one attached toB’s GMF, but the call operator ends up being the one attached toA’s GMF).This causes issues in situations where the call operator is in a template and accesses declarations in the surrounding context: when those declarations are instantated, a mapping is introduced from the original node in the template to that of the instantiation. If such an instantiation happens in
B, and we then try to instantiateA’s call operator, any nodes in that call operator refer to declarations in the template inA, but theLocalInstantiationScopeonly contains mappings for declarations inB! This causes the following assertion (for godbolt links and more, see the issue above):My tentative fix for this is to first search our own methods for a call operator ingetLambdaCallOperatorHelper, and only fall back to a regular lookup if that fails. This works, but I’m also not terribly familiar with modules, and it seems like a rather hacky solution to me, so please let me know if you have any better ideas...Update: We now walk the redecl chain of the call operator to find the one that is in the same module as the record decl.
This fixes #110401.