| Message ID | 20191115080838.48090-1-luoxhu@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] Update iterator of next | expand |
> next is initialized only in the loop before, it is never updated > in it's own loop. > > gcc/ChangeLog > > 2019-11-15 Xiong Hu Luo <luoxhu@linux.ibm.com> > > * ipa-inline.c (inline_small_functions): Update iterator of next. OK, thanks! Honza > --- > gcc/ipa-inline.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c > index 78ec0ec685f..96aefaf514b 100644 > --- a/gcc/ipa-inline.c > +++ b/gcc/ipa-inline.c > @@ -1945,12 +1945,15 @@ inline_small_functions (void) > } > if (has_speculative) > for (edge = node->callees; edge; edge = next) > - if (edge->speculative && !speculation_useful_p (edge, > - edge->aux != NULL)) > - { > - edge->resolve_speculation (); > - update = true; > - } > + { > + if (edge->speculative > + && !speculation_useful_p (edge, edge->aux != NULL)) > + { > + edge->resolve_speculation (); > + update = true; > + } > + next = edge->next_callee; > + } > if (update) > { > struct cgraph_node *where = node->inlined_to > -- > 2.21.0.777.g83232e3864 >
On Fri, Nov 15, 2019 at 9:10 AM Jan Hubicka <hubicka@ucw.cz> wrote: > > > next is initialized only in the loop before, it is never updated > > in it's own loop. > > > > gcc/ChangeLog > > > > 2019-11-15 Xiong Hu Luo <luoxhu@linux.ibm.com> > > > > * ipa-inline.c (inline_small_functions): Update iterator of next. > > OK, > thanks! This breaks bootstrap and the loop before is similarly odd. (gdb) p edge $1 = (cgraph_edge *) 0xa5a5a5a5a5a5a5a5 so apparently edge->next_callee is GCed (thus 'edge' itself is freed?) Richard. > Honza > > --- > > gcc/ipa-inline.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c > > index 78ec0ec685f..96aefaf514b 100644 > > --- a/gcc/ipa-inline.c > > +++ b/gcc/ipa-inline.c > > @@ -1945,12 +1945,15 @@ inline_small_functions (void) > > } > > if (has_speculative) > > for (edge = node->callees; edge; edge = next) > > - if (edge->speculative && !speculation_useful_p (edge, > > - edge->aux != NULL)) > > - { > > - edge->resolve_speculation (); > > - update = true; > > - } > > + { > > + if (edge->speculative > > + && !speculation_useful_p (edge, edge->aux != NULL)) > > + { > > + edge->resolve_speculation (); > > + update = true; > > + } > > + next = edge->next_callee; > > + } > > if (update) > > { > > struct cgraph_node *where = node->inlined_to > > -- > > 2.21.0.777.g83232e3864 > >
On Fri, Nov 15, 2019 at 10:16 AM Richard Biener <richard.guenther@gmail.com> wrote: > > On Fri, Nov 15, 2019 at 9:10 AM Jan Hubicka <hubicka@ucw.cz> wrote: > > > > > next is initialized only in the loop before, it is never updated > > > in it's own loop. > > > > > > gcc/ChangeLog > > > > > > 2019-11-15 Xiong Hu Luo <luoxhu@linux.ibm.com> > > > > > > * ipa-inline.c (inline_small_functions): Update iterator of next. > > > > OK, > > thanks! > > This breaks bootstrap and the loop before is similarly odd. > > (gdb) p edge > $1 = (cgraph_edge *) 0xa5a5a5a5a5a5a5a5 > > so apparently edge->next_callee is GCed (thus 'edge' itself is freed?) edge->resolve_speculation (); can remove the edge, so maybe Index: gcc/ipa-inline.c =================================================================== --- gcc/ipa-inline.c (revision 278280) +++ gcc/ipa-inline.c (working copy) @@ -1932,13 +1932,13 @@ inline_small_functions (void) if (has_speculative) for (edge = node->callees; edge; edge = next) { + next = edge->next_callee; if (edge->speculative && !speculation_useful_p (edge, edge->aux != NULL)) { edge->resolve_speculation (); update = true; } - next = edge->next_callee; } if (update) { which I'll commit after verifying it fixes bootstrap. But the whole function needs audit for this 'next' thing (as said, the previous loop never loops either). Richard. > Richard. > > > Honza > > > --- > > > gcc/ipa-inline.c | 15 +++++++++------ > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c > > > index 78ec0ec685f..96aefaf514b 100644 > > > --- a/gcc/ipa-inline.c > > > +++ b/gcc/ipa-inline.c > > > @@ -1945,12 +1945,15 @@ inline_small_functions (void) > > > } > > > if (has_speculative) > > > for (edge = node->callees; edge; edge = next) > > > - if (edge->speculative && !speculation_useful_p (edge, > > > - edge->aux != NULL)) > > > - { > > > - edge->resolve_speculation (); > > > - update = true; > > > - } > > > + { > > > + if (edge->speculative > > > + && !speculation_useful_p (edge, edge->aux != NULL)) > > > + { > > > + edge->resolve_speculation (); > > > + update = true; > > > + } > > > + next = edge->next_callee; > > > + } > > > if (update) > > > { > > > struct cgraph_node *where = node->inlined_to > > > -- > > > 2.21.0.777.g83232e3864 > > >
> On Fri, Nov 15, 2019 at 9:10 AM Jan Hubicka <hubicka@ucw.cz> wrote: > > > > > next is initialized only in the loop before, it is never updated > > > in it's own loop. > > > > > > gcc/ChangeLog > > > > > > 2019-11-15 Xiong Hu Luo <luoxhu@linux.ibm.com> > > > > > > * ipa-inline.c (inline_small_functions): Update iterator of next. > > > > OK, > > thanks! > > This breaks bootstrap and the loop before is similarly odd. > > (gdb) p edge > $1 = (cgraph_edge *) 0xa5a5a5a5a5a5a5a5 > > so apparently edge->next_callee is GCed (thus 'edge' itself is freed?) Loop before seems OK, it has next variable because the edge it works with may be removed. Only the next loop seems to omit set of next. > > > + { > > > + if (edge->speculative > > > + && !speculation_useful_p (edge, edge->aux != NULL)) > > > + { > > > + edge->resolve_speculation (); > > > + update = true; > > > + } > > > + next = edge->next_callee; The problem seems that next should be set before and not after resolvign (which indeed may remove edge). I will fix that. Honza > > > + } > > > if (update) > > > { > > > struct cgraph_node *where = node->inlined_to > > > -- > > > 2.21.0.777.g83232e3864 > > >
> On Fri, Nov 15, 2019 at 10:16 AM Richard Biener > <richard.guenther@gmail.com> wrote: > > > > On Fri, Nov 15, 2019 at 9:10 AM Jan Hubicka <hubicka@ucw.cz> wrote: > > > > > > > next is initialized only in the loop before, it is never updated > > > > in it's own loop. > > > > > > > > gcc/ChangeLog > > > > > > > > 2019-11-15 Xiong Hu Luo <luoxhu@linux.ibm.com> > > > > > > > > * ipa-inline.c (inline_small_functions): Update iterator of next. > > > > > > OK, > > > thanks! > > > > This breaks bootstrap and the loop before is similarly odd. > > > > (gdb) p edge > > $1 = (cgraph_edge *) 0xa5a5a5a5a5a5a5a5 > > > > so apparently edge->next_callee is GCed (thus 'edge' itself is freed?) > > edge->resolve_speculation (); > > can remove the edge, so maybe > > Index: gcc/ipa-inline.c > =================================================================== > --- gcc/ipa-inline.c (revision 278280) > +++ gcc/ipa-inline.c (working copy) > @@ -1932,13 +1932,13 @@ inline_small_functions (void) > if (has_speculative) > for (edge = node->callees; edge; edge = next) > { > + next = edge->next_callee; > if (edge->speculative > && !speculation_useful_p (edge, edge->aux != NULL)) > { > edge->resolve_speculation (); > update = true; > } > - next = edge->next_callee; > } > if (update) > { > > which I'll commit after verifying it fixes bootstrap. I stasrted testing: Index: ../gcc/ipa-inline.c =================================================================== --- ../gcc/ipa-inline.c (revision 278281) +++ ../gcc/ipa-inline.c (working copy) @@ -1913,9 +1913,8 @@ inline_small_functions (void) if (dump_file) fprintf (dump_file, "Enqueueing calls in %s.\n", node->dump_name ()); - for (edge = node->callees; edge; edge = next) + for (edge = node->callees; edge; edge = edge->next_callee) { - next = edge->next_callee; if (edge->inline_failed && !edge->aux && can_inline_edge_p (edge, true) @@ -1932,13 +1931,13 @@ inline_small_functions (void) if (has_speculative) for (edge = node->callees; edge; edge = next) { + next = edge->next_callee; if (edge->speculative && !speculation_useful_p (edge, edge->aux != NULL)) { edge->resolve_speculation (); update = true; } - next = edge->next_callee; } if (update) { Since the previous loop never removes edges (I suppose the bug came to be by splitting this loop to two incorrectly). Where is the other non-looping loop? Honza
On 2019/11/15 17:19, Jan Hubicka wrote: >> On Fri, Nov 15, 2019 at 9:10 AM Jan Hubicka <hubicka@ucw.cz> wrote: >>> >>>> next is initialized only in the loop before, it is never updated >>>> in it's own loop. >>>> >>>> gcc/ChangeLog >>>> >>>> 2019-11-15 Xiong Hu Luo <luoxhu@linux.ibm.com> >>>> >>>> * ipa-inline.c (inline_small_functions): Update iterator of next. >>> >>> OK, >>> thanks! >> >> This breaks bootstrap and the loop before is similarly odd. >> >> (gdb) p edge >> $1 = (cgraph_edge *) 0xa5a5a5a5a5a5a5a5 >> >> so apparently edge->next_callee is GCed (thus 'edge' itself is freed?) > Loop before seems OK, it has next variable because the edge it works > with may be removed. > > Only the next loop seems to omit set of next. >>>> + { >>>> + if (edge->speculative >>>> + && !speculation_useful_p (edge, edge->aux != NULL)) >>>> + { >>>> + edge->resolve_speculation (); >>>> + update = true; >>>> + } >>>> + next = edge->next_callee; > The problem seems that next should be set before and not after resolvign > (which indeed may remove edge). > > I will fix that. Sorry to break the bootstrap. This was my lack of consideration when splitting this small piece of code from the previous patch, the line should be in the first line of the second loop. Could you please add some comments that edge may be freed in resolve_speculation()? Thanks. Xiong Hu > > Honza >>>> + } >>>> if (update) >>>> { >>>> struct cgraph_node *where = node->inlined_to >>>> -- >>>> 2.21.0.777.g83232e3864 >>>>
Hi, On Fri, Nov 15 2019, luoxhu wrote: > > Sorry to break the bootstrap. This was my lack of consideration when > splitting this small piece of code from the previous patch, the line > should be in the first line of the second loop. Could you please add > some comments that edge may be freed in resolve_speculation()? > Thanks. First and foremost, resolve_speculation should not free its this pointer. This is the third time it has caused us a headache in the last few months. If we cannot think of a better solution in the next week or three, I will revert it back to a non-member function in this stage 3 ...and then we can (and should and will) document that it can free the edge. Martin
> Hi, > > On Fri, Nov 15 2019, luoxhu wrote: > > > > Sorry to break the bootstrap. This was my lack of consideration when > > splitting this small piece of code from the previous patch, the line > > should be in the first line of the second loop. Could you please add > > some comments that edge may be freed in resolve_speculation()? > > Thanks. > > First and foremost, resolve_speculation should not free its this > pointer. This is the third time it has caused us a headache in the last > few months. If we cannot think of a better solution in the next week or > three, I will revert it back to a non-member function in this stage 3 The bug in the walker loop here IMO predated C++ conversion, but to avoid member function freeing THIS we probably want to have: cgraph_edge::remove (e), symtab_node::remove (e), ipa_ref::remove (ref), cgraph_node::resolve_speculation (e, target) I suppose good time for the conversion would be after remaining patches are merget. If you would have time to do that even better ;) > ...and then we can (and should and will) document that it can free the > edge. It speaks about removing edges and return value, Speculative call edge turned out to be direct call to CALLEE_DECL. Remove the speculative call sequence and return edge representing the call. It is up to caller to redirect the call as appropriate. We could add Edge E may be freed if it is resolved to other edge of the speculative call. Honza > > Martin
> The bug in the walker loop here IMO predated C++ conversion, but to > avoid member function freeing THIS we probably want to have: > > cgraph_edge::remove (e), > symtab_node::remove (e), > ipa_ref::remove (ref), > cgraph_node::resolve_speculation (e, target) Actually thinking of this, perhas remove_speculation is better to make it clear that it removes something ;) Thanks for reminiding me of this issue :) Honza > > I suppose good time for the conversion would be after remaining patches > are merget. If you would have time to do that even better ;) > > > ...and then we can (and should and will) document that it can free the > > edge. > It speaks about removing edges and return value, > > Speculative call edge turned out to be direct call to CALLEE_DECL. > Remove the speculative call sequence and return edge representing the call. > It is up to caller to redirect the call as appropriate. > > We could add > Edge E may be freed if it is resolved to other edge of the > speculative call. > > Honza > > > > Martin
diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c index 78ec0ec685f..96aefaf514b 100644 --- a/gcc/ipa-inline.c +++ b/gcc/ipa-inline.c @@ -1945,12 +1945,15 @@ inline_small_functions (void) } if (has_speculative) for (edge = node->callees; edge; edge = next) - if (edge->speculative && !speculation_useful_p (edge, - edge->aux != NULL)) - { - edge->resolve_speculation (); - update = true; - } + { + if (edge->speculative + && !speculation_useful_p (edge, edge->aux != NULL)) + { + edge->resolve_speculation (); + update = true; + } + next = edge->next_callee; + } if (update) { struct cgraph_node *where = node->inlined_to