[RFC][C++20][Modules] Fix crash when function and lambda inside loaded from different modules #104512
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Dmitry Polukhin (dmpolukhin) ChangesSummary: I’m not sure that it is the right fix for the problem so any ideas how to fix it better are very appreciated. Test Plan: check-clang Full diff: https://github.com/llvm/llvm-project/pull/104512.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 9a6cd2cd0ab751..f784de3a20acf2 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -4363,6 +4363,11 @@ LocalInstantiationScope::findInstantiationOf(const Decl *D) {
// a previous declaration.
if (const TagDecl *Tag = dyn_cast<TagDecl>(CheckD))
CheckD = Tag->getPreviousDecl();
+ else if (const VarDecl *VD = dyn_cast<VarDecl>(CheckD))
+ // Check re-declaration chain for variable to deduplicate variables
+ // that might be captured inside lambdas. Function and lambda class
+ // inside can be loaded from different modules.
+ CheckD = VD->getPreviousDecl();
else
CheckD = nullptr;
} while (CheckD);
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index c118f3818467d9..bd46ac3f29af56 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -3286,6 +3286,13 @@ DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader,
if (auto *TU = dyn_cast<TranslationUnitDecl>(DC))
return TU->getPrimaryContext();
+ // Merge VarDecls inside functions to deduplicate variables that might be
+ // captured inside lambdas. Function and lambda class inside can be loaded
+ // from different modules.
+ if (auto *FD = dyn_cast<FunctionDecl>(DC))
+ if (FD->getOwningModule())
+ return FD->getCanonicalDecl();
+
return nullptr;
}
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules.cpp b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules.cpp
new file mode 100644
index 00000000000000..80844a58ad825a
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules.cpp
@@ -0,0 +1,76 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header -Werror=uninitialized folly-conv.h
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header -Werror=uninitialized thrift_cpp2_base.h
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header -Werror=uninitialized -fmodule-file=folly-conv.pcm -fmodule-file=thrift_cpp2_base.pcm logger_base.h
+
+//--- Conv.h
+#pragma once
+
+template <typename _Tp, typename _Up = _Tp&&>
+_Up __declval(int);
+
+template <typename _Tp>
+auto declval() noexcept -> decltype(__declval<_Tp>(0));
+
+namespace folly {
+
+template <class Value, class Error>
+struct Expected {
+ template <class Yes>
+ auto thenOrThrow() -> decltype(declval<Value&>()) {
+ return 1;
+ }
+};
+
+struct ExpectedHelper {
+ template <class Error, class T>
+ static constexpr Expected<T, Error> return_(T) {
+ return Expected<T, Error>();
+ }
+
+ template <class This, class Fn, class E = int, class T = ExpectedHelper>
+ static auto then_(This&&, Fn&&)
+ -> decltype(T::template return_<E>((declval<Fn>()(true), 0))) {
+ return Expected<int, int>();
+ }
+};
+
+template <class Tgt>
+inline Expected<Tgt, const char*> tryTo() {
+ Tgt result = 0;
+ // In build with asserts:
+ // clang/lib/Sema/SemaTemplateInstantiate.cpp: llvm::PointerUnion<Decl *, LocalInstantiationScope::DeclArgumentPack *> *clang::LocalInstantiationScope::findInstantiationOf(const Decl *): Assertion `isa<LabelDecl>(D) && "declaration not instantiated in this scope"' failed.
+ // In release build compilation error on the line below inside lambda:
+ // error: variable 'result' is uninitialized when used here [-Werror,-Wuninitialized]
+ ExpectedHelper::then_(Expected<bool, int>(), [&](bool) { return result; });
+ return {};
+}
+
+} // namespace folly
+
+inline void bar() {
+ folly::tryTo<int>();
+}
+// expected-no-diagnostics
+
+//--- folly-conv.h
+#pragma once
+#include "Conv.h"
+// expected-no-diagnostics
+
+//--- thrift_cpp2_base.h
+#pragma once
+#include "Conv.h"
+// expected-no-diagnostics
+
+//--- logger_base.h
+#pragma once
+import "folly-conv.h";
+import "thrift_cpp2_base.h";
+
+inline void foo() {
+ folly::tryTo<unsigned>();
+}
+// expected-no-diagnostics
|
ChuanqiXu9
reviewed
Aug 16, 2024
There was a problem hiding this comment.
I feel the change is somewhat odd to me. Since the description is about the lambda but the change is about VarDecls. I feel there is a mismatch and I feel this may not solve the underlying problem fundamentally. I feel it'll be better to make it more straight forward. e.g., load the context when loading the lambda (or only make it if there is captures.)
It was my initial approach to make sure that clang loads function and its lambdas from the same module. But I haven't found examples in clang how to make that canonical decls for two related things are always loaded from the same module. Clang starts loading function |
|
Here is another example that merging lambdas are problematic: #102721. Although I think we need to solve the problems separately. They are different problems.
But the current patch still smells bad. Let's try to find a cleaner way to proceed. IIUC, the key of the current problem is that, the CXXRecordDecl of the lambda are assigned to the wrong FunctionDecl as the DeclContext? Or the LambdaExpr appears in the wrong FunctionDecl? |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Thank you for the reference, we need this fix too.
Yeah, I agree. Problem with current code is not wrong assignment (everything match respectively) but which decl becomes canonical. Function decl from module A became canonical but for lambda canonical decl is chosen from module B. It happens because of the order of loading template specialization. Lookup by name makes canonical decl for function It doesn't completely solve the issue because it might be other code paths that result making canonical decl in different order. Alternative solution is abandon lazy loading for function and load its internal lambda right after making any function canonical. I think it will negatively affect performance because more things will be loaded that can be avoided with current approach. WDYT? |
|
Thanks. So the problem is that, when we load a function Decl as a canonical decl, we may not load its corresponding lambda as the canonical decl in the context. Right? And I still think the current solution doesn't handle the problem directly and I still feel it is somewhat tricky. Especially I had a patch to change the order of loading specializations (#83237). And I am still slightly unclear about how we load the wrong lambda first. I mean, do we load the wrong lambda by loading the body of the wrong function? If yes, can we try to load the body of the canonical decl before loading the body of the function? Another idea is what you described to load the body of the canonical decl eagerly. And it looks indeed not good. But we can do some pruning for it. e.g., we can record if its body contains a lambda and if its lambda has a capture to its local variables. The first suggestion looks better if it is doable. The second one may be simpler and more stable but not the best for the performance. |
Here is stack trace how decl from wrong module becomes canonical. Full stack traceShort version: My testing on real code showed that reverting the order of loading specialization is not enough if instead of two modules there are much more modules. Therefore I'm keep working on a solution that works on real example with many modules. |
|
@ChuanqiXu9 please take another look. I changed approach to remember which lambdas exist in FunctionDecl and load them for the first decl. I'm still testing it but interested to know does it look like something that address your concernes? |
|
Yeah, while the implementation need to be polished, the shape is what I want. |
|
Quick update, all clang/cxx-library tests pass with the change but when I test it on my real project with modules I see crash or assert in ASTDeclMerger::MergeDefinitionData about faked lambda. I'm still creating small reproducer but it looks like it happens when lambda is inside class member function + some other conditions that I don't understand yet. |
Thanks. And I am curious about your real project, is it open source? |
dmpolukhin
force-pushed
the
assert-not-instantiated-in-scope-cxx-modules
branch
from
d7db624 to
90de760
Compare
Unfortunately, it is not an open source project but we use bunch of OSS libraries like Folly, Thrift and many other so I see bugs that cannot be easily minimized and reproduced. I added test-case for the crash I observe (now asserts happens in different place but it is the same root cause that I changed order of lambda deserialization). I'm still working on a fix. |
Got it. I am pretty interested use case for modules. And from your commit history, it looks like you prefer header units than named modules? |
Yes, we prefer header units for the time being because it can be implemented with minimal sources changes and gives moderate compile time improvements. Also it should open future path to named modules. Also with header units we have some flexibility how to compose modules. In our experiments build time perforce significantly depends on number of modules required for compilation number of AST nodes that are merged. |
Got it. Looking for your public reports : ) |
…d from different modules
Summary:
Because AST loading code is lazy and happens in unpredictable order it could happen that function and lambda inside function can be loaded from different modules. In this case, captured DeclRefExpr won’t match the corresponding VarDecl inside function. In AST it looks like this:
```
FunctionDecl 0x555564f4aff0 <Conv.h:33:1, line:41:1> line:33:35 imported in ./thrift_cpp2_base.h hidden tryTo 'Expected<Tgt, const char *> ()' inline
|-also in ./folly-conv.h
`-CompoundStmt 0x555564f7cfc8 <col:43, line:41:1>
|-DeclStmt 0x555564f7ced8 <line:34:3, col:17>
| `-VarDecl 0x555564f7cef8 <col:3, col:16> col:7 imported in ./thrift_cpp2_base.h hidden referenced result 'Tgt' cinit
| `-IntegerLiteral 0x555564f7d080 <col:16> 'int' 0
|-CallExpr 0x555564f7cea8 <line:39:3, col:76> '<dependent type>'
| |-UnresolvedLookupExpr 0x555564f7bea0 <col:3, col:19> '<overloaded function type>' lvalue (no ADL) = 'then_' 0x555564f7bef0
| |-CXXTemporaryObjectExpr 0x555564f7bcb0 <col:25, col:45> 'Expected<bool, int>':'folly::Expected<bool, int>' 'void () noexcept' zeroing
| `-LambdaExpr 0x555564f7bc88 <col:48, col:75> '(lambda at Conv.h:39:48)'
| |-CXXRecordDecl 0x555564f76b88 <col:48> col:48 imported in ./folly-conv.h hidden implicit <undeserialized declarations> class definition
| | |-also in ./thrift_cpp2_base.h
| | `-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init
| | |-DefaultConstructor defaulted_is_constexpr
| | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveConstructor exists simple trivial needs_implicit
| | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveAssignment
| | `-Destructor simple irrelevant trivial constexpr needs_implicit
| `-CompoundStmt 0x555564f7d1a8 <col:58, col:75>
| `-ReturnStmt 0x555564f7d198 <col:60, col:67>
| `-DeclRefExpr 0x555564f7d0a0 <col:67> 'Tgt' lvalue Var 0x555564f7d0c8 'result' 'Tgt' refers_to_enclosing_variable_or_capture
`-ReturnStmt 0x555564f7bc78 <line:40:3, col:11>
`-InitListExpr 0x555564f7bc38 <col:10, col:11> 'void'
```
I’m not sure that it is the right fix for the problem so any ideas how to fix it better are very appreciated.
Test Plan: check-clang
…d from different modules
Summary:
Because AST loading code is lazy and happens in unpredictable order it could happen that function and lambda inside function can be loaded from different modules. In this case, captured DeclRefExpr won’t match the corresponding VarDecl inside function. In AST it looks like this:
```
FunctionDecl 0x555564f4aff0 <Conv.h:33:1, line:41:1> line:33:35 imported in ./thrift_cpp2_base.h hidden tryTo 'Expected<Tgt, const char *> ()' inline
|-also in ./folly-conv.h
`-CompoundStmt 0x555564f7cfc8 <col:43, line:41:1>
|-DeclStmt 0x555564f7ced8 <line:34:3, col:17>
| `-VarDecl 0x555564f7cef8 <col:3, col:16> col:7 imported in ./thrift_cpp2_base.h hidden referenced result 'Tgt' cinit
| `-IntegerLiteral 0x555564f7d080 <col:16> 'int' 0
|-CallExpr 0x555564f7cea8 <line:39:3, col:76> '<dependent type>'
| |-UnresolvedLookupExpr 0x555564f7bea0 <col:3, col:19> '<overloaded function type>' lvalue (no ADL) = 'then_' 0x555564f7bef0
| |-CXXTemporaryObjectExpr 0x555564f7bcb0 <col:25, col:45> 'Expected<bool, int>':'folly::Expected<bool, int>' 'void () noexcept' zeroing
| `-LambdaExpr 0x555564f7bc88 <col:48, col:75> '(lambda at Conv.h:39:48)'
| |-CXXRecordDecl 0x555564f76b88 <col:48> col:48 imported in ./folly-conv.h hidden implicit <undeserialized declarations> class definition
| | |-also in ./thrift_cpp2_base.h
| | `-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init
| | |-DefaultConstructor defaulted_is_constexpr
| | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveConstructor exists simple trivial needs_implicit
| | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveAssignment
| | `-Destructor simple irrelevant trivial constexpr needs_implicit
| `-CompoundStmt 0x555564f7d1a8 <col:58, col:75>
| `-ReturnStmt 0x555564f7d198 <col:60, col:67>
| `-DeclRefExpr 0x555564f7d0a0 <col:67> 'Tgt' lvalue Var 0x555564f7d0c8 'result' 'Tgt' refers_to_enclosing_variable_or_capture
`-ReturnStmt 0x555564f7bc78 <line:40:3, col:11>
`-InitListExpr 0x555564f7bc38 <col:10, col:11> 'void'
```
I’m not sure that it is the right fix for the problem so any ideas how to fix it better are very appreciated.
Test Plan: check-clang
This fixes a build break from [llvm/llvm-project] Reland [CGData] llvm-cgdata llvm#89884 (PR llvm#101461)
Add missing dependency that sometimes makes a build fails with ninja.
This fixes a build break from [llvm/llvm-project] Reland [CGData] llvm-cgdata llvm#89884 (PR llvm#101461)
dmpolukhin
force-pushed
the
assert-not-instantiated-in-scope-cxx-modules
branch
from
90de760 to
b5f6a30
Compare
|
@ChuanqiXu9 please take a look, I had to load lambdas delayed to avoid bad cycles when current function was not fully deserialized yet but lambda inside reference it. Now this change fixes my original issue and doesn't introduce any new crashes on our codebase so it is step forward. Unfortunately it seems that it doesn't fix all lambda related issues but I need to reduce them and fix. |
ChuanqiXu9
reviewed
Sep 9, 2024
Comment on lines
1192
to
1193
| /// It is required to have the right canonical declaration for lambda class | ||
| /// from the same module as the function. |
There was a problem hiding this comment.
nit: maybe it is better to expand this statement as we did in this patch. So that other readers can have a better understanding for it.
| for (unsigned I = 0; I != NumLambdas; ++I) | ||
| Reader.PendingLambdas.push_back(Record.readDeclID()); | ||
| } else { | ||
| (void)Record.readIntArray(NumLambdas); |
There was a problem hiding this comment.
nit: skipInts might be better API here.
There was a problem hiding this comment.
Done, thank for the suggestion, I missed this function.
Comment on lines
799
to
802
| llvm::SmallVector<const Decl *, 2> Lambdas = collectLambdas(D); | ||
| Record.push_back(Lambdas.size()); | ||
| for (const auto *L : Lambdas) | ||
| Record.AddDeclRef(L); |
There was a problem hiding this comment.
Is it good to only do this for the first decl?
There was a problem hiding this comment.
Seems that doing it only for canonical is good enough, at least I don't see other examples.
Comment on lines
799
to
802
| llvm::SmallVector<const Decl *, 2> Lambdas = collectLambdas(D); | ||
| Record.push_back(Lambdas.size()); | ||
| for (const auto *L : Lambdas) | ||
| Record.AddDeclRef(L); |
There was a problem hiding this comment.
Also probably we need to update the abbreviation. You need to look at DeclCXXMethodAbbrev and getFunctionDeclAbbrev.
There was a problem hiding this comment.
it seems that it is not required but added comment there to clarify it.
|
@ChuanqiXu9 thank you a lot for the review and help with the changes, very appreciated! |
|
this is causing a crash with precompiled headers. I can try to reduce, but hopefully the problem is obvious from this assert/stack trace? |
|
I am seeing internal crashes due to this as well. I am reverting this as it's causing issues in chromium open-source project as well as @aeubanks mentioned above. Feel free to reland. |
pranavk
added a commit
to pranavk/llvm-project
that referenced
this pull request
Sep 12, 2024
…de loaded from different modules (llvm#104512)" This reverts commit d778689.
ChuanqiXu9
reviewed
Sep 12, 2024
| for (unsigned I = 0; I != NumLambdas; ++I) | ||
| Reader.PendingLambdas.push_back(Record.readDeclID()); | ||
| } else { | ||
| Record.skipInts(NumLambdas); |
There was a problem hiding this comment.
Here is not paired with writing process. It will only write a zero if it is not the first.
There was a problem hiding this comment.
@ChuanqiXu9 could you please elaborate why you think it may not be paired? Code in ASTDeclWriter looks like this:
if (D->isCanonicalDecl()) {
llvm::SmallVector<const Decl *, 2> Lambdas = collectLambdas(D);
Record.push_back(Lambdas.size());
for (const auto *L : Lambdas)
Record.AddDeclRef(L);
} else {
Record.push_back(0);
}
So in the first case it writes size + following decls, in the second case it writes 0. I looked the implementation of skipInts for skipInts(0) it does nothing.
There was a problem hiding this comment.
Sorry, it was an oversight. I took too quickly from the above stack trace.
@aeubanks could you please create a reproducer or at least some instruction how it can be reproduced? From the stack trace it is not clear. The crash happens on translating GlobalDeclID but it has no clue what was wrong with the value. |
|
let me try to come up with a reduced repro |
|
edit: got a better reduced repro, see attachment |
Thank you for the reproducer, I see the crash and I'm working on it. |
dmpolukhin
added a commit
that referenced
this pull request
Sep 25, 2024
…m different modules (#109167) Summary: Because AST loading code is lazy and happens in unpredictable order, it is possible that a function and lambda inside the function can be loaded from different modules. As a result, the captured DeclRefExpr won’t match the corresponding VarDecl inside the function. This situation is reflected in the AST as follows: ``` FunctionDecl 0x555564f4aff0 <Conv.h:33:1, line:41:1> line:33:35 imported in ./thrift_cpp2_base.h hidden tryTo 'Expected<Tgt, const char *> ()' inline |-also in ./folly-conv.h `-CompoundStmt 0x555564f7cfc8 <col:43, line:41:1> |-DeclStmt 0x555564f7ced8 <line:34:3, col:17> | `-VarDecl 0x555564f7cef8 <col:3, col:16> col:7 imported in ./thrift_cpp2_base.h hidden referenced result 'Tgt' cinit | `-IntegerLiteral 0x555564f7d080 <col:16> 'int' 0 |-CallExpr 0x555564f7cea8 <line:39:3, col:76> '<dependent type>' | |-UnresolvedLookupExpr 0x555564f7bea0 <col:3, col:19> '<overloaded function type>' lvalue (no ADL) = 'then_' 0x555564f7bef0 | |-CXXTemporaryObjectExpr 0x555564f7bcb0 <col:25, col:45> 'Expected<bool, int>':'folly::Expected<bool, int>' 'void () noexcept' zeroing | `-LambdaExpr 0x555564f7bc88 <col:48, col:75> '(lambda at Conv.h:39:48)' | |-CXXRecordDecl 0x555564f76b88 <col:48> col:48 imported in ./folly-conv.h hidden implicit <undeserialized declarations> class definition | | |-also in ./thrift_cpp2_base.h | | `-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init | | |-DefaultConstructor defaulted_is_constexpr | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param | | |-MoveConstructor exists simple trivial needs_implicit | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param | | |-MoveAssignment | | `-Destructor simple irrelevant trivial constexpr needs_implicit | `-CompoundStmt 0x555564f7d1a8 <col:58, col:75> | `-ReturnStmt 0x555564f7d198 <col:60, col:67> | `-DeclRefExpr 0x555564f7d0a0 <col:67> 'Tgt' lvalue Var 0x555564f7d0c8 'result' 'Tgt' refers_to_enclosing_variable_or_capture `-ReturnStmt 0x555564f7bc78 <line:40:3, col:11> `-InitListExpr 0x555564f7bc38 <col:10, col:11> 'void' ``` This diff modifies the AST deserialization process to load lambdas within the canonical function declaration sooner, immediately following the function, ensuring that they are loaded from the same module. Re-land #104512 Added test case that caused crash due to multiple enclosed lambdas deserialization. Test Plan: check-clang
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Because AST loading code is lazy and happens in unpredictable order it could happen that function and lambda inside function can be loaded from different modules. In this case, captured DeclRefExpr won’t match the corresponding VarDecl inside function. In AST it looks like this:
This diff changes AST deserialization to load lambdas inside canonical function declaration earlier right after the function to make sure that they loaded from the same module.
Test Plan: check-clang