From 8772795571c9e36c498cb3dba5496e695dfcb7a2 Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin Date: Thu, 15 Aug 2024 14:03:57 -0700 Subject: [PATCH 01/12] [RFC][C++20][Modules] Fix crash when function and lambda inside loaded from different modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 line:33:35 imported in ./thrift_cpp2_base.h hidden tryTo 'Expected ()' inline |-also in ./folly-conv.h `-CompoundStmt 0x555564f7cfc8 |-DeclStmt 0x555564f7ced8 | `-VarDecl 0x555564f7cef8 col:7 imported in ./thrift_cpp2_base.h hidden referenced result 'Tgt' cinit | `-IntegerLiteral 0x555564f7d080 'int' 0 |-CallExpr 0x555564f7cea8 '' | |-UnresolvedLookupExpr 0x555564f7bea0 '' lvalue (no ADL) = 'then_' 0x555564f7bef0 | |-CXXTemporaryObjectExpr 0x555564f7bcb0 'Expected':'folly::Expected' 'void () noexcept' zeroing | `-LambdaExpr 0x555564f7bc88 '(lambda at Conv.h:39:48)' | |-CXXRecordDecl 0x555564f76b88 col:48 imported in ./folly-conv.h hidden implicit 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 | `-ReturnStmt 0x555564f7d198 | `-DeclRefExpr 0x555564f7d0a0 'Tgt' lvalue Var 0x555564f7d0c8 'result' 'Tgt' refers_to_enclosing_variable_or_capture `-ReturnStmt 0x555564f7bc78 `-InitListExpr 0x555564f7bc38 '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 --- ...rash-instantiated-in-scope-cxx-modules.cpp | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 clang/test/Headers/crash-instantiated-in-scope-cxx-modules.cpp 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 0000000000000..80844a58ad825 --- /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 +_Up __declval(int); + +template +auto declval() noexcept -> decltype(__declval<_Tp>(0)); + +namespace folly { + +template +struct Expected { + template + auto thenOrThrow() -> decltype(declval()) { + return 1; + } +}; + +struct ExpectedHelper { + template + static constexpr Expected return_(T) { + return Expected(); + } + + template + static auto then_(This&&, Fn&&) + -> decltype(T::template return_((declval()(true), 0))) { + return Expected(); + } +}; + +template +inline Expected tryTo() { + Tgt result = 0; + // In build with asserts: + // clang/lib/Sema/SemaTemplateInstantiate.cpp: llvm::PointerUnion *clang::LocalInstantiationScope::findInstantiationOf(const Decl *): Assertion `isa(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) { return result; }); + return {}; +} + +} // namespace folly + +inline void bar() { + folly::tryTo(); +} +// 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(); +} +// expected-no-diagnostics From 524344e16691b6a5c84600a59d0c3a7225234bd0 Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin Date: Thu, 15 Aug 2024 14:03:57 -0700 Subject: [PATCH 02/12] [RFC][C++20][Modules] Fix crash when function and lambda inside loaded from different modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 line:33:35 imported in ./thrift_cpp2_base.h hidden tryTo 'Expected ()' inline |-also in ./folly-conv.h `-CompoundStmt 0x555564f7cfc8 |-DeclStmt 0x555564f7ced8 | `-VarDecl 0x555564f7cef8 col:7 imported in ./thrift_cpp2_base.h hidden referenced result 'Tgt' cinit | `-IntegerLiteral 0x555564f7d080 'int' 0 |-CallExpr 0x555564f7cea8 '' | |-UnresolvedLookupExpr 0x555564f7bea0 '' lvalue (no ADL) = 'then_' 0x555564f7bef0 | |-CXXTemporaryObjectExpr 0x555564f7bcb0 'Expected':'folly::Expected' 'void () noexcept' zeroing | `-LambdaExpr 0x555564f7bc88 '(lambda at Conv.h:39:48)' | |-CXXRecordDecl 0x555564f76b88 col:48 imported in ./folly-conv.h hidden implicit 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 | `-ReturnStmt 0x555564f7d198 | `-DeclRefExpr 0x555564f7d0a0 'Tgt' lvalue Var 0x555564f7d0c8 'result' 'Tgt' refers_to_enclosing_variable_or_capture `-ReturnStmt 0x555564f7bc78 `-InitListExpr 0x555564f7bc38 '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 --- clang/lib/Sema/SemaTemplateInstantiate.cpp | 5 +++++ clang/lib/Serialization/ASTReaderDecl.cpp | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index c42cc250bb904..c439d0deba29c 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -4387,6 +4387,11 @@ LocalInstantiationScope::findInstantiationOf(const Decl *D) { // a previous declaration. if (const TagDecl *Tag = dyn_cast(CheckD)) CheckD = Tag->getPreviousDecl(); + else if (const VarDecl *VD = dyn_cast(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 9272e23c7da3f..96d052b8cc529 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -3291,6 +3291,13 @@ DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader, if (auto *TU = dyn_cast(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(DC)) + if (FD->getOwningModule()) + return FD->getCanonicalDecl(); + return nullptr; } From f5606eb56b95046d1ad9b852f5a645af2f1c31c5 Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin Date: Tue, 20 Aug 2024 02:45:54 -0700 Subject: [PATCH 03/12] Try different approach withreverse order of loading specializations --- clang/lib/AST/DeclTemplate.cpp | 8 ++++++-- clang/lib/Sema/SemaTemplateInstantiate.cpp | 5 ----- clang/lib/Serialization/ASTReaderDecl.cpp | 7 ------- clang/test/Modules/odr_hash.cpp | 4 ++-- 4 files changed, 8 insertions(+), 16 deletions(-) diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp index 976b3a3e1eced..7dc9afd615946 100644 --- a/clang/lib/AST/DeclTemplate.cpp +++ b/clang/lib/AST/DeclTemplate.cpp @@ -349,8 +349,12 @@ void RedeclarableTemplateDecl::loadLazySpecializationsImpl() const { GlobalDeclID *Specs = CommonBasePtr->LazySpecializations; CommonBasePtr->LazySpecializations = nullptr; unsigned SpecSize = (*Specs++).getRawValue(); - for (unsigned I = 0; I != SpecSize; ++I) - (void)Context.getExternalSource()->GetExternalDecl(Specs[I]); + // Load the specializations in reverse order so that the most recent + // specialization are visited first so they become canonical declarations. + // This order matches the order in which namelookup discovers declarations + // coming from modules. + for (unsigned I = SpecSize; I != 0; --I) + (void)Context.getExternalSource()->GetExternalDecl(Specs[I-1]); } } diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index c439d0deba29c..c42cc250bb904 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -4387,11 +4387,6 @@ LocalInstantiationScope::findInstantiationOf(const Decl *D) { // a previous declaration. if (const TagDecl *Tag = dyn_cast(CheckD)) CheckD = Tag->getPreviousDecl(); - else if (const VarDecl *VD = dyn_cast(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 96d052b8cc529..9272e23c7da3f 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -3291,13 +3291,6 @@ DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader, if (auto *TU = dyn_cast(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(DC)) - if (FD->getOwningModule()) - return FD->getCanonicalDecl(); - return nullptr; } diff --git a/clang/test/Modules/odr_hash.cpp b/clang/test/Modules/odr_hash.cpp index fa8b2c81ab46e..7cea3af3f41bd 100644 --- a/clang/test/Modules/odr_hash.cpp +++ b/clang/test/Modules/odr_hash.cpp @@ -3084,8 +3084,8 @@ struct S5 { }; #else S5 s5; -// expected-error@second.h:* {{'PointersAndReferences::S5::x' from module 'SecondModule' is not present in definition of 'PointersAndReferences::S5' in module 'FirstModule'}} -// expected-note@first.h:* {{declaration of 'x' does not match}} +// expected-error@first.h:* {{'PointersAndReferences::S5::x' from module 'FirstModule' is not present in definition of 'PointersAndReferences::S5' in module 'SecondModule'}} +// expected-note@second.h:* {{declaration of 'x' does not match}} #endif #if defined(FIRST) From 11b798d51eecd38d98bad87458196cd6860c184c Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin Date: Tue, 20 Aug 2024 02:54:48 -0700 Subject: [PATCH 04/12] Fix clang-format issue --- clang/lib/AST/DeclTemplate.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp index 7dc9afd615946..bdd40b01c0c6f 100644 --- a/clang/lib/AST/DeclTemplate.cpp +++ b/clang/lib/AST/DeclTemplate.cpp @@ -354,7 +354,7 @@ void RedeclarableTemplateDecl::loadLazySpecializationsImpl() const { // This order matches the order in which namelookup discovers declarations // coming from modules. for (unsigned I = SpecSize; I != 0; --I) - (void)Context.getExternalSource()->GetExternalDecl(Specs[I-1]); + (void)Context.getExternalSource()->GetExternalDecl(Specs[I - 1]); } } From 1fc4693acf27e20e720a001bc8e7a9044e00e236 Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin Date: Thu, 22 Aug 2024 10:31:01 -0700 Subject: [PATCH 05/12] Visit lambdas right after FunctionDecl --- clang/lib/AST/DeclTemplate.cpp | 8 ++--- clang/lib/Serialization/ASTReaderDecl.cpp | 9 ++++++ clang/lib/Serialization/ASTWriterDecl.cpp | 37 +++++++++++++++++++++++ clang/test/Modules/odr_hash.cpp | 4 +-- 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp index bdd40b01c0c6f..976b3a3e1eced 100644 --- a/clang/lib/AST/DeclTemplate.cpp +++ b/clang/lib/AST/DeclTemplate.cpp @@ -349,12 +349,8 @@ void RedeclarableTemplateDecl::loadLazySpecializationsImpl() const { GlobalDeclID *Specs = CommonBasePtr->LazySpecializations; CommonBasePtr->LazySpecializations = nullptr; unsigned SpecSize = (*Specs++).getRawValue(); - // Load the specializations in reverse order so that the most recent - // specialization are visited first so they become canonical declarations. - // This order matches the order in which namelookup discovers declarations - // coming from modules. - for (unsigned I = SpecSize; I != 0; --I) - (void)Context.getExternalSource()->GetExternalDecl(Specs[I - 1]); + for (unsigned I = 0; I != SpecSize; ++I) + (void)Context.getExternalSource()->GetExternalDecl(Specs[I]); } } diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index 9272e23c7da3f..3864f60bfe8d0 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -1155,6 +1155,15 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) { for (unsigned I = 0; I != NumParams; ++I) Params.push_back(readDeclAs()); FD->setParams(Reader.getContext(), Params); + + // For the first decl read all lambdas inside, otherwise skip them. + unsigned NumLambdas = Record.readInt(); + if (FD->isFirstDecl()) { + for (unsigned I = 0; I != NumLambdas; ++I) + readDecl(); + } else { + (void)Record.readIntArray(NumLambdas); + } } void ASTDeclReader::VisitObjCMethodDecl(ObjCMethodDecl *MD) { diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index 555f6325da646..1c8dc0024a353 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 collectLambdas(FunctionDecl *D) { + struct LambdaCollector : public ConstStmtVisitor { + llvm::SmallVectorImpl &Lambdas; + + LambdaCollector(llvm::SmallVectorImpl &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 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,15 @@ 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. + llvm::SmallVector Lambdas = collectLambdas(D); + Record.push_back(Lambdas.size()); + for (const auto *L : Lambdas) + Record.AddDeclRef(L); + Code = serialization::DECL_FUNCTION; } diff --git a/clang/test/Modules/odr_hash.cpp b/clang/test/Modules/odr_hash.cpp index 7cea3af3f41bd..fa8b2c81ab46e 100644 --- a/clang/test/Modules/odr_hash.cpp +++ b/clang/test/Modules/odr_hash.cpp @@ -3084,8 +3084,8 @@ struct S5 { }; #else S5 s5; -// expected-error@first.h:* {{'PointersAndReferences::S5::x' from module 'FirstModule' is not present in definition of 'PointersAndReferences::S5' in module 'SecondModule'}} -// expected-note@second.h:* {{declaration of 'x' does not match}} +// expected-error@second.h:* {{'PointersAndReferences::S5::x' from module 'SecondModule' is not present in definition of 'PointersAndReferences::S5' in module 'FirstModule'}} +// expected-note@first.h:* {{declaration of 'x' does not match}} #endif #if defined(FIRST) From 64230ad7377f3983d776bae59b2d1c3dd56b579f Mon Sep 17 00:00:00 2001 From: Kyungwoo Lee Date: Tue, 20 Aug 2024 09:10:22 -0700 Subject: [PATCH 06/12] Fix a warning for -Wcovered-switch-default (#105054) This fixes a build break from [llvm/llvm-project] Reland [CGData] llvm-cgdata #89884 (PR #101461) From 4a964a71850a223eb43c6fa0dddde7f7c95b20ce Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin Date: Mon, 2 Sep 2024 07:14:31 -0700 Subject: [PATCH 07/12] Add minimal reroducer for the crash after changing deserialization order --- ...ash-instantiated-in-scope-cxx-modules2.cpp | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 clang/test/Headers/crash-instantiated-in-scope-cxx-modules2.cpp 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 0000000000000..ac9bbec1ddb7d --- /dev/null +++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules2.cpp @@ -0,0 +1,31 @@ +// 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 +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 From 01375e311b0180362a73655634a1976d8a184da4 Mon Sep 17 00:00:00 2001 From: Kyungwoo Lee Date: Tue, 20 Aug 2024 09:10:22 -0700 Subject: [PATCH 08/12] Fix a warning for -Wcovered-switch-default (#105054) This fixes a build break from [llvm/llvm-project] Reland [CGData] llvm-cgdata #89884 (PR #101461) From 2b588e5db68580d4d119215f77e9c75aa037aae4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Valentin=20Clement=20=28=E3=83=90=E3=83=AC=E3=83=B3?= =?UTF-8?q?=E3=82=BF=E3=82=A4=E3=83=B3=20=E3=82=AF=E3=83=AC=E3=83=A1?= =?UTF-8?q?=E3=83=B3=29?= Date: Tue, 27 Aug 2024 17:36:14 -0700 Subject: [PATCH 09/12] [flang][cuda] Add missing dependency (#106298) Add missing dependency that sometimes makes a build fails with ninja. From db65f698a38bb3da180fd29a77ae09842d476a47 Mon Sep 17 00:00:00 2001 From: Kyungwoo Lee Date: Tue, 20 Aug 2024 09:10:22 -0700 Subject: [PATCH 10/12] Fix a warning for -Wcovered-switch-default (#105054) This fixes a build break from [llvm/llvm-project] Reland [CGData] llvm-cgdata #89884 (PR #101461) From b5f6a30e7136c191e37c41eeb3e743b57f225402 Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin Date: Fri, 6 Sep 2024 02:02:43 -0700 Subject: [PATCH 11/12] Delay loading lambdas --- clang/include/clang/Serialization/ASTReader.h | 5 +++++ clang/lib/Serialization/ASTReader.cpp | 8 +++++++- clang/lib/Serialization/ASTReaderDecl.cpp | 5 +++-- .../Headers/crash-instantiated-in-scope-cxx-modules2.cpp | 1 - 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 898f4392465fd..95e3d152b4834 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -1188,6 +1188,11 @@ class ASTReader /// once recursing loading has been completed. llvm::SmallVector PendingOdrMergeChecks; + /// Lambdas that need to be loaded right after the function they belong to. + /// It is required to have the right canonical declaration for lambda class + /// from the same module as the function. + SmallVector PendingLambdas; + using DataPointers = std::pair; using ObjCInterfaceDataPointers = diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index e5a1e20a26561..0ee53e43dff39 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -9782,7 +9782,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 = @@ -9927,6 +9928,11 @@ void ASTReader::finishPendingActions() { } PendingObjCExtensionIvarRedeclarations.pop_back(); } + + // Load any pendiong lambdas. + for (auto ID : PendingLambdas) + GetDecl(ID); + PendingLambdas.clear(); } // 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 3864f60bfe8d0..9b122c46de0d1 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -1156,11 +1156,12 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) { Params.push_back(readDeclAs()); FD->setParams(Reader.getContext(), Params); - // For the first decl read all lambdas inside, otherwise skip them. + // 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) - readDecl(); + Reader.PendingLambdas.push_back(Record.readDeclID()); } else { (void)Record.readIntArray(NumLambdas); } diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules2.cpp b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules2.cpp index ac9bbec1ddb7d..5b1a904e928a6 100644 --- a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules2.cpp +++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules2.cpp @@ -23,7 +23,6 @@ struct B { }); } }; - // expected-no-diagnostics //--- main.cpp From bd6386f28fff96e9cffe5aa960062a97dfda9021 Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin Date: Mon, 9 Sep 2024 08:14:35 -0700 Subject: [PATCH 12/12] Comments resolved --- clang/include/clang/Serialization/ASTReader.h | 8 ++++++-- clang/lib/Serialization/ASTReaderDecl.cpp | 2 +- clang/lib/Serialization/ASTWriterDecl.cpp | 13 +++++++++---- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 95e3d152b4834..7331bcf249266 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -1189,8 +1189,12 @@ class ASTReader llvm::SmallVector PendingOdrMergeChecks; /// Lambdas that need to be loaded right after the function they belong to. - /// It is required to have the right canonical declaration for lambda class - /// from the same module as the function. + /// It is required to have canonical declaration for lambda class from the + /// same module as enclosing function. This is required to correctly resolve + /// 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 the AST nodes + /// that don't exist in the function. SmallVector PendingLambdas; using DataPointers = diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index 9b122c46de0d1..20e577404d997 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -1163,7 +1163,7 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) { for (unsigned I = 0; I != NumLambdas; ++I) Reader.PendingLambdas.push_back(Record.readDeclID()); } else { - (void)Record.readIntArray(NumLambdas); + Record.skipInts(NumLambdas); } } diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index 1c8dc0024a353..732a6e21f340d 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -796,10 +796,14 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) { // 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. - llvm::SmallVector Lambdas = collectLambdas(D); - Record.push_back(Lambdas.size()); - for (const auto *L : Lambdas) - Record.AddDeclRef(L); + if (D->isCanonicalDecl()) { + llvm::SmallVector 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; } @@ -2276,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.