| Message ID | 20140813122549.GA29331@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 13, 2014 at 2:25 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > Hi, > > This patch is to fix wrong refactoring for cgraph_node::function_symbol introduced by this patch: https://gcc.gnu.org/ml/gcc-cvs/2014-07/msg00805.html. Here is how function was refactored: > > -cgraph_function_node (struct cgraph_node *node, enum availability *availability) > +cgraph_node * > +cgraph_node::function_symbol (enum availability *availability) > { > + cgraph_node *node = NULL; > + > do > { > - node = cgraph_function_or_thunk_node (node, availability); > + node = ultimate_alias_target (availability); > if (node->thunk.thunk_p) > { > node = node->callees->callee; > if (availability) > { > enum availability a; > - a = cgraph_function_body_availability (node); > + a = node->get_availability (); > if (a < *availability) > *availability = a; > } > - node = cgraph_function_or_thunk_node (node, availability); > + node = node->ultimate_alias_target (availability); > } > } while (node && node->thunk.thunk_p); > return node; > } > > first ultimate_alias_target call always uses 'this' instead of 'node'. This causes infinite loop. > > Patch was bootstrapped and regtested on linux-x86_64. OK for trunk? Ok. Do you have a testcase? Thanks, Richard. > Thanks, > Ilya > -- > > 2014-08-13 Ilya Enkovich <ilya.enkovich@intel.com> > > * cgraph.c (cgraph_node::function_symbol): Fix wrong > cgraph_function_node to cgraph_node::function_symbol > refactoring. > > diff --git a/gcc/cgraph.c b/gcc/cgraph.c > index 5a0b903..370a96a 100644 > --- a/gcc/cgraph.c > +++ b/gcc/cgraph.c > @@ -3000,11 +3000,11 @@ cgraph_node::verify_cgraph_nodes (void) > cgraph_node * > cgraph_node::function_symbol (enum availability *availability) > { > - cgraph_node *node = NULL; > + cgraph_node *node = this; > > do > { > - node = ultimate_alias_target (availability); > + node = node->ultimate_alias_target (availability); > if (node->thunk.thunk_p) > { > node = node->callees->callee;
2014-08-13 16:48 GMT+04:00 Richard Biener <richard.guenther@gmail.com>: > On Wed, Aug 13, 2014 at 2:25 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> Hi, >> >> This patch is to fix wrong refactoring for cgraph_node::function_symbol introduced by this patch: https://gcc.gnu.org/ml/gcc-cvs/2014-07/msg00805.html. Here is how function was refactored: >> >> -cgraph_function_node (struct cgraph_node *node, enum availability *availability) >> +cgraph_node * >> +cgraph_node::function_symbol (enum availability *availability) >> { >> + cgraph_node *node = NULL; >> + >> do >> { >> - node = cgraph_function_or_thunk_node (node, availability); >> + node = ultimate_alias_target (availability); >> if (node->thunk.thunk_p) >> { >> node = node->callees->callee; >> if (availability) >> { >> enum availability a; >> - a = cgraph_function_body_availability (node); >> + a = node->get_availability (); >> if (a < *availability) >> *availability = a; >> } >> - node = cgraph_function_or_thunk_node (node, availability); >> + node = node->ultimate_alias_target (availability); >> } >> } while (node && node->thunk.thunk_p); >> return node; >> } >> >> first ultimate_alias_target call always uses 'this' instead of 'node'. This causes infinite loop. >> >> Patch was bootstrapped and regtested on linux-x86_64. OK for trunk? > > Ok. Do you have a testcase? No, I found this problem testing pointer bounds checker, which produces many instrumentation thunks. It seems in a regular case we do not have such thunks chains and one loop iteration is enough. Ilya > > Thanks, > Richard. > >> Thanks, >> Ilya >> -- >> >> 2014-08-13 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * cgraph.c (cgraph_node::function_symbol): Fix wrong >> cgraph_function_node to cgraph_node::function_symbol >> refactoring. >> >> diff --git a/gcc/cgraph.c b/gcc/cgraph.c >> index 5a0b903..370a96a 100644 >> --- a/gcc/cgraph.c >> +++ b/gcc/cgraph.c >> @@ -3000,11 +3000,11 @@ cgraph_node::verify_cgraph_nodes (void) >> cgraph_node * >> cgraph_node::function_symbol (enum availability *availability) >> { >> - cgraph_node *node = NULL; >> + cgraph_node *node = this; >> >> do >> { >> - node = ultimate_alias_target (availability); >> + node = node->ultimate_alias_target (availability); >> if (node->thunk.thunk_p) >> { >> node = node->callees->callee;
> 2014-08-13 Ilya Enkovich <ilya.enkovich@intel.com> > > * cgraph.c (cgraph_node::function_symbol): Fix wrong > cgraph_function_node to cgraph_node::function_symbol > refactoring. OK, thanks1 Honza > > diff --git a/gcc/cgraph.c b/gcc/cgraph.c > index 5a0b903..370a96a 100644 > --- a/gcc/cgraph.c > +++ b/gcc/cgraph.c > @@ -3000,11 +3000,11 @@ cgraph_node::verify_cgraph_nodes (void) > cgraph_node * > cgraph_node::function_symbol (enum availability *availability) > { > - cgraph_node *node = NULL; > + cgraph_node *node = this; > > do > { > - node = ultimate_alias_target (availability); > + node = node->ultimate_alias_target (availability); > if (node->thunk.thunk_p) > { > node = node->callees->callee;
On 08/13/2014 02:25 PM, Ilya Enkovich wrote: > Hi, > > This patch is to fix wrong refactoring for cgraph_node::function_symbol introduced by this patch: https://gcc.gnu.org/ml/gcc-cvs/2014-07/msg00805.html. Here is how function was refactored: > > -cgraph_function_node (struct cgraph_node *node, enum availability *availability) > +cgraph_node * > +cgraph_node::function_symbol (enum availability *availability) > { > + cgraph_node *node = NULL; > + > do > { > - node = cgraph_function_or_thunk_node (node, availability); > + node = ultimate_alias_target (availability); > if (node->thunk.thunk_p) > { > node = node->callees->callee; > if (availability) > { > enum availability a; > - a = cgraph_function_body_availability (node); > + a = node->get_availability (); > if (a < *availability) > *availability = a; > } > - node = cgraph_function_or_thunk_node (node, availability); > + node = node->ultimate_alias_target (availability); > } > } while (node && node->thunk.thunk_p); > return node; > } > > first ultimate_alias_target call always uses 'this' instead of 'node'. This causes infinite loop. > > Patch was bootstrapped and regtested on linux-x86_64. OK for trunk? Hello. Thank you for the fix. Unfortunately, there's no test case that would show me the problem. Martin > > Thanks, > Ilya > -- > > 2014-08-13 Ilya Enkovich <ilya.enkovich@intel.com> > > * cgraph.c (cgraph_node::function_symbol): Fix wrong > cgraph_function_node to cgraph_node::function_symbol > refactoring. > > diff --git a/gcc/cgraph.c b/gcc/cgraph.c > index 5a0b903..370a96a 100644 > --- a/gcc/cgraph.c > +++ b/gcc/cgraph.c > @@ -3000,11 +3000,11 @@ cgraph_node::verify_cgraph_nodes (void) > cgraph_node * > cgraph_node::function_symbol (enum availability *availability) > { > - cgraph_node *node = NULL; > + cgraph_node *node = this; > > do > { > - node = ultimate_alias_target (availability); > + node = node->ultimate_alias_target (availability); > if (node->thunk.thunk_p) > { > node = node->callees->callee;
2014-08-22 16:36 GMT+04:00 Martin Liška <mliska@suse.cz>: > On 08/13/2014 02:25 PM, Ilya Enkovich wrote: >> Hi, >> >> This patch is to fix wrong refactoring for cgraph_node::function_symbol introduced by this patch: https://gcc.gnu.org/ml/gcc-cvs/2014-07/msg00805.html. Here is how function was refactored: >> >> -cgraph_function_node (struct cgraph_node *node, enum availability *availability) >> +cgraph_node * >> +cgraph_node::function_symbol (enum availability *availability) >> { >> + cgraph_node *node = NULL; >> + >> do >> { >> - node = cgraph_function_or_thunk_node (node, availability); >> + node = ultimate_alias_target (availability); >> if (node->thunk.thunk_p) >> { >> node = node->callees->callee; >> if (availability) >> { >> enum availability a; >> - a = cgraph_function_body_availability (node); >> + a = node->get_availability (); >> if (a < *availability) >> *availability = a; >> } >> - node = cgraph_function_or_thunk_node (node, availability); >> + node = node->ultimate_alias_target (availability); >> } >> } while (node && node->thunk.thunk_p); >> return node; >> } >> >> first ultimate_alias_target call always uses 'this' instead of 'node'. This causes infinite loop. >> >> Patch was bootstrapped and regtested on linux-x86_64. OK for trunk? > Hello. > Thank you for the fix. Unfortunately, there's no test case that would show me the problem. You should have have at least two thunks in a chain of aliases and thunks to fall into an infinite loop here. I do not know when such chains exist in regular cases. I hit this bug testing pointer bounds checker which transforms functions to thunks and therefore get longer chains of thunks. Thanks, Ilya > > Martin >> >> Thanks, >> Ilya >> -- >> >> 2014-08-13 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * cgraph.c (cgraph_node::function_symbol): Fix wrong >> cgraph_function_node to cgraph_node::function_symbol >> refactoring. >> >> diff --git a/gcc/cgraph.c b/gcc/cgraph.c >> index 5a0b903..370a96a 100644 >> --- a/gcc/cgraph.c >> +++ b/gcc/cgraph.c >> @@ -3000,11 +3000,11 @@ cgraph_node::verify_cgraph_nodes (void) >> cgraph_node * >> cgraph_node::function_symbol (enum availability *availability) >> { >> - cgraph_node *node = NULL; >> + cgraph_node *node = this; >> >> do >> { >> - node = ultimate_alias_target (availability); >> + node = node->ultimate_alias_target (availability); >> if (node->thunk.thunk_p) >> { >> node = node->callees->callee; >
diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 5a0b903..370a96a 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -3000,11 +3000,11 @@ cgraph_node::verify_cgraph_nodes (void) cgraph_node * cgraph_node::function_symbol (enum availability *availability) { - cgraph_node *node = NULL; + cgraph_node *node = this; do { - node = ultimate_alias_target (availability); + node = node->ultimate_alias_target (availability); if (node->thunk.thunk_p) { node = node->callees->callee;
Hi, This patch is to fix wrong refactoring for cgraph_node::function_symbol introduced by this patch: https://gcc.gnu.org/ml/gcc-cvs/2014-07/msg00805.html. Here is how function was refactored: -cgraph_function_node (struct cgraph_node *node, enum availability *availability) +cgraph_node * +cgraph_node::function_symbol (enum availability *availability) { + cgraph_node *node = NULL; + do { - node = cgraph_function_or_thunk_node (node, availability); + node = ultimate_alias_target (availability); if (node->thunk.thunk_p) { node = node->callees->callee; if (availability) { enum availability a; - a = cgraph_function_body_availability (node); + a = node->get_availability (); if (a < *availability) *availability = a; } - node = cgraph_function_or_thunk_node (node, availability); + node = node->ultimate_alias_target (availability); } } while (node && node->thunk.thunk_p); return node; } first ultimate_alias_target call always uses 'this' instead of 'node'. This causes infinite loop. Patch was bootstrapped and regtested on linux-x86_64. OK for trunk? Thanks, Ilya -- 2014-08-13 Ilya Enkovich <ilya.enkovich@intel.com> * cgraph.c (cgraph_node::function_symbol): Fix wrong cgraph_function_node to cgraph_node::function_symbol refactoring.