This is an archive of the discontinued LLVM Phabricator instance.

MS ABI: Properly call global delete when invoking virtual destructors
ClosedPublic

Authored by majnemer on Oct 26 2014, 1:00 PM.

Details

Summary

The Itanium ABI approach of using offset-to-top isn't possible with the
MS ABI, it doesn't have that kind of information lying around.

Instead, we do the following:

  • Call the virtual deleting destructor with the "don't delete the object flag" set. The virtual deleting destructor will return a pointer to 'this' adjusted to the most derived class.
  • Call the global delete using the adjusted 'this' pointer.

Diff Detail

Repository
rL LLVM

Event Timeline

majnemer updated this revision to Diff 15469.Oct 26 2014, 1:00 PM
majnemer retitled this revision from to MS ABI: Properly call global delete when invoking virtual destructors.
majnemer updated this object.
majnemer added a reviewer: rnk.
majnemer added a subscriber: Unknown Object (MLST).
rnk edited edge metadata.Oct 27 2014, 6:23 PM

We also discussed how to deal with the ARM side brokenness when clang compiles this code:

struct A { virtual ~A(); };
struct B { virtual ~B(); };
struct C : A, B { virtual ~C(); };
C::~C() {}

The deleting destructor thunk for B lies about it's return value. The 'returned' attribute is also a lie.

lib/CodeGen/CGCXXABI.h
94–96 ↗(On Diff #15469)

Can we write a wrapper for these that takes the this type and returns the return type of void, void*, or decltype(this)?

lib/CodeGen/CGCall.cpp
241 ↗(On Diff #15469)

Does it make sense to check this for constructors, given that ctors can't really return the most derived types?

rnk accepted this revision.Oct 31 2014, 12:10 PM
rnk edited edge metadata.

lgtm

lib/CodeGen/CGCXXABI.h
94–96 ↗(On Diff #15469)

We discussed this in person, and it doesn't seem worth it.

This revision is now accepted and ready to land.Oct 31 2014, 12:10 PM
majnemer closed this revision.Oct 31 2014, 1:19 PM
majnemer updated this revision to Diff 15642.

Closed by commit rL220993 (authored by @majnemer).