[CHKP] Never expand instrumentation thunks

[CHKP] Never expand instrumentation thunks

[Ilya Enkovich]

Hi,

This patch disables attempts to expand instrumentation thunks which appear when we create specialized function versions.  Problem was found during SPEC2006 insatrumentation with '-Ofast -flto'.  Unfortunately I couldn't make a small reproducer.  Bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?

Thanks,
Ilya
--
2015-03-18  Ilya Enkovich  <ilya.enkovich@intel.com>

	* cgraphunit.c (cgraph_node::expand_thunk): Don't expand
	instrumentation thunks.


diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index e640907..abc9cfe 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1508,6 +1508,10 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
   tree thunk_fndecl = decl;
   tree a;
 
+  /* Instrumentation thunk is the same function with
+     a different signature.  Never need to expand it.  */
+  if (thunk.add_pointer_bounds_args)
+    return false;
 
   if (!force_gimple_thunk && this_adjusting
       && targetm.asm_out.can_output_mi_thunk (thunk_fndecl, fixed_offset,

Re: [CHKP] Never expand instrumentation thunks

[Ilya Enkovich]

Ping. Patch doesn't affect not instrumented code.

Thanks,
Ilya

2015-03-18 15:19 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
> Hi,
>
> This patch disables attempts to expand instrumentation thunks which appear when we create specialized function versions.  Problem was found during SPEC2006 insatrumentation with '-Ofast -flto'.  Unfortunately I couldn't make a small reproducer.  Bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?
>
> Thanks,
> Ilya
> --
> 2015-03-18  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         * cgraphunit.c (cgraph_node::expand_thunk): Don't expand
>         instrumentation thunks.
>
>
> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> index e640907..abc9cfe 100644
> --- a/gcc/cgraphunit.c
> +++ b/gcc/cgraphunit.c
> @@ -1508,6 +1508,10 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
>    tree thunk_fndecl = decl;
>    tree a;
>
> +  /* Instrumentation thunk is the same function with
> +     a different signature.  Never need to expand it.  */
> +  if (thunk.add_pointer_bounds_args)
> +    return false;
>
>    if (!force_gimple_thunk && this_adjusting
>        && targetm.asm_out.can_output_mi_thunk (thunk_fndecl, fixed_offset,

Re: [CHKP] Never expand instrumentation thunks

[Jeff Law]

On 04/02/2015 08:51 AM, Ilya Enkovich wrote:
> Ping. Patch doesn't affect not instrumented code.
>
> Thanks,
> Ilya
>
> 2015-03-18 15:19 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>> Hi,
>>
>> This patch disables attempts to expand instrumentation thunks which appear when we create specialized function versions.  Problem was found during SPEC2006 insatrumentation with '-Ofast -flto'.  Unfortunately I couldn't make a small reproducer.  Bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?
>>
>> Thanks,
>> Ilya
>> --
>> 2015-03-18  Ilya Enkovich  <ilya.enkovich@intel.com>
>>
>>          * cgraphunit.c (cgraph_node::expand_thunk): Don't expand
>>          instrumentation thunks.
So is it the case that these instrumentation thunks have a gimple body 
and thus will be expanded elsewhere?  Or are you trying to solve some 
different problem?

Jeff

Re: [CHKP] Never expand instrumentation thunks

[Jan Hubicka]

> > 2015-03-18  Ilya Enkovich  <ilya.enkovich@intel.com>
> >
> >         * cgraphunit.c (cgraph_node::expand_thunk): Don't expand
> >         instrumentation thunks.
> >
> >
> > diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> > index e640907..abc9cfe 100644
> > --- a/gcc/cgraphunit.c
> > +++ b/gcc/cgraphunit.c
> > @@ -1508,6 +1508,10 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
> >    tree thunk_fndecl = decl;
> >    tree a;
> >
> > +  /* Instrumentation thunk is the same function with
> > +     a different signature.  Never need to expand it.  */
> > +  if (thunk.add_pointer_bounds_args)
> > +    return false;

Yeah, this is another case where we hit problem with transparent alias pretending
to be thunk :)
I guess the patch is OK for GCC-5 and for next stage1 we can clean this up.

Honza
> >
> >    if (!force_gimple_thunk && this_adjusting
> >        && targetm.asm_out.can_output_mi_thunk (thunk_fndecl, fixed_offset,

Re: [CHKP] Never expand instrumentation thunks

[Jeff Law]

On 04/02/2015 02:11 PM, Jan Hubicka wrote:
>>> 2015-03-18  Ilya Enkovich  <ilya.enkovich@intel.com>
>>>
>>>          * cgraphunit.c (cgraph_node::expand_thunk): Don't expand
>>>          instrumentation thunks.
>>>
>>>
>>> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
>>> index e640907..abc9cfe 100644
>>> --- a/gcc/cgraphunit.c
>>> +++ b/gcc/cgraphunit.c
>>> @@ -1508,6 +1508,10 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
>>>     tree thunk_fndecl = decl;
>>>     tree a;
>>>
>>> +  /* Instrumentation thunk is the same function with
>>> +     a different signature.  Never need to expand it.  */
>>> +  if (thunk.add_pointer_bounds_args)
>>> +    return false;
>
> Yeah, this is another case where we hit problem with transparent alias pretending
> to be thunk :)
> I guess the patch is OK for GCC-5 and for next stage1 we can clean this up.
I was actually building a compiler so I could take a look at this one 
under a debugger ;-)

jeff

Re: [CHKP] Never expand instrumentation thunks

[Jan Hubicka]

> On 04/02/2015 02:11 PM, Jan Hubicka wrote:
> >>>2015-03-18  Ilya Enkovich  <ilya.enkovich@intel.com>
> >>>
> >>>         * cgraphunit.c (cgraph_node::expand_thunk): Don't expand
> >>>         instrumentation thunks.
> >>>
> >>>
> >>>diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> >>>index e640907..abc9cfe 100644
> >>>--- a/gcc/cgraphunit.c
> >>>+++ b/gcc/cgraphunit.c
> >>>@@ -1508,6 +1508,10 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
> >>>    tree thunk_fndecl = decl;
> >>>    tree a;
> >>>
> >>>+  /* Instrumentation thunk is the same function with
> >>>+     a different signature.  Never need to expand it.  */
> >>>+  if (thunk.add_pointer_bounds_args)
> >>>+    return false;
> >
> >Yeah, this is another case where we hit problem with transparent alias pretending
> >to be thunk :)
> >I guess the patch is OK for GCC-5 and for next stage1 we can clean this up.
> I was actually building a compiler so I could take a look at this
> one under a debugger ;-)

I think it is really the transparent alias issue.  The comment seems pretty clear about it.
What is confusing is that instrumentation thunks are called thunks when they are
really not - thunk is a small hunk of code, while instrumentation thunk is a transparent
alias.

Too bad I did not notice we introduced transparent aliases, i would push out my
code for that. I will compare Ilya's changes with mine and hopefully we can
catch more bugs and unify the code next stage1.

Honza

Re: [CHKP] Never expand instrumentation thunks

[Jeff Law]

On 04/02/2015 03:13 PM, Jan Hubicka wrote:
>
> I think it is really the transparent alias issue.  The comment seems pretty clear about it.
> What is confusing is that instrumentation thunks are called thunks when they are
> really not - thunk is a small hunk of code, while instrumentation thunk is a transparent
> alias.
And it may be the terminology gets in the way.  I was at least partly 
interested in what code we generated for that testcase.

>
> Too bad I did not notice we introduced transparent aliases, i would push out my
> code for that. I will compare Ilya's changes with mine and hopefully we can
> catch more bugs and unify the code next stage1.
Sounds good.
jeff

Re: [CHKP] Never expand instrumentation thunks

[Ilya Enkovich]

2015-04-03 0:13 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>:
>> On 04/02/2015 02:11 PM, Jan Hubicka wrote:
>> >>>2015-03-18  Ilya Enkovich  <ilya.enkovich@intel.com>
>> >>>
>> >>>         * cgraphunit.c (cgraph_node::expand_thunk): Don't expand
>> >>>         instrumentation thunks.
>> >>>
>> >>>
>> >>>diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
>> >>>index e640907..abc9cfe 100644
>> >>>--- a/gcc/cgraphunit.c
>> >>>+++ b/gcc/cgraphunit.c
>> >>>@@ -1508,6 +1508,10 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
>> >>>    tree thunk_fndecl = decl;
>> >>>    tree a;
>> >>>
>> >>>+  /* Instrumentation thunk is the same function with
>> >>>+     a different signature.  Never need to expand it.  */
>> >>>+  if (thunk.add_pointer_bounds_args)
>> >>>+    return false;
>> >
>> >Yeah, this is another case where we hit problem with transparent alias pretending
>> >to be thunk :)
>> >I guess the patch is OK for GCC-5 and for next stage1 we can clean this up.
>> I was actually building a compiler so I could take a look at this
>> one under a debugger ;-)
>
> I think it is really the transparent alias issue.  The comment seems pretty clear about it.
> What is confusing is that instrumentation thunks are called thunks when they are
> really not - thunk is a small hunk of code, while instrumentation thunk is a transparent
> alias.
>
> Too bad I did not notice we introduced transparent aliases, i would push out my
> code for that. I will compare Ilya's changes with mine and hopefully we can
> catch more bugs and unify the code next stage1.

Thunk was the best I could find to represent the same function but
with different signature. It would be great to have a more specialized
way for that.

Thanks,
Ilya

>
> Honza

Re: [CHKP] Never expand instrumentation thunks

[Jan Hubicka]

> 2015-04-03 0:13 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>:
> >> On 04/02/2015 02:11 PM, Jan Hubicka wrote:
> >> >>>2015-03-18  Ilya Enkovich  <ilya.enkovich@intel.com>
> >> >>>
> >> >>>         * cgraphunit.c (cgraph_node::expand_thunk): Don't expand
> >> >>>         instrumentation thunks.
> >> >>>
> >> >>>
> >> >>>diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> >> >>>index e640907..abc9cfe 100644
> >> >>>--- a/gcc/cgraphunit.c
> >> >>>+++ b/gcc/cgraphunit.c
> >> >>>@@ -1508,6 +1508,10 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
> >> >>>    tree thunk_fndecl = decl;
> >> >>>    tree a;
> >> >>>
> >> >>>+  /* Instrumentation thunk is the same function with
> >> >>>+     a different signature.  Never need to expand it.  */
> >> >>>+  if (thunk.add_pointer_bounds_args)
> >> >>>+    return false;
> >> >
> >> >Yeah, this is another case where we hit problem with transparent alias pretending
> >> >to be thunk :)
> >> >I guess the patch is OK for GCC-5 and for next stage1 we can clean this up.
> >> I was actually building a compiler so I could take a look at this
> >> one under a debugger ;-)
> >
> > I think it is really the transparent alias issue.  The comment seems pretty clear about it.
> > What is confusing is that instrumentation thunks are called thunks when they are
> > really not - thunk is a small hunk of code, while instrumentation thunk is a transparent
> > alias.
> >
> > Too bad I did not notice we introduced transparent aliases, i would push out my
> > code for that. I will compare Ilya's changes with mine and hopefully we can
> > catch more bugs and unify the code next stage1.
> 
> Thunk was the best I could find to represent the same function but
> with different signature. It would be great to have a more specialized
> way for that.

Yeah, that is also what we need for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61886
I just did not notice you are independently solving the same issue :(

Honza