https://github.com/Maoyao233 updated https://github.com/llvm/llvm-project/pull/235673
>From d3dc5b27fac82821a2454558f6ec5b3e8eaa26bc Mon Sep 17 00:00:00 2001 From: Maoyao Ye <[email protected]> Date: Mon, 26 Oct 2026 17:10:34 +0800 Subject: [PATCH 1/6] [Clang][modules] Don't drop resolved operator delete updates in reduced BMIs On a Microsoft ABI target, a module interface unit that instantiates a class whose virtual destructor is declared but not defined in that module makes an importing TU crash while generating code: CodeGenFunction::EnterDtorCleanups() finds a null CXXDestructorDecl::getOperatorDelete(), which is an assertion failure in an assertions build and a null pointer dereference otherwise. In the Microsoft ABI the deleting destructor is emitted together with the vtable, so Sema::MarkVTableUsed() calls CheckDestructor() for a user-declared, not-yet-defined virtual destructor and that resolves the destructor's 'operator delete'. When the destructor declaration is owned by another PCM (for example it is part of an 'extern template' specialization), the resolution can only be recorded as a DeclUpdate. Module interface units are compiled as reduced BMIs by default, and ASTWriter::prepareLazyUpdates() keeps every update except CXXAddedFunctionDefinition lazy until the module writes a declaration reference to the updated declaration; if it never does, the update is dropped. A consumer that emits the vtable and deleting destructor itself (there are no key functions in the Microsoft ABI) does not re-resolve 'operator delete' on that codegen path, so it ends up in EnterDtorCleanups() without one. Keep the CXXResolvedDtor* updates eager, like CXXAddedFunctionDefinition: an importer cannot recompute them, but codegen needs them to emit the deleting destructor. Fixes #231859. Assisted by: Deepseek V4.1-flash-1017 --- clang/lib/Serialization/ASTWriter.cpp | 10 ++++ .../msvc-dtor-operator-delete-reduced-bmi.cpp | 46 +++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 clang/test/Modules/msvc-dtor-operator-delete-reduced-bmi.cpp diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 0d9ec7a460dfbf..35b1a17d015d12 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -5731,6 +5731,16 @@ void ASTWriter::prepareLazyUpdates() { if (Kind == DeclUpdateKind::CXXAddedFunctionDefinition) DeclUpdatesTmp[D].push_back( ASTWriter::DeclUpdate(DeclUpdateKind::CXXAddedFunctionDefinition)); + else if (Kind == DeclUpdateKind::CXXResolvedDtorDelete || + Kind == DeclUpdateKind::CXXResolvedDtorGlobDelete || + Kind == DeclUpdateKind::CXXResolvedDtorArrayDelete || + Kind == DeclUpdateKind::CXXResolvedDtorGlobArrayDelete) + // These record the 'operator delete' that Sema resolved for a virtual + // destructor in this TU. An importer cannot recompute it: the + // destructor may already have a body or be defined in another TU. But + // codegen needs it, since in the Microsoft ABI the deleting destructor + // is emitted along with the vtable. Keep them eager. + DeclUpdatesTmp[D].push_back(Update); else DeclUpdatesLazy[D].push_back(Update); } diff --git a/clang/test/Modules/msvc-dtor-operator-delete-reduced-bmi.cpp b/clang/test/Modules/msvc-dtor-operator-delete-reduced-bmi.cpp new file mode 100644 index 00000000000000..4ab4dfa12ceea2 --- /dev/null +++ b/clang/test/Modules/msvc-dtor-operator-delete-reduced-bmi.cpp @@ -0,0 +1,46 @@ +// RUN: rm -rf %t +// RUN: split-file %s %t +// +// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++20 %t/a.cppm \ +// RUN: -emit-obj -fmodules-reduced-bmi -fmodule-output=%t/a.pcm -o %t/a.o +// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++20 %t/b.cppm \ +// RUN: -emit-obj -fmodules-reduced-bmi -fmodule-output=%t/b.pcm \ +// RUN: -fmodule-file=a=%t/a.pcm -o %t/b.o +// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++20 %t/main.cpp \ +// RUN: -emit-obj -fmodule-file=a=%t/a.pcm -fmodule-file=b=%t/b.pcm -o %t/main.o + +// In the Microsoft ABI the deleting destructor is emitted together with the +// vtable, so it needs the 'operator delete' that Sema resolved for the virtual +// destructor. Here 'operator delete' is resolved for a destructor declaration +// that is owned by module 'a' (the 'extern template' declaration puts it in +// a.pcm), so the resolution is recorded as a DeclUpdate on an imported +// declaration. With reduced BMIs such an update used to be kept lazily and +// then dropped, because the module never wrote a declaration reference to the +// destructor; a consumer that emits the vtable then crashed in +// CGClass.cpp's EnterDtorCleanups with a null getOperatorDelete(). + +//--- a.cppm +export module a; + +export template <class T> struct Base { + __attribute__((exclude_from_explicit_instantiation)) Base() {} + __attribute__((exclude_from_explicit_instantiation)) virtual ~Base(); +}; +extern template struct Base<int>; + +//--- b.cppm +export module b; +import a; + +// Instantiating this constructor marks the vtable used (ActOnFinishFunctionBody) +// and Sema then resolves 'operator delete' for the imported destructor. +export void make() { new Base<int>; } + +//--- main.cpp +import a; +import b; + +struct Local : Base<int> {}; +Local l; + +int main() { return 0; } >From ab9d67095bf495da3b161e88eed656a06ea6c3ea Mon Sep 17 00:00:00 2001 From: Maoyao Ye <[email protected]> Date: Mon, 26 Oct 2026 18:36:56 +0800 Subject: [PATCH 2/6] fixup! [Clang][modules] Don't drop resolved operator delete updates in reduced BMIs --- ...reduced-bmi-msvc-dtor-operator-delete.cpp} | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) rename clang/test/Modules/{msvc-dtor-operator-delete-reduced-bmi.cpp => reduced-bmi-msvc-dtor-operator-delete.cpp} (62%) diff --git a/clang/test/Modules/msvc-dtor-operator-delete-reduced-bmi.cpp b/clang/test/Modules/reduced-bmi-msvc-dtor-operator-delete.cpp similarity index 62% rename from clang/test/Modules/msvc-dtor-operator-delete-reduced-bmi.cpp rename to clang/test/Modules/reduced-bmi-msvc-dtor-operator-delete.cpp index 4ab4dfa12ceea2..90535ee21c7bb1 100644 --- a/clang/test/Modules/msvc-dtor-operator-delete-reduced-bmi.cpp +++ b/clang/test/Modules/reduced-bmi-msvc-dtor-operator-delete.cpp @@ -1,14 +1,17 @@ +// REQUIRES: x86-registered-target +// // RUN: rm -rf %t // RUN: split-file %s %t // // RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++20 %t/a.cppm \ -// RUN: -emit-obj -fmodules-reduced-bmi -fmodule-output=%t/a.pcm -o %t/a.o +// RUN: -emit-reduced-module-interface -fmodules-reduced-bmi -o %t/a.pcm // RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++20 %t/b.cppm \ -// RUN: -emit-obj -fmodules-reduced-bmi -fmodule-output=%t/b.pcm \ -// RUN: -fmodule-file=a=%t/a.pcm -o %t/b.o +// RUN: -emit-reduced-module-interface -fmodules-reduced-bmi \ +// RUN: -fmodule-file=a=%t/a.pcm -o %t/b.pcm // RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++20 %t/main.cpp \ -// RUN: -emit-obj -fmodule-file=a=%t/a.pcm -fmodule-file=b=%t/b.pcm -o %t/main.o - +// RUN: -fmodule-file=a=%t/a.pcm -fmodule-file=b=%t/b.pcm \ +// RUN: -emit-llvm -o - | FileCheck %s +// // In the Microsoft ABI the deleting destructor is emitted together with the // vtable, so it needs the 'operator delete' that Sema resolved for the virtual // destructor. Here 'operator delete' is resolved for a destructor declaration @@ -17,7 +20,12 @@ // declaration. With reduced BMIs such an update used to be kept lazily and // then dropped, because the module never wrote a declaration reference to the // destructor; a consumer that emits the vtable then crashed in -// CGClass.cpp's EnterDtorCleanups with a null getOperatorDelete(). +// EnterDtorCleanups() in CGClass.cpp with a null getOperatorDelete(). + +// The consumer emits Base<int>'s deleting destructor, which calls the resolved +// global operator delete. +// CHECK-LABEL: define {{.*}}@"??_G?$Base@H@@UEAAPEAXI@Z" +// CHECK: call void @"??3@YAXPEAX_K@Z" //--- a.cppm export module a; @@ -32,8 +40,8 @@ extern template struct Base<int>; export module b; import a; -// Instantiating this constructor marks the vtable used (ActOnFinishFunctionBody) -// and Sema then resolves 'operator delete' for the imported destructor. +// Instantiating this constructor marks the vtable used, and Sema then resolves +// 'operator delete' for the imported destructor (ActOnFinishFunctionBody). export void make() { new Base<int>; } //--- main.cpp >From 8a8185cb31284cacd922670c5f9358288db1fe8f Mon Sep 17 00:00:00 2001 From: Maoyao Ye <[email protected]> Date: Tue, 27 Oct 2026 13:10:22 +0800 Subject: [PATCH 3/6] fixup! [Clang][modules] Don't drop resolved operator delete updates in reduced BMIs --- clang/lib/Serialization/ASTWriter.cpp | 33 +++++++++++++++++++-------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 35b1a17d015d12..d6bbacf50719ba 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -5715,13 +5715,28 @@ void ASTWriter::prepareLazyUpdates() { return; DeclUpdateMap DeclUpdatesTmp; - // Move updates to DeclUpdatesLazy but leave CXXAddedFunctionDefinition as is. - // Since added function definition is critical to the AST. If we don't take - // care of it, user might meet missing definition error at linking time. - // Here we leave all CXXAddedFunctionDefinition unconditionally to avoid - // potential issues. + // Move updates to DeclUpdatesLazy but leave the updates that may be needed by + // code generation eager. + // + // Added function definitions are critical to the AST. If we don't take care + // of them, users might meet missing definition errors at linking time. Here + // we leave all CXXAddedFunctionDefinition updates eager unconditionally to + // avoid potential issues. // TODO: Try to refine the strategy to handle CXXAddedFunctionDefinition // precisely. + // + // The CXXResolvedDtor* updates record the 'operator delete' that Sema + // resolved for a virtual destructor. They are only produced for the Microsoft + // ABI: both paths that can record them for an imported destructor declaration + // are Microsoft-ABI-only (the vtable-use hook in Sema::MarkVTableUsed() and + // vector deleting destructors in Sema::BuildCXXNew()). In that ABI the + // deleting destructor is emitted together with the vtable, so an importer can + // need the resolution without a declaration reference to the destructor, and + // it cannot recompute it: the destructor may already have a body, or be + // defined in another TU. Keep these updates eager as well. The branch is + // deliberately not guarded by the ABI because the updates do not exist for + // other ABIs, and the writer should not silently drop them if another path + // starts creating them. for (auto &DeclUpdate : DeclUpdates) { const Decl *D = DeclUpdate.first; @@ -5735,11 +5750,9 @@ void ASTWriter::prepareLazyUpdates() { Kind == DeclUpdateKind::CXXResolvedDtorGlobDelete || Kind == DeclUpdateKind::CXXResolvedDtorArrayDelete || Kind == DeclUpdateKind::CXXResolvedDtorGlobArrayDelete) - // These record the 'operator delete' that Sema resolved for a virtual - // destructor in this TU. An importer cannot recompute it: the - // destructor may already have a body or be defined in another TU. But - // codegen needs it, since in the Microsoft ABI the deleting destructor - // is emitted along with the vtable. Keep them eager. + // These are only produced for the Microsoft ABI (see the comment + // above), but a consumer may need the resolved 'operator delete' + // without a declaration reference to the destructor. DeclUpdatesTmp[D].push_back(Update); else DeclUpdatesLazy[D].push_back(Update); >From 682cb02e0ff128795cf330ba1c53e97edfb20350 Mon Sep 17 00:00:00 2001 From: Maoyao Ye <[email protected]> Date: Tue, 27 Oct 2026 13:40:32 +0800 Subject: [PATCH 4/6] simplify the comments --- clang/lib/Serialization/ASTWriter.cpp | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index d6bbacf50719ba..2231a75927e204 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -5724,19 +5724,6 @@ void ASTWriter::prepareLazyUpdates() { // avoid potential issues. // TODO: Try to refine the strategy to handle CXXAddedFunctionDefinition // precisely. - // - // The CXXResolvedDtor* updates record the 'operator delete' that Sema - // resolved for a virtual destructor. They are only produced for the Microsoft - // ABI: both paths that can record them for an imported destructor declaration - // are Microsoft-ABI-only (the vtable-use hook in Sema::MarkVTableUsed() and - // vector deleting destructors in Sema::BuildCXXNew()). In that ABI the - // deleting destructor is emitted together with the vtable, so an importer can - // need the resolution without a declaration reference to the destructor, and - // it cannot recompute it: the destructor may already have a body, or be - // defined in another TU. Keep these updates eager as well. The branch is - // deliberately not guarded by the ABI because the updates do not exist for - // other ABIs, and the writer should not silently drop them if another path - // starts creating them. for (auto &DeclUpdate : DeclUpdates) { const Decl *D = DeclUpdate.first; @@ -5750,9 +5737,12 @@ void ASTWriter::prepareLazyUpdates() { Kind == DeclUpdateKind::CXXResolvedDtorGlobDelete || Kind == DeclUpdateKind::CXXResolvedDtorArrayDelete || Kind == DeclUpdateKind::CXXResolvedDtorGlobArrayDelete) - // These are only produced for the Microsoft ABI (see the comment - // above), but a consumer may need the resolved 'operator delete' - // without a declaration reference to the destructor. + // The CXXResolvedDtor* updates record the 'operator delete' that Sema + // resolved for a virtual destructor. They are only produced for the + // Microsoft ABI. In that ABI the deleting destructor is emitted + // together with the vtable, so an importer can need the resolution + // without a declaration reference to the destructor, and it cannot + // recompute it. Keep these updates eager as well. DeclUpdatesTmp[D].push_back(Update); else DeclUpdatesLazy[D].push_back(Update); >From 27d7f214da693d1fa9018baf030d18aeda0953cc Mon Sep 17 00:00:00 2001 From: Maoyao Ye <[email protected]> Date: Tue, 27 Oct 2026 13:46:57 +0800 Subject: [PATCH 5/6] move comment into branches --- clang/lib/Serialization/ASTWriter.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 2231a75927e204..2d8e146b64257f 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -5717,13 +5717,6 @@ void ASTWriter::prepareLazyUpdates() { DeclUpdateMap DeclUpdatesTmp; // Move updates to DeclUpdatesLazy but leave the updates that may be needed by // code generation eager. - // - // Added function definitions are critical to the AST. If we don't take care - // of them, users might meet missing definition errors at linking time. Here - // we leave all CXXAddedFunctionDefinition updates eager unconditionally to - // avoid potential issues. - // TODO: Try to refine the strategy to handle CXXAddedFunctionDefinition - // precisely. for (auto &DeclUpdate : DeclUpdates) { const Decl *D = DeclUpdate.first; @@ -5731,6 +5724,12 @@ void ASTWriter::prepareLazyUpdates() { DeclUpdateKind Kind = Update.getKind(); if (Kind == DeclUpdateKind::CXXAddedFunctionDefinition) + // Added function definitions are critical to the AST. If we don't take + // care of them, users might meet missing definition errors at linking + // time. Here we leave all CXXAddedFunctionDefinition updates eager + // unconditionally to avoid potential issues. + // TODO: Try to refine the strategy to handle CXXAddedFunctionDefinition + // precisely. DeclUpdatesTmp[D].push_back( ASTWriter::DeclUpdate(DeclUpdateKind::CXXAddedFunctionDefinition)); else if (Kind == DeclUpdateKind::CXXResolvedDtorDelete || >From aac0c8bf80cc1e9abbe146676cd94cac051f3511 Mon Sep 17 00:00:00 2001 From: Maoyao Ye <[email protected]> Date: Thu, 29 Oct 2026 18:15:35 +0800 Subject: [PATCH 6/6] add explicit ABI check --- clang/include/clang/Serialization/ASTWriter.h | 2 +- clang/lib/Serialization/ASTWriter.cpp | 27 ++++++++++--------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h index 576eb863959291..96b0ef1d3981ec 100644 --- a/clang/include/clang/Serialization/ASTWriter.h +++ b/clang/include/clang/Serialization/ASTWriter.h @@ -435,7 +435,7 @@ class ASTWriter : public ASTDeserializationListener, DeclUpdateMap DeclUpdatesLazy; /// Convert non-lazy updates into lazy updates if we're in reduced BMI. - void prepareLazyUpdates(); + void prepareLazyUpdates(ASTContext &Context); /// Apply lazy update if the update is touched during the writing process. void getLazyUpdates(const Decl *D); diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 2d8e146b64257f..3876839a45cc2c 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -5708,7 +5708,7 @@ void ASTWriter::computeNonAffectingInputFiles() { FileMgr.trackVFSUsage(false); } -void ASTWriter::prepareLazyUpdates() { +void ASTWriter::prepareLazyUpdates(ASTContext &Context) { // In C++20 named modules with reduced BMI, we only apply the update // if these updates are touched. if (!GeneratingReducedBMI) @@ -5717,6 +5717,8 @@ void ASTWriter::prepareLazyUpdates() { DeclUpdateMap DeclUpdatesTmp; // Move updates to DeclUpdatesLazy but leave the updates that may be needed by // code generation eager. + // FIXME: The condition below is ugly. We need a better mechanism to decide + // which updates must be eager. for (auto &DeclUpdate : DeclUpdates) { const Decl *D = DeclUpdate.first; @@ -5728,20 +5730,19 @@ void ASTWriter::prepareLazyUpdates() { // care of them, users might meet missing definition errors at linking // time. Here we leave all CXXAddedFunctionDefinition updates eager // unconditionally to avoid potential issues. - // TODO: Try to refine the strategy to handle CXXAddedFunctionDefinition - // precisely. DeclUpdatesTmp[D].push_back( ASTWriter::DeclUpdate(DeclUpdateKind::CXXAddedFunctionDefinition)); - else if (Kind == DeclUpdateKind::CXXResolvedDtorDelete || - Kind == DeclUpdateKind::CXXResolvedDtorGlobDelete || - Kind == DeclUpdateKind::CXXResolvedDtorArrayDelete || - Kind == DeclUpdateKind::CXXResolvedDtorGlobArrayDelete) + else if (Context.getTargetInfo().getCXXABI().isMicrosoft() && + (Kind == DeclUpdateKind::CXXResolvedDtorDelete || + Kind == DeclUpdateKind::CXXResolvedDtorGlobDelete || + Kind == DeclUpdateKind::CXXResolvedDtorArrayDelete || + Kind == DeclUpdateKind::CXXResolvedDtorGlobArrayDelete)) // The CXXResolvedDtor* updates record the 'operator delete' that Sema - // resolved for a virtual destructor. They are only produced for the - // Microsoft ABI. In that ABI the deleting destructor is emitted - // together with the vtable, so an importer can need the resolution - // without a declaration reference to the destructor, and it cannot - // recompute it. Keep these updates eager as well. + // resolved for a virtual destructor. In Microsoft ABI the deleting + // destructor is emitted together with the vtable, so an importer can + // need the resolution without a declaration reference to the + // destructor, and it cannot recompute it. Keep these updates eager as + // well. DeclUpdatesTmp[D].push_back(Update); else DeclUpdatesLazy[D].push_back(Update); @@ -5759,7 +5760,7 @@ void ASTWriter::PrepareWritingSpecialDecls(Sema &SemaRef) { bool isModule = WritingModule != nullptr; - prepareLazyUpdates(); + prepareLazyUpdates(Context); // Set up predefined declaration IDs. auto RegisterPredefDecl = [&] (Decl *D, PredefinedDeclIDs ID) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
