[1/2] Update iterator of next

Message ID 20191115080838.48090-1-luoxhu@linux.ibm.com
State New
Headers show
Series
  • [1/2] Update iterator of next
Related show

Commit Message

luoxhu Nov. 15, 2019, 8:08 a.m.
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.
---
 gcc/ipa-inline.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

-- 
2.21.0.777.g83232e3864

Comments

Jan Hubicka Nov. 15, 2019, 8:10 a.m. | #1
> 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

>
Richard Biener Nov. 15, 2019, 9:16 a.m. | #2
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

> >
Richard Biener Nov. 15, 2019, 9:19 a.m. | #3
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

> > >
Jan Hubicka Nov. 15, 2019, 9:19 a.m. | #4
> 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

> > >
Jan Hubicka Nov. 15, 2019, 9:22 a.m. | #5
> 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
luoxhu Nov. 15, 2019, 10:57 a.m. | #6
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

>>>>
Martin Jambor Nov. 15, 2019, 12:13 p.m. | #7
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
Jan Hubicka Nov. 15, 2019, 12:46 p.m. | #8
> 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
Jan Hubicka Nov. 15, 2019, 12:58 p.m. | #9
> 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

Patch

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