RFA: PATCH to ipa-devirt for c++/58678

The failure with -flto is due to make_decl_local clearing DECL_COMDAT, 
so the check in ipa_devirt allows devirtualization to an implicitly 
declared destructor.  It's not clear to me why make_decl_local needs to 
clear DECL_COMDAT, but it's simpler to just remove that check from 
ipa_devirt.

Tested x86_64-pc-linux-gnu.  OK for trunk and 4.9?

> The failure with -flto is due to make_decl_local clearing
> DECL_COMDAT, so the check in ipa_devirt allows devirtualization to
> an implicitly declared destructor.  It's not clear to me why
> make_decl_local needs to clear DECL_COMDAT, but it's simpler to just
> remove that check from ipa_devirt.
> 
> Tested x86_64-pc-linux-gnu.  OK for trunk and 4.9?

> commit f88473ffec00cd8b537f7d92102f99fbd855b685
> Author: Jason Merrill <jason@redhat.com>
> Date:   Fri Aug 29 12:44:54 2014 -0400
> 
>     	PR c++/58678
>     	* ipa-devirt.c (ipa_devirt): Don't check DECL_COMDAT.
> 
> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> index f98a18e..948ae23 100644
> --- a/gcc/ipa-devirt.c
> +++ b/gcc/ipa-devirt.c
> @@ -3952,8 +3952,7 @@ ipa_devirt (void)
>  	    /* Don't use an implicitly-declared destructor (c++/58678).  */
>  	    struct cgraph_node *non_thunk_target
>  	      = likely_target->function_symbol ();
> -	    if (DECL_ARTIFICIAL (non_thunk_target->decl)
> -		&& DECL_COMDAT (non_thunk_target->decl))
> +	    if (DECL_ARTIFICIAL (non_thunk_target->decl))

If we know that we are going to output the destructor in current unit (i.e. it is not COMDAT or EXTERNAL
and it have definition), I think we ought to devirtualize to it.
So (DECL_COMDAT || DECL_EXTERNAL), but then it won't helpif the function was localized.

OK, I see:
_ZN1BD2Ev/3 (__base_dtor ) @0x7ffff756b000
  Type: function definition analyzed
  Visibility: externally_visible public weak comdat comdat_group:_ZN1BD5Ev one_only artificial
  Same comdat group as: _ZN1BD1Ev/4
  Address is taken.
  References: _ZTV1B/7 (addr)
  Referring: _ZN1BD1Ev/4 (alias)
  Read from file: t.o
  First run: 0
  Function flags:
  Called by:
  Calls: _ZN1AD2Ev/8 (0.00 per call) (can throw external)

so we have comdat destructor that we do not want to access.  Next we do:

_ZN1BD2Ev/3 (__base_dtor ) @0x7ffff756b000
  Type: function definition analyzed
  Visibility: prevailing_def_ironly artificial
  Address is taken.
  References: _ZTV1B/7 (addr)
  Referring: _ZN1BD1Ev/4 (alias)
  Read from file: t.o
  Availability: available
  First run: 0
  Function flags:
  Called by:
  Calls: _ZN1AD2Ev/8 (0.00 per call) (can throw external)

i.e. we bring the comdat local because we know we are only user of it in DSO.
From this point on it is a normal static function and therefore everyone is
welcome to refer to it.

Again - I think main problem is that we provide a middle end a function body
that it is not allowed to use.  We do not really have a concept of function
definitions that we may not use and worse yet, we are preventing that just in
one special case - i.e. in the speculative devirtualization. It is kind of semi
useful to have these because we can still analyze their side effects. Otherwise
i would say we should never get them out of FE.

I guess best approach would be avoid localizing those odd beasts, so add logic
into comdat_can_be_unshared_p_1 that will check for abstract destructors and
for those check if they do have some real references to them (other than from
another COMDAT/EXTERN symbols) and return false if they doesn't?

I wonder how many extra symbols we introduce this way, but for 4.9 it is
probably the way to go. For mainline we can play with localizing them later 
if it turns out to be important for bigger C++ libraries.
(localization brings quite nice reductions to dynamic linker tables that
is always iportant)

Honza
>  	      {
>  		if (dump_file)
>  		  fprintf (dump_file, "Target is artificial\n\n");
> diff --git a/gcc/testsuite/g++.dg/ipa/devirt-28a.C b/gcc/testsuite/g++.dg/ipa/devirt-28a.C
> new file mode 100644
> index 0000000..bdd1682
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ipa/devirt-28a.C
> @@ -0,0 +1,15 @@
> +// PR c++/58678
> +// { dg-options "-O3 -flto -shared -fPIC -Wl,--no-undefined" }
> +// { dg-do link { target { gld && fpic } } }
> +
> +struct A {
> +  virtual ~A();
> +};
> +struct B : A {
> +  virtual int m_fn1();
> +};
> +void fn1(B* b) {
> +  delete b;
> +}
> +
> +int main() {}

Hi
and forgot to add, I can implement the change if it seems to make sense to you ;)

Honza

On 09/10/2014 06:47 PM, Jan Hubicka wrote:
> Again - I think main problem is that we provide a middle end a function body
> that it is not allowed to use.  We do not really have a concept of function
> definitions that we may not use and worse yet, we are preventing that just in
> one special case - i.e. in the speculative devirtualization.

Because that special case is the only time we want to prevent it, as I 
argued in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58678#c19

My patch shouldn't change any behavior except for fixing this LTO bug, 
since implicitly-declared member functions always have vague linkage. 
It seems to me a simple, safe fix.

Jason

> On 09/10/2014 06:47 PM, Jan Hubicka wrote:
> >Again - I think main problem is that we provide a middle end a function body
> >that it is not allowed to use.  We do not really have a concept of function
> >definitions that we may not use and worse yet, we are preventing that just in
> >one special case - i.e. in the speculative devirtualization.
> 
> Because that special case is the only time we want to prevent it, as
> I argued in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58678#c19
> 
> My patch shouldn't change any behavior except for fixing this LTO
> bug, since implicitly-declared member functions always have vague
> linkage. It seems to me a simple, safe fix.

Your patch disables speuclative devirtualization to all implicitely produced
destructors.  At LTO, in typical case, we can safely devirtualize those. This
is because constructors and thus virutal tables are likely all part of the
given LTO translation unit (if it is close to whole program) and thus the body
of virtual destructor will be output anyway. That is why I think we can
just avoid privatizing of these when they are not used directly already.

But I guess especially for 4.9 branch you patch is safe and practical, so lets
go with it (both mainline and branch) and I will try to get some stats on
how many devirtualizations are missed this way.

Honza