[MS] Mark vbase dtors used when marking dtor used

In the MS C++ ABI, the complete destructor variant for a class with
virtual bases is emitted whereever it is needed, instead of directly
alongside the base destructor variant. The complete destructor calls the
base destructor of the current class and the base destructors of each
virtual base. In order for this to work reliably, translation units that
use the destructor of a class also need to mark destructors of virtual
bases of that class used.

Fixes PR38521

Reviewed By: rsmith

Differential Revision: https://reviews.llvm.org/D77081

clang/include/clang/Sema/Sema.h

@@ -6667,6 +6667,22 @@ public:
   void MarkBaseAndMemberDestructorsReferenced(SourceLocation Loc,
                                               CXXRecordDecl *Record);
 
+  /// Mark destructors of virtual bases of this class referenced. In the Itanium
+  /// C++ ABI, this is done when emitting a destructor for any non-abstract
+  /// class. In the Microsoft C++ ABI, this is done any time a class's
+  /// destructor is referenced.
+  void MarkVirtualBaseDestructorsReferenced(
+      SourceLocation Location, CXXRecordDecl *ClassDecl,
+      llvm::SmallPtrSetImpl<const RecordType *> *DirectVirtualBases = nullptr);
+
+  /// Do semantic checks to allow the complete destructor variant to be emitted
+  /// when the destructor is defined in another translation unit. In the Itanium
+  /// C++ ABI, destructor variants are emitted together. In the MS C++ ABI, they
+  /// can be emitted in separate TUs. To emit the complete variant, run a subset
+  /// of the checks performed when emitting a regular destructor.
+  void CheckCompleteDestructorVariant(SourceLocation CurrentLocation,
+                                      CXXDestructorDecl *Dtor);
+
   /// The list of classes whose vtables have been used within
   /// this translation unit, and the source locations at which the
   /// first use occurred.

clang/lib/Sema/SemaDeclCXX.cpp

@@ -5443,6 +5443,15 @@ Sema::MarkBaseAndMemberDestructorsReferenced(SourceLocation Location,
   // subobjects.
   bool VisitVirtualBases = !ClassDecl->isAbstract();
 
+  // If the destructor exists and has already been marked used in the MS ABI,
+  // then virtual base destructors have already been checked and marked used.
+  // Skip checking them again to avoid duplicate diagnostics.
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+    CXXDestructorDecl *Dtor = ClassDecl->getDestructor();
+    if (Dtor && Dtor->isUsed())
+      VisitVirtualBases = false;
+  }
+
   llvm::SmallPtrSet<const RecordType *, 8> DirectVirtualBases;
 
   // Bases.
@@ -5477,16 +5486,21 @@ Sema::MarkBaseAndMemberDestructorsReferenced(SourceLocation Location,
     DiagnoseUseOfDecl(Dtor, Location);
   }
 
-  if (!VisitVirtualBases)
-    return;
+  if (VisitVirtualBases)
+    MarkVirtualBaseDestructorsReferenced(Location, ClassDecl,
+                                         &DirectVirtualBases);
+}
 
+void Sema::MarkVirtualBaseDestructorsReferenced(
+    SourceLocation Location, CXXRecordDecl *ClassDecl,
+    llvm::SmallPtrSetImpl<const RecordType *> *DirectVirtualBases) {
   // Virtual bases.
   for (const auto &VBase : ClassDecl->vbases()) {
     // Bases are always records in a well-formed non-dependent class.
     const RecordType *RT = VBase.getType()->castAs<RecordType>();
 
-    // Ignore direct virtual bases.
-    if (DirectVirtualBases.count(RT))
+    // Ignore already visited direct virtual bases.
+    if (DirectVirtualBases && DirectVirtualBases->count(RT))
       continue;
 
     CXXRecordDecl *BaseClassDecl = cast<CXXRecordDecl>(RT->getDecl());
@@ -13202,6 +13216,25 @@ void Sema::DefineImplicitDestructor(SourceLocation CurrentLocation,
   }
 }
 
+void Sema::CheckCompleteDestructorVariant(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);
+
+  MarkVirtualBaseDestructorsReferenced(Destructor->getLocation(), ClassDecl);
+}
+
 /// Perform any semantic analysis which needs to be delayed until all
 /// pending class member declarations have been parsed.
 void Sema::ActOnFinishCXXMemberDecls() {

clang/lib/Sema/SemaExpr.cpp

@@ -16488,6 +16488,20 @@ void Sema::MarkFunctionReferenced(SourceLocation Loc, FunctionDecl *Func,
     if (funcHasParameterSizeMangling(*this, Func))
       CheckCompleteParameterTypesForMangler(*this, Func, Loc);
 
+    // In the MS C++ ABI, the compiler emits destructor variants where they are
+    // used. If the destructor is used here but defined elsewhere, mark the
+    // virtual base destructors referenced. If those virtual base destructors
+    // are inline, this will ensure they are defined when emitting the complete
+    // destructor variant. This checking may be redundant if the destructor is
+    // provided later in this TU.
+    if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+      if (auto *Dtor = dyn_cast<CXXDestructorDecl>(Func)) {
+        CXXRecordDecl *Parent = Dtor->getParent();
+        if (Parent->getNumVBases() > 0 && !Dtor->getBody())
+          CheckCompleteDestructorVariant(Loc, Dtor);
+      }
+    }
+
     Func->markUsed(Context);
   }
 }

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: %clang_cc1 -triple x86_64-windows-msvc -fms-compatibility-version=19 -fcxx-exceptions -fexceptions -fsyntax-only -verify %s
 
 // C++0x [class.access]p4:
 
@@ -139,7 +142,7 @@ namespace test3 {
     A local; // expected-error {{variable of type 'test3::A' has private destructor}}
   }
 
-#if __cplusplus < 201103L
+#if __cplusplus < 201103L && !defined(_MSC_VER)
   template <unsigned N> class Base { ~Base(); }; // expected-note 14 {{declared private here}}
   class Base2 : virtual Base<2> { ~Base2(); }; // expected-note 3 {{declared private here}} \
                                                // expected-error {{base class 'Base<2>' has private destructor}}
@@ -161,15 +164,43 @@ namespace test3 {
 
   class Derived3 : // expected-error 2 {{inherited virtual base class 'Base<2>' has private destructor}} \
                    // expected-error 2 {{inherited virtual base class 'Base<3>' has private destructor}} \
-    // expected-note 2{{implicit default constructor}}
+                   // expected-note 2{{implicit default constructor}}
     Base<0>,  // expected-error 2 {{base class 'Base<0>' has private destructor}}
     virtual Base<1>, // expected-error 2 {{base class 'Base<1>' has private destructor}}
     Base2, // expected-error 2 {{base class 'test3::Base2' has private destructor}}
     virtual Base3
-  {}; 
-  Derived3 d3; // expected-note 3{{implicit default constructor}}\
-               // expected-note{{implicit destructor}}}
-#else
+  {};
+  Derived3 d3; // expected-note{{implicit destructor}}} \
+      // expected-note 3 {{implicit default constructor}}
+#elif __cplusplus < 201103L && defined(_MSC_VER)
+  template <unsigned N> class Base { ~Base(); }; // expected-note 14 {{declared private here}}
+  class Base2 : virtual Base<2> { ~Base2(); }; // expected-note 3 {{declared private here}} \
+                                               // expected-error {{base class 'Base<2>' has private destructor}}
+  class Base3 : virtual Base<3> { public: ~Base3(); }; // expected-error {{base class 'Base<3>' has private destructor}}
+
+  // These don't cause diagnostics because we don't need the destructor.
+  class Derived0 : Base<0> { ~Derived0(); };
+  class Derived1 : Base<1> { };
+
+  class Derived2 : // expected-error {{inherited virtual base class 'Base<2>' has private destructor}} \
+                   // expected-error {{inherited virtual base class 'Base<3>' has private destructor}}
+    Base<0>,  // expected-error {{base class 'Base<0>' has private destructor}}
+    virtual Base<1>, // expected-error {{base class 'Base<1>' has private destructor}}
+    Base2, // expected-error {{base class 'test3::Base2' has private destructor}}
+    virtual Base3
+  {
+    ~Derived2() {} // expected-note 2{{in implicit destructor}}
+  };
+
+  class Derived3 : // expected-error 2 {{inherited virtual base class 'Base<2>' has private destructor}} \
+                   // expected-error 2 {{inherited virtual base class 'Base<3>' has private destructor}}
+    Base<0>,  // expected-error 2 {{base class 'Base<0>' has private destructor}}
+    virtual Base<1>, // expected-error 2 {{base class 'Base<1>' has private destructor}}
+    Base2, // expected-error 2 {{base class 'test3::Base2' has private destructor}}
+    virtual Base3
+  {};
+  Derived3 d3; // expected-note{{implicit destructor}}} expected-note {{implicit default constructor}}
+#elif __cplusplus >= 201103L && !defined(_MSC_VER)
   template <unsigned N> class Base { ~Base(); }; // expected-note 4{{declared private here}}
   class Base2 : virtual Base<2> { ~Base2(); }; // expected-note 1{{declared private here}}
   class Base3 : virtual Base<3> { public: ~Base3(); };
@@ -193,8 +224,40 @@ namespace test3 {
     virtual Base<1>,
     Base2,
     virtual Base3
-  {}; 
+  {};
   Derived3 d3; // expected-error {{implicitly-deleted default constructor}}
+#elif __cplusplus >= 201103L && defined(_MSC_VER)
+  template <unsigned N> class Base { ~Base(); }; // expected-note 6{{declared private here}}
+  // expected-error@+1 {{inherited virtual base class 'Base<2>' has private destructor}}
+  class Base2 : virtual Base<2> { ~Base2(); }; // expected-note 1{{declared private here}}
+  // expected-error@+1 {{inherited virtual base class 'Base<3>' has private destructor}}
+  class Base3 : virtual Base<3> { public: ~Base3(); };
+
+  // These don't cause diagnostics because we don't need the destructor.
+  class Derived0 : Base<0> { ~Derived0(); };
+  class Derived1 : Base<1> { };
+
+  class Derived2 : // expected-error {{inherited virtual base class 'Base<2>' has private destructor}} \
+                   // expected-error {{inherited virtual base class 'Base<3>' has private destructor}}
+    Base<0>,  // expected-error {{base class 'Base<0>' has private destructor}}
+    virtual Base<1>, // expected-error {{base class 'Base<1>' has private destructor}}
+    Base2, // expected-error {{base class 'test3::Base2' has private destructor}}
+    virtual Base3
+  {
+    // expected-note@+2 {{in implicit destructor for 'test3::Base2' first required here}}
+    // expected-note@+1 {{in implicit destructor for 'test3::Base3' first required here}}
+    ~Derived2() {}
+  };
+
+  class Derived3 :
+    Base<0>, // expected-note {{deleted because base class 'Base<0>' has an inaccessible destructor}}
+    virtual Base<1>,
+    Base2,
+    virtual Base3
+  {};
+  Derived3 d3; // expected-error {{implicitly-deleted default constructor}}
+#else
+#error "missing case of MSVC cross C++ versions"
 #endif
 }
 

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* %{{.*}})
+

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 <unsigned N>
+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}}
+}