Fix PR48182

Submitted by Marek Polacek on April 5, 2013, 3:22 p.m.

Details

Message ID 20130405152230.GH24873@redhat.com
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

--- 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.  */