Page MenuHomePhabricator

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

Authored by rnk on Mar 30 2020, 12:16 PM.

Details

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

Diff Detail

Event Timeline

rnk created this revision.Mar 30 2020, 12:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2020, 12:16 PM
rnk updated this revision to Diff 253715.Mar 30 2020, 3:17 PM
  • add def data bit
  • add tests
  • fix test
rnk edited the summary of this revision. (Show Details)Mar 30 2020, 3:17 PM
rnk added a reviewer: hans.

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

rsmith added inline comments.Mar 30 2020, 3:44 PM
clang/include/clang/AST/DeclCXX.h
959–963 ↗(On Diff #253715)

"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
5580–5583

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

clang/lib/Sema/SemaExpr.cpp
16386–16391

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 marked 2 inline comments as done.Mar 30 2020, 4:37 PM

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 ↗(On Diff #253715)

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
16386–16391

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 marked an inline comment as done.Mar 30 2020, 6:56 PM
rsmith added inline comments.
clang/lib/Sema/SemaExpr.cpp
16386–16391

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 updated this revision to Diff 253933.Mar 31 2020, 11:07 AM
  • Remove definition data bit tracking, use destructor isUsed bit
rnk edited the summary of this revision. (Show Details)Mar 31 2020, 11:07 AM

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

rnk updated this revision to Diff 253972.Mar 31 2020, 12:44 PM
  • finish refactoring, build & test
rnk added a comment.Apr 9 2020, 2:07 PM

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.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 9 2020, 2:25 PM
This revision was automatically updated to reflect the committed changes.