[PATCH v2] Re: PR62304 (was Re: (Still) ICE for cris-elf at r214710)

Fri Aug 29 18:10:00 GMT 2014

On Fri, 2014-08-29 at 18:15 +0200, Hans-Peter Nilsson wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Date: Fri, 29 Aug 2014 17:33:54 +0200
> 
> > On Fri, 2014-08-29 at 16:48 +0200, Hans-Peter Nilsson wrote:
> > > Sorry, but that didn't help.  I still get the exact same error.
> > > (Yep, I double-checked that I didn't goof testing...)
> 
> Famous last words...
> 
> > Fully identical, or just the top 2 frames?  The error in the above
> > backtrace is the call to JUMP_LABEL_AS_INSN here:
> > 
> > > > 0x9119c2 find_dead_or_set_registers
> > > > >         /tmp/hpautotest-gcc1/gcc/gcc/resource.c:500
> > 
> > which I believe the patch removes.
> > 
> > That said, PR62304 has *two* backtraces: the one you posted earlier,
> > plus a similar-looking one due to a different overzealous cast by me at:
> > 0xae862f follow_jumps
> >         /vol/gcc/src/hg/trunk/local/gcc/reorg.c:2326
> > 
> > Maybe you're seeing that one?  (or a third...)
> 
> (Oh my, how embarrassing: by "exact same" I must have meant "in
> about the first 80 characters and the first frame".)
> 
> It seems it's a third one.  Yay for reorg.c.  Or rather, nay.
> 
> /tmp/pr62304/gcc/libgcc/libgcc2.c: In function '__absvsi2':
> /tmp/pr62304/gcc/libgcc/libgcc2.c:232:1: internal compiler error: in safe_as_a, at is-a.h:205
>  }
>  ^
> 0x90bb53 safe_as_a<rtx_insn*, rtx_def>
> 	/tmp/pr62304/gcc/gcc/is-a.h:205
> 0x90bb53 NEXT_INSN
> 	/tmp/pr62304/gcc/gcc/rtl.h:1338
> 0x90bb53 follow_jumps
> 	/tmp/pr62304/gcc/gcc/reorg.c:2315
> 0x90f50c relax_delay_slots
> 	/tmp/pr62304/gcc/gcc/reorg.c:3175
> 0x90f50c dbr_schedule
> 	/tmp/pr62304/gcc/gcc/reorg.c:3743
> 0x91088f rest_of_handle_delay_slots
> 	/tmp/pr62304/gcc/gcc/reorg.c:3885
> 0x91088f execute
> 	/tmp/pr62304/gcc/gcc/reorg.c:3916
> 
> For:
> int
> __absvsi2 (int a)
> {
>   int w = a;
>   if (a < 0)
>     w = -(unsigned int) a;
>   if (w < 0)
>     __builtin_abort ();
>    return w;
> }
> With "./cc1 -fpreprocessed -O2 this.i"

Yes: I made various mistakes in reorg.c and resource.c where I assumed
that a JUMP_LABEL(insn) was an insn, whereas the existing code is set up
to handle RETURN nodes.

BTW, in another email in the thread you said:

> Thanks for the heads-up.  BTW, the ChangeLog entries should say
> "what" not "why"; that goes into a comment in the source.

OK.   Where possible I've added comments in the new code, and chosen
variable names that express that we could be dealing with both insns and
RETURN nodes.  Much of the patch is simply reverting changes made
earlier.  Does it make sense to go in and add comments where I'm
reverting an change?  I felt that adding stuff to a ChangeLog makes
sense from a "belt and braces" POV; if we have to review the change in 6
months time, it's easier to have bonus "why", as it were.

The attached patch fixes both reproducers you posted (tested with
cris-elf), and the case seen in PR62304 (tested with
sparc64-sun-solaris2.10); bootstrap on x86_64 in progress.

It eliminates all uses of JUMP_LABEL_AS_INSN from reorg.c, and indeed
after that there are only 6 uses in the tree (including config subdirs).

2014-08-29  David Malcolm  <dmalcolm@redhat.com>

	PR bootstrap/62304

	* gcc/reorg.c (skip_consecutive_labels): Convert return type and
	param back from rtx_insn * to rtx.  Rename param from "label" to
	"label_or_return", reintroducing "label" as an rtx_insn * after
	we've ensured it's not a RETURN.
	(first_active_target_insn): Likewise for return type and param;
	add a checked cast to rtx_insn * once we've ensured "insn" is not
	a RETURN.
	(steal_delay_list_from_target): Convert param "pnew_thread" back
	from rtx_insn ** to rtx *.  Replace use of JUMP_LABEL_AS_INSN
	with JUMP_LABEL.
	(own_thread_p): Convert param "thread" back from an rtx_insn * to
	an rtx.  Introduce local rtx_insn * "thread_insn" with a checked
	cast once we've established we're not dealing with a RETURN,
	renaming subsequent uses of "thread" to "thread_insn".
	(fill_simple_delay_slots): Convert uses of JUMP_LABEL_AS_INSN back
	to JUMP_LABEL.
	(follow_jumps): Convert return type and param "label" from
	rtx_insn * back to rtx.  Move initialization of "value" to after
	the handling for ANY_RETURN_P, adding a checked cast there to
	rtx_insn *.  Convert local rtx_insn * "this_label" to an rtx and
	rename to "this_label_or_return", reintroducing "this_label" as
	an rtx_insn * once we've handled the case where it could be an
	ANY_RETURN_P.
	(fill_slots_from_thread): Rename param "thread" to
	"thread_or_return", converting from an rtx_insn * back to an rtx.
	Reintroduce name "thread" as an rtx_insn * local with a checked
	cast once we've handled the case of it being an ANY_RETURN_P.
	Convert local "new_thread" from an rtx_insn * back to an rtx.
	Add a checked cast when assigning to "trial" from "new_thread".
	Convert use of JUMP_LABEL_AS_INSN back to JUMP_LABEL.  Add a
	checked cast to rtx_insn * from "new_thread" when invoking
	get_label_before.
	(fill_eager_delay_slots): Convert locals "target_label",
	"insn_at_target" from rtx_insn * back to rtx.
	Convert uses of JUMP_LABEL_AS_INSN back to JUMP_LABEL.
	(relax_delay_slots): Convert locals "trial", "target_label" from
	rtx_insn * back to rtx.  Convert uses of JUMP_LABEL_AS_INSN back
	to JUMP_LABEL.  Add a checked cast to rtx_insn * on "trial" when
	invoking update_block.
	(dbr_schedule): Convert use of JUMP_LABEL_AS_INSN back to
	JUMP_LABEL; this removes all JUMP_LABEL_AS_INSN from reorg.c.

	* resource.h (mark_target_live_regs): Undo erroneous conversion
	of second param of r214693, converting it back from rtx_insn * to
	rtx, since it could be a RETURN.

	* resource.c (find_dead_or_set_registers): Similarly, convert
	param "jump_target" back from an rtx_insn ** to an rtx *, as we
	could be writing back a RETURN.  Rename local rtx_insn * "next" to
	"next_insn", and introduce "lab_or_return" as a local rtx,
	handling the case where JUMP_LABEL (this_jump_insn) is a RETURN.
	(mark_target_live_regs): Undo erroneous conversion
	of second param of r214693, converting it back from rtx_insn * to
	rtx, since it could be a RETURN.  Rename it from "target" to
	"target_maybe_return", reintroducing the name "target" as a local
	rtx_insn * with a checked cast, after we've handled the case of
	ANY_RETURN_P.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR62304-v2.patch
Type: text/x-patch
Size: 15232 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20140829/cef55474/attachment.bin>