| Message ID | 20150318121912.GB64546@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
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,
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
> > 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,
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
> 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
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
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
> 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
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,