[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG55efb68c19b4: [MS] Mark vbase dtors used when marking dtor
used (authored by rnk).
Changed prior to commit:
https://reviews.llvm.org/D77081?vs=253972=256415#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77081/new/
https://reviews.llvm.org/D77081
Files:
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/CXX/class.access/p4.cpp
clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
Index: clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -std=c++17 -verify -Wno-defaulted-function-deleted %s -triple x86_64-windows-msvc
+
+// MSVC emits the complete destructor as if it were its own special member.
+// Clang attempts to do the same. This affects the diagnostics clang emits,
+// because deleting a type with a user declared constructor implicitly
+// references the destructors of virtual bases, which might be deleted or access
+// controlled.
+
+namespace t1 {
+struct A {
+ ~A() = delete; // expected-note {{deleted here}}
+};
+struct B {
+ B() = default;
+ A o; // expected-note {{destructor of 'B' is implicitly deleted because field 'o' has a deleted destructor}}
+};
+struct C : virtual B {
+ ~C(); // expected-error {{attempt to use a deleted function}}
+};
+void delete1(C *p) { delete p; } // expected-note {{in implicit destructor for 't1::C' first required here}}
+void delete2(C *p) { delete p; }
+}
+
+namespace t2 {
+struct A {
+private:
+ ~A();
+};
+struct B {
+ B() = default;
+ A o; // expected-note {{destructor of 'B' is implicitly deleted because field 'o' has an inaccessible destructor}}
+};
+struct C : virtual B {
+ ~C(); // expected-error {{attempt to use a deleted function}}
+};
+void useCompleteDtor(C *p) { delete p; } // expected-note {{in implicit destructor for 't2::C' first required here}}
+}
+
+namespace t3 {
+template
+class Base { ~Base(); }; // expected-note 1{{declared private here}}
+// No diagnostic.
+class Derived0 : virtual Base<0> { ~Derived0(); };
+class Derived1 : virtual Base<1> {};
+// Emitting complete dtor causes a diagnostic.
+struct Derived2 : // expected-error {{inherited virtual base class 'Base<2>' has private destructor}}
+ virtual Base<2> {
+ ~Derived2();
+};
+void useCompleteDtor(Derived2 *p) { delete p; } // expected-note {{in implicit destructor for 't3::Derived2' first required here}}
+}
Index: clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++17 -emit-llvm %s -triple x86_64-windows-msvc -o - | FileCheck %s
+
+// Make sure virtual base base destructors get referenced and emitted if
+// necessary when the complete ("vbase") destructor is emitted. In this case,
+// clang previously did not emit ~DefaultedDtor.
+struct HasDtor { ~HasDtor(); };
+struct DefaultedDtor {
+ ~DefaultedDtor() = default;
+ HasDtor o;
+};
+struct HasCompleteDtor : virtual DefaultedDtor {
+ ~HasCompleteDtor();
+};
+void useCompleteDtor(HasCompleteDtor *p) { delete p; }
+
+// CHECK-LABEL: define dso_local void @"?useCompleteDtor@@YAXPEAUHasCompleteDtor@@@Z"(%struct.HasCompleteDtor* %p)
+// CHECK: call void @"??_DHasCompleteDtor@@QEAAXXZ"({{.*}})
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??_DHasCompleteDtor@@QEAAXXZ"(%struct.HasCompleteDtor* %this)
+// CHECK: call void @"??1HasCompleteDtor@@QEAA@XZ"({{.*}})
+// CHECK: call void @"??1DefaultedDtor@@QEAA@XZ"({{.*}})
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??1DefaultedDtor@@QEAA@XZ"(%struct.DefaultedDtor* %this)
+// CHECK: call void @"??1HasDtor@@QEAA@XZ"(%struct.HasDtor* %{{.*}})
+
Index: clang/test/CXX/class.access/p4.cpp
===
--- clang/test/CXX/class.access/p4.cpp
+++ clang/test/CXX/class.access/p4.cpp
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rnk added a comment. I'm going to go ahead and push this today. Richard hasn't stamped it, but I did incorporate the feedback, and I'm fairly happy with the results and confident that I addressed the feedback adquately. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77081/new/ https://reviews.llvm.org/D77081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rnk added a comment. ptal Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77081/new/ https://reviews.llvm.org/D77081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rnk updated this revision to Diff 253972.
rnk added a comment.
- finish refactoring, build & test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77081/new/
https://reviews.llvm.org/D77081
Files:
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/CXX/class.access/p4.cpp
clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
Index: clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -std=c++17 -verify -Wno-defaulted-function-deleted %s -triple x86_64-windows-msvc
+
+// MSVC emits the complete destructor as if it were its own special member.
+// Clang attempts to do the same. This affects the diagnostics clang emits,
+// because deleting a type with a user declared constructor implicitly
+// references the destructors of virtual bases, which might be deleted or access
+// controlled.
+
+namespace t1 {
+struct A {
+ ~A() = delete; // expected-note {{deleted here}}
+};
+struct B {
+ B() = default;
+ A o; // expected-note {{destructor of 'B' is implicitly deleted because field 'o' has a deleted destructor}}
+};
+struct C : virtual B {
+ ~C(); // expected-error {{attempt to use a deleted function}}
+};
+void delete1(C *p) { delete p; } // expected-note {{in implicit destructor for 't1::C' first required here}}
+void delete2(C *p) { delete p; }
+}
+
+namespace t2 {
+struct A {
+private:
+ ~A();
+};
+struct B {
+ B() = default;
+ A o; // expected-note {{destructor of 'B' is implicitly deleted because field 'o' has an inaccessible destructor}}
+};
+struct C : virtual B {
+ ~C(); // expected-error {{attempt to use a deleted function}}
+};
+void useCompleteDtor(C *p) { delete p; } // expected-note {{in implicit destructor for 't2::C' first required here}}
+}
+
+namespace t3 {
+template
+class Base { ~Base(); }; // expected-note 1{{declared private here}}
+// No diagnostic.
+class Derived0 : virtual Base<0> { ~Derived0(); };
+class Derived1 : virtual Base<1> {};
+// Emitting complete dtor causes a diagnostic.
+struct Derived2 : // expected-error {{inherited virtual base class 'Base<2>' has private destructor}}
+ virtual Base<2> {
+ ~Derived2();
+};
+void useCompleteDtor(Derived2 *p) { delete p; } // expected-note {{in implicit destructor for 't3::Derived2' first required here}}
+}
Index: clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++17 -emit-llvm %s -triple x86_64-windows-msvc -o - | FileCheck %s
+
+// Make sure virtual base base destructors get referenced and emitted if
+// necessary when the complete ("vbase") destructor is emitted. In this case,
+// clang previously did not emit ~DefaultedDtor.
+struct HasDtor { ~HasDtor(); };
+struct DefaultedDtor {
+ ~DefaultedDtor() = default;
+ HasDtor o;
+};
+struct HasCompleteDtor : virtual DefaultedDtor {
+ ~HasCompleteDtor();
+};
+void useCompleteDtor(HasCompleteDtor *p) { delete p; }
+
+// CHECK-LABEL: define dso_local void @"?useCompleteDtor@@YAXPEAUHasCompleteDtor@@@Z"(%struct.HasCompleteDtor* %p)
+// CHECK: call void @"??_DHasCompleteDtor@@QEAAXXZ"({{.*}})
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??_DHasCompleteDtor@@QEAAXXZ"(%struct.HasCompleteDtor* %this)
+// CHECK: call void @"??1HasCompleteDtor@@QEAA@XZ"({{.*}})
+// CHECK: call void @"??1DefaultedDtor@@QEAA@XZ"({{.*}})
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??1DefaultedDtor@@QEAA@XZ"(%struct.DefaultedDtor* %this)
+// CHECK: call void @"??1HasDtor@@QEAA@XZ"(%struct.HasDtor* %{{.*}})
+
Index: clang/test/CXX/class.access/p4.cpp
===
--- clang/test/CXX/class.access/p4.cpp
+++ clang/test/CXX/class.access/p4.cpp
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
+// RUN:
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rnk added a comment. I'm glad to report that your suggestion worked out well! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77081/new/ https://reviews.llvm.org/D77081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rnk updated this revision to Diff 253933.
rnk added a comment.
- Remove definition data bit tracking, use destructor isUsed bit
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77081/new/
https://reviews.llvm.org/D77081
Files:
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/CXX/class.access/p4.cpp
clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
Index: clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -std=c++17 -verify -Wno-defaulted-function-deleted %s -triple x86_64-windows-msvc
+
+// MSVC emits the complete destructor as if it were its own special member.
+// Clang attempts to do the same. This affects the diagnostics clang emits,
+// because deleting a type with a user declared constructor implicitly
+// references the destructors of virtual bases, which might be deleted or access
+// controlled.
+
+namespace t1 {
+struct A {
+ ~A() = delete; // expected-note {{deleted here}}
+};
+struct B { // expected-error {{attempt to use a deleted function}}
+ B() = default;
+ A o; // expected-note {{destructor of 'B' is implicitly deleted because field 'o' has a deleted destructor}}
+};
+struct C : virtual B {
+ ~C();
+};
+void delete1(C *p) { delete p; } // expected-note {{in implicit destructor for 't1::C' first required here}}
+void delete2(C *p) { delete p; }
+}
+
+namespace t2 {
+struct A {
+private:
+ ~A();
+};
+struct B { // expected-error {{attempt to use a deleted function}}
+ B() = default;
+ A o; // expected-note {{destructor of 'B' is implicitly deleted because field 'o' has an inaccessible destructor}}
+};
+struct C : virtual B {
+ ~C();
+};
+void useCompleteDtor(C *p) { delete p; } // expected-note {{in implicit destructor for 't2::C' first required here}}
+}
+
+namespace t3 {
+template
+class Base { ~Base(); }; // expected-note 1{{declared private here}}
+// No diagnostic.
+class Derived0 : virtual Base<0> { ~Derived0(); };
+class Derived1 : virtual Base<1> {};
+// Emitting complete dtor causes a diagnostic.
+struct Derived2 : // expected-error {{inherited virtual base class 'Base<2>' has private destructor}}
+ virtual Base<2> {
+ ~Derived2();
+};
+void useCompleteDtor(Derived2 *p) { delete p; } // expected-note {{in implicit destructor for 't3::Derived2' first required here}}
+}
Index: clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++17 -emit-llvm %s -triple x86_64-windows-msvc -o - | FileCheck %s
+
+// Make sure virtual base base destructors get referenced and emitted if
+// necessary when the complete ("vbase") destructor is emitted. In this case,
+// clang previously did not emit ~DefaultedDtor.
+struct HasDtor { ~HasDtor(); };
+struct DefaultedDtor {
+ ~DefaultedDtor() = default;
+ HasDtor o;
+};
+struct HasCompleteDtor : virtual DefaultedDtor {
+ ~HasCompleteDtor();
+};
+void useCompleteDtor(HasCompleteDtor *p) { delete p; }
+
+// CHECK-LABEL: define dso_local void @"?useCompleteDtor@@YAXPEAUHasCompleteDtor@@@Z"(%struct.HasCompleteDtor* %p)
+// CHECK: call void @"??_DHasCompleteDtor@@QEAAXXZ"({{.*}})
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??_DHasCompleteDtor@@QEAAXXZ"(%struct.HasCompleteDtor* %this)
+// CHECK: call void @"??1HasCompleteDtor@@QEAA@XZ"({{.*}})
+// CHECK: call void @"??1DefaultedDtor@@QEAA@XZ"({{.*}})
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??1DefaultedDtor@@QEAA@XZ"(%struct.DefaultedDtor* %this)
+// CHECK: call void @"??1HasDtor@@QEAA@XZ"(%struct.HasDtor* %{{.*}})
+
Index: clang/test/CXX/class.access/p4.cpp
===
--- clang/test/CXX/class.access/p4.cpp
+++ clang/test/CXX/class.access/p4.cpp
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19 -fcxx-exceptions -fexceptions -fsyntax-only -verify
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rsmith marked an inline comment as done.
rsmith added inline comments.
Comment at: clang/lib/Sema/SemaExpr.cpp:16008-16013
+// In the MS ABI, the complete destructor is implicitly defined,
+// even if the base destructor is user defined.
+CXXRecordDecl *Parent = Destructor->getParent();
+if (Parent->getNumVBases() > 0 &&
+!Parent->definedImplicitCompleteDestructor())
+ DefineImplicitCompleteDestructor(Loc, Destructor);
rnk wrote:
> rsmith wrote:
> > Can we avoid doing this when we know the call is to a base subobject
> > destructor or uses virtual dispatch? Should we? (I mean I guess ideally
> > we'd change this function to take a `GlobalDecl` instead of a
> > `FunctionDecl*`, but that seems like a lot of work.)
> >
> > How does MSVC behave?
> I think we can skip this if virtual dispatch is used, but I'm not sure what
> kinds of devirtualization might break my assumptions.
>
> For example, uncommenting `final` here causes MSVC to diagnose the error, but
> it otherwise it compiles:
> ```
> struct NoDtor { ~NoDtor() = delete; };
> struct DefaultedDtor : NoDtor { ~DefaultedDtor() = default; };
> struct HasVBases /*final*/ : virtual DefaultedDtor {
> virtual ~HasVBases();
> };
> void deleteIt1(HasVBases *p) { delete p; }
> ```
>
> If the NoDtor destructor is undeleted and the class is made final, then both
> deleting and complete dtors are emitted for HasVBases. Clang would need to
> mark DefaultedDtor referenced in this case.
>
> I think it's OK to "overdiagnose" in this case. It's not possible to emit a
> vtable here without diagnosing the error.
Great, if we're happy to treat all uses of the destructor as using the complete
object destructor, then doing this work as part of marking the destructor used
seems like it would be the best solution.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77081/new/
https://reviews.llvm.org/D77081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rnk marked 2 inline comments as done.
rnk added a comment.
Thanks for the feedback, I'm going to investigate if we can use the `used`
destructor bit to do this.
Comment at: clang/include/clang/AST/DeclCXX.h:959-963
+ /// Indicates if the complete destructor has been implicitly declared
+ /// yet. Only relevant in the Microsoft C++.
+ bool definedImplicitCompleteDestructor() const {
+return data().DefinedImplicitCompleteDestructor;
+ }
rsmith wrote:
> "declared" in comment but "defined" in function name. Which is it?
>
> I wonder if perhaps the right answer is neither: if we had separate `Decl`s
> for the complete and base destructors, this would be tracked by the "used"
> bit on the complete object destructor. So perhaps `isCompleteDestructorUsed`
> and `setCompleteDestructorUsed` would make sense? (We could consider tracking
> this on the destructor instead of here, but I suppose tracking it here gives
> us the serialization and merging logic for free.)
Actually, can we just reuse the `used` bit on the destructor? I don't think
there is any way for the user to "use" the base destructor without using the
complete destructor. The only case I can think of that calls the base
destructor (ignoring aliasing optimizations) is the complete destructor of a
derived class. Such a class will also reference all the virtual bases of the
current class, so the semantic checks we are doing here are redundant, not
wrong.
Calling a pseudo-destructor for example uses the complete destructor, I think.
Comment at: clang/lib/Sema/SemaExpr.cpp:16008-16013
+// In the MS ABI, the complete destructor is implicitly defined,
+// even if the base destructor is user defined.
+CXXRecordDecl *Parent = Destructor->getParent();
+if (Parent->getNumVBases() > 0 &&
+!Parent->definedImplicitCompleteDestructor())
+ DefineImplicitCompleteDestructor(Loc, Destructor);
rsmith wrote:
> Can we avoid doing this when we know the call is to a base subobject
> destructor or uses virtual dispatch? Should we? (I mean I guess ideally we'd
> change this function to take a `GlobalDecl` instead of a `FunctionDecl*`, but
> that seems like a lot of work.)
>
> How does MSVC behave?
I think we can skip this if virtual dispatch is used, but I'm not sure what
kinds of devirtualization might break my assumptions.
For example, uncommenting `final` here causes MSVC to diagnose the error, but
it otherwise it compiles:
```
struct NoDtor { ~NoDtor() = delete; };
struct DefaultedDtor : NoDtor { ~DefaultedDtor() = default; };
struct HasVBases /*final*/ : virtual DefaultedDtor {
virtual ~HasVBases();
};
void deleteIt1(HasVBases *p) { delete p; }
```
If the NoDtor destructor is undeleted and the class is made final, then both
deleting and complete dtors are emitted for HasVBases. Clang would need to mark
DefaultedDtor referenced in this case.
I think it's OK to "overdiagnose" in this case. It's not possible to emit a
vtable here without diagnosing the error.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77081/new/
https://reviews.llvm.org/D77081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rsmith added inline comments.
Comment at: clang/include/clang/AST/DeclCXX.h:959-963
+ /// Indicates if the complete destructor has been implicitly declared
+ /// yet. Only relevant in the Microsoft C++.
+ bool definedImplicitCompleteDestructor() const {
+return data().DefinedImplicitCompleteDestructor;
+ }
"declared" in comment but "defined" in function name. Which is it?
I wonder if perhaps the right answer is neither: if we had separate `Decl`s for
the complete and base destructors, this would be tracked by the "used" bit on
the complete object destructor. So perhaps `isCompleteDestructorUsed` and
`setCompleteDestructorUsed` would make sense? (We could consider tracking this
on the destructor instead of here, but I suppose tracking it here gives us the
serialization and merging logic for free.)
Comment at: clang/include/clang/Sema/Sema.h:5562-5565
+ /// Define an implicit complete constructor for classes with vbases in the MS
+ /// ABI.
+ void DefineImplicitCompleteDestructor(SourceLocation CurrentLocation,
+CXXDestructorDecl *Dtor);
Following the prior comment, naming this `MarkCompleteDestructorUsed` might
make sense.
Comment at: clang/lib/Sema/SemaExpr.cpp:16008-16013
+// In the MS ABI, the complete destructor is implicitly defined,
+// even if the base destructor is user defined.
+CXXRecordDecl *Parent = Destructor->getParent();
+if (Parent->getNumVBases() > 0 &&
+!Parent->definedImplicitCompleteDestructor())
+ DefineImplicitCompleteDestructor(Loc, Destructor);
Can we avoid doing this when we know the call is to a base subobject destructor
or uses virtual dispatch? Should we? (I mean I guess ideally we'd change this
function to take a `GlobalDecl` instead of a `FunctionDecl*`, but that seems
like a lot of work.)
How does MSVC behave?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77081/new/
https://reviews.llvm.org/D77081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rnk added a comment. PTAL, here's how I imagine this is supposed to look, but the definition data bit name could probably be improved. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77081/new/ https://reviews.llvm.org/D77081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rnk updated this revision to Diff 253715.
rnk added a comment.
- add def data bit
- add tests
- fix test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77081/new/
https://reviews.llvm.org/D77081
Files:
clang/include/clang/AST/CXXRecordDeclDefinitionBits.def
clang/include/clang/AST/DeclCXX.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/DeclCXX.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/CXX/class.access/p4.cpp
clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
Index: clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/ms-implicit-complete-dtor.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -std=c++17 -verify -Wno-defaulted-function-deleted %s -triple x86_64-windows-msvc
+
+// MSVC emits the complete destructor as if it were its own special member.
+// Clang attempts to do the same. This affects the diagnostics clang emits,
+// because deleting a type with a user declared constructor implicitly
+// references the destructors of virtual bases, which might be deleted or access
+// controlled.
+
+namespace t1 {
+struct A {
+ ~A() = delete; // expected-note {{deleted here}}
+};
+struct B { // expected-error {{attempt to use a deleted function}}
+ B() = default;
+ A o; // expected-note {{destructor of 'B' is implicitly deleted because field 'o' has a deleted destructor}}
+};
+struct C : virtual B {
+ ~C();
+};
+void delete1(C *p) { delete p; } // expected-note {{in implicit destructor for 't1::C' first required here}}
+void delete2(C *p) { delete p; }
+}
+
+namespace t2 {
+struct A {
+private:
+ ~A();
+};
+struct B { // expected-error {{attempt to use a deleted function}}
+ B() = default;
+ A o; // expected-note {{destructor of 'B' is implicitly deleted because field 'o' has an inaccessible destructor}}
+};
+struct C : virtual B {
+ ~C();
+};
+void useCompleteDtor(C *p) { delete p; } // expected-note {{in implicit destructor for 't2::C' first required here}}
+}
+
+namespace t3 {
+template
+class Base { ~Base(); }; // expected-note 1{{declared private here}}
+// No diagnostic.
+class Derived0 : virtual Base<0> { ~Derived0(); };
+class Derived1 : virtual Base<1> {};
+// Emitting complete dtor causes a diagnostic.
+struct Derived2 : // expected-error {{inherited virtual base class 'Base<2>' has private destructor}}
+ virtual Base<2> {
+ ~Derived2();
+};
+void useCompleteDtor(Derived2 *p) { delete p; } // expected-note {{in implicit destructor for 't3::Derived2' first required here}}
+}
Index: clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++17 -emit-llvm %s -triple x86_64-windows-msvc -o - | FileCheck %s
+
+// Make sure virtual base base destructors get referenced and emitted if
+// necessary when the complete ("vbase") destructor is emitted. In this case,
+// clang previously did not emit ~DefaultedDtor.
+struct HasDtor { ~HasDtor(); };
+struct DefaultedDtor {
+ ~DefaultedDtor() = default;
+ HasDtor o;
+};
+struct HasCompleteDtor : virtual DefaultedDtor {
+ ~HasCompleteDtor();
+};
+void useCompleteDtor(HasCompleteDtor *p) { delete p; }
+
+// CHECK-LABEL: define dso_local void @"?useCompleteDtor@@YAXPEAUHasCompleteDtor@@@Z"(%struct.HasCompleteDtor* %p)
+// CHECK: call void @"??_DHasCompleteDtor@@QEAAXXZ"({{.*}})
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??_DHasCompleteDtor@@QEAAXXZ"(%struct.HasCompleteDtor* %this)
+// CHECK: call void @"??1HasCompleteDtor@@QEAA@XZ"({{.*}})
+// CHECK: call void @"??1DefaultedDtor@@QEAA@XZ"({{.*}})
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??1DefaultedDtor@@QEAA@XZ"(%struct.DefaultedDtor* %this)
+// CHECK: call void @"??1HasDtor@@QEAA@XZ"(%struct.HasDtor* %{{.*}})
+
Index: clang/test/CXX/class.access/p4.cpp
===
--- clang/test/CXX/class.access/p4.cpp
+++ clang/test/CXX/class.access/p4.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++98 %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
// C++0x [class.access]p4:
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++
[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor
rnk created this revision.
rnk added a reviewer: rsmith.
Herald added a project: clang.
DONOTSUBMIT: Uploading for feedback on approach, still have test failures:
Failing Tests (1):
Clang :: CXX/class.access/p4.cpp
In the MS C++ ABI, complete destructors for classes with virtual bases
are emitted whereever they are needed. The complete destructor calls:
- the base dtor
- for each vbase, its base dtor
Even when a destructor is user-defined in another TU, clang needs to
mark the virtual base dtors referenced in TUs that call destructors.
This fixes PR38521.
FIXME: What about separating base and complete dtors? i.e. what about
when only base dtors are ref'd?
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D77081
Files:
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15998,10 +15998,19 @@
} else if (CXXDestructorDecl *Destructor =
dyn_cast(Func)) {
Destructor = cast(Destructor->getFirstDecl());
-if (Destructor->isDefaulted() && !Destructor->isDeleted()) {
- if (Destructor->isTrivial() && !Destructor->hasAttr())
-return;
- DefineImplicitDestructor(Loc, Destructor);
+if (!Destructor->isDeleted()) {
+ if (Destructor->isDefaulted()) {
+if (Destructor->isTrivial() &&
+!Destructor->hasAttr())
+ return;
+DefineImplicitDestructor(Loc, Destructor);
+ } else if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+// In the MS ABI, the complete destructor is implicitly defined,
+// even if the base destructor is user defined.
+// FIXME: Need a way to do it only once. "setBody" seems wrong.
+if (Destructor->getParent()->getNumVBases() > 0)
+ DefineImplicitCompleteDestructor(Loc, Destructor);
+ }
}
if (Destructor->isVirtual() && getLangOpts().AppleKext)
MarkVTableUsed(Loc, Destructor->getParent());
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -13202,6 +13202,53 @@
}
}
+void Sema::DefineImplicitCompleteDestructor(SourceLocation CurrentLocation,
+CXXDestructorDecl *Destructor) {
+ if (Destructor->isInvalidDecl())
+return;
+
+ CXXRecordDecl *ClassDecl = Destructor->getParent();
+ assert(Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+ "implicit complete dtors unneeded outside MS ABI");
+ assert(ClassDecl->getNumVBases() > 0 &&
+ "complete dtor only exists for classes with vbases");
+
+ SynthesizedFunctionScope Scope(*this, Destructor);
+
+ // Add a context note for diagnostics produced after this point.
+ Scope.addContextNote(CurrentLocation);
+
+ // No need to resolve the exception specification of the base destructor or
+ // mark vtables referenced. That will be done when the base dtor is defined.
+
+ // Similar to what is done above in MarkBaseAndMemberDestructorsReferenced,
+ // but only for virtual bases.
+ for (const CXXBaseSpecifier : ClassDecl->vbases()) {
+// Bases are always records in a well-formed non-dependent class.
+CXXRecordDecl *BaseRD = VBase.getType()->getAsCXXRecordDecl();
+
+// If our base class is invalid, we probably can't get its dtor anyway.
+if (!BaseRD || BaseRD->isInvalidDecl() || BaseRD->hasIrrelevantDestructor())
+ continue;
+
+CXXDestructorDecl *Dtor = LookupDestructor(BaseRD);
+assert(Dtor && "No dtor found for BaseRD!");
+if (CheckDestructorAccess(
+ClassDecl->getLocation(), Dtor,
+PDiag(diag::err_access_dtor_vbase)
+<< Context.getTypeDeclType(ClassDecl) << VBase.getType(),
+Context.getTypeDeclType(ClassDecl)) == AR_accessible) {
+ CheckDerivedToBaseConversion(
+ Context.getTypeDeclType(ClassDecl), VBase.getType(),
+ diag::err_access_dtor_vbase, 0, ClassDecl->getLocation(),
+ SourceRange(), DeclarationName(), nullptr);
+}
+
+MarkFunctionReferenced(Dtor->getLocation(), Dtor);
+DiagnoseUseOfDecl(Dtor, Dtor->getLocation());
+ }
+}
+
/// Perform any semantic analysis which needs to be delayed until all
/// pending class member declarations have been parsed.
void Sema::ActOnFinishCXXMemberDecls() {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -5559,6 +5559,11 @@
void DefineImplicitDestructor(SourceLocation CurrentLocation,
CXXDestructorDecl *Destructor);
+
