Skip to content

[C++20] [Modules] Don't insert class not in named modules to PendingEmittingVTables #106501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

ChuanqiXu9
Copy link
Member

Close #102933

The root cause of the issue is an oversight in
#102287 that I didn't notice that PendingEmittingVTables should only accept classes in named modules.

…mittingVTables

Close llvm#102933

The root cause of the issue is an oversight in
llvm#102287 that I didn't notice
that PendingEmittingVTables should only accept classes in named modules.
@ChuanqiXu9 ChuanqiXu9 added clang:modules C++20 modules and Clang Header Modules skip-precommit-approval PR for CI feedback, not intended for review labels Aug 29, 2024
@ChuanqiXu9 ChuanqiXu9 self-assigned this Aug 29, 2024
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Aug 29, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 29, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Changes

Close #102933

The root cause of the issue is an oversight in
#102287 that I didn't notice that PendingEmittingVTables should only accept classes in named modules.


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

3 Files Affected:

  • (modified) clang/include/clang/Serialization/ASTWriter.h (+2-2)
  • (modified) clang/lib/Serialization/ASTWriter.cpp (+3)
  • (added) clang/test/Modules/pr106483.cppm (+39)
diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h
index a4cc95cd1373fa..10a50b711043a8 100644
--- a/clang/include/clang/Serialization/ASTWriter.h
+++ b/clang/include/clang/Serialization/ASTWriter.h
@@ -500,8 +500,8 @@ class ASTWriter : public ASTDeserializationListener,
   std::vector<SourceRange> NonAffectingRanges;
   std::vector<SourceLocation::UIntTy> NonAffectingOffsetAdjustments;
 
-  /// A list of classes which need to emit the VTable in the corresponding
-  /// object file.
+  /// A list of classes in named modules which need to emit the VTable in
+  /// the corresponding object file.
   llvm::SmallVector<CXXRecordDecl *> PendingEmittingVTables;
 
   /// Computes input files that didn't affect compilation of the current module,
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 5cfb98c2a1060a..3e60f1425f88ea 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -3963,6 +3963,9 @@ void ASTWriter::WriteIdentifierTable(Preprocessor &PP,
 }
 
 void ASTWriter::handleVTable(CXXRecordDecl *RD) {
+  if (!RD->isInNamedModule())
+    return;
+
   PendingEmittingVTables.push_back(RD);
 }
 
diff --git a/clang/test/Modules/pr106483.cppm b/clang/test/Modules/pr106483.cppm
new file mode 100644
index 00000000000000..08f124dd0d08ca
--- /dev/null
+++ b/clang/test/Modules/pr106483.cppm
@@ -0,0 +1,39 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++23 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++23 %t/b.cppm -emit-module-interface -o %t/b.pcm \
+// RUN:   -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++23 -fprebuilt-module-path=%t %t/b.pcm -emit-llvm \
+// RUN:     -disable-llvm-passes -o - | FileCheck %t/b.cppm
+
+//--- a.cppm
+module;
+
+struct base {
+    virtual void f() const;
+};
+
+inline void base::f() const {
+}
+
+export module a;
+export using ::base;
+
+//--- b.cppm
+module;
+
+struct base {
+    virtual void f() const;
+};
+
+inline void base::f() const {
+}
+
+export module b;
+import a;
+export using ::base;
+
+// We only need to check that the IR are successfully emitted instead of crash.
+// CHECK: define

@ChuanqiXu9 ChuanqiXu9 merged commit 47615ff into llvm:main Aug 29, 2024
8 checks passed
@ChuanqiXu9 ChuanqiXu9 added this to the LLVM 19.X Release milestone Aug 29, 2024
@ChuanqiXu9
Copy link
Member Author

/cherry-pick 47615ff

@ChuanqiXu9 ChuanqiXu9 deleted the crash-in-two-virutal-functions branch August 29, 2024 07:45
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 29, 2024
…mittingVTables (llvm#106501)

Close llvm#102933

The root cause of the issue is an oversight in
llvm#102287 that I didn't notice
that PendingEmittingVTables should only accept classes in named modules.

(cherry picked from commit 47615ff)
@llvmbot
Copy link
Member

llvmbot commented Aug 29, 2024

/pull-request #106504

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Sep 1, 2024
…mittingVTables (llvm#106501)

Close llvm#102933

The root cause of the issue is an oversight in
llvm#102287 that I didn't notice
that PendingEmittingVTables should only accept classes in named modules.

(cherry picked from commit 47615ff)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category skip-precommit-approval PR for CI feedback, not intended for review
Projects
2 participants