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

On 2011/10/25 02:04 AM, Bernd Schmidt wrote:
> 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.

Which does mean that, at least through the two call sites that my
original patch modified, it may be hard to ever find out later, if patch
applied.

>> 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'm suggesting a new patch, as attached. Before reload_completed, we
directly return 0 upon nlabel == NULL, which should be identical with
old behavior, while asserting fail if after reload (where we assume the
simple_return/return distinction is required).

This should ensure better that, if a post-prologue case of redirecting
to the exit block ever happens we will more easily know (by some future
PR :P)

Bootstrapped and tested on i686, and cross tested on ARM using QEMU.
Eric, is this approach okay?

Thanks,
Chung-Lin

2011-10-31  Chung-Lin Tang  <cltang@codesourcery.com>

	* jump.c (redirect_jump): Assert fail on nlabel == NULL_RTX
	only after reload. Add comments.

> I'm suggesting a new patch, as attached. Before reload_completed, we
> directly return 0 upon nlabel == NULL, which should be identical with
> old behavior, while asserting fail if after reload (where we assume the
> simple_return/return distinction is required).
>
> This should ensure better that, if a post-prologue case of redirecting
> to the exit block ever happens we will more easily know (by some future
> PR :P)
>
> Bootstrapped and tested on i686, and cross tested on ARM using QEMU.
> Eric, is this approach okay?

Don't you want epilogue_completed instead of reload_completed?  Otherwise,
yes, the approach is fine with me, but wait for Bernd's input.

> 2011-10-31  Chung-Lin Tang  <cltang@codesourcery.com>
>
> 	* jump.c (redirect_jump): Assert fail on nlabel == NULL_RTX
> 	only after reload. Add comments.

Minor rewording of the comment below:

+  if (!nlabel)
+    {

/* If there is no label, we are asked to redirect to the EXIT block.  Now,
   before the epilogue is emitted, return/simple_return cannot be created
   so we return 0 immediately.  After the epilogue is emitted, we always
   expect a label, either a non-null label, or a return/simple_return RTX.
 
+      if (!reload_completed)
+	return 0;
+      gcc_unreachable ();
+    }

On 10/31/11 10:11, Eric Botcazou wrote:
>> I'm suggesting a new patch, as attached. Before reload_completed, we
>> directly return 0 upon nlabel == NULL, which should be identical with
>> old behavior, while asserting fail if after reload (where we assume the
>> simple_return/return distinction is required).
>>
>> This should ensure better that, if a post-prologue case of redirecting
>> to the exit block ever happens we will more easily know (by some future
>> PR :P)
>>
>> Bootstrapped and tested on i686, and cross tested on ARM using QEMU.
>> Eric, is this approach okay?
> 
> Don't you want epilogue_completed instead of reload_completed?  Otherwise,
> yes, the approach is fine with me, but wait for Bernd's input.

That looks good to me too.


Bernd