Message ID | 5110D3DF.2050904@mentor.com |
---|---|
State | New |
Headers | show |
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
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 >
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
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;