| Message ID | 1407345815-14551-36-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/06/14 11:20, David Malcolm wrote: > gcc/ > * rtl.h (unlink_insn_chain): Strengthen return type from rtx to > rtx_insn *. > (duplicate_insn_chain): Likewise. > * cfgrtl.c (unlink_insn_chain): Strengthen return type from rtx to > rtx_insn *, also for locals "prevfirst" and "nextlast". Add a > checked cast for now (until we can strengthen the params in the > same way). > (duplicate_insn_chain): Likewise. OK. jeff
On Wed, 2014-08-13 at 11:56 -0600, Jeff Law wrote: > On 08/06/14 11:20, David Malcolm wrote: > > gcc/ > > * rtl.h (unlink_insn_chain): Strengthen return type from rtx to > > rtx_insn *. > > (duplicate_insn_chain): Likewise. > > * cfgrtl.c (unlink_insn_chain): Strengthen return type from rtx to > > rtx_insn *, also for locals "prevfirst" and "nextlast". Add a > > checked cast for now (until we can strengthen the params in the > > same way). > > (duplicate_insn_chain): Likewise. > OK. Thanks. Committed to trunk as r214197, having bootstrapped®rtested on x86_64-unknown-linux-gnu (Fedora 20) albeit in combination with patches 30-39, and verified that it builds standalone on several targets.
David Malcolm <dmalcolm@redhat.com> writes: > @@ -4083,7 +4083,7 @@ cfg_layout_can_duplicate_bb_p (const_basic_block bb) > return true; > } > > -rtx > +rtx_insn * > duplicate_insn_chain (rtx from, rtx to) > { > rtx insn, next, copy; > @@ -4169,7 +4169,7 @@ duplicate_insn_chain (rtx from, rtx to) > } > insn = NEXT_INSN (last); > delete_insn (last); > - return insn; > + return as_a <rtx_insn *> (insn); This is wrong, insn may be NULL. Andreas.
On Wed, 2014-08-20 at 10:20 +0200, Andreas Schwab wrote: > David Malcolm <dmalcolm@redhat.com> writes: > > > @@ -4083,7 +4083,7 @@ cfg_layout_can_duplicate_bb_p (const_basic_block bb) > > return true; > > } > > > > -rtx > > +rtx_insn * > > duplicate_insn_chain (rtx from, rtx to) > > { > > rtx insn, next, copy; > > @@ -4169,7 +4169,7 @@ duplicate_insn_chain (rtx from, rtx to) > > } > > insn = NEXT_INSN (last); > > delete_insn (last); > > - return insn; > > + return as_a <rtx_insn *> (insn); > > This is wrong, insn may be NULL. Thanks; and indeed, this broke the bootstrap, as noted in: https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01964.html Fixed in r214207: https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01971.html Sorry. I'm trying to figure out why this didn't break during my testing. Patch #60 of the original series contained an equivalent as_a -> as_a_nullable hunk: https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00708.html - albeit with a misleading ChangeLog entry (I'm assuming I messed that up during a rebase) - but this hadn't been applied yet. Dave
On Wed, 2014-08-20 at 06:19 -0400, David Malcolm wrote: > On Wed, 2014-08-20 at 10:20 +0200, Andreas Schwab wrote: > > David Malcolm <dmalcolm@redhat.com> writes: > > > > > @@ -4083,7 +4083,7 @@ cfg_layout_can_duplicate_bb_p (const_basic_block bb) > > > return true; > > > } > > > > > > -rtx > > > +rtx_insn * > > > duplicate_insn_chain (rtx from, rtx to) > > > { > > > rtx insn, next, copy; > > > @@ -4169,7 +4169,7 @@ duplicate_insn_chain (rtx from, rtx to) > > > } > > > insn = NEXT_INSN (last); > > > delete_insn (last); > > > - return insn; > > > + return as_a <rtx_insn *> (insn); > > > > This is wrong, insn may be NULL. > > Thanks; and indeed, this broke the bootstrap, as noted in: > https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01964.html > > Fixed in r214207: > https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01971.html ...and this has subsequently been filed as PR62203 (retitling accordingly). > Sorry. I'm trying to figure out why this didn't break during my > testing. Patch #60 of the original series contained an equivalent as_a > -> as_a_nullable hunk: > https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00708.html > - albeit with a misleading ChangeLog entry (I'm assuming I messed that > up during a rebase) - but this hadn't been applied yet. I just reran a bootstrap with patches 4-35 containing the unsafe as_a cast from #35, on top of r213973 (from 2014-08-14), and somehow it successfully bootstrapped and regression tested on x86_64-unknown-linux-gnu (Fedora 20). r213973 has been the baseline I've been testing and patching against. So presumably something changed *since* r213973 to make the bug in patch #35 show itself when applied on a more recent trunk (I was able to reproduce the bug on the same machine, with the same configure flags). Bother. I'll rebase against a more recent trunk and retest accordingly for the followup patches. I'll also take another look at the patches that add as_a<> (as opposed to safe_as_a<>) [Does this make a case for adding the safety check to the is_a_helper functions, and dropping safe_as_a variant altogether?] Sorry about this Dave
On Wed, 2014-08-20 at 12:09 -0400, David Malcolm wrote: > On Wed, 2014-08-20 at 06:19 -0400, David Malcolm wrote: > > On Wed, 2014-08-20 at 10:20 +0200, Andreas Schwab wrote: > > > David Malcolm <dmalcolm@redhat.com> writes: > > > > > > > @@ -4083,7 +4083,7 @@ cfg_layout_can_duplicate_bb_p (const_basic_block bb) > > > > return true; > > > > } > > > > > > > > -rtx > > > > +rtx_insn * > > > > duplicate_insn_chain (rtx from, rtx to) > > > > { > > > > rtx insn, next, copy; > > > > @@ -4169,7 +4169,7 @@ duplicate_insn_chain (rtx from, rtx to) > > > > } > > > > insn = NEXT_INSN (last); > > > > delete_insn (last); > > > > - return insn; > > > > + return as_a <rtx_insn *> (insn); > > > > > > This is wrong, insn may be NULL. > > > > Thanks; and indeed, this broke the bootstrap, as noted in: > > https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01964.html > > > > Fixed in r214207: > > https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01971.html > > ...and this has subsequently been filed as PR62203 (retitling > accordingly). > > > Sorry. I'm trying to figure out why this didn't break during my > > testing. Patch #60 of the original series contained an equivalent as_a > > -> as_a_nullable hunk: > > https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00708.html > > - albeit with a misleading ChangeLog entry (I'm assuming I messed that > > up during a rebase) - but this hadn't been applied yet. > > I just reran a bootstrap with patches 4-35 containing the unsafe as_a > cast from #35, on top of r213973 (from 2014-08-14), and somehow it > successfully bootstrapped and regression tested on > x86_64-unknown-linux-gnu (Fedora 20). r213973 has been the baseline > I've been testing and patching against. > > So presumably something changed *since* r213973 to make the bug in patch > #35 show itself when applied on a more recent trunk (I was able to > reproduce the bug on the same machine, with the same configure flags). > > Bother. I figured this out: my testing bootstrap had a stray --enable-checking=release which meant that the as_a wasn't running the is_a_helper, and hence gracefully handled a NULL insn. Fixed now in my testing setup, and am rerunning bootstrap for the followup patches. Sorry again. > I'll rebase against a more recent trunk and retest accordingly for the > followup patches. I'll also take another look at the patches that add > as_a<> (as opposed to safe_as_a<>) [Does this make a case for adding the > safety check to the is_a_helper functions, and dropping safe_as_a > variant altogether?] > > Sorry about this > Dave
diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c index 9f15a7d..5611ab8 100644 --- a/gcc/cfgrtl.c +++ b/gcc/cfgrtl.c @@ -3304,11 +3304,11 @@ fixup_abnormal_edges (void) /* Cut the insns from FIRST to LAST out of the insns stream. */ -rtx +rtx_insn * unlink_insn_chain (rtx first, rtx last) { - rtx prevfirst = PREV_INSN (first); - rtx nextlast = NEXT_INSN (last); + rtx_insn *prevfirst = PREV_INSN (first); + rtx_insn *nextlast = NEXT_INSN (last); SET_PREV_INSN (first) = NULL; SET_NEXT_INSN (last) = NULL; @@ -3320,7 +3320,7 @@ unlink_insn_chain (rtx first, rtx last) set_last_insn (prevfirst); if (!prevfirst) set_first_insn (nextlast); - return first; + return as_a <rtx_insn *> (first); } /* Skip over inter-block insns occurring after BB which are typically @@ -4083,7 +4083,7 @@ cfg_layout_can_duplicate_bb_p (const_basic_block bb) return true; } -rtx +rtx_insn * duplicate_insn_chain (rtx from, rtx to) { rtx insn, next, copy; @@ -4169,7 +4169,7 @@ duplicate_insn_chain (rtx from, rtx to) } insn = NEXT_INSN (last); delete_insn (last); - return insn; + return as_a <rtx_insn *> (insn); } /* Create a duplicate of the basic block BB. */ diff --git a/gcc/rtl.h b/gcc/rtl.h index b4027aa..2cce7d4 100644 --- a/gcc/rtl.h +++ b/gcc/rtl.h @@ -3072,7 +3072,7 @@ extern void delete_insn (rtx); extern rtx_insn *entry_of_function (void); extern void emit_insn_at_entry (rtx); extern void delete_insn_chain (rtx, rtx, bool); -extern rtx unlink_insn_chain (rtx, rtx); +extern rtx_insn *unlink_insn_chain (rtx, rtx); extern void delete_insn_and_edges (rtx); extern rtx gen_lowpart_SUBREG (enum machine_mode, rtx); extern rtx gen_const_mem (enum machine_mode, rtx); @@ -3148,7 +3148,7 @@ extern int fixup_args_size_notes (rtx, rtx, int); /* In cfgrtl.c */ extern void print_rtl_with_bb (FILE *, const_rtx, int); -extern rtx duplicate_insn_chain (rtx, rtx); +extern rtx_insn *duplicate_insn_chain (rtx, rtx); /* In expmed.c */ extern void init_expmed (void);