diff mbox

[PR,60461] Fix loop condition at the end of ipa_modify_call_arguments

Message ID 20140313155612.GF19304@virgil.suse
State New
Headers show

Commit Message

Martin Jambor March 13, 2014, 3:56 p.m. UTC 
Hi,

the reference re-building part of ipa_modify_call_arguments has a
stupid bug in the while loop condition, which means it does not look
at statements produced by force_gimple_operand_gsi which may lead to
bugs such as PR 60461.

The 4.8 branch does not have this code and does not exhibit the bug so
for th time being I'm leaving the code alone there.

I have bootstrapped and tested the following patch on x86_64-linux on
trunk and will commit it there shortly as obvious.

Thanks,

Martin


2014-03-13  Martin Jambor  <mjambor@suse.cz>

	PR lto/60461
	* ipa-prop.c (ipa_modify_call_arguments): Fix iteration condition.

testsuite/
	* gcc.dg/lto/pr60461_0.c: New test.

Comments

Jakub Jelinek March 13, 2014, 4:19 p.m. UTC | #1 
On Thu, Mar 13, 2014 at 04:56:12PM +0100, Martin Jambor wrote:
> 	PR lto/60461
> 	* ipa-prop.c (ipa_modify_call_arguments): Fix iteration condition.
> 
> testsuite/
> 	* gcc.dg/lto/pr60461_0.c: New test.
> 
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 4fb916a..efe8c7a 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -3901,7 +3901,7 @@ ipa_modify_call_arguments (struct cgraph_edge *cs, gimple stmt,
>        gsi_prev (&gsi);
>      }
>    while ((gsi_end_p (prev_gsi) && !gsi_end_p (gsi))
> -	 || (!gsi_end_p (prev_gsi) && gsi_stmt (gsi) == gsi_stmt (prev_gsi)));
> +	 || (!gsi_end_p (prev_gsi) && gsi_stmt (gsi) != gsi_stmt (prev_gsi)));
>  }

Doesn't (in 4.8+ of course) gsi_stmt return NULL iff gsi_end_p?
Thus, can't this be simplified into:
    }
  while (gsi_stmt (gsi) != gsi_stmt (prev_gsi));
?

	Jakub
Martin Jambor March 13, 2014, 5:49 p.m. UTC | #2 
On Thu, Mar 13, 2014 at 05:19:02PM +0100, Jakub Jelinek wrote:
> On Thu, Mar 13, 2014 at 04:56:12PM +0100, Martin Jambor wrote:
> > 	PR lto/60461
> > 	* ipa-prop.c (ipa_modify_call_arguments): Fix iteration condition.
> > 
> > testsuite/
> > 	* gcc.dg/lto/pr60461_0.c: New test.
> > 
> > diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> > index 4fb916a..efe8c7a 100644
> > --- a/gcc/ipa-prop.c
> > +++ b/gcc/ipa-prop.c
> > @@ -3901,7 +3901,7 @@ ipa_modify_call_arguments (struct cgraph_edge *cs, gimple stmt,
> >        gsi_prev (&gsi);
> >      }
> >    while ((gsi_end_p (prev_gsi) && !gsi_end_p (gsi))
> > -	 || (!gsi_end_p (prev_gsi) && gsi_stmt (gsi) == gsi_stmt (prev_gsi)));
> > +	 || (!gsi_end_p (prev_gsi) && gsi_stmt (gsi) != gsi_stmt (prev_gsi)));
> >  }
> 
> Doesn't (in 4.8+ of course) gsi_stmt return NULL iff gsi_end_p?
> Thus, can't this be simplified into:
>     }
>   while (gsi_stmt (gsi) != gsi_stmt (prev_gsi));
> ?
> 

Apparently it does.  I will commit the simplified condition after
testing then.

Thanks,

Martin
diff mbox

Patch

diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 4fb916a..efe8c7a 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -3901,7 +3901,7 @@  ipa_modify_call_arguments (struct cgraph_edge *cs, gimple stmt,
       gsi_prev (&gsi);
     }
   while ((gsi_end_p (prev_gsi) && !gsi_end_p (gsi))
-	 || (!gsi_end_p (prev_gsi) && gsi_stmt (gsi) == gsi_stmt (prev_gsi)));
+	 || (!gsi_end_p (prev_gsi) && gsi_stmt (gsi) != gsi_stmt (prev_gsi)));
 }
 
 /* If the expression *EXPR should be replaced by a reduction of a parameter, do
diff --git a/gcc/testsuite/gcc.dg/lto/pr60461_0.c b/gcc/testsuite/gcc.dg/lto/pr60461_0.c
new file mode 100644
index 0000000..cad6a8d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/lto/pr60461_0.c
@@ -0,0 +1,37 @@ 
+/* { dg-lto-do link } */
+/* { dg-lto-options {{-Os -flto} } } */
+
+
+struct S
+{
+  int f1;
+  int f2;
+} a[1] = { {0, 0} }; 
+
+int b, c;
+
+static unsigned short fn1 (struct S);
+
+void
+fn2 ()
+{
+  for (; c;)
+    ;
+  b = 0;
+  fn1 (a[0]);
+}
+
+unsigned short
+fn1 (struct S p)
+{
+  if (p.f1)
+    fn2 ();
+  return 0;
+}
+
+int
+main ()
+{
+  fn2 ();
+  return 0;
+}