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

login
register
mail settings
Submitter Eric Botcazou
Date Oct. 15, 2011, 2:21 p.m.
Message ID <201110151621.27366.ebotcazou@adacore.com>
Download mbox | patch
Permalink /patch/119975/
State New
Headers show

Comments

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)?
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

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;