[C++20][Modules] Fix crash/compiler error due broken AST links #123648

dmpolukhin · 2025-01-20T17:16:27Z

Summary:
This PR fixes bugreport #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

Summary: This PR fixes bugreport llvm#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

llvmbot · 2025-01-20T17:17:01Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Dmitry Polukhin (dmpolukhin)

Changes

Summary:
This PR fixes bugreport #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

Full diff: https://github.com/llvm/llvm-project/pull/123648.diff

2 Files Affected:

(modified) clang/lib/Serialization/ASTWriterDecl.cpp (+3-2)
(added) clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp (+92)

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

ChuanqiXu9

LGTM

hokein

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.

I'm new to the module codebase, and it is not obvious to me. Could you elaborate on this? Perhaps you could use the provided test case to illustrate the distinction.

clang/lib/Serialization/ASTWriterDecl.cpp

…assserts All improve comments

hokein

We have initiated tests to verify this fix within our codebase. It will take some time to get the results.

clang/lib/Serialization/ASTWriterDecl.cpp

dmpolukhin · 2025-01-22T10:39:40Z

On our codebase I see no difference with and without this fix but at least it seems that it doesn't break anything. @hokein please let me know the results of your testing when ready. I'll wait before merging this PR.

hokein

I might be missing some historical context, but am I correct in understanding that #104512, #109167, #111992, and this patch together form a complete (and hopefully final) fix for the crashes that occur when a function and its associated lambda are loaded from different modules?

And we seem to miss a release note in llvm-project/clang/docs/ReleaseNotes.rst.

hokein · 2025-01-22T09:56:02Z

clang/include/clang/Serialization/ASTReader.h

+  /// 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.


If I understand correctly, this map is used to ensure that the main declaration and its related declarations are always deserialized from the same module in the following cases:

Lambda inside a function declaration: The main declaration is the enclosing function, and the related declarations are the lambda declarations.

Defined friend declaration inside a CXXRecord declaration: The main declaration is the enclosing record, and the related declarations are the friend declarations.

Is my understanding correct?

Yes, that's right understanding. Initially it was only for lambda functions but later case with inline friend functions were identified. The mechanism in its current form is very generic so it can be used for other case if we find them.

Thanks for confirming.

I think it would be worth documenting this (it took me some time to fully understand what RelatedDeclsMap is and how it's used). Clear documentation could help others in the future.

hokein · 2025-01-22T10:43:08Z

On our codebase I see no difference with and without this fix but at least it seems that it doesn't break anything. @hokein please let me know the results of your testing when ready. I'll wait before merging this PR.

Our tests were completed today, and the results look good. There are no new breakages, and the patch fixes the issue we encountered in #122493. I think this patch is good.

dmpolukhin

Added release note

dmpolukhin · 2025-01-22T10:43:59Z

clang/include/clang/Serialization/ASTReader.h

+  /// 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.


Yes, that's right understanding. Initially it was only for lambda functions but later case with inline friend functions were identified. The mechanism in its current form is very generic so it can be used for other case if we find them.

clang/lib/Serialization/ASTWriterDecl.cpp

hokein

Thanks for the fix! It looks good from my side, just small nits. Feel free to land it.

hokein · 2025-01-22T12:23:22Z

clang/include/clang/Serialization/ASTReader.h

+  /// 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.


Thanks for confirming.

I think it would be worth documenting this (it took me some time to fully understand what RelatedDeclsMap is and how it's used). Clear documentation could help others in the future.

hokein · 2025-01-22T12:25:50Z

clang/lib/Serialization/ASTWriterDecl.cpp

+    // 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());
+        isDefinitionInDependentContext(RD)) {


nit: we seem to miss checking RD is not nullptr

Added but I think it cannot happen after getFriendObjectKind check.

hokein

Let's ship it.

dmpolukhin requested review from ilya-biryukov and ChuanqiXu9 January 20, 2025 17:16

llvmbot added clang Clang issues not falling into any other category clang:modules C++20 modules and Clang Header Modules labels Jan 20, 2025

dmpolukhin mentioned this pull request Jan 20, 2025

Assertion failure and invalid Wunitialized error with modules and lambdas #122493

Closed

ChuanqiXu9 approved these changes Jan 21, 2025

View reviewed changes

hokein reviewed Jan 21, 2025

View reviewed changes

clang/lib/Serialization/ASTWriterDecl.cpp Outdated Show resolved Hide resolved

Add -Werror=uninitialized to add compilation error in builds without …

0125479

…assserts All improve comments

hokein reviewed Jan 21, 2025

View reviewed changes

clang/lib/Serialization/ASTWriterDecl.cpp Outdated Show resolved Hide resolved

dmpolukhin added 2 commits January 22, 2025 02:31

Extract check into template function.

127e2fb

Fix typo

21d8956

hokein reviewed Jan 22, 2025

View reviewed changes

Add a release note

34cca28

dmpolukhin commented Jan 22, 2025

View reviewed changes

hokein reviewed Jan 22, 2025

View reviewed changes

clang/lib/Serialization/ASTWriterDecl.cpp Outdated Show resolved Hide resolved

Reverted to verified version + NFC

7c1f46d

hokein approved these changes Jan 22, 2025

View reviewed changes

Improved comments and add nullptr check

5977b98

hokein approved these changes Jan 23, 2025

View reviewed changes

Merge branch 'main' into assert-not-instantiated-in-scope-cxx-modules5

030ce75

dmpolukhin merged commit cad6bba into llvm:main Jan 23, 2025
9 checks passed

[C++20][Modules] Fix crash/compiler error due broken AST links #123648

[C++20][Modules] Fix crash/compiler error due broken AST links #123648

Uh oh!

Conversation

dmpolukhin commented Jan 20, 2025

Uh oh!

llvmbot commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Uh oh!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmpolukhin commented Jan 22, 2025

Uh oh!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

hokein Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

dmpolukhin Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

hokein Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

hokein commented Jan 22, 2025

Uh oh!

dmpolukhin left a comment

Choose a reason for hiding this comment

Uh oh!

dmpolukhin Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

hokein Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

hokein Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

dmpolukhin Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jan 20, 2025 •

edited

Loading