Skip to content

Commit d778689

Browse files
dmpolukhindmpolukhin
authored
[RFC][C++20][Modules] Fix crash when function and lambda inside loaded from different modules (llvm#104512)
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' ``` This diff changes AST deserialization to load lambdas inside canonical function declaration earlier right after the function to make sure that their canonical decl is loaded from the same module. Test Plan: check-clang
1 parent 56a0334 commit d778689

File tree

6 files changed

+174
-1
lines changed

6 files changed

+174
-1
lines changed

‎clang/include/clang/Serialization/ASTReader.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1188,6 +1188,15 @@ class ASTReader
11881188
/// once recursing loading has been completed.
11891189
llvm::SmallVector<NamedDecl *, 16> PendingOdrMergeChecks;
11901190

1191+
/// Lambdas that need to be loaded right after the function they belong to.
1192+
/// It is required to have canonical declaration for lambda class from the
1193+
/// same module as enclosing function. This is required to correctly resolve
1194+
/// captured variables in the lambda. Without this, due to lazy
1195+
/// deserialization canonical declarations for the function and lambdas can
1196+
/// be from different modules and DeclRefExprs may refer to the AST nodes
1197+
/// that don't exist in the function.
1198+
SmallVector<GlobalDeclID, 4> PendingLambdas;
1199+
11911200
using DataPointers =
11921201
std::pair<CXXRecordDecl *, struct CXXRecordDecl::DefinitionData *>;
11931202
using ObjCInterfaceDataPointers =

‎clang/lib/Serialization/ASTReader.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9782,7 +9782,8 @@ void ASTReader::finishPendingActions() {
97829782
!PendingDeducedVarTypes.empty() || !PendingIncompleteDeclChains.empty() ||
97839783
!PendingDeclChains.empty() || !PendingMacroIDs.empty() ||
97849784
!PendingDeclContextInfos.empty() || !PendingUpdateRecords.empty() ||
9785-
!PendingObjCExtensionIvarRedeclarations.empty()) {
9785+
!PendingObjCExtensionIvarRedeclarations.empty() ||
9786+
!PendingLambdas.empty()) {
97869787
// If any identifiers with corresponding top-level declarations have
97879788
// been loaded, load those declarations now.
97889789
using TopLevelDeclsMap =
@@ -9927,6 +9928,11 @@ void ASTReader::finishPendingActions() {
99279928
}
99289929
PendingObjCExtensionIvarRedeclarations.pop_back();
99299930
}
9931+
9932+
// Load any pendiong lambdas.
9933+
for (auto ID : PendingLambdas)
9934+
GetDecl(ID);
9935+
PendingLambdas.clear();
99309936
}
99319937

99329938
// At this point, all update records for loaded decls are in place, so any

‎clang/lib/Serialization/ASTReaderDecl.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1155,6 +1155,16 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
11551155
for (unsigned I = 0; I != NumParams; ++I)
11561156
Params.push_back(readDeclAs<ParmVarDecl>());
11571157
FD->setParams(Reader.getContext(), Params);
1158+
1159+
// For the first decl add all lambdas inside for loading them later,
1160+
// otherwise skip them.
1161+
unsigned NumLambdas = Record.readInt();
1162+
if (FD->isFirstDecl()) {
1163+
for (unsigned I = 0; I != NumLambdas; ++I)
1164+
Reader.PendingLambdas.push_back(Record.readDeclID());
1165+
} else {
1166+
Record.skipInts(NumLambdas);
1167+
}
11581168
}
11591169

11601170
void ASTDeclReader::VisitObjCMethodDecl(ObjCMethodDecl *MD) {

‎clang/lib/Serialization/ASTWriterDecl.cpp

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "clang/AST/Expr.h"
1919
#include "clang/AST/OpenMPClause.h"
2020
#include "clang/AST/PrettyDeclStackTrace.h"
21+
#include "clang/AST/StmtVisitor.h"
2122
#include "clang/Basic/SourceManager.h"
2223
#include "clang/Serialization/ASTReader.h"
2324
#include "clang/Serialization/ASTRecordWriter.h"
@@ -625,6 +626,33 @@ void ASTDeclWriter::VisitDeclaratorDecl(DeclaratorDecl *D) {
625626
: QualType());
626627
}
627628

629+
static llvm::SmallVector<const Decl *, 2> collectLambdas(FunctionDecl *D) {
630+
struct LambdaCollector : public ConstStmtVisitor<LambdaCollector> {
631+
llvm::SmallVectorImpl<const Decl *> &Lambdas;
632+
633+
LambdaCollector(llvm::SmallVectorImpl<const Decl *> &Lambdas)
634+
: Lambdas(Lambdas) {}
635+
636+
void VisitLambdaExpr(const LambdaExpr *E) {
637+
VisitStmt(E);
638+
Lambdas.push_back(E->getLambdaClass());
639+
}
640+
641+
void VisitStmt(const Stmt *S) {
642+
if (!S)
643+
return;
644+
for (const Stmt *Child : S->children())
645+
if (Child)
646+
Visit(Child);
647+
}
648+
};
649+
650+
llvm::SmallVector<const Decl *, 2> Lambdas;
651+
if (D->hasBody())
652+
LambdaCollector(Lambdas).VisitStmt(D->getBody());
653+
return Lambdas;
654+
}
655+
628656
void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
629657
static_assert(DeclContext::NumFunctionDeclBits == 44,
630658
"You need to update the serializer after you change the "
@@ -764,6 +792,19 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
764792
Record.push_back(D->param_size());
765793
for (auto *P : D->parameters())
766794
Record.AddDeclRef(P);
795+
796+
// Store references to all lambda decls inside function to load them
797+
// immediately after loading the function to make sure that canonical
798+
// decls for lambdas will be from the same module.
799+
if (D->isCanonicalDecl()) {
800+
llvm::SmallVector<const Decl *, 2> Lambdas = collectLambdas(D);
801+
Record.push_back(Lambdas.size());
802+
for (const auto *L : Lambdas)
803+
Record.AddDeclRef(L);
804+
} else {
805+
Record.push_back(0);
806+
}
807+
767808
Code = serialization::DECL_FUNCTION;
768809
}
769810

@@ -2239,6 +2280,7 @@ getFunctionDeclAbbrev(serialization::DeclCode Code) {
22392280
//
22402281
// This is:
22412282
// NumParams and Params[] from FunctionDecl, and
2283+
// NumLambdas, Lambdas[] from FunctionDecl, and
22422284
// NumOverriddenMethods, OverriddenMethods[] from CXXMethodDecl.
22432285
//
22442286
// Add an AbbrevOp for 'size then elements' and use it here.

‎clang/test/Headers/crash-instantiated-in-scope-cxx-modules.cpp

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// RUN: rm -fR %t
2+
// RUN: split-file %s %t
3+
// RUN: cd %t
4+
// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header -Werror=uninitialized folly-conv.h
5+
// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header -Werror=uninitialized thrift_cpp2_base.h
6+
// 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
7+
8+
//--- Conv.h
9+
#pragma once
10+
11+
template <typename _Tp, typename _Up = _Tp&&>
12+
_Up __declval(int);
13+
14+
template <typename _Tp>
15+
auto declval() noexcept -> decltype(__declval<_Tp>(0));
16+
17+
namespace folly {
18+
19+
template <class Value, class Error>
20+
struct Expected {
21+
template <class Yes>
22+
auto thenOrThrow() -> decltype(declval<Value&>()) {
23+
return 1;
24+
}
25+
};
26+
27+
struct ExpectedHelper {
28+
template <class Error, class T>
29+
static constexpr Expected<T, Error> return_(T) {
30+
return Expected<T, Error>();
31+
}
32+
33+
template <class This, class Fn, class E = int, class T = ExpectedHelper>
34+
static auto then_(This&&, Fn&&)
35+
-> decltype(T::template return_<E>((declval<Fn>()(true), 0))) {
36+
return Expected<int, int>();
37+
}
38+
};
39+
40+
template <class Tgt>
41+
inline Expected<Tgt, const char*> tryTo() {
42+
Tgt result = 0;
43+
// In build with asserts:
44+
// 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.
45+
// In release build compilation error on the line below inside lambda:
46+
// error: variable 'result' is uninitialized when used here [-Werror,-Wuninitialized]
47+
ExpectedHelper::then_(Expected<bool, int>(), [&](bool) { return result; });
48+
return {};
49+
}
50+
51+
} // namespace folly
52+
53+
inline void bar() {
54+
folly::tryTo<int>();
55+
}
56+
// expected-no-diagnostics
57+
58+
//--- folly-conv.h
59+
#pragma once
60+
#include "Conv.h"
61+
// expected-no-diagnostics
62+
63+
//--- thrift_cpp2_base.h
64+
#pragma once
65+
#include "Conv.h"
66+
// expected-no-diagnostics
67+
68+
//--- logger_base.h
69+
#pragma once
70+
import "folly-conv.h";
71+
import "thrift_cpp2_base.h";
72+
73+
inline void foo() {
74+
folly::tryTo<unsigned>();
75+
}
76+
// expected-no-diagnostics

‎clang/test/Headers/crash-instantiated-in-scope-cxx-modules2.cpp

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// RUN: rm -fR %t
2+
// RUN: split-file %s %t
3+
// RUN: cd %t
4+
// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header header.h
5+
// RUN: %clang_cc1 -std=c++20 -fmodule-file=header.pcm main.cpp
6+
7+
//--- header.h
8+
template <typename T>
9+
void f(T) {}
10+
11+
class A {
12+
virtual ~A();
13+
};
14+
15+
inline A::~A() {
16+
f([](){});
17+
}
18+
19+
struct B {
20+
void g() {
21+
f([](){
22+
[](){};
23+
});
24+
}
25+
};
26+
// expected-no-diagnostics
27+
28+
//--- main.cpp
29+
import "header.h";
30+
// expected-no-diagnostics

0 commit comments

Comments
 (0)