[PATCH] Fix PR ipa/61190

[PATCH] Fix PR ipa/61190

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

Hi,

the test case g++.old-deja/g++.mike/p4736b.C is mis-compiled with with all
optimization levels, except -O0 and -Og. This probably started with gcc 4.7.x.

The constructor Main::Main() is first inlined and then completely optimized
away in the dce1 pass. But the thunk is still using the vtable, and therefore
crashes.

Well, the problem seems to be, that the thunk code is
not emitted in the normal way using gimple code,
but instead by the i386 back-end with a callback.

This makes the thunk invisible to the ipa-pure-const pass,
which assumes that all values just flow straight thu the thunk.

See ipa-pure-const.c (analyze_function):

  if (fn->thunk.thunk_p || fn->alias)
    {
      /* Thunk gets propagated through, so nothing interesting happens.  */
      gcc_assert (ipa);
      return l;
    }

But this is not true for a virtual base class:
The thunk itself references the vtable, so if nothing else might need
the vtable, the optimizer may remove the initalization of the vtable,
which happened in this example.

The most simple work around would be the attached patch, which
simply falls back to emitting gimple code, if virtual base classes
are used.

Boot-Strapped and Regression-Tested on x86_64-linux-gnu.
Ok for trunk?


Thanks
Bernd.
 		 	   		  

[-- Attachment #2: changelog-pr61190.txt --]
[-- Type: text/plain, Size: 335 bytes --]

2014-06-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR ipa/61190
	* cgraphunit.c (expand_thunk): Don't try to output the thunk for
	a virtual base class via targetm.asm_out.output_mi_thunk. 

testsuite/ChangeLog:
2014-06-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR ipa/61190
	* g++.old-deja/g++.mike/p4736b.C: Use -O2.


[-- Attachment #3: patch-pr61190.diff --]
[-- Type: application/octet-stream, Size: 793 bytes --]

Index: gcc/cgraphunit.c
===================================================================
--- gcc/cgraphunit.c	(revision 211088)
+++ gcc/cgraphunit.c	(working copy)
@@ -1481,7 +1481,7 @@ expand_thunk (struct cgraph_node *node, bool outpu
   tree a;
 
 
-  if (this_adjusting
+  if (this_adjusting && !node->thunk.virtual_offset_p
       && targetm.asm_out.can_output_mi_thunk (thunk_fndecl, fixed_offset,
 					      virtual_value, alias))
     {
Index: gcc/testsuite/g++.old-deja/g++.mike/p4736b.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.mike/p4736b.C	(revision 211088)
+++ gcc/testsuite/g++.old-deja/g++.mike/p4736b.C	(working copy)
@@ -1,4 +1,5 @@
 // { dg-do run  }
+// { dg-options "-O2" }
 // prms-id: 4736
 
 class Rep {

Re: [PATCH] Fix PR ipa/61190

[Richard Biener]

On Mon, Jun 2, 2014 at 11:00 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
> the test case g++.old-deja/g++.mike/p4736b.C is mis-compiled with with all
> optimization levels, except -O0 and -Og. This probably started with gcc 4.7.x.
>
> The constructor Main::Main() is first inlined and then completely optimized
> away in the dce1 pass. But the thunk is still using the vtable, and therefore
> crashes.
>
> Well, the problem seems to be, that the thunk code is
> not emitted in the normal way using gimple code,
> but instead by the i386 back-end with a callback.
>
> This makes the thunk invisible to the ipa-pure-const pass,
> which assumes that all values just flow straight thu the thunk.
>
> See ipa-pure-const.c (analyze_function):
>
>   if (fn->thunk.thunk_p || fn->alias)
>     {
>       /* Thunk gets propagated through, so nothing interesting happens.  */
>       gcc_assert (ipa);
>       return l;
>     }
>
> But this is not true for a virtual base class:
> The thunk itself references the vtable, so if nothing else might need
> the vtable, the optimizer may remove the initalization of the vtable,
> which happened in this example.
>
> The most simple work around would be the attached patch, which
> simply falls back to emitting gimple code, if virtual base classes
> are used.
>
> Boot-Strapped and Regression-Tested on x86_64-linux-gnu.
> Ok for trunk?

Honza should know better.  I'd rather skip the above for
thunks going the output_mi_thunk way.

That is, the target hook docs should be updated to reflect which
kind of thunks it is supposed to handle.

Richard.

>
> Thanks
> Bernd.
>

RE: [PATCH] Fix PR ipa/61190

[Bernd Edlinger]

Hi,

On Mon, 2 Jun 2014 12:06:12, Richard Biener wrote:
>
> On Mon, Jun 2, 2014 at 11:00 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hi,
>>
>> the test case g++.old-deja/g++.mike/p4736b.C is mis-compiled with with all
>> optimization levels, except -O0 and -Og. This probably started with gcc 4.7.x.
>>
>> The constructor Main::Main() is first inlined and then completely optimized
>> away in the dce1 pass. But the thunk is still using the vtable, and therefore
>> crashes.
>>
>> Well, the problem seems to be, that the thunk code is
>> not emitted in the normal way using gimple code,
>> but instead by the i386 back-end with a callback.
>>
>> This makes the thunk invisible to the ipa-pure-const pass,
>> which assumes that all values just flow straight thu the thunk.
>>
>> See ipa-pure-const.c (analyze_function):
>>
>> if (fn->thunk.thunk_p || fn->alias)
>> {
>> /* Thunk gets propagated through, so nothing interesting happens. */
>> gcc_assert (ipa);
>> return l;
>> }
>>
>> But this is not true for a virtual base class:
>> The thunk itself references the vtable, so if nothing else might need
>> the vtable, the optimizer may remove the initalization of the vtable,
>> which happened in this example.
>>
>> The most simple work around would be the attached patch, which
>> simply falls back to emitting gimple code, if virtual base classes
>> are used.
>>
>> Boot-Strapped and Regression-Tested on x86_64-linux-gnu.
>> Ok for trunk?
>
> Honza should know better. I'd rather skip the above for
> thunks going the output_mi_thunk way.
>

Ahh Yes, that was of course my first attempt...

But there is not BB to enumerate in that case.
And the loop below just crashed in:

  FOR_EACH_BB_FN (this_block, cfun)
 

There is also another interesting thing to mention: when the
output_mi_thunk outputs the virtual thunk, there is no inlining at all.

But with this patch, the thunk inlines the called function,
and therefore the generated code looks rather nifty, compared to
the state before the patch.


Thanks
Bernd.

> That is, the target hook docs should be updated to reflect which
> kind of thunks it is supposed to handle.
>
> Richard.
>
>>
>> Thanks
>> Bernd.
>>
 		 	   		  

Re: [PATCH] Fix PR ipa/61190

[Jan Hubicka]

> Hi,
> 
> On Mon, 2 Jun 2014 12:06:12, Richard Biener wrote:
> >
> > On Mon, Jun 2, 2014 at 11:00 AM, Bernd Edlinger
> > <bernd.edlinger@hotmail.de> wrote:
> >> Hi,
> >>
> >> the test case g++.old-deja/g++.mike/p4736b.C is mis-compiled with with all
> >> optimization levels, except -O0 and -Og. This probably started with gcc 4.7.x.
> >>
> >> The constructor Main::Main() is first inlined and then completely optimized
> >> away in the dce1 pass. But the thunk is still using the vtable, and therefore
> >> crashes.

Why the ctor is eliminated? Is it because ipa-pure-const identifies the thunk as const?
I think we need to update it to notice the read of vtable in those thunks.   I will
take a look.

Honza
> >>
> >> Well, the problem seems to be, that the thunk code is
> >> not emitted in the normal way using gimple code,
> >> but instead by the i386 back-end with a callback.
> >>
> >> This makes the thunk invisible to the ipa-pure-const pass,
> >> which assumes that all values just flow straight thu the thunk.
> >>
> >> See ipa-pure-const.c (analyze_function):
> >>
> >> if (fn->thunk.thunk_p || fn->alias)
> >> {
> >> /* Thunk gets propagated through, so nothing interesting happens. */
> >> gcc_assert (ipa);
> >> return l;
> >> }
> >>
> >> But this is not true for a virtual base class:
> >> The thunk itself references the vtable, so if nothing else might need
> >> the vtable, the optimizer may remove the initalization of the vtable,
> >> which happened in this example.
> >>
> >> The most simple work around would be the attached patch, which
> >> simply falls back to emitting gimple code, if virtual base classes
> >> are used.
> >>
> >> Boot-Strapped and Regression-Tested on x86_64-linux-gnu.
> >> Ok for trunk?
> >
> > Honza should know better. I'd rather skip the above for
> > thunks going the output_mi_thunk way.
> >
> 
> Ahh Yes, that was of course my first attempt...
> 
> But there is not BB to enumerate in that case.
> And the loop below just crashed in:
> 
>   FOR_EACH_BB_FN (this_block, cfun)
>  
> 
> There is also another interesting thing to mention: when the
> output_mi_thunk outputs the virtual thunk, there is no inlining at all.
> 
> But with this patch, the thunk inlines the called function,
> and therefore the generated code looks rather nifty, compared to
> the state before the patch.
> 
> 
> Thanks
> Bernd.
> 
> > That is, the target hook docs should be updated to reflect which
> > kind of thunks it is supposed to handle.
> >
> > Richard.
> >
> >>
> >> Thanks
> >> Bernd.
> >>
>  		 	   		  

RE: [PATCH] Fix PR ipa/61190

[Bernd Edlinger]

Hi Honza,

On Mon, 2 Jun 2014 18:12:10, Jan Hubicka wrote:
>
>> Hi,
>>
>> On Mon, 2 Jun 2014 12:06:12, Richard Biener wrote:
>>>
>>> On Mon, Jun 2, 2014 at 11:00 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> Hi,
>>>>
>>>> the test case g++.old-deja/g++.mike/p4736b.C is mis-compiled with with all
>>>> optimization levels, except -O0 and -Og. This probably started with gcc 4.7.x.
>>>>
>>>> The constructor Main::Main() is first inlined and then completely optimized
>>>> away in the dce1 pass. But the thunk is still using the vtable, and therefore
>>>> crashes.
>
> Why the ctor is eliminated? Is it because ipa-pure-const identifies the thunk as const?
> I think we need to update it to notice the read of vtable in those thunks. I will
> take a look.
>

have you been able to find an alternative solution yet?


Thanks
Bernd.

> Honza
>>>>
>>>> Well, the problem seems to be, that the thunk code is
>>>> not emitted in the normal way using gimple code,
>>>> but instead by the i386 back-end with a callback.
>>>>
>>>> This makes the thunk invisible to the ipa-pure-const pass,
>>>> which assumes that all values just flow straight thu the thunk.
>>>>
>>>> See ipa-pure-const.c (analyze_function):
>>>>
>>>> if (fn->thunk.thunk_p || fn->alias)
>>>> {
>>>> /* Thunk gets propagated through, so nothing interesting happens. */
>>>> gcc_assert (ipa);
>>>> return l;
>>>> }
>>>>
>>>> But this is not true for a virtual base class:
>>>> The thunk itself references the vtable, so if nothing else might need
>>>> the vtable, the optimizer may remove the initalization of the vtable,
>>>> which happened in this example.
>>>>
>>>> The most simple work around would be the attached patch, which
>>>> simply falls back to emitting gimple code, if virtual base classes
>>>> are used.
>>>>
>>>> Boot-Strapped and Regression-Tested on x86_64-linux-gnu.
>>>> Ok for trunk?
>>>
>>> Honza should know better. I'd rather skip the above for
>>> thunks going the output_mi_thunk way.
>>>
>>
>> Ahh Yes, that was of course my first attempt...
>>
>> But there is not BB to enumerate in that case.
>> And the loop below just crashed in:
>>
>>   FOR_EACH_BB_FN (this_block, cfun)
>>
>>
>> There is also another interesting thing to mention: when the
>> output_mi_thunk outputs the virtual thunk, there is no inlining at all.
>>
>> But with this patch, the thunk inlines the called function,
>> and therefore the generated code looks rather nifty, compared to
>> the state before the patch.
>>
>>
>> Thanks
>> Bernd.
>>
>>> That is, the target hook docs should be updated to reflect which
>>> kind of thunks it is supposed to handle.
>>>
>>> Richard.
>>>
>>>>
>>>> Thanks
>>>> Bernd.
>>>>
>>