diff mbox

Fix PR56131 - gcc.dg/pr56035.c ICEs gcc on sparc-linux

Message ID 5110D3DF.2050904@mentor.com
State New
Headers show

Commit Message

Tom de Vries Feb. 5, 2013, 9:41 a.m. UTC 
Eric,

thanks for the review.

On 05/02/13 10:02, Eric Botcazou wrote:
>> The problem is that in delete_insn, while deleting an undeletable label (in
>> other words, transforming a label into a INSN_NOTE_DELETED_LABEL):
>> - we try to find the bb of a NOTE_INSN_BASIC_BLOCK following the label using
>> BLOCK_FOR_INSN (which is NULL) instead of NOTE_BASIC_BLOCK (which is not
>> NULL), and
>> - we don't handle the case that we find a NULL pointer for that bb
>> gracefully.
> 
> Can the NOTE_BASIC_BLOCK of a NOTE_INSN_BASIC_BLOCK really be NULL?
> 

I don't know, I was just trying to do defensive programming.

>> This comment in insn-notes.def tells us that the bb info could be in either
>> of the two:
>> ...
>> /* Record the struct for the following basic block.  Uses
>>    NOTE_BASIC_BLOCK.  FIXME: Redundant with the basic block pointer
>>    now included in every insn.  */
>> INSN_NOTE (BASIC_BLOCK)
>> ...
>>
>> The patch fixes the problems by:
>> - using NOTE_BASIC_BLOCK to find the bb of NOTE_INSN_BASIC_BLOCK, and
>> - explicitly handling the cases that the bb of either the label or the note
>> is NULL.
> 
> I don't think that we need to handle the very last case.
> 

OK.

>> 2013-02-04  Tom de Vries  <tom@codesourcery.com>
>>
>> 	PR rtl-optimization/56131
>> 	* cfgrtl.c (delete_insn): Use NOTE_BASIC_BLOCK instead of BLOCK_FOR_INSN
>> 	to get the bb of a NOTE_INSN_BASIC_BLOCK.  Handle the case that the bb
>> 	of the label is NULL.  Add comment.
> 
> OK on principle, but remove the handling of the NULL NOTE_BASIC_BLOCK and keep 
> everything in a single condition.
> 

I've adopted the patch according to your comments, rebuild sparc-linux cc1 and
tested that the ICE does not occur.

I'll bootstrap and retest on x86_64 and check in attached patch.

Thanks,
- Tom

Comments

Jakub Jelinek Feb. 5, 2013, 9:50 a.m. UTC | #1 
On Tue, Feb 05, 2013 at 10:41:51AM +0100, Tom de Vries wrote:
> On 05/02/13 10:02, Eric Botcazou wrote:
> >> The problem is that in delete_insn, while deleting an undeletable label (in
> >> other words, transforming a label into a INSN_NOTE_DELETED_LABEL):
> >> - we try to find the bb of a NOTE_INSN_BASIC_BLOCK following the label using
> >> BLOCK_FOR_INSN (which is NULL) instead of NOTE_BASIC_BLOCK (which is not
> >> NULL), and
> >> - we don't handle the case that we find a NULL pointer for that bb
> >> gracefully.
> > 
> > Can the NOTE_BASIC_BLOCK of a NOTE_INSN_BASIC_BLOCK really be NULL?
> > 
> 
> I don't know, I was just trying to do defensive programming.

I'd say the bug is in whatever made the note not contain the appropriate bb.

	Jakub
Tom de Vries Feb. 5, 2013, 10:01 a.m. UTC | #2 
On 05/02/13 10:50, Jakub Jelinek wrote:
> On Tue, Feb 05, 2013 at 10:41:51AM +0100, Tom de Vries wrote:
>> On 05/02/13 10:02, Eric Botcazou wrote:
>>>> The problem is that in delete_insn, while deleting an undeletable label (in
>>>> other words, transforming a label into a INSN_NOTE_DELETED_LABEL):
>>>> - we try to find the bb of a NOTE_INSN_BASIC_BLOCK following the label using
>>>> BLOCK_FOR_INSN (which is NULL) instead of NOTE_BASIC_BLOCK (which is not
>>>> NULL), and
>>>> - we don't handle the case that we find a NULL pointer for that bb
>>>> gracefully.
>>>
>>> Can the NOTE_BASIC_BLOCK of a NOTE_INSN_BASIC_BLOCK really be NULL?
>>>
>>
>> I don't know, I was just trying to do defensive programming.
> 
> I'd say the bug is in whatever made the note not contain the appropriate bb.
> 

Jakub,

I'm not sure I understand your comment.

The BLOCK_FOR_INSN of the note was NULL. The NOTE_BASIC_BLOCK of the note was
correct. Are you saying that the BLOCK_FOR_INSN should not have been NULL?

Thanks,
- Tom

> 	Jakub
>
Jakub Jelinek Feb. 5, 2013, 10:12 a.m. UTC | #3 
On Tue, Feb 05, 2013 at 11:01:22AM +0100, Tom de Vries wrote:
> I'm not sure I understand your comment.
> 
> The BLOCK_FOR_INSN of the note was NULL. The NOTE_BASIC_BLOCK of the note was
> correct. Are you saying that the BLOCK_FOR_INSN should not have been NULL?

Yeah, I mean the following invariant should hold IMHO:
!NOTE_INSN_BASIC_BLOCK_P (insn) || NOTE_BASIC_BLOCK (insn) == BLOCK_FOR_INSN (insn)
NOTE_INSN_BASIC_BLOCK for some bb outside of that bb?  That looks fishy.
Haven't bootstrapped/regtested with such a check anywhere, just compiled one
largish C++ testcase with it.

	Jakub
diff mbox

Patch

Index: gcc/cfgrtl.c
===================================================================
--- gcc/cfgrtl.c (revision 195747)
+++ gcc/cfgrtl.c (working copy)
@@ -135,7 +135,7 @@  delete_insn (rtx insn)
       if (! can_delete_label_p (insn))
 	{
 	  const char *name = LABEL_NAME (insn);
-	  basic_block bb = BLOCK_FOR_INSN (insn);
+	  basic_block bb, label_bb = BLOCK_FOR_INSN (insn);
 	  rtx bb_note = NEXT_INSN (insn);
 
 	  really_delete = false;
@@ -143,10 +143,16 @@  delete_insn (rtx insn)
 	  NOTE_KIND (insn) = NOTE_INSN_DELETED_LABEL;
 	  NOTE_DELETED_LABEL_NAME (insn) = name;
 
-	  if (bb_note != NULL_RTX && NOTE_INSN_BASIC_BLOCK_P (bb_note)
-	      && BLOCK_FOR_INSN (bb_note) == bb)
+	  /* If the note following the label starts a basic block, and the
+	     label is a member of the same basic block, interchange the two.
+	     If the label is not marked with a bb, assume it's the same bb.  */
+	  if (bb_note != NULL_RTX
+	      && NOTE_INSN_BASIC_BLOCK_P (bb_note)
+	      && (label_bb == NOTE_BASIC_BLOCK (bb_note)
+		  || label_bb == NULL))
 	    {
 	      reorder_insns_nobb (insn, insn, bb_note);
+	      bb = NOTE_BASIC_BLOCK (bb_note);
 	      BB_HEAD (bb) = bb_note;
 	      if (BB_END (bb) == bb_note)
 		BB_END (bb) = insn;