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

[Reid Kleckner via Phabricator via cfe-commits]

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