llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: /bin/cat (Maoyao233)
<details>
<summary>Changes</summary>
## Summary
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 LLVM IR.
**Root cause**: Because the destructor's declaration lives in another PCM, its
resolved `operator delete` can only travel as a `CXXResolvedDtorDelete`
`DeclUpdate`, which the reduced-BMI writer drops when the module never
references that declaration — leaving the importing TU with a null
`getOperatorDelete()` and crashing in `EnterDtorCleanups`. A release build
dereferences the null pointer instead.
This PR is a simple stopgap: it fixes this particular crash, but the underlying
issue -- wheter a `DeclUpdate` can be dropped is decided by per kind instead of
from what codegen will actually need -- affects other update kinds as well. I
plan to follow up with an RFC proposing an obligation-driven model.
## Reproducer
```C++
// 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;
export void make() { new Base<int>; }
// main.cpp
import a;
import b;
struct Local : Base<int> {};
Local l;
int main() { return 0; }
```
```bash
$ clang++ --target=x86_64-pc-windows-msvc -std=c++20 -c a.cppm -o a.o
$ clang++ --target=x86_64-pc-windows-msvc -std=c++20 -fmodule-file=a=a.pcm -c
b.cppm -o b.o
$ clang++ --target=x86_64-pc-windows-msvc -std=c++20 -fmodule-file=a=a.pcm
-fmodule-file=b=b.pcm -c main.cpp -o main.o
# the last command crashes
```
## Root Cause
1. `Sema::MarkVTableUsed()` (`SemaDeclCXX.cpp:19470`) has an MS-ABI-only hook:
"the deleting
destructor is emitted with the vtable, not with the destructor definition as
in the Itanium
ABI", so when the vtable is marked used it calls `CheckDestructor()`
(`SemaDeclCXX.cpp:11265`) for a user-declared, not-yet-defined virtual
destructor. That
resolves the `operator delete` (`CXXDestructorDecl::setOperatorDelete`,
`DeclCXX.cpp:3172` →
`ASTContext::addOperatorDeleteForVDtor`, `ASTContext.cpp:13770`). In
`b.cppm` the vtable use
comes from instantiating the inline constructor: `ActOnFinishFunctionBody`
(`SemaDecl.cpp:17063`) marks the vtable used for any constructor/destructor
whose body is
defined — including instantiated ones
(`SemaTemplateInstantiateDecl.cpp:6302`).
2. That destructor declaration is not owned by `b.cppm`: `extern template
struct Base<int>;`
placed it in `a.pcm`, so it is imported. The resolution is therefore
recorded as an update on
an imported declaration (`ASTWriter::ResolvedOperatorDelete`,
`ASTWriter.cpp:7847`):
```cpp
Chain->forEachImportedKeyDecl(DD, [&](const Decl *D) {
DeclUpdates[D].push_back(DeclUpdate(DeclUpdateKind::CXXResolvedDtorDelete,
Delete));
});
```
(A destructor owned by the module would be serialized into its own
declaration record,
`ASTWriterDecl.cpp:1791`.)
3. Module interface units are compiled as **reduced BMIs** by default (the
driver adds
`-fmodules-reduced-bmi`, `Clang.cpp:4328`). `ASTWriter::prepareLazyUpdates()`
(`ASTWriter.cpp:5710`) moves every update except
`CXXAddedFunctionDefinition` into
`DeclUpdatesLazy`; a lazy update is materialized only if the module writes a
declaration
reference to that declaration (`getLazyUpdates`, `ASTWriter.cpp:7233`,
called from
`GetDeclRef`). `b.cppm` never references `Base<int>::~Base` — the
emitted deleting
destructor and vtable slot are referred to by mangled name — so the update
is silently
dropped and `b.pcm` never carries the resolved `operator delete`.
4. The consumer emits `Base<int>`'s vtable (no key functions in the
Microsoft ABI) and its
deleting destructor: its locally emitted copy of the materialized
`Base<int>::Base()` stores
the vtable pointer → `CodeGenModule::addDeferredVTable`
(`CodeGenModule.h:1648`) →
`EmitDeferredVTables` → `GenerateClassData` →
`EnterDtorCleanups(Dtor_Deleting)`. That path
does not go through Sema's `MarkVTableUsed` hook, so nothing re-resolves the
`operator delete`
in the consumer.
An Itanium-ABI target does not crash: `Base<T>::~Base()` is the key
function of the
specialization (first user-declared, non-inline virtual member) and is never
defined, so
`Sema::DefineUsedVTables()` (`SemaDeclCXX.cpp:19501`) sets `DefineVTable =
false` and the vtable
is never emitted.
Instrumented runs confirm each step: the module TU resolves the `operator
delete` and queues the
update, `WriteDeclUpdatesBlocks` never emits a `DECL_UPDATES` record for it,
and the consumer's
`ASTReader::loadDeclUpdateRecords` reports no pending update for that
declaration.
## Fix
Keep the `CXXResolvedDtor*` updates eager, like `CXXAddedFunctionDefinition`.
An importer cannot recompute them -- the destructor may already have a body or
be defined in another TU -- but codegen
needs them to emit the deleting destructor with the vtable. The size cost is
negligible: these updates only exist when Sema resolved a delete operator for a
destructor declaration that is owned by another module.
An Itanium-ABI target does not crash: the not-yet-defined destructor is the key
function of the specialization, so no TU emits the vtable. Full BMIs (with
`-fno-modules-reduced-bmi`) do not crash either, which pins this on the lazy
update handling above.
Test: `clang/test/Modules/msvc-dtor-operator-delete-reduced-bmi.cpp`, a
split-file test that runs the three compilations above through %clang_cc1 with
`-fmodules-reduced-bmi`; it aborts on main without the fix and passes with it
(verified with an assertions build).
Fixes #<!-- -->231859.
Assisted by: Deepseek V4.1-flash-1017
---
Full diff: https://github.com/llvm/llvm-project/pull/235673.diff
2 Files Affected:
- (modified) clang/lib/Serialization/ASTWriter.cpp (+10)
- (added) clang/test/Modules/msvc-dtor-operator-delete-reduced-bmi.cpp (+46)
``````````diff
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; }
``````````
</details>
https://github.com/llvm/llvm-project/pull/235673
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
