diff mbox

RFA: ipa-devirt PATCH for c++/58678 (devirt causes KDE build failure)

Message ID 5310FE73.6060700@redhat.com
State New
Headers show

Commit Message

Jason Merrill Feb. 28, 2014, 9:24 p.m. UTC 
On 02/28/2014 03:56 PM, Jan Hubicka wrote:
> I think we can safely test here DECL_ARTIFICIAL && (DECL_EXTERNAL ||
> DECL_COMDAT).  If the dtor is going to be output anyway, we are safe to use it.

We already skipped DECL_EXTERNAL decls, and artificial members are 
always DECL_COMDAT, but I'll add the COMDAT check.

> Are those programs valid by C++ standard? (I believe it is not valid to include
> stuff whose implementation you do not link with.).

Symbol visibility is outside the scope of the standard.

> If we just want to avoid
> breaking python and libreoffice (I fixed libreoffice part however), we may just
> go with the ipa-devirt change as you propose (with external&comdat check).
>
> If this is an correctness issue, I think we want to be safe that other optimizations
> won't do the same. In that case your check seems misplaced.
>
> If DECL_ARTIFICIAL destructors are not safe to inline, I would add it into
> function_attribute_inlinable_p.  If the dtor is not safe to refer, then I would
> add it into can_refer_decl_in_current_unit_p

> Both such changes would however inhibit quite some optimization, since
> artificial destructors are quite common case, right? Or is there some reason why
> only speculative devirtualization count possibly work out reference to these?

Normally, it's fine to inline destructors, and refer to them.  The 
problem comes when we turn what had been a virtual call (which goes 
through the vtable that is hidden in the DSO) into a direct call to a 
hidden function.  We don't do that for user-defined virtual functions 
because the user controls whether or not they are defined in the header, 
and we don't devirtualize if no definition is available, but 
implicitly-declared functions are different because the user has no way 
to prevent the definition from being available.

This also isn't a problem for cprop devirtualization, because in that 
situation we must have already referred to the vtable.

Jason

Comments

Jason Merrill March 1, 2014, 1:56 a.m. UTC | #1 
I went ahead and checked in my patch so that the regression is fixed 
over the weekend.

Jason
Jan Hubicka March 1, 2014, 8:52 a.m. UTC | #2 
> On 02/28/2014 03:56 PM, Jan Hubicka wrote:
> >I think we can safely test here DECL_ARTIFICIAL && (DECL_EXTERNAL ||
> >DECL_COMDAT).  If the dtor is going to be output anyway, we are safe to use it.
> 
> We already skipped DECL_EXTERNAL decls, and artificial members are
> always DECL_COMDAT, but I'll add the COMDAT check.
> 
> >Are those programs valid by C++ standard? (I believe it is not valid to include
> >stuff whose implementation you do not link with.).
> 
> Symbol visibility is outside the scope of the standard.
> 
> >If we just want to avoid
> >breaking python and libreoffice (I fixed libreoffice part however), we may just
> >go with the ipa-devirt change as you propose (with external&comdat check).
> >
> >If this is an correctness issue, I think we want to be safe that other optimizations
> >won't do the same. In that case your check seems misplaced.
> >
> >If DECL_ARTIFICIAL destructors are not safe to inline, I would add it into
> >function_attribute_inlinable_p.  If the dtor is not safe to refer, then I would
> >add it into can_refer_decl_in_current_unit_p
> 
> >Both such changes would however inhibit quite some optimization, since
> >artificial destructors are quite common case, right? Or is there some reason why
> >only speculative devirtualization count possibly work out reference to these?
> 
> Normally, it's fine to inline destructors, and refer to them.  The
> problem comes when we turn what had been a virtual call (which goes
> through the vtable that is hidden in the DSO) into a direct call to
> a hidden function.  We don't do that for user-defined virtual
> functions because the user controls whether or not they are defined
> in the header, and we don't devirtualize if no definition is
> available, but implicitly-declared functions are different because
> the user has no way to prevent the definition from being available.
> 
> This also isn't a problem for cprop devirtualization, because in
> that situation we must have already referred to the vtable.

Is this always the case? For normal virtual method you can have something
like:

foo(class bla a)
{
  a->bar();
}

Here we will devirutalize bar into bla's method without ever seeing the vtable.
I am not sure if I can construct a testcase for dtor w/o referencing the
vtable, since generally those are cases where dtor is generated autmatically.
But it is a bit fragile assumption to be made.

Jan
> 
> Jason
> 

> commit 2a05a09c268ce3abb373aa86cf731d20aac8dd7a
> Author: Jason Merrill <jason@redhat.com>
> Date:   Fri Feb 28 14:03:19 2014 -0500
> 
>     	PR c++/58678
>     	* ipa-devirt.c (ipa_devirt): Don't choose an implicitly-declared
>     	function.
> 
> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> index 21649cb..2f84f17 100644
> --- a/gcc/ipa-devirt.c
> +++ b/gcc/ipa-devirt.c
> @@ -1710,7 +1710,7 @@ ipa_devirt (void)
>  
>    int npolymorphic = 0, nspeculated = 0, nconverted = 0, ncold = 0;
>    int nmultiple = 0, noverwritable = 0, ndevirtualized = 0, nnotdefined = 0;
> -  int nwrong = 0, nok = 0, nexternal = 0;;
> +  int nwrong = 0, nok = 0, nexternal = 0, nartificial = 0;
>  
>    FOR_EACH_DEFINED_FUNCTION (n)
>      {	
> @@ -1820,6 +1820,17 @@ ipa_devirt (void)
>  		nexternal++;
>  		continue;
>  	      }
> +	    /* Don't use an implicitly-declared destructor (c++/58678).  */
> +	    struct cgraph_node *non_thunk_target
> +	      = cgraph_function_node (likely_target);
> +	    if (DECL_ARTIFICIAL (non_thunk_target->decl)
> +		&& DECL_COMDAT (non_thunk_target->decl))
> +	      {
> +		if (dump_file)
> +		  fprintf (dump_file, "Target is artificial\n\n");
> +		nartificial++;
> +		continue;
> +	      }
>  	    if (cgraph_function_body_availability (likely_target)
>  		<= AVAIL_OVERWRITABLE
>  		&& symtab_can_be_discarded (likely_target))
> @@ -1862,10 +1873,10 @@ ipa_devirt (void)
>  	     " %i speculatively devirtualized, %i cold\n"
>  	     "%i have multiple targets, %i overwritable,"
>  	     " %i already speculated (%i agree, %i disagree),"
> -	     " %i external, %i not defined\n",
> +	     " %i external, %i not defined, %i artificial\n",
>  	     npolymorphic, ndevirtualized, nconverted, ncold,
>  	     nmultiple, noverwritable, nspeculated, nok, nwrong,
> -	     nexternal, nnotdefined);
> +	     nexternal, nnotdefined, nartificial);
>    return ndevirtualized ? TODO_remove_functions : 0;
>  }
>  
> diff --git a/gcc/testsuite/g++.dg/ipa/devirt-28.C b/gcc/testsuite/g++.dg/ipa/devirt-28.C
> new file mode 100644
> index 0000000..e18b818
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ipa/devirt-28.C
> @@ -0,0 +1,17 @@
> +// PR c++/58678
> +// { dg-options "-O3 -fdump-ipa-devirt" }
> +
> +struct A {
> +  virtual ~A();
> +};
> +struct B : A {
> +  virtual int m_fn1();
> +};
> +void fn1(B* b) {
> +  delete b;
> +}
> +
> +// { dg-final { scan-assembler-not "_ZN1AD2Ev" } }
> +// { dg-final { scan-assembler-not "_ZN1BD0Ev" } }
> +// { dg-final { scan-ipa-dump "Target is artificial" "devirt" } }
> +// { dg-final { cleanup-ipa-dump "devirt" } }
Jason Merrill March 1, 2014, 2:13 p.m. UTC | #3 
On 03/01/2014 03:52 AM, Jan Hubicka wrote:
>> a hidden function.  We don't do that for user-defined virtual
>> functions because the user controls whether or not they are defined
>> in the header, and we don't devirtualize if no definition is
>> available, but implicitly-declared functions are different because
>> the user has no way to prevent the definition from being available.
>>
>> This also isn't a problem for cprop devirtualization, because in
>> that situation we must have already referred to the vtable.
>
> Is this always the case? For normal virtual method you can have something
> like:
>
> foo(class bla a)
> {
>    a->bar();
> }
>
> Here we will devirutalize bar into bla's method without ever seeing the vtable.
> I am not sure if I can construct a testcase for dtor w/o referencing the
> vtable, since generally those are cases where dtor is generated autmatically.
> But it is a bit fragile assumption to be made.

For pass-by-value we need to be able to call a constructor, which needs 
to be able to reference the vtable, so I think it should be OK 
typically.  Sure, people can do all kinds of weird and broken things 
with visibility; I'm just trying to make what seems to be a common and 
understandable pattern work.

Jason
Jan Hubicka March 1, 2014, 6:09 p.m. UTC | #4 
> On 03/01/2014 03:52 AM, Jan Hubicka wrote:
> >>a hidden function.  We don't do that for user-defined virtual
> >>functions because the user controls whether or not they are defined
> >>in the header, and we don't devirtualize if no definition is
> >>available, but implicitly-declared functions are different because
> >>the user has no way to prevent the definition from being available.
> >>
> >>This also isn't a problem for cprop devirtualization, because in
> >>that situation we must have already referred to the vtable.
> >
> >Is this always the case? For normal virtual method you can have something
> >like:
> >
> >foo(class bla a)
> >{
> >   a->bar();
> >}
> >
> >Here we will devirutalize bar into bla's method without ever seeing the vtable.
> >I am not sure if I can construct a testcase for dtor w/o referencing the
> >vtable, since generally those are cases where dtor is generated autmatically.
> >But it is a bit fragile assumption to be made.
> 
> For pass-by-value we need to be able to call a constructor, which

The call may live in other DSO, but I agree it is weird.

> needs to be able to reference the vtable, so I think it should be OK
> typically.  Sure, people can do all kinds of weird and broken things
> with visibility; I'm just trying to make what seems to be a common
> and understandable pattern work.

Yes, this seems resonable.  I guess I will need to find some central place
where to document those little implementation decisions (there are some
in can_refer_in_current_unit_p, some in visibility logic and some in the
new devirt logic)

Honza
> 
> Jason
diff mbox

Patch

commit 2a05a09c268ce3abb373aa86cf731d20aac8dd7a
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Feb 28 14:03:19 2014 -0500

    	PR c++/58678
    	* ipa-devirt.c (ipa_devirt): Don't choose an implicitly-declared
    	function.

diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
index 21649cb..2f84f17 100644
--- a/gcc/ipa-devirt.c
+++ b/gcc/ipa-devirt.c
@@ -1710,7 +1710,7 @@  ipa_devirt (void)
 
   int npolymorphic = 0, nspeculated = 0, nconverted = 0, ncold = 0;
   int nmultiple = 0, noverwritable = 0, ndevirtualized = 0, nnotdefined = 0;
-  int nwrong = 0, nok = 0, nexternal = 0;;
+  int nwrong = 0, nok = 0, nexternal = 0, nartificial = 0;
 
   FOR_EACH_DEFINED_FUNCTION (n)
     {	
@@ -1820,6 +1820,17 @@  ipa_devirt (void)
 		nexternal++;
 		continue;
 	      }
+	    /* Don't use an implicitly-declared destructor (c++/58678).  */
+	    struct cgraph_node *non_thunk_target
+	      = cgraph_function_node (likely_target);
+	    if (DECL_ARTIFICIAL (non_thunk_target->decl)
+		&& DECL_COMDAT (non_thunk_target->decl))
+	      {
+		if (dump_file)
+		  fprintf (dump_file, "Target is artificial\n\n");
+		nartificial++;
+		continue;
+	      }
 	    if (cgraph_function_body_availability (likely_target)
 		<= AVAIL_OVERWRITABLE
 		&& symtab_can_be_discarded (likely_target))
@@ -1862,10 +1873,10 @@  ipa_devirt (void)
 	     " %i speculatively devirtualized, %i cold\n"
 	     "%i have multiple targets, %i overwritable,"
 	     " %i already speculated (%i agree, %i disagree),"
-	     " %i external, %i not defined\n",
+	     " %i external, %i not defined, %i artificial\n",
 	     npolymorphic, ndevirtualized, nconverted, ncold,
 	     nmultiple, noverwritable, nspeculated, nok, nwrong,
-	     nexternal, nnotdefined);
+	     nexternal, nnotdefined, nartificial);
   return ndevirtualized ? TODO_remove_functions : 0;
 }
 
diff --git a/gcc/testsuite/g++.dg/ipa/devirt-28.C b/gcc/testsuite/g++.dg/ipa/devirt-28.C
new file mode 100644
index 0000000..e18b818
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/devirt-28.C
@@ -0,0 +1,17 @@ 
+// PR c++/58678
+// { dg-options "-O3 -fdump-ipa-devirt" }
+
+struct A {
+  virtual ~A();
+};
+struct B : A {
+  virtual int m_fn1();
+};
+void fn1(B* b) {
+  delete b;
+}
+
+// { dg-final { scan-assembler-not "_ZN1AD2Ev" } }
+// { dg-final { scan-assembler-not "_ZN1BD0Ev" } }
+// { dg-final { scan-ipa-dump "Target is artificial" "devirt" } }
+// { dg-final { cleanup-ipa-dump "devirt" } }