C++ PATCH for c++/58678 (devirt vs. KDE)

> Honza suggested that if the destructor for an abstract class can't
> ever be called through the vtable, the front end could avoid
> referring to it from the vtable.  This patch replaces such a
> destructor with __cxa_pure_virtual in the vtable.
> 
> Tested x86_64-pc-linux-gnu, applying to trunk.

Thank you!  would preffer different marker than cxa_pure_virtual in the vtable,
most probably simply NULL.

The reason is that __cxa_pure_virtual will appear as a possible target in the
list and it will prevent devirtualiztion to happen when we end up with
__cxa_pure_virtual and real destructor in the list of possible targets.
gimple_get_virt_method_for_vtable knows that lookup in vtable that do not
result in FUNCTION_DECL should be translated to BUILTIN_UNREACHABLE and
ipa-devirt drops these from list of targets, unlike __cxa_pure_virtual that
stays.

Other problem with cxa_pure_virtual is that it needs external relocation.
I sort of wonedered if we don't want to produce hidden comdat wrapper for
it, so C++ programs are easier to relocate.

I will still keep the patch to mark ABSTACT classes by BINFO flag and will
send out the patch I made to make ABSTRACT classes to be ignored for
anonymous namespace types.  It seems to make difference for libreoffice
that uses a lot of abstracts.

What do you think of the following patch that makes ipa-devirt to conclude
that destructor calls are never done on types in construction.
If effect of doing so is undefined, I think it is safe to drop them from
list of targets and that really helps to reduce lists down.

On 03/17/2014 04:39 AM, Jan Hubicka wrote:
> Thank you!  would preffer different marker than cxa_pure_virtual in the vtable,
> most probably simply NULL.
>
> The reason is that __cxa_pure_virtual will appear as a possible target in the
> list and it will prevent devirtualization to happen when we end up with
> __cxa_pure_virtual and real destructor in the list of possible targets.

Hmm?  __cxa_pure_virtual is not considered likely, so why wouldn't 
devirtualization choose the real function instead?

> gimple_get_virt_method_for_vtable knows that lookup in vtable that do not
> result in FUNCTION_DECL should be translated to BUILTIN_UNREACHABLE and
> ipa-devirt drops these from list of targets, unlike __cxa_pure_virtual that
> stays.

I don't see the reason for that distinction; either way you get 
undefined behavior.  The only purpose of __cxa_pure_virtual is to give a 
friendly diagnostic before terminating the program.

> Other problem with cxa_pure_virtual is that it needs external relocation.
> I sort of wondered if we don't want to produce hidden comdat wrapper for
> it, so C++ programs are easier to relocate.

Sure, that would make sense.

> What do you think of the following patch that makes ipa-devirt to conclude
> that destructor calls are never done on types in construction.
> If effect of doing so is undefined, I think it is safe to drop them from
> list of targets and that really helps to reduce lists down.

That looks good to me.

Jason

> On 03/17/2014 04:39 AM, Jan Hubicka wrote:
> >Thank you!  would preffer different marker than cxa_pure_virtual in the vtable,
> >most probably simply NULL.
> >
> >The reason is that __cxa_pure_virtual will appear as a possible target in the
> >list and it will prevent devirtualization to happen when we end up with
> >__cxa_pure_virtual and real destructor in the list of possible targets.
> 
> Hmm?  __cxa_pure_virtual is not considered likely, so why wouldn't
> devirtualization choose the real function instead?

If you get list like ~foo(), __cxa_pure_virtual, you will get speculative devirtualization
to ~foo.
If you get ~foo(), NULL, the NULL will get translated to BUILTIN_UNREACHABLE and
that will be dropped from the list, so you will end up with unconditional call of ~foo().

I think in general we can not skip cxa_pure_virtual, since people want friendly
diagnostics on broken programs insted of getting devirtualized call to random other
function. I was under impression in this case we know that the virtual table entry won't
be used, so full devirtualization would be possible.
> 
> >gimple_get_virt_method_for_vtable knows that lookup in vtable that do not
> >result in FUNCTION_DECL should be translated to BUILTIN_UNREACHABLE and
> >ipa-devirt drops these from list of targets, unlike __cxa_pure_virtual that
> >stays.
> 
> I don't see the reason for that distinction; either way you get
> undefined behavior.  The only purpose of __cxa_pure_virtual is to
> give a friendly diagnostic before terminating the program.

I can drop the handling of cxa_pure_virtual if unconditoinal devirtualization is
desirable, or perhaps do it under some switch.
Targets list containing one cxa_pure_virtual and one extra function are common.
> 
> >Other problem with cxa_pure_virtual is that it needs external relocation.
> >I sort of wondered if we don't want to produce hidden comdat wrapper for
> >it, so C++ programs are easier to relocate.
> 
> Sure, that would make sense.
> 
> >What do you think of the following patch that makes ipa-devirt to conclude
> >that destructor calls are never done on types in construction.
> >If effect of doing so is undefined, I think it is safe to drop them from
> >list of targets and that really helps to reduce lists down.
> 
> That looks good to me.

Thanks, I am away for next 4 days to allaska hut w/o electricity, will check my email afterwards,
> 
> Jason