[MS] Mark vbase dtors ref'd when ref'ing dtor

[rnk]

Summary

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

[rnk]

• add def data bit
• add tests
• fix test

[rnk]

PTAL, here's how I imagine this is supposed to look, but the definition data bit name could probably be improved.

[rsmith]

clang/include/clang/AST/DeclCXX.h

959–963

"declared" in comment but "defined" in function name. Which is it?

I wonder if perhaps the right answer is neither: if we had separate Decls 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.)

clang/include/clang/Sema/Sema.h

5562–5565

Following the prior comment, naming this MarkCompleteDestructorUsed might make sense.

clang/lib/Sema/SemaExpr.cpp

16008–16013

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?

[rnk]

Thanks for the feedback, I'm going to investigate if we can use the used destructor bit to do this.

clang/include/clang/AST/DeclCXX.h

959–963

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.

clang/lib/Sema/SemaExpr.cpp

16008–16013

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.

[rsmith]

clang/lib/Sema/SemaExpr.cpp

16008–16013

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.

[rnk]

• Remove definition data bit tracking, use destructor isUsed bit

[rnk]

I'm glad to report that your suggestion worked out well!

[rnk]

• finish refactoring, build & test

[rnk]

ptal

[rnk]

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.

clang/include/clang/AST/CXXRecordDeclDefinitionBits.def

 /// Whether any declared copy constructor has a const-qualified
 /// parameter.
 FIELD(HasDeclaredCopyConstructorWithConstParam, 1, MERGE_OR)
 
 /// Whether any declared copy assignment operator has either a
 /// const-qualified reference parameter or a non-reference parameter.
 FIELD(HasDeclaredCopyAssignmentWithConstParam, 1, MERGE_OR)
 
+/// Indicates if the complete destructor has been implicitly declared
+/// yet. Only relevant in the Microsoft C++.
+FIELD(DefinedImplicitCompleteDestructor, 1, MERGE_OR)
+
 #undef FIELD

clang/include/clang/AST/DeclCXX.h

public:
   }
 
   /// Determine whether we need to eagerly declare a destructor for this
   /// class.
   bool needsOverloadResolutionForDestructor() const {
     return data().NeedOverloadResolutionForDestructor;
   }
 
+  /// Indicates if the complete destructor has been implicitly declared
+  /// yet. Only relevant in the Microsoft C++.
+  bool definedImplicitCompleteDestructor() const {
+    return data().DefinedImplicitCompleteDestructor;
+  }
+
+  void setDefinedImplicitDestructor() {
+    assert(!data().DefinedImplicitCompleteDestructor &&
+           "defining complete dtor more than once");
+    data().DefinedImplicitCompleteDestructor = true;
+  }
+
   /// Determine whether this class describes a lambda function object.
   bool isLambda() const {
     // An update record can't turn a non-lambda into a lambda.
     auto *DD = DefinitionData;
     return DD && DD->IsLambda;
   }
 
   /// Determine whether this class describes a generic

clang/include/clang/Sema/Sema.h

public:
   /// \returns The implicitly-declared destructor.
   CXXDestructorDecl *DeclareImplicitDestructor(CXXRecordDecl *ClassDecl);
 
   /// DefineImplicitDestructor - Checks for feasibility of
   /// defining this destructor as the default destructor.
   void DefineImplicitDestructor(SourceLocation CurrentLocation,
                                 CXXDestructorDecl *Destructor);
 
+  /// Define an implicit complete constructor for classes with vbases in the MS
+  /// ABI.
+  void DefineImplicitCompleteDestructor(SourceLocation CurrentLocation,
+                                        CXXDestructorDecl *Dtor);
+
   /// Build an exception spec for destructors that don't have one.
   ///
   /// C++11 says that user-defined destructors with no exception spec get one
   /// that looks as if the destructor was implicitly declared.
   void AdjustDestructorExceptionSpec(CXXDestructorDecl *Destructor);
 
   /// Define the specified inheriting constructor.
   void DefineInheritingConstructor(SourceLocation UseLoc,

clang/lib/AST/DeclCXX.cpp

: UserDeclaredConstructor(false), UserDeclaredSpecialMembers(0),
       HasConstexprDefaultConstructor(false),
       DefaultedDestructorIsConstexpr(true),
       HasNonLiteralTypeFieldsOrBases(false),
       UserProvidedDefaultConstructor(false), DeclaredSpecialMembers(0),
       ImplicitCopyConstructorCanHaveConstParamForVBase(true),
       ImplicitCopyConstructorCanHaveConstParamForNonVBase(true),
       ImplicitCopyAssignmentHasConstParam(true),
       HasDeclaredCopyConstructorWithConstParam(false),
-      HasDeclaredCopyAssignmentWithConstParam(false), IsLambda(false),
+      HasDeclaredCopyAssignmentWithConstParam(false),
+      DefinedImplicitCompleteDestructor(false), IsLambda(false),
       IsParsingBaseSpecifiers(false), ComputedVisibleConversions(false),
       HasODRHash(false), Definition(D) {}
 
 CXXBaseSpecifier *CXXRecordDecl::DefinitionData::getBasesSlowCase() const {
   return Bases.get(Definition->getASTContext().getExternalSource());
 }
 
 CXXBaseSpecifier *CXXRecordDecl::DefinitionData::getVBasesSlowCase() const {

clang/lib/Sema/SemaDeclCXX.cpp

void Sema::DefineImplicitDestructor(SourceLocation CurrentLocation,
   Destructor->setBody(new (Context) CompoundStmt(Loc));
   Destructor->markUsed(Context);
 
   if (ASTMutationListener *L = getASTMutationListener()) {
     L->CompletedImplicitDefinition(Destructor);
   }
 }
 
+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 &VBase : 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());
+  }
+
+  ClassDecl->setDefinedImplicitDestructor();
+}
+
 /// Perform any semantic analysis which needs to be delayed until all
 /// pending class member declarations have been parsed.
 void Sema::ActOnFinishCXXMemberDecls() {
   // If the context is an invalid C++ class, just suppress these checks.
   if (CXXRecordDecl *Record = dyn_cast<CXXRecordDecl>(CurContext)) {
     if (Record->isInvalidDecl()) {
       DelayedOverridingExceptionSpecChecks.clear();
       DelayedEquivalentExceptionSpecChecks.clear();

clang/lib/Sema/SemaExpr.cpp

runWithSufficientStackSpace(Loc, [&] {
             DefineImplicitMoveConstructor(Loc, Constructor);
           }
         } else if (Constructor->getInheritedConstructor()) {
           DefineInheritingConstructor(Loc, Constructor);
         }
       } else if (CXXDestructorDecl *Destructor =
                      dyn_cast<CXXDestructorDecl>(Func)) {
         Destructor = cast<CXXDestructorDecl>(Destructor->getFirstDecl());
-        if (Destructor->isDefaulted() && !Destructor->isDeleted()) {
-          if (Destructor->isTrivial() && !Destructor->hasAttr<DLLExportAttr>())
+        if (!Destructor->isDeleted()) {
+          if (Destructor->isDefaulted()) {
+            if (Destructor->isTrivial() &&
+                !Destructor->hasAttr<DLLExportAttr>())
               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.
+            CXXRecordDecl *Parent = Destructor->getParent();
+            if (Parent->getNumVBases() > 0 &&
+                !Parent->definedImplicitCompleteDestructor())
+              DefineImplicitCompleteDestructor(Loc, Destructor);
+          }
         }
         if (Destructor->isVirtual() && getLangOpts().AppleKext)
           MarkVTableUsed(Loc, Destructor->getParent());
       } else if (CXXMethodDecl *MethodDecl = dyn_cast<CXXMethodDecl>(Func)) {
         if (MethodDecl->isOverloadedOperator() &&
             MethodDecl->getOverloadedOperator() == OO_Equal) {
           MethodDecl = cast<CXXMethodDecl>(MethodDecl->getFirstDecl());
           if (MethodDecl->isDefaulted() && !MethodDecl->isDeleted()) {

clang/test/CXX/class.access/p4.cpp

-// 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:
 
 //   Access control is applied uniformly to all names, whether the
 //   names are referred to from declarations or expressions.  In the
 //   case of overloaded function names, access control is applied to
 //   the function selected by overload resolution.
 

clang/test/CodeGenCXX/microsoft-abi-vbase-dtor.cpp

+// 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

+// 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 <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}}
+}