resent2 [PATCH] Fix ICE in redirect_jump, at jump.c:1497 PR50496

Submitted by Eric Botcazou on Oct. 15, 2011, 2:21 p.m.

Details

Message ID 201110151621.27366.ebotcazou@adacore.com
State New
Headers show

Commit Message

Eric Botcazou Oct. 15, 2011, 2:21 p.m.
> 	PR middle-end/50496
> 	* cfgrtl.c (try_redirect_by_replacing_jump): Treat EXIT_BLOCK_PTR case
> 	separately before call to redirect_jump(). Add assertion.
> 	(patch_jump_insn): Same.

This will definitely disable redirections to the exit block.  Now...

> diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
> index b3f045b..57f561f 100644
> --- a/gcc/cfgrtl.c
> +++ b/gcc/cfgrtl.c
> @@ -846,11 +846,10 @@ try_redirect_by_replacing_jump (edge e, basic_block
> target, bool in_cfglayout) if (dump_file)
>  	fprintf (dump_file, "Redirecting jump %i from %i to %i.\n",
>  		 INSN_UID (insn), e->dest->index, target->index);
> -      if (!redirect_jump (insn, block_label (target), 0))
> -	{
> -	  gcc_assert (target == EXIT_BLOCK_PTR);
> -	  return NULL;
> -	}
> +      if (target == EXIT_BLOCK_PTR)
> +	return NULL;
> +      if (! redirect_jump (insn, block_label (target), 0))
> +	gcc_unreachable ();
>      }
>
>    /* Cannot do anything for target exit block.  */
> @@ -1030,11 +1029,10 @@ patch_jump_insn (rtx insn, rtx old_label,
> basic_block new_bb) /* If the substitution doesn't succeed, die.  This can
> happen
>  	     if the back end emitted unrecognizable instructions or if
>  	     target is exit block on some arches.  */
> -	  if (!redirect_jump (insn, block_label (new_bb), 0))
> -	    {
> -	      gcc_assert (new_bb == EXIT_BLOCK_PTR);
> -	      return false;
> -	    }
> +	  if (new_bb == EXIT_BLOCK_PTR)
> +	    return false;
> +	  if (! redirect_jump (insn, block_label (new_bb), 0))
> +	    gcc_unreachable ();
>  	}
>      }
>    return true;

... the code is pretty clear: attempting to redirect to the exit block is a 
valid operation (since its potential failure is expected by the assertion).

The problem is that the interface to redirect_jump/redirect_jump_1 has recently 
been changed:

2011-07-28  Bernd Schmidt  <bernds@codesourcery.com>

[...]

	* jump.c (delete_related_insns): Likewise.
	(jump_to_label_p): New function.
	(redirect_target): New static function.
	(redirect_exp_1): Use it.  Adjust to handle ret_rtx in JUMP_LABELS.
	(redirect_jump_1): Assert that the new label is nonnull.
	(redirect_jump): Likewise.
	(redirect_jump_2): Check for ANY_RETURN_P rather than NULL labels.

[...]

but all the callers haven't apparently been updated.  The model seems to be:

	(dead_or_predicable): Change NEW_DEST arg to DEST_EDGE.  All callers
	changed.  Ensure that the right label is passed to redirect_jump.

@@ -4134,10 +4137,16 @@ dead_or_predicable (basic_block test_bb,
   old_dest = JUMP_LABEL (jump);
   if (other_bb != new_dest)
     {
-      new_label = block_label (new_dest);
+      if (JUMP_P (BB_END (dest_edge->src)))
+       new_dest_label = JUMP_LABEL (BB_END (dest_edge->src));
+      else if (new_dest == EXIT_BLOCK_PTR)
+       new_dest_label = ret_rtx;
+      else
+       new_dest_label = block_label (new_dest);
+
       if (reversep
-         ? ! invert_jump_1 (jump, new_label)
-         : ! redirect_jump_1 (jump, new_label))
+         ? ! invert_jump_1 (jump, new_dest_label)
+         : ! redirect_jump_1 (jump, new_dest_label))
        goto cancel;
     }

so the correct fix is very likely something like:



Bernd, should all the callers of redirect_jump/redirect_jump_1 be audited for 
this pattern (there are 3 of them in cfgrtl.c for example)?

Comments

Bernd Schmidt Oct. 17, 2011, 10:18 a.m.
On 10/15/11 16:21, Eric Botcazou wrote:
> so the correct fix is very likely something like:
> 
> Index: cfgrtl.c
> ===================================================================
> --- cfgrtl.c    (revision 179844)
> +++ cfgrtl.c    (working copy)
> @@ -1024,13 +1024,20 @@ patch_jump_insn (rtx insn, rtx old_label
> 
>        if (!currently_expanding_to_rtl || JUMP_LABEL (insn) == old_label)
>         {
> +         rtx new_label;
> +
>           /* If the insn doesn't go where we think, we're confused.  */
>           gcc_assert (JUMP_LABEL (insn) == old_label);
> 
> +         if (new_bb == EXIT_BLOCK_PTR)
> +           new_label = ret_rtx;
> +         else
> +           new_label = block_label (new_bb);
> +
>           /* If the substitution doesn't succeed, die.  This can happen
>              if the back end emitted unrecognizable instructions or if
>              target is exit block on some arches.  */
> -         if (!redirect_jump (insn, block_label (new_bb), 0))
> +         if (!redirect_jump (insn, new_label, 0))
>             {
>               gcc_assert (new_bb == EXIT_BLOCK_PTR);
>               return false;
> 
> 
> Bernd, should all the callers of redirect_jump/redirect_jump_1 be audited for 
> this pattern (there are 3 of them in cfgrtl.c for example)?

I think first we'll need to find the caller and make sure it really
wants a return and not a simple_return.


Bernd
Bernd Schmidt Oct. 17, 2011, 10:44 a.m.
On 10/15/11 16:21, Eric Botcazou wrote:
>> 	PR middle-end/50496
>> 	* cfgrtl.c (try_redirect_by_replacing_jump): Treat EXIT_BLOCK_PTR case
>> 	separately before call to redirect_jump(). Add assertion.
>> 	(patch_jump_insn): Same.
> 
> This will definitely disable redirections to the exit block.

Yes. However, in the case that caused the PR, this was attempted with
reload_completed == 0, so we cannot generate return patterns anyway and
must fail.

I think the original patch is OK.


Bernd
Eric Botcazou Oct. 17, 2011, 5:43 p.m.
> Yes. However, in the case that caused the PR, this was attempted with
> reload_completed == 0, so we cannot generate return patterns anyway and
> must fail.

OK, but we clean up the CFG after reload is completed (e.g. just before 
emitting the prologue) so in this case we can generate return patterns.
Bernd Schmidt Oct. 17, 2011, 5:55 p.m.
On 10/17/11 19:43, Eric Botcazou wrote:
>> Yes. However, in the case that caused the PR, this was attempted with
>> reload_completed == 0, so we cannot generate return patterns anyway and
>> must fail.
> 
> OK, but we clean up the CFG after reload is completed (e.g. just before 
> emitting the prologue) so in this case we can generate return patterns.

thread_prologue_and_epilogue_insns should detect all cases where a
return insn can be created. So any CFG cleanup that runs before it does
not need this functionality.


Bernd
Eric Botcazou Oct. 18, 2011, 8:03 a.m.
> thread_prologue_and_epilogue_insns should detect all cases where a
> return insn can be created. So any CFG cleanup that runs before it does
> not need this functionality.

So we're left with CFG cleanups that run after it and could forward edges to an 
edge from a return insn to the exit block in order to build a new return insn.
Chung-Lin Tang Oct. 24, 2011, 6:02 p.m.
On 2011/10/18 04:03 PM, Eric Botcazou wrote:
>> thread_prologue_and_epilogue_insns should detect all cases where a
>> return insn can be created. So any CFG cleanup that runs before it does
>> not need this functionality.
> 
> So we're left with CFG cleanups that run after it and could forward edges to an 
> edge from a return insn to the exit block in order to build a new return insn.

Bernd, why can't we simply remove the assertion? The pre-reload case
will fail at validation and return 0, matching pre-reload,
pre-shrink-wrap behavior, while any possible remaining post-reload
redirection to the exit block can just use 'ret_rtx' as the rare
fallback (I see you have retained the NULL case in redirect_target())

Chung-Lin
Bernd Schmidt Oct. 24, 2011, 6:04 p.m.
On 10/24/11 20:02, Chung-Lin Tang wrote:
> On 2011/10/18 04:03 PM, Eric Botcazou wrote:
>>> thread_prologue_and_epilogue_insns should detect all cases where a
>>> return insn can be created. So any CFG cleanup that runs before it does
>>> not need this functionality.
>>
>> So we're left with CFG cleanups that run after it and could forward edges to an 
>> edge from a return insn to the exit block in order to build a new return insn.

We have no testcases to suggest that this ever happens.

> Bernd, why can't we simply remove the assertion? The pre-reload case
> will fail at validation and return 0, matching pre-reload,
> pre-shrink-wrap behavior, while any possible remaining post-reload
> redirection to the exit block can just use 'ret_rtx' as the rare
> fallback

No, after prologue insertion we have to distinguish between ret_rtx and
simple_return_rtx.

> (I see you have retained the NULL case in redirect_target())

That may just be a thinko.


Bernd

Patch hide | download patch | download mbox

Index: cfgrtl.c
===================================================================
--- cfgrtl.c    (revision 179844)
+++ cfgrtl.c    (working copy)
@@ -1024,13 +1024,20 @@  patch_jump_insn (rtx insn, rtx old_label

       if (!currently_expanding_to_rtl || JUMP_LABEL (insn) == old_label)
        {
+         rtx new_label;
+
          /* If the insn doesn't go where we think, we're confused.  */
          gcc_assert (JUMP_LABEL (insn) == old_label);

+         if (new_bb == EXIT_BLOCK_PTR)
+           new_label = ret_rtx;
+         else
+           new_label = block_label (new_bb);
+
          /* If the substitution doesn't succeed, die.  This can happen
             if the back end emitted unrecognizable instructions or if
             target is exit block on some arches.  */
-         if (!redirect_jump (insn, block_label (new_bb), 0))
+         if (!redirect_jump (insn, new_label, 0))
            {
              gcc_assert (new_bb == EXIT_BLOCK_PTR);
              return false;