https://github.com/dmpolukhin updated https://github.com/llvm/llvm-project/pull/123648
>From e5cd06ddbc4193f9d5910eba93f0eb309d67063c Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin <dmitry.poluk...@gmail.com> Date: Mon, 20 Jan 2025 09:03:25 -0800 Subject: [PATCH 1/6] [C++20][Modules] Fix crash/compiler error due broken AST links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493 The root problem is the same as before lambda function and DeclRefExpr references a variable that does not belong to the same module as the enclosing function body. Therefore iteration over the function body doesn’t visit the VarDecl. Before this change RelatedDeclsMap was created only for canonical decl but in reality it has to be done for the definition of the function that does not always match the canonical decl. Test Plan: check-clang --- clang/lib/Serialization/ASTWriterDecl.cpp | 5 +- ...ash-instantiated-in-scope-cxx-modules5.cpp | 92 +++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index f8ed155ca389d7..371b4b29e85991 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D) { } else { Record.push_back(0); } - // For lambdas inside canonical FunctionDecl remember the mapping. + // For lambdas inside template functions, remember the mapping to + // deserialize them together. if (auto FD = llvm::dyn_cast_or_null<FunctionDecl>(D->getDeclContext()); - FD && FD->isCanonicalDecl()) { + FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) { Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back( Writer.GetDeclRef(D)); } diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp new file mode 100644 index 00000000000000..ca80a3fd21dd00 --- /dev/null +++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp @@ -0,0 +1,92 @@ +// RUN: rm -fR %t +// RUN: split-file %s %t +// RUN: cd %t +// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap -fmodule-name=mock_resolver -o mock_resolver.pcm +// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap -fmodule-name=sql_internal -o sql_internal.pcm +// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm -fmodule-file=sql_internal.pcm main.cc -o main.o + +//--- module.cppmap +module "mock_resolver" { + export * + module "mock_resolver.h" { + export * + header "mock_resolver.h" + } +} + +module "sql_internal" { + export * + module "sql_transform_builder.h" { + export * + header "sql_transform_builder.h" + } +} + +//--- set_bits2.h +// expected-no-diagnostics +#pragma once + +template <typename T> +void fwd(const T& x) {} + +namespace vox::bitset { + +template <typename TFunc> +void ForEachSetBit2(const TFunc&) { + fwd([](int) { + const int bit_index_base = 0; + (void)[&](int) { + int v = bit_index_base; + }; + }); +} + +} // namespace vox::bitset + +//--- sql_transform_builder.h +// expected-no-diagnostics +#pragma once + +#include "set_bits2.h" + +class QualifyingSet3 { + public: + void GetIndexes() const { + vox::bitset::ForEachSetBit2([]() {}); + } +}; + +template <typename T> +void DoTransform() { + vox::bitset::ForEachSetBit2([]() {}); +} + +//--- mock_resolver.h +// expected-no-diagnostics +#pragma once +#include "set_bits2.h" + +class QualifyingSet2 { + public: + void GetIndexes() const { + vox::bitset::ForEachSetBit2([]() {}); + } +}; + +//--- main.cc +// expected-no-diagnostics +#include "sql_transform_builder.h" + +template <typename Callable> +void get(const Callable& fn) { + fwd<Callable>(fn); +} + +namespace { + +void test() { + get([]() {}); + DoTransform<int>(); +} + +} // namespace >From 01254792e2b75e00cc44259b27fcb4e38e719629 Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin <dmitry.poluk...@gmail.com> Date: Tue, 21 Jan 2025 02:32:33 -0800 Subject: [PATCH 2/6] Add -Werror=uninitialized to add compilation error in builds without assserts All improve comments --- clang/include/clang/Serialization/ASTReader.h | 11 ++++++----- clang/lib/Serialization/ASTWriterDecl.cpp | 6 +++--- .../crash-instantiated-in-scope-cxx-modules5.cpp | 6 +++--- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 9f978762a6fb6b..d27a49863fc557 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -539,11 +539,12 @@ class ASTReader /// Mapping from main decl ID to the related decls IDs. /// - /// These related decls have to be loaded right after the main decl. - /// It is required to have canonical declaration for related decls from the - /// same module as the enclosing main decl. Without this, due to lazy - /// deserialization, canonical declarations for the main decl and related can - /// be selected from different modules. + /// The key is the main decl ID, and the value is a vector of related decls + /// that must be loaded immediately after the main decl. This is necessary + /// to ensure that the definition for related decls comes from the same module + /// as the enclosing main decl. Without this, due to lazy deserialization, + /// the definition for the main decl and related decls may come from different + /// modules. llvm::DenseMap<GlobalDeclID, SmallVector<GlobalDeclID, 4>> RelatedDeclsMap; struct PendingUpdateRecord { diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index 371b4b29e85991..9835f58543e179 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -799,9 +799,9 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) { } if (D->getFriendObjectKind()) { - // For a function defined inline within a class template, we have to force - // the canonical definition to be the one inside the canonical definition of - // the template. Remember this relation to deserialize them together. + // For a friend function defined inline within a class template, we have to + // force the definition to be the one inside the definition of the template + // class. Remember this relation to deserialize them together. if (auto *RD = dyn_cast<CXXRecordDecl>(D->getLexicalParent())) if (RD->isDependentContext() && RD->isThisDeclarationADefinition()) { Writer.RelatedDeclsMap[Writer.GetDeclRef(RD)].push_back( diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp index ca80a3fd21dd00..352e0125fe4342 100644 --- a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp +++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp @@ -1,9 +1,9 @@ // RUN: rm -fR %t // RUN: split-file %s %t // RUN: cd %t -// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap -fmodule-name=mock_resolver -o mock_resolver.pcm -// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap -fmodule-name=sql_internal -o sql_internal.pcm -// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm -fmodule-file=sql_internal.pcm main.cc -o main.o +// RUN: %clang_cc1 -verify -std=c++20 -Werror=uninitialized -xc++ -emit-module module.cppmap -fmodule-name=mock_resolver -o mock_resolver.pcm +// RUN: %clang_cc1 -verify -std=c++20 -Werror=uninitialized -xc++ -emit-module module.cppmap -fmodule-name=sql_internal -o sql_internal.pcm +// RUN: %clang_cc1 -verify -std=c++20 -Werror=uninitialized -xc++ -fmodule-file=mock_resolver.pcm -fmodule-file=sql_internal.pcm main.cc -o main.o //--- module.cppmap module "mock_resolver" { >From 127e2fb66f25856c0d4653b75bdf13cf440f6955 Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin <dmitry.poluk...@gmail.com> Date: Wed, 22 Jan 2025 02:10:34 -0800 Subject: [PATCH 3/6] Extract check into template function. --- clang/lib/Serialization/ASTWriterDecl.cpp | 29 +++++++++++++++++------ 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index 9835f58543e179..e5bdd0ae43a6ed 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -27,6 +27,24 @@ using namespace clang; using namespace serialization; +//===----------------------------------------------------------------------===// +// Utility functions +//===----------------------------------------------------------------------===// + +namespace { + +// Helper function that returns dependent decl context for the declaration +// passed in the argument. +template <typename DCT> DCT *getDependentDeclContext(Decl *D) { + if (auto *DC = llvm::dyn_cast_or_null<DCT>(D->getDeclContext()); + DC && DC->isDependentContext() && DC->isThisDeclarationADefinition()) { + return DC; + } + return nullptr; +} + +} // namespace + //===----------------------------------------------------------------------===// // Declaration serialization //===----------------------------------------------------------------------===// @@ -802,11 +820,9 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) { // For a friend function defined inline within a class template, we have to // force the definition to be the one inside the definition of the template // class. Remember this relation to deserialize them together. - if (auto *RD = dyn_cast<CXXRecordDecl>(D->getLexicalParent())) - if (RD->isDependentContext() && RD->isThisDeclarationADefinition()) { - Writer.RelatedDeclsMap[Writer.GetDeclRef(RD)].push_back( - Writer.GetDeclRef(D)); - } + if (auto *RD = getDependentDeclContext<CXXRecordDecl>(D)) + Writer.RelatedDeclsMap[Writer.GetDeclRef(RD)].push_back( + Writer.GetDeclRef(D)); } Record.push_back(D->param_size()); @@ -1573,8 +1589,7 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D) { } // For lambdas inside template functions, remember the mapping to // deserialize them together. - if (auto FD = llvm::dyn_cast_or_null<FunctionDecl>(D->getDeclContext()); - FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) { + if (auto *FD = getDependentDeclContext<CXXRecordDecl>(D)) { Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back( Writer.GetDeclRef(D)); } >From 21d89562e5c539b16fe449aa5c2a0da6afc487a4 Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin <dmitry.poluk...@gmail.com> Date: Wed, 22 Jan 2025 02:33:25 -0800 Subject: [PATCH 4/6] Fix typo --- clang/lib/Serialization/ASTWriterDecl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index e5bdd0ae43a6ed..82c5bd7de28c8d 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -1589,7 +1589,7 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D) { } // For lambdas inside template functions, remember the mapping to // deserialize them together. - if (auto *FD = getDependentDeclContext<CXXRecordDecl>(D)) { + if (auto *FD = getDependentDeclContext<FunctionDecl>(D)) { Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back( Writer.GetDeclRef(D)); } >From 34cca28965c9e681aeb1cf133d3b4124efda0798 Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin <dmitry.poluk...@gmail.com> Date: Wed, 22 Jan 2025 02:56:44 -0800 Subject: [PATCH 5/6] Add a release note --- clang/docs/ReleaseNotes.rst | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 5a48d6fbc01fa3..0e3de8ae4a5d20 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -287,7 +287,7 @@ C++23 Feature Support - Extend lifetime of temporaries in mem-default-init for P2718R0. Clang now fully supports `P2718R0 Lifetime extension in range-based for loops <https://wg21.link/P2718R0>`_. - + - ``__cpp_explicit_this_parameter`` is now defined. (#GH82780) C++20 Feature Support @@ -688,7 +688,7 @@ Improvements to Clang's diagnostics - Clang now diagnoses dangling references for C++20's parenthesized aggregate initialization (#101957). -- Fixed a bug where Clang would not emit ``-Wunused-private-field`` warnings when an unrelated class +- Fixed a bug where Clang would not emit ``-Wunused-private-field`` warnings when an unrelated class defined a defaulted comparison operator (#GH116270). .. code-block:: c++ @@ -907,7 +907,7 @@ Bug Fixes to C++ Support - Fixed an assertion failure caused by invalid default argument substitutions in non-defining friend declarations. (#GH113324) - Fix a crash caused by incorrect argument position in merging deduced template arguments. (#GH113659) -- Fixed a parser crash when using pack indexing as a nested name specifier. (#GH119072) +- Fixed a parser crash when using pack indexing as a nested name specifier. (#GH119072) - Fixed a null pointer dereference issue when heuristically computing ``sizeof...(pack)`` expressions. (#GH81436) - Fixed an assertion failure caused by mangled names with invalid identifiers. (#GH112205) - Fixed an incorrect lambda scope of generic lambdas that caused Clang to crash when computing potential lambda @@ -922,6 +922,8 @@ Bug Fixes to C++ Support (`LWG3929 <https://wg21.link/LWG3929>`__.) (#GH121278) - Clang now identifies unexpanded parameter packs within the type constraint on a non-type template parameter. (#GH88866) - Fixed an issue while resolving type of expression indexing into a pack of values of non-dependent type (#GH121242) +- Fixed assertions or false compiler diagnostics in the case of C++ modules for + lambda functions or inline friend functions defined inside templates (#GH122493). Bug Fixes to AST Handling ^^^^^^^^^^^^^^^^^^^^^^^^^ >From 7c1f46d7b917dc5a9e653caa0dbacde4c1fd4c9c Mon Sep 17 00:00:00 2001 From: Dmitry Polukhin <dmitry.poluk...@gmail.com> Date: Wed, 22 Jan 2025 04:13:10 -0800 Subject: [PATCH 6/6] Reverted to verified version + NFC --- clang/lib/Serialization/ASTWriterDecl.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index 82c5bd7de28c8d..61009871c23984 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -33,14 +33,10 @@ using namespace serialization; namespace { -// Helper function that returns dependent decl context for the declaration -// passed in the argument. -template <typename DCT> DCT *getDependentDeclContext(Decl *D) { - if (auto *DC = llvm::dyn_cast_or_null<DCT>(D->getDeclContext()); - DC && DC->isDependentContext() && DC->isThisDeclarationADefinition()) { - return DC; - } - return nullptr; +// Helper function that returns true if the decl passed in the argument is +// a defintion in dependent contxt. +template <typename DT> bool isDefinitionInDependentContext(DT *D) { + return D->isDependentContext() && D->isThisDeclarationADefinition(); } } // namespace @@ -820,9 +816,11 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) { // For a friend function defined inline within a class template, we have to // force the definition to be the one inside the definition of the template // class. Remember this relation to deserialize them together. - if (auto *RD = getDependentDeclContext<CXXRecordDecl>(D)) + if (auto *RD = dyn_cast<CXXRecordDecl>(D->getLexicalParent()); + isDefinitionInDependentContext(RD)) { Writer.RelatedDeclsMap[Writer.GetDeclRef(RD)].push_back( Writer.GetDeclRef(D)); + } } Record.push_back(D->param_size()); @@ -1589,7 +1587,8 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D) { } // For lambdas inside template functions, remember the mapping to // deserialize them together. - if (auto *FD = getDependentDeclContext<FunctionDecl>(D)) { + if (auto *FD = llvm::dyn_cast_or_null<FunctionDecl>(D->getDeclContext()); + FD && isDefinitionInDependentContext(FD)) { Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back( Writer.GetDeclRef(D)); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
