r194288 - If a linkonce_odr dtor/ctor is identical to another one, just rauw.

[David Blaikie]

On Tue, Nov 12, 2013 at 10:36 AM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> On 12 November 2013 13:21, David Blaikie <dblaikie at gmail.com> wrote:
> > Talked with Chandler a bit (just for a pseudo-random (weighted by
> > convenience) C++ programmer perspective) about how problematic/surprising
> > this would be during debugging.
> >
> > Had a couple of thoughts:
> >
> > 1) In the case of a local "derived" object, we could possibly use
> inlining
> > debug info to describe the base dtor call to be an inlined instance of
> the
> > derived dtor. On further consideration I'm not sure this works generally
> > because the choice of aliasing/inlining could be made from another TU
> (does
> > this optimization fire for an out of line "= default" definition of a
> > special member?).
> >
> > Tested the out-of-line = default case, this optimization doesn't seem to
> > fire and I still see the derived dtor on the stack.
>
> What do you mean by "out-of-line = default"?
>

struct derived: base {
  ~derived();
};

derived::~derived() = default;


>
> > 2) In the case of virtual destruction, I assume this optimization doesn't
> > fire due to the derived dtor needing to do vtable changes? Maybe? On
> > experimentation this seems to be the case, though narrowly so.
> >
> > Given:
> > struct base { virtual ~base() { f(); } };
> > struct derived: base { };
> >
> > This:
> > derived d;
> > derived *p = new derived();
> > delete p;
> >
> > Shows the base dtor run without the derived dtor in the call stack, but
> > this:
> >
> > base *b = new derived();
> > delete b;
> >
> > does show the derived dtor in the call stack. So that seems OK.
>
> Yes, that is what should happen. The code has:
>
>  // If we need to manipulate a VTT parameter, give up.
>   if (Class->getNumVBases()) {
>

Yet when I wrote this:

struct base {
  virtual ~base() { func(); }
};

struct derived : base {
};

int main() {
  derived d;
}

the call stack at "func()" did not include a derived dtor - perhaps this is
an existing optimization not related to your change, then?


>
> >
> > So... what's the benefit of this optimization compared to letting the
> > backend inline the known-trivial dtors? Then we'd get the inlining debug
> > info for free in (1) (and the non-polymorphic cases of (2)) rather than
> > having to implement a special-case inlining in Clang. Does this fire in
> > other cases where LLVM cannot do this? Is there measurable overhead in
> > emitting trivial derived dtors that simply call their base dtor?
>
> It fires in cases we don't have the body of the destructor we will be
> calling. For example:
>
> class foo {
>   ~foo();
> };
>
> class bar : public foo {
> };
>
> LLVM also does'n implement replacing the pointers in the vtables.


But it doesn't look like your optimization was happening with vtables
either, at least not in my tests. When the destruction is actually virtual
(delete base_ptr_to_derived_obj) I see both base and derived dtors on the
stack.


> We
> probably could (and should) implement an optimization in llvm so that
> a trivial unnamed_addr function that just calls another is replaced
> with an alias or replaced with the callee.
>

Probably, but I'm not sure that's what I'm asking about.

I'm asking about the simple cases of known-derived object destruction
skipping the derived dtor (such as the example above with "derived d;" in
main). But as I said - maybe that's a different optimization that's not
related to your change?


>
> It is probably still a good idea to keep the special case in clang, at
> the very least until llvm can cover this cases. It is also not clear
> exactly where we should put such optimization in llvm. It should
> happen fairly early to avoid one being inlinned into the other. We
> also need some logic like this for the Base/Complete destructors where
> msvc only outputs one if they would be identical.
>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131112/722d1024/attachment.html>