[C++20][Modules] Fix crash when function and lambda inside loaded from different modules #109167
Conversation
…m different modules
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 llvm#104512
Added test case that caused crash due to multiple enclosed lambdas
deserialization.
Test Plan: check-clang
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Dmitry Polukhin (dmpolukhin) ChangesSummary: 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 Full diff: https://github.com/llvm/llvm-project/pull/109167.diff 7 Files Affected:
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 898f4392465fdf..1b3812e7fd4523 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -1188,6 +1188,15 @@ class ASTReader
/// once recursing loading has been completed.
llvm::SmallVector<NamedDecl *, 16> PendingOdrMergeChecks;
+ /// Lambdas that need to be loaded immediately after the function they belong
+ /// to. It is necessary to have a canonical declaration for the lambda class
+ /// from the same module as the enclosing function. This requirement ensures
+ /// the correct resolution of captured variables in the lambda. Without this,
+ /// due to lazy deserialization, canonical declarations for the function and
+ /// lambdas can be from different modules, and DeclRefExprs may refer to AST
+ /// nodes that do not exist in the function.
+ SmallVector<GlobalDeclID, 4> PendingLambdas;
+
using DataPointers =
std::pair<CXXRecordDecl *, struct CXXRecordDecl::DefinitionData *>;
using ObjCInterfaceDataPointers =
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 7efcc81e194d95..d7bad6bbad0c90 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9777,7 +9777,8 @@ void ASTReader::finishPendingActions() {
!PendingDeducedVarTypes.empty() || !PendingIncompleteDeclChains.empty() ||
!PendingDeclChains.empty() || !PendingMacroIDs.empty() ||
!PendingDeclContextInfos.empty() || !PendingUpdateRecords.empty() ||
- !PendingObjCExtensionIvarRedeclarations.empty()) {
+ !PendingObjCExtensionIvarRedeclarations.empty() ||
+ !PendingLambdas.empty()) {
// If any identifiers with corresponding top-level declarations have
// been loaded, load those declarations now.
using TopLevelDeclsMap =
@@ -9922,6 +9923,17 @@ void ASTReader::finishPendingActions() {
}
PendingObjCExtensionIvarRedeclarations.pop_back();
}
+
+ // Load any pending lambdas. During the deserialization of pending lambdas,
+ // more lambdas can be discovered, so swap the current PendingLambdas with a
+ // local empty vector. Newly discovered lambdas will be deserialized in the
+ // next iteration.
+ if (!PendingLambdas.empty()) {
+ SmallVector<GlobalDeclID, 4> DeclIDs;
+ DeclIDs.swap(PendingLambdas);
+ for (auto ID : DeclIDs)
+ GetDecl(ID);
+ }
}
// At this point, all update records for loaded decls are in place, so any
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 9272e23c7da3fc..20e577404d997d 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1155,6 +1155,16 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
for (unsigned I = 0; I != NumParams; ++I)
Params.push_back(readDeclAs<ParmVarDecl>());
FD->setParams(Reader.getContext(), Params);
+
+ // For the first decl add all lambdas inside for loading them later,
+ // otherwise skip them.
+ unsigned NumLambdas = Record.readInt();
+ if (FD->isFirstDecl()) {
+ for (unsigned I = 0; I != NumLambdas; ++I)
+ Reader.PendingLambdas.push_back(Record.readDeclID());
+ } else {
+ Record.skipInts(NumLambdas);
+ }
}
void ASTDeclReader::VisitObjCMethodDecl(ObjCMethodDecl *MD) {
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 555f6325da646b..732a6e21f340d6 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -18,6 +18,7 @@
#include "clang/AST/Expr.h"
#include "clang/AST/OpenMPClause.h"
#include "clang/AST/PrettyDeclStackTrace.h"
+#include "clang/AST/StmtVisitor.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Serialization/ASTReader.h"
#include "clang/Serialization/ASTRecordWriter.h"
@@ -625,6 +626,33 @@ void ASTDeclWriter::VisitDeclaratorDecl(DeclaratorDecl *D) {
: QualType());
}
+static llvm::SmallVector<const Decl *, 2> collectLambdas(FunctionDecl *D) {
+ struct LambdaCollector : public ConstStmtVisitor<LambdaCollector> {
+ llvm::SmallVectorImpl<const Decl *> &Lambdas;
+
+ LambdaCollector(llvm::SmallVectorImpl<const Decl *> &Lambdas)
+ : Lambdas(Lambdas) {}
+
+ void VisitLambdaExpr(const LambdaExpr *E) {
+ VisitStmt(E);
+ Lambdas.push_back(E->getLambdaClass());
+ }
+
+ void VisitStmt(const Stmt *S) {
+ if (!S)
+ return;
+ for (const Stmt *Child : S->children())
+ if (Child)
+ Visit(Child);
+ }
+ };
+
+ llvm::SmallVector<const Decl *, 2> Lambdas;
+ if (D->hasBody())
+ LambdaCollector(Lambdas).VisitStmt(D->getBody());
+ return Lambdas;
+}
+
void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
static_assert(DeclContext::NumFunctionDeclBits == 44,
"You need to update the serializer after you change the "
@@ -764,6 +792,19 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
Record.push_back(D->param_size());
for (auto *P : D->parameters())
Record.AddDeclRef(P);
+
+ // Store references to all lambda decls inside function to load them
+ // immediately after loading the function to make sure that canonical
+ // decls for lambdas will be from the same module.
+ 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);
+ }
+
Code = serialization::DECL_FUNCTION;
}
@@ -2239,6 +2280,7 @@ getFunctionDeclAbbrev(serialization::DeclCode Code) {
//
// This is:
// NumParams and Params[] from FunctionDecl, and
+ // NumLambdas, Lambdas[] from FunctionDecl, and
// NumOverriddenMethods, OverriddenMethods[] from CXXMethodDecl.
//
// Add an AbbrevOp for 'size then elements' and use it here.
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
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules2.cpp b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules2.cpp
new file mode 100644
index 00000000000000..5b1a904e928a68
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules2.cpp
@@ -0,0 +1,30 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header header.h
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=header.pcm main.cpp
+
+//--- header.h
+template <typename T>
+void f(T) {}
+
+class A {
+ virtual ~A();
+};
+
+inline A::~A() {
+ f([](){});
+}
+
+struct B {
+ void g() {
+ f([](){
+ [](){};
+ });
+ }
+};
+// expected-no-diagnostics
+
+//--- main.cpp
+import "header.h";
+// expected-no-diagnostics
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules3.cpp b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules3.cpp
new file mode 100644
index 00000000000000..646ff9f745710b
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules3.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 %s -std=c++11 -emit-pch -o %t
+// RUN: %clang_cc1 %s -std=c++11 -include-pch %t -fsyntax-only -verify
+
+// expected-no-diagnostics
+#ifndef HEADER
+#define HEADER
+
+// No crash or assertion failure on multiple nested lambdas deserialization.
+template <typename T>
+void b() {
+ [] {
+ []{
+ []{
+ []{
+ []{
+ }();
+ }();
+ }();
+ }();
+ }();
+}
+
+void foo() {
+ b<int>();
+}
+#endif
|
|
Would you like to explain more why this fail previously in more detail? Also I am thinking if we can make the process more efficiently: I immediately expectation is that (1) is possible but (2) may not be not safe. For (1), my idea is to record/register the information during the writing of LambdaExpr in ASTWriterStmt and then we can write this record after we write all the decls and types. Then we need to read this record eagerly. Maybe we can optimize it further by adding a slot in FunctionDecl to record the offset of such informations. So we can avoid the eager linear reading process. For (2), whether or not it is safe is, can we we load the lambdas without loading the definition of the functions? If not, we can add a code when getting the definition of the functions like: Then we can delay the loading of the lambdas to make it more efficient. |
Original code in The issue here is that the code uses implicit iterators for In the new code, before reading the lambdas, I copy the existing lambdas to a new array. Alternatively, I could use an integer index for iteration and read the size of the vector on each iteration. Both approaches work fine, but I decided that running other pending actions might be better before starting to deserialize new lambdas. I can change it if you think iteration with an integer index is better.
Yeah, it will complicate things a lot, we visit statements only after serializing FunctionDecl. I run some profiling and it seems that
It will be too late in my example. Without my change lambdas get loaded during function body deserialization but it was too late because loading template specialization caused choosing canonical record for the lambda from another modules. Function itself is selected from the last loaded modules by lookup (i.e. it happens in reverse order of module loading) but template specialisations get loaded in direct order. I tried to change order of specilization loading to opposite but in real word it not always works because some modules can be loaded for example in case of |
|
With DeclContext visitor collectLambdas takes 0.03% or smaller, sometime I don't even see it in sampling profile. |
Thanks, it is clear.
it won't be much more complicated. You only need: I think the steps here are clear enough. On the one hand, 0.3% is not too small, we see 0.5% as significant in middle end. On the other hand, the steps are pretty common in Serializations. I think it will be helpful for you to understand the process. Sorry in a head that I approve the previous solution but now ask for something more complex. But let's try to do things better if possible.
So in short, in my question, can I treat your answer as, we can load a lambda or choose a lambda as the canonical decl without deserializing the corresponding body of the function decl? And if yes, can we try to explore in which situations? It will be helpful to optimize our strategy. |
|
@ChuanqiXu9 thank you for the detailed steps, I uploaded new version. Does it look like something you had in mind? |
|
Heads-up - this commit might have introduced compilation non-determinism. Will try to cook the reproducer soon. Meanwhile, can you please double-check there are no non-deterministic pieces, such as sorting by address, etc.? |
|
A comment on another non-determinism issue that might be related - #106855 (comment) |
|
@eaeltsin do you know which artifacts become non-deterministic? Is it produced object file or serialized AST? |
|
We see non-determinism in pcm files when compiling some stl stuff. Bisection pointed to this commit, but there is still some chance this is because of our internal changes though. |
|
I think it is due to this change :( I'm iterating over |
|
Perfect, thanks! |
Summary: llvm#109167 serialized FunctionToLambdasMap in the order of pointers in DenseMap. It gives different order with different memeory leayout. Fix this issue by using LocalDeclID instead of pointers. Test Plan: check-clang
Summary: #109167 serializes FunctionToLambdasMap in the order of pointers in DenseMap. It gives different order with different memory layouts. Fix this issue by using LocalDeclID instead of pointers. Test Plan: check-clang
Summary: llvm#109167 serializes FunctionToLambdasMap in the order of pointers in DenseMap. It gives different order with different memory layouts. Fix this issue by using LocalDeclID instead of pointers. Test Plan: check-clang
Summary: llvm/llvm-project#109167 serializes FunctionToLambdasMap in the order of pointers in DenseMap. It gives different order with different memory layouts. Fix this issue by using LocalDeclID instead of pointers. Test Plan: check-clang
Summary: llvm/llvm-project#109167 serializes FunctionToLambdasMap in the order of pointers in DenseMap. It gives different order with different memory layouts. Fix this issue by using LocalDeclID instead of pointers. Test Plan: check-clang
|
this has also caused the following issue to pop up in our modules build hopefully the error provides some guidance for where to look, but I can try to get a reduced repro |
Please create a reproducer without it is not possible to debug and fix. On a build without asserts my original example gives the same error. It means that there are cases when some lambda is not deserialized early enough. |
|
ah there's a stack trace in an asserts build of clang still trying to reduce the reproducer... |
Please create a reproducer, the stack trace doesn't have information to understand what is causing the issue. |
|
Here is the reproducer that crashes at head. |
…ical decl Summary: Fix crash from reproducer provided in llvm#109167 (comment) Test Plan: TBD
I reproduced this issue on my machine and confirm that it is indeed due to my changes. #111992 has fix that seems to fix the reproducer and doesn't introduce new issue as far as I know. |
I've checked #111992 and it does fix the problem, so let's land it? |
Yes, I would like to create a test case to don't regress this feature in future. I need to reduce libc++ |
Right, definitely +1 to adding a test case. I decided to hold off that comment until the PR moves away from the draft state, because I thought that's something that's on your mind anyway. |
…ical decl Summary: Fix crash from reproducer provided in llvm#109167 (comment) Test Plan: TBD
…ical decl Summary: Fix crash from reproducer provided in llvm#109167 (comment) Test Plan: TBD
…cal decl (#111992) Summary: Fix crash from reproducer provided in #109167 (comment) Also fix issues with merged inline friend functions merged during deserialization. Test Plan: check-clang
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:
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