resent [PATCH] Fix PR50496

resent [PATCH] Fix PR50496

[Markus Trippelsdorf]

This patch, originally from Chung-Lin Tang, fixes PR50496.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50496

Can someone please review and commit it? 
Thanks.

Bootstrapped and tested on x86_64-pc-linux-gnu.

	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.

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;

-- 
Markus

Re: resent [PATCH] Fix PR50496

[Eric Botcazou]

> This patch, originally from Chung-Lin Tang, fixes PR50496.
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50496
>
> Can someone please review and commit it?

A proper patch submission should include a description of the problem and a 
rationale for the proposed fix (unless it is trivial).  

-- 
Eric Botcazou

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

[Markus Trippelsdorf]

Consider this testcase:

 $ cat test.cpp
class GCAlloc {
};
class BaseAlloc {
};
class String;
class Base {
public:
     virtual void destroy( String *str ) const =0;
};
class String: public GCAlloc {
     const Base *m_class;
public:
     enum constants {
     };
     String( const char *data );
     ~String() {
          m_class->destroy( this );
     }
     void copy( const String &other );
     String & operator=( const char *other ) {
          copy( String( other ) );
     }
};
class ListElement: public BaseAlloc {
};
class List: public BaseAlloc {
     ListElement *m_head;
     void (*m_deletor)( void *);
public:
     List():       m_deletor(0) {
     }
     const void *back() const {
     }
     bool empty() const {
          return m_head == 0;
     }
     void popBack();
};
class FalconData: public BaseAlloc {
public:
     virtual ~FalconData() {
     }
};
class Stream: public FalconData {
};
class SrcLexer: public BaseAlloc {
     List m_streams;
     String m_whiteLead;
     void reset();
};
void SrcLexer::reset()
{
     m_whiteLead = "";
     while( ! m_streams.empty() ) {
          Stream *s = (Stream *) m_streams.back();
          m_streams.popBack();
          if ( !m_streams.empty() )          delete s;
     }
}

 % g++ -O2 test.cpp
test.cpp: In member function ‘void SrcLexer::reset()’:
test.cpp:59:1: internal compiler error: in redirect_jump, at jump.c:1497

It hits the following assertion:
 gcc_assert (nlabel != NULL_RTX);

In this case target(or new_bb)=EXIT_BLOCK_PTR and block_label(EXIT_BLOCK_PTR)==NULL_RTX.
Fix this by treating the target=EXIT_BLOCK_PTR case before calling
redirect_jump in gcc/cfgrtl.c.


	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.

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;

-- 
Markus

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

[Eric Botcazou]

> 	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:

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

-- 
Eric Botcazou

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

[Bernd Schmidt]

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

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

[Bernd Schmidt]

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

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

[Eric Botcazou]

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

-- 
Eric Botcazou

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

[Bernd Schmidt]

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

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

[Eric Botcazou]

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

-- 
Eric Botcazou

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

[Chung-Lin Tang]

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

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

[Bernd Schmidt]

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

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

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

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.

[-- Attachment #2: pr50496-2.diff --]
[-- Type: text/plain, Size: 723 bytes --]

Index: jump.c
===================================================================
--- jump.c	(revision 180421)
+++ jump.c	(working copy)
@@ -1495,8 +1495,19 @@ redirect_jump (rtx jump, rtx nlabel, int delete_un
 {
   rtx olabel = JUMP_LABEL (jump);
 
-  gcc_assert (nlabel != NULL_RTX);
+  if (!nlabel)
+    {
+      /* For nlabel == NULL_RTX cases, if reload_completed == 0,
+	 return/simple_return are not yet creatable, thus we return 0
+	 immediately;  if reload_completed, we do not accept !nlabel
+	 at all, either a non-null label, or return/simple_return RTX.
+	 In that case assert fail.  */
 
+      if (!reload_completed)
+	return 0;
+      gcc_unreachable ();
+    }
+
   if (nlabel == olabel)
     return 1;
 

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

[Eric Botcazou]

> 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 ();
+    }

-- 
Eric Botcazou

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

[Bernd Schmidt]

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

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

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On 2011/11/2 11:47 PM, Bernd Schmidt wrote:
> 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

(returning to this...)

I've re-tested using epilogue_completed to be sure, everything's clean
as expected. Just applied, attaching committed patch.

Thanks,
Chung-Lin

[-- Attachment #2: jump.diff --]
[-- Type: text/plain, Size: 1172 bytes --]

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 181662)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2011-11-23  Chung-Lin Tang  <cltang@codesourcery.com>
+
+	PR rtl-optimization/50496
+	* jump.c (redirect_jump): Assert fail on nlabel == NULL_RTX
+	only after epilogue is created. Add comments.
+
 2011-11-22  Richard Henderson  <rth@redhat.com>
 
 	* config/ia64/ia64.c (ia64_expand_atomic_op): Add model parameter.
Index: jump.c
===================================================================
--- jump.c	(revision 181662)
+++ jump.c	(working copy)
@@ -1495,8 +1495,19 @@
 {
   rtx olabel = JUMP_LABEL (jump);
 
-  gcc_assert (nlabel != NULL_RTX);
+  if (!nlabel)
+    {
+      /* If there is no label, we are asked to redirect to the EXIT block.
+	 When 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 (!epilogue_completed)
+	return 0;
+      gcc_unreachable ();
+    }
+
   if (nlabel == olabel)
     return 1;