Committed: verify_flow_info fixes exposed by libgfortran (at least on MMIX)

Committed: verify_flow_info fixes exposed by libgfortran (at least on MMIX)

[Hans-Peter Nilsson]

I worked around the stdint.h issue for libgfortran cross to newlib
targets, specifically mmix-knuth-mmixware.  A build then crashes for
really bad reasons:

/home/hp/builds2/mmixware-sim/gcc/xgcc -B/home/hp/builds2/mmixware-sim/gcc/ -nostdinc -B/home/hp/builds2/mmixware-sim/mmix/newlib\
/ -isystem /home/hp/builds2/mmixware-sim/mmix/newlib/targ-include -isystem /home/hp/cvs_areas/combined/cvs_write/newlib/libc/incl\
ude -B/home/hp/builds2/mmixware-sim/../install-mmix/mmix/bin/ -B/home/hp/builds2/mmixware-sim/../install-mmix/mmix/lib/ -isystem \
/home/hp/builds2/mmixware-sim/../install-mmix/mmix/include -isystem /home/hp/builds2/mmixware-sim/../install-mmix/mmix/sys-includ\
e -L/home/hp/builds2/mmixware-sim/ld -DHAVE_CONFIG_H -I. -I/home/hp/cvs_areas/combined/cvs_write/libgfortran -I. -I/home/hp/cvs_a\
reas/combined/cvs_write/libgfortran/io -O2 -g -O2 -std=gnu99 -O2 -g -O2 -c /home/hp/cvs_areas/combined/cvs_write/libgfortran/io/l\
ist_read.c -o list_read.o
(barrier 89 87 88)
/home/hp/cvs_areas/combined/cvs_write/libgfortran/io/list_read.c: In function 'next_char':
/home/hp/cvs_areas/combined/cvs_write/libgfortran/io/list_read.c:160: internal compiler error: Segmentation fault

There are two bugs here.  The segmentation fault is because BLOCK_FOR_INSN
for a barrier is never set; it contains 0xafafafaf GGC markers.  Thus the
debug code, handling NULL:s but not bad pointers, crashes referencing
(struct basic_block_def *) 0xafafafaf trying to emit debug-output for that
bb.  Comparing with emit_note, it's obvious that BLOCK_FOR_INSN for
barriers should be set (well, cleared) at creation of barriers.  Code that
is supposed to set BLOCK_FOR_INSN skips barriers.

The other bug is that rtl_verify_flow_info_1 doesn't allow a barrier in a
bb.  It's obvious from looking at control_flow_insn_p (called in another
iteration over insns in a bb in rtl_verify_flow_info_1) that barriers are
valid here:
    case BARRIER:
      /* It is nonsense to reach barrier when looking for the
         end of basic block, but before dead code is eliminated
         this may happen.  */
Which is spot on.

The actual reason there's a barrier in this bb is that "call" for mmix
expands to two insns: the call itself and a return-address-register
restore.  The call is to longjmp, so there's a barrier (validly) emitted
after the actual call insn.  Because the barrier is not *last* in the
sequence, it is not skipped when setting the bb head and end pointers.
This is at tree-expand-time, so not reached dead-code-elimination yet.

This problem was harder to debug than necessary.  I was intent on blaming
the new table-driven passes structure, at least in part: the RTL passes
seemed hidden in some pass_expand structure iterated over, so I couldn't
"(gdb) up" from a SEGV or abort to see which specific pass that was
responsible.  Looking closer, it seems this only happens for the initial
rtl pass (now called "expand") and rest_of_compilation each; the RTL
passes called from rest_of_compilation are ok.  Never mind...

Still, verify_flow_info really should not be called before dumping
the initial RTL.  In this case, a call-flow bug (rather, checking-bug)
meant there was no initial RTL dump to inspect.  I think the
verify_flow_info call from tree_expand_cfg can be moved to the next pass
(right before its initialization), or to a pass *after* the initial RTL
expansion.  BTW, the initial RTL dump file should be named 00.rtl, not
00.expand: there are no trees in there to excuse the name change.  (That
might be unintended, but it doesn't seem like anybody has cared to make
that intention happen: there are no trees dumped there, so the name should
be changed back.  I intend to do that. ;-)

Built and tested native i686-pc-linux-gnu (FC2) (no Ada) and the cross to
mmix-knuth-mmixware now completes building.  As explained, I consider the
corrections to initialization and checking obvious, so committed as such.

	* cfgrtl.c (rtl_verify_flow_info_1): When checking insns in a bb,
	handle barriers in a bb by checking that it points to a NULL bb.
	* emit-rtl.c (emit_barrier_before): Set BLOCK_FOR_INSN to NULL.
	(emit_barrier_after, emit_barrier): Ditto.

Index: cfgrtl.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cfgrtl.c,v
retrieving revision 1.136
diff -p -c -r1.136 cfgrtl.c
*** cfgrtl.c	30 Sep 2004 21:25:44 -0000	1.136
--- cfgrtl.c	17 Oct 2004 21:22:48 -0000
*************** rtl_verify_flow_info_1 (void)
*** 2073,2079 ****
  	}

        for (x = BB_HEAD (bb); x != NEXT_INSN (BB_END (bb)); x = NEXT_INSN (x))
! 	if (BLOCK_FOR_INSN (x) != bb)
  	  {
  	    debug_rtx (x);
  	    if (! BLOCK_FOR_INSN (x))
--- 2073,2082 ----
  	}

        for (x = BB_HEAD (bb); x != NEXT_INSN (BB_END (bb)); x = NEXT_INSN (x))
! 	/* We may have a barrier inside a basic block before dead code
! 	   elimination.  They always have a NULL BLOCK_FOR_INSN.  */
! 	if (BLOCK_FOR_INSN (x) != bb
! 	    && !(BARRIER_P (x) && BLOCK_FOR_INSN (x) == NULL))
  	  {
  	    debug_rtx (x);
  	    if (! BLOCK_FOR_INSN (x))
Index: emit-rtl.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/emit-rtl.c,v
retrieving revision 1.419
diff -p -c -r1.419 emit-rtl.c
*** emit-rtl.c	8 Oct 2004 19:59:22 -0000	1.419
--- emit-rtl.c	17 Oct 2004 21:22:53 -0000
*************** emit_barrier_before (rtx before)
*** 4057,4062 ****
--- 4057,4063 ----
    rtx insn = rtx_alloc (BARRIER);

    INSN_UID (insn) = cur_insn_uid++;
+   BLOCK_FOR_INSN (insn) = NULL;

    add_insn_before (insn, before);
    return insn;
*************** emit_barrier_after (rtx after)
*** 4272,4277 ****
--- 4273,4279 ----
    rtx insn = rtx_alloc (BARRIER);

    INSN_UID (insn) = cur_insn_uid++;
+   BLOCK_FOR_INSN (insn) = NULL;

    add_insn_after (insn, after);
    return insn;
*************** emit_barrier (void)
*** 4668,4673 ****
--- 4670,4676 ----
  {
    rtx barrier = rtx_alloc (BARRIER);
    INSN_UID (barrier) = cur_insn_uid++;
+   BLOCK_FOR_INSN (barrier) = NULL;
    add_insn (barrier);
    return barrier;
  }

brgds, H-P

Re: Committed: verify_flow_info fixes exposed by libgfortran (at least on MMIX)

[Eric Botcazou]

> Built and tested native i686-pc-linux-gnu (FC2) (no Ada) and the cross to
> mmix-knuth-mmixware now completes building.  As explained, I consider the
> corrections to initialization and checking obvious, so committed as such.
>
> 	* cfgrtl.c (rtl_verify_flow_info_1): When checking insns in a bb,
> 	handle barriers in a bb by checking that it points to a NULL bb.
> 	* emit-rtl.c (emit_barrier_before): Set BLOCK_FOR_INSN to NULL.
> 	(emit_barrier_after, emit_barrier): Ditto.

Tested without RTL checking I presume:

+===========================GNAT BUG DETECTED==============================+
| 4.0.0 20041019 (experimental) (sparc-sun-solaris2.8) GCC error:          |
| RTL check: expected elt 3 type 'B', have '0' (rtx barrier) in            |
|    emit_barrier, at emit-rtl.c:4673                                      |
| No source file position information available                            |
| Please submit a bug report; see http://gcc.gnu.org/bugs.html.            |
| Include the entire contents of this bug box in the report.               |
| Include the exact gcc or gnatmake command that you entered.              |
| Also include sources listed below in gnatchop format                     |
| (concatenated together with no headers between files).                   |
+==========================================================================+


-- 
Eric Botcazou

Re: Committed: verify_flow_info fixes exposed by libgfortran (at least on MMIX)

[Hans-Peter Nilsson]

On Tue, 19 Oct 2004, Eric Botcazou wrote:
> > Built and tested native i686-pc-linux-gnu (FC2) (no Ada) and the cross to
> > mmix-knuth-mmixware now completes building.  As explained, I consider the
> > corrections to initialization and checking obvious, so committed as such.
> >
> > 	* cfgrtl.c (rtl_verify_flow_info_1): When checking insns in a bb,
> > 	handle barriers in a bb by checking that it points to a NULL bb.
> > 	* emit-rtl.c (emit_barrier_before): Set BLOCK_FOR_INSN to NULL.
> > 	(emit_barrier_after, emit_barrier): Ditto.
>
> Tested without RTL checking I presume:

No, but without Ada, as I mentioned and as you quote above.
Still, the code I changed was only to checking parts (which
failed before) and initializing where things weren't
initialized, so if there's a bug, it is elsewhere.

When you compile with my patch reverted, what happens?

brgds, H-P

Re: Committed: verify_flow_info fixes exposed by libgfortran (at least on MMIX)

[Hans-Peter Nilsson]

On Tue, 19 Oct 2004, Eric Botcazou wrote:
> Tested without RTL checking I presume:
>
> +===========================GNAT BUG DETECTED==============================+
> | 4.0.0 20041019 (experimental) (sparc-sun-solaris2.8) GCC error:          |
> | RTL check: expected elt 3 type 'B', have '0' (rtx barrier) in            |

Oh, now I see what you mean:

DEF_RTL_EXPR(BARRIER, "barrier", "iuu000000", RTX_EXTRA)
DEF_RTL_EXPR(INSN, "insn", "iuuBieiee", RTX_INSN)

Bummer.  I won't be able to build for a few hours, so if you can
bootstrap with my patch reverted on some target (but *with
rtlchecking*), I'm happy with reverting it and will submit a
better patch.  Or maybe it's safer to let barriers have a bb
pointer?

BTW, I used configure with no enable/disable options when
testing.  I guess building with rtl-checking enabled for a few
targets would show interesting things.

brgds, H-P

Re: Committed: verify_flow_info fixes exposed by libgfortran (at least on MMIX)

[Eric Botcazou]

> Bummer.  I won't be able to build for a few hours, so if you can
> bootstrap with my patch reverted on some target (but *with
> rtlchecking*), I'm happy with reverting it and will submit a
> better patch.

I bootstrapped with RTL checking a tree dated 10/17 on i486 yesterday.

> Or maybe it's safer to let barriers have a bb 
> pointer?

I don't really know.  If you think it's the best solution.

> BTW, I used configure with no enable/disable options when
> testing.  I guess building with rtl-checking enabled for a few
> targets would show interesting things.

I regularly bootstrap with RTL checking on amd64.  I'm under the impression 
I'm not the only one (Andreas Jaeger from SuSE comes to mind).

-- 
Eric Botcazou

Re: Committed: verify_flow_info fixes exposed by libgfortran (at least on MMIX)

[Graham Stott]

 --- Hans-Peter Nilsson <hp@bitrange.com> wrote: 
> BTW, I used configure with no enable/disable options when
> testing.  I guess building with rtl-checking enabled for a few
> targets would show interesting things.
> 
Actually it's only the rarely use backends that show anything insteresting
with rtl-checking enabled.

FWIW I always bootstrap i686-pc-linux-gnu all languages (including Ada) with
all checking enabled with the exception of GCAC.

My last bootstrap was performed on Sunday.

I also do various crosses also with the same checking enabled and most
of those also build without problem. The vax backend currently doesn't
build due to issues with CASE_DROPS_THROUGH
> brgds, H-P
>

Graham  

Re: Committed: verify_flow_info fixes exposed by libgfortran (at least on MMIX)

[Eric Botcazou]

> Bummer.  I won't be able to build for a few hours, so if you can
> bootstrap with my patch reverted on some target (but *with
> rtlchecking*), I'm happy with reverting it and will submit a
> better patch.

Done on i486-mandrake-linux-gnu.

-- 
Eric Botcazou