Patchwork Fix PR48182

login
register
mail settings
Submitter Marek Polacek
Date April 5, 2013, 3:22 p.m.
Message ID <20130405152230.GH24873@redhat.com>
Download mbox | patch
Permalink /patch/234179/
State New
Headers show

Comments

Marek Polacek - April 5, 2013, 3:22 p.m.
This patch prevents segfault when using --param min-crossjump-insns=0.
What can happen in that case is that flow_find_cross_jump returns 0,
thus nmatch is 0, then
nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS)
doesn't hold, thus we continue, but we segfault later on when
doing split_block.  I think it's better to just bail out in that
case; moreover setting min-crossjump-insns to 0 isn't very common...

Regtested/bootstrapped on x86_64-linux, ok for trunk/4.8?

2013-04-05  Marek Polacek  <polacek@redhat.com>

	PR rtl-optimization/48182
	* cfgcleanup.c (try_crossjump_to_edge): Bail out if
	PARAM_MIN_CROSSJUMP_INSNS is 0.

	* gcc.dg/pr48182.c: New test.


	Marek
Jeff Law - April 5, 2013, 8:21 p.m.
On 04/05/2013 09:22 AM, Marek Polacek wrote:
> This patch prevents segfault when using --param min-crossjump-insns=0.
> What can happen in that case is that flow_find_cross_jump returns 0,
> thus nmatch is 0, then
> nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS)
> doesn't hold, thus we continue, but we segfault later on when
> doing split_block.  I think it's better to just bail out in that
> case; moreover setting min-crossjump-insns to 0 isn't very common...
>
> Regtested/bootstrapped on x86_64-linux, ok for trunk/4.8?
>
> 2013-04-05  Marek Polacek  <polacek@redhat.com>
>
> 	PR rtl-optimization/48182
> 	* cfgcleanup.c (try_crossjump_to_edge): Bail out if
> 	PARAM_MIN_CROSSJUMP_INSNS is 0.
>
> 	* gcc.dg/pr48182.c: New test.
OK for the trunk.  Release manager's decision for 4.8.

jeff
Jakub Jelinek - April 5, 2013, 8:33 p.m.
On Fri, Apr 05, 2013 at 02:21:57PM -0600, Jeff Law wrote:
> On 04/05/2013 09:22 AM, Marek Polacek wrote:
> >This patch prevents segfault when using --param min-crossjump-insns=0.
> >What can happen in that case is that flow_find_cross_jump returns 0,
> >thus nmatch is 0, then
> >nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS)
> >doesn't hold, thus we continue, but we segfault later on when
> >doing split_block.  I think it's better to just bail out in that
> >case; moreover setting min-crossjump-insns to 0 isn't very common...
> >
> >Regtested/bootstrapped on x86_64-linux, ok for trunk/4.8?
> >
> >2013-04-05  Marek Polacek  <polacek@redhat.com>
> >
> >	PR rtl-optimization/48182
> >	* cfgcleanup.c (try_crossjump_to_edge): Bail out if
> >	PARAM_MIN_CROSSJUMP_INSNS is 0.
> >
> >	* gcc.dg/pr48182.c: New test.
> OK for the trunk.  Release manager's decision for 4.8.

Wouldn't it be better to change params.def to instead say:
     5, 1, 0)
Because with the cfgcleanup.c change, --param min-crossjump-insns=0
is handled as =infinity rather than something smaller than 0.

	Jakub
Jeff Law - April 5, 2013, 8:42 p.m.
On 04/05/2013 02:33 PM, Jakub Jelinek wrote:
> On Fri, Apr 05, 2013 at 02:21:57PM -0600, Jeff Law wrote:
>> On 04/05/2013 09:22 AM, Marek Polacek wrote:
>>> This patch prevents segfault when using --param min-crossjump-insns=0.
>>> What can happen in that case is that flow_find_cross_jump returns 0,
>>> thus nmatch is 0, then
>>> nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS)
>>> doesn't hold, thus we continue, but we segfault later on when
>>> doing split_block.  I think it's better to just bail out in that
>>> case; moreover setting min-crossjump-insns to 0 isn't very common...
>>>
>>> Regtested/bootstrapped on x86_64-linux, ok for trunk/4.8?
>>>
>>> 2013-04-05  Marek Polacek  <polacek@redhat.com>
>>>
>>> 	PR rtl-optimization/48182
>>> 	* cfgcleanup.c (try_crossjump_to_edge): Bail out if
>>> 	PARAM_MIN_CROSSJUMP_INSNS is 0.
>>>
>>> 	* gcc.dg/pr48182.c: New test.
>> OK for the trunk.  Release manager's decision for 4.8.
>
> Wouldn't it be better to change params.def to instead say:
>       5, 1, 0)
> Because with the cfgcleanup.c change, --param min-crossjump-insns=0
> is handled as =infinity rather than something smaller than 0.
?  I must be missing something, the change causes an early bail out from 
try_crossjump_to_edge.

We don't want to raise the min to > 0 as that doesn't allow the user to 
turn on this specific transformation.

jeff
Jakub Jelinek - April 5, 2013, 8:50 p.m.
On Fri, Apr 05, 2013 at 02:42:19PM -0600, Jeff Law wrote:
> ?  I must be missing something, the change causes an early bail out
> from try_crossjump_to_edge.
> 
> We don't want to raise the min to > 0 as that doesn't allow the user
> to turn on this specific transformation.

The condition is
  if (nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS))
    return false; // aka "don't crossjump"
So, the smaller the N in --param min-crossjump-insns=N is, the more likely
we crossjump.  Thus N=0 should mean that it is most likely we crossjump,
and as N=1 requires that at least one insn matches, N=0 would mean that
even zero insns can match.  If we for --param min-crossjump-insns=0
always return false, it means we never crossjump, so it is least likely
that we crossjump, which corresponds to largest possible N, not smallest
one.

	Jakub
Jeff Law - April 5, 2013, 9 p.m.
On 04/05/2013 02:50 PM, Jakub Jelinek wrote:
> On Fri, Apr 05, 2013 at 02:42:19PM -0600, Jeff Law wrote:
>> ?  I must be missing something, the change causes an early bail out
>> from try_crossjump_to_edge.
>>
>> We don't want to raise the min to > 0 as that doesn't allow the user
>> to turn on this specific transformation.
>
> The condition is
>    if (nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS))
>      return false; // aka "don't crossjump"
> So, the smaller the N in --param min-crossjump-insns=N is, the more likely
> we crossjump.  Thus N=0 should mean that it is most likely we crossjump,
> and as N=1 requires that at least one insn matches, N=0 would mean that
> even zero insns can match.  If we for --param min-crossjump-insns=0
> always return false, it means we never crossjump, so it is least likely
> that we crossjump, which corresponds to largest possible N, not smallest
> one.
Yes the smaller the N, the more likely we are to crossjump, of course 
the value 0 would make no sense (I'm clearly out of practice on reviews :-).

Yea, changing the min value in params.def to 1 would be a better way to 
fix.  Consider that patch pre-approved.

jeff

Patch

--- gcc/testsuite/gcc.dg/pr48182.c.mp	2013-04-05 14:58:06.373269392 +0200
+++ gcc/testsuite/gcc.dg/pr48182.c	2013-04-05 14:57:47.867211373 +0200
@@ -0,0 +1,11 @@ 
+/* PR rtl-optimization/48182 */
+/* { dg-do compile } */
+/* { dg-options "-fcrossjumping --param min-crossjump-insns=0" } */
+
+extern int bar (void);
+
+int
+foo (int x)
+{
+  return x && bar ();
+}
--- gcc/cfgcleanup.c.mp	2013-04-05 14:55:01.634675751 +0200
+++ gcc/cfgcleanup.c	2013-04-05 16:33:23.701814048 +0200
@@ -1929,8 +1929,9 @@  try_crossjump_to_edge (int mode, edge e1
      of matching instructions or the 'from' block was totally matched
      (such that its predecessors will hopefully be redirected and the
      block removed).  */
-  if ((nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS))
+  if ((nmatch < PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS)
       && (newpos1 != BB_HEAD (src1)))
+      || PARAM_VALUE (PARAM_MIN_CROSSJUMP_INSNS) == 0)
     return false;
 
   /* Avoid deleting preserve label when redirecting ABNORMAL edges.  */