[clang] WIP: [clang] MicrosoftCXXABI: Fix exception copy constructor LUT after loading AST (PR #114075)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 29 08:52:30 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-modules
@llvm/pr-subscribers-clang
Author: Andrey Glebov (glebov-andrey)
<details>
<summary>Changes</summary>
Adds a serialized `HasCopyConstructorForExceptionObject` bit to `CXXRecordDecl` which is used to inform `ASTReader` that this class's copy constructor needs to be added to the `MicrosoftCXXABI::RecordToCopyCtor` LUT.
Includes a regression test for the original issue with a PCH.
Fixes #<!-- -->53486
I'm not sure if this is the best (or even correct) way of fixing the issue, but it does appear to work in simple test cases with PCHs and modules.
The existence of this issue also raises the question of other tables which are stored in the `MicrosoftCXXABI` AST - do they also need to be restored in `ASTReader`?
---
Full diff: https://github.com/llvm/llvm-project/pull/114075.diff
7 Files Affected:
- (modified) clang/include/clang/AST/CXXRecordDeclDefinitionBits.def (+5)
- (modified) clang/include/clang/AST/DeclCXX.h (+8)
- (modified) clang/lib/AST/DeclCXX.cpp (+4-3)
- (modified) clang/lib/Sema/SemaExprCXX.cpp (+4)
- (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+14)
- (added) clang/test/PCH/cxx-exception-copy-ctor-crash.cpp (+19)
- (added) clang/test/PCH/cxx-exception-copy-ctor-crash.h (+14)
``````````diff
diff --git a/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def b/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
index 6620840df0ced2..7560691a0e4172 100644
--- a/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
+++ b/clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
@@ -249,6 +249,11 @@ FIELD(HasDeclaredCopyAssignmentWithConstParam, 1, MERGE_OR)
/// base classes or fields have a no-return destructor
FIELD(IsAnyDestructorNoReturn, 1, NO_MERGE)
+/// Microsoft CXX ABI specific:
+/// Whether the copy constructor is used by a `throw` expression.
+/// Used by ASTReader to restore the sidecar RecordToCopyCtor LUT.
+FIELD(HasCopyConstructorForExceptionObject, 1, MERGE_OR)
+
/// Whether the record type is intangible (if any base classes or fields have
/// type that is intangible). HLSL only.
FIELD(IsHLSLIntangible, 1, NO_MERGE)
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index 2693cc0e95b4b2..f9b2ecf8af665c 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -1551,6 +1551,14 @@ class CXXRecordDecl : public RecordDecl {
/// a field or in base class.
bool isHLSLIntangible() const { return data().IsHLSLIntangible; }
+ bool hasCopyConstructorForExceptionObject() const {
+ return data().HasCopyConstructorForExceptionObject;
+ }
+
+ void setHasCopyConstructorForExceptionObject() {
+ data().HasCopyConstructorForExceptionObject = true;
+ }
+
/// If the class is a local class [class.local], returns
/// the enclosing function declaration.
const FunctionDecl *isLocalClass() const {
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index db0ea62a2323eb..ff21c44bf8fba2 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -109,9 +109,10 @@ CXXRecordDecl::DefinitionData::DefinitionData(CXXRecordDecl *D)
ImplicitCopyAssignmentHasConstParam(true),
HasDeclaredCopyConstructorWithConstParam(false),
HasDeclaredCopyAssignmentWithConstParam(false),
- IsAnyDestructorNoReturn(false), IsHLSLIntangible(false), IsLambda(false),
- IsParsingBaseSpecifiers(false), ComputedVisibleConversions(false),
- HasODRHash(false), Definition(D) {}
+ IsAnyDestructorNoReturn(false),
+ HasCopyConstructorForExceptionObject(false), IsHLSLIntangible(false),
+ IsLambda(false), IsParsingBaseSpecifiers(false),
+ ComputedVisibleConversions(false), HasODRHash(false), Definition(D) {}
CXXBaseSpecifier *CXXRecordDecl::DefinitionData::getBasesSlowCase() const {
return Bases.get(Definition->getASTContext().getExternalSource());
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 50c1b24fce6da7..586ca74a848da2 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -1082,6 +1082,10 @@ bool Sema::CheckCXXThrowOperand(SourceLocation ThrowLoc,
// friendship or any other means).
Context.addCopyConstructorForExceptionObject(Subobject, CD);
+ // Store the bit in CXXRecordDecl so that ASTReader can restore this
+ // mapping later.
+ Subobject->setHasCopyConstructorForExceptionObject();
+
// We don't keep the instantiated default argument expressions around so
// we must rebuild them here.
for (unsigned I = 1, E = CD->getNumParams(); I != E; ++I) {
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index d4e392dcc6bcd0..fdb1da4026db1c 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -2316,6 +2316,20 @@ void ASTDeclReader::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
}
VisitCXXMethodDecl(D);
+
+ // Microsoft CXX ABI specific:
+ // Restore the RecordToCopyCtor sidecar LUT entry so that `throw` expressions
+ // find the correct copy constructor for exceptions during codegen.
+ // There is no need to check the target info because the
+ // HasCopyConstructorForExceptionObject bit only gets set for the MS ABI.
+ if (D->isCopyConstructor()) {
+ // TODO What if this is not the same copy constructor which was chosen by
+ // LookupCopyingConstructor() in SemaExprCXX? Is there a better way?
+ auto *R = cast<CXXRecordDecl>(D->getDeclContext());
+ if (R->hasCopyConstructorForExceptionObject()) {
+ Reader.getContext().addCopyConstructorForExceptionObject(R, D);
+ }
+ }
}
void ASTDeclReader::VisitCXXDestructorDecl(CXXDestructorDecl *D) {
diff --git a/clang/test/PCH/cxx-exception-copy-ctor-crash.cpp b/clang/test/PCH/cxx-exception-copy-ctor-crash.cpp
new file mode 100644
index 00000000000000..29bcb114f20f0a
--- /dev/null
+++ b/clang/test/PCH/cxx-exception-copy-ctor-crash.cpp
@@ -0,0 +1,19 @@
+// REQUIRES: system-windows, target={{.*-windows-msvc}}
+// RUN: %clang_cc1 -x c++ -std=c++17 -fcxx-exceptions -fexceptions -triple=%ms_abi_triple -emit-pch -building-pch-with-obj -fmodules-codegen -o %t.pch %S/cxx-exception-copy-ctor-crash.h
+// RUN: %clang_cc1 -x c++ -std=c++17 -fcxx-exceptions -fexceptions -triple=%ms_abi_triple -include-pch %t.pch -emit-obj -building-pch-with-obj -fmodules-codegen -o %t.pch.obj
+// RUN: %clang_cc1 -x c++ -std=c++17 -fcxx-exceptions -fexceptions -triple=%ms_abi_triple -include-pch %t.pch -emit-obj -o %t.obj %s
+// RUN: lld-link -subsystem:console -out:%t.exe %t.pch.obj %t.obj libucrt.lib libvcruntime.lib libcmt.lib
+// RUN: %t.exe
+
+// Regression test for https://github.com/llvm/llvm-project/issues/53486
+
+int main() {
+ try {
+ throw Exception();
+ } catch (const Exception ex) { // catch by value to trigger copy constructor
+ }
+ if (ctor_count != dtor_count) {
+ return 1;
+ }
+ return 0;
+}
diff --git a/clang/test/PCH/cxx-exception-copy-ctor-crash.h b/clang/test/PCH/cxx-exception-copy-ctor-crash.h
new file mode 100644
index 00000000000000..9645df56c786e3
--- /dev/null
+++ b/clang/test/PCH/cxx-exception-copy-ctor-crash.h
@@ -0,0 +1,14 @@
+// Header for PCH test cxx-exception-copy-ctor-crash.cpp
+
+inline int ctor_count = 0;
+inline int dtor_count = 0;
+
+struct Exception {
+ Exception() { ++ctor_count; }
+ ~Exception() { ++dtor_count; }
+ Exception(const Exception &) noexcept { ++ctor_count; }
+};
+
+inline void throw_exception() {
+ throw Exception();
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/114075
More information about the cfe-commits
mailing list