Re: egcs 1.0.2 is broken on x86 and a patch for it.

[Bernd Schmidt <crux@pool.informatik.rwth-aachen.de>]

> This is an extremely confusing explanation, primarily because you
> use "insn"/INSN to refer to three different things.
> 
> Basically is sounds like you have
> 
> 	insn 1
>         insn 2
> 
> 
> It sounds like we thought we wanted to insert after insn 1, but
> because of other reg-stack actions we really wanted to insert
> after insn 2.

> I'm going to go ahead and hesitantly install the change, but further
> comments from folks that understand this code would be greatly
> appreciated.

Here's how I see things.
In convert_regs, subst_stack_regs is called in a loop for each insn in
a basic block. subst_stack_regs makes sure that the insn is valid, by
substituting insns in it and by emitting additional insns around it.
It then updates the information in the regstack variable to match the
state of the stack _after all the insns it has emitted_. This means that
the loop in convert_regs handles things properly, since it takes NEXT_INSN
before calling subst_stack_regs, so it will use the proper insn in the
next iteration.
It then fails to recognize the case when subst_stack_regs had to emit
something after the last insn in the basic block. It will use the insn
that used to be the last one instead of one of those which subst_stack_regs
emitted after it, but this is inconsistent with the information in
reg_stack.

I think that all of the further calls in convert_regs need to use
PREV_INSN (next) instead of insn. This is essentially what HJ's two patches
achieve, although it can be done with a smaller patch (see below).
The question is what do to about this piece of code:

      if (GET_CODE (insn) == JUMP_INSN)
        goto_block_pat (insn, &regstack, PATTERN (insn));

subst_stack_regs will never emit a jump insn, so this test is not meaningful
for an instruction emitted by subst_stack_regs. I think that it is illegal
for subst_stack_regs to emit anything after a JUMP_INSN (just like you can't
have output reloads for a jump). So I think it's safe to abort if
  (GET_CODE (insn) == JUMP_INSN && insn != PREV_INSN (next))

The patch below, which applies against egcs-1.0.2, fixes HJ's test case and
does not introduce additional failures (in the egcs-1.0.2 test suite).
It should be almost equivalent to HJ's two patches applied on top of each
other, with the exception of the new sanity test (I'm testing whether INSN
is a JUMP, while he tests NEW, but since the only function in reg-stack
that can emit a jump is goto_block_pat, that can obviously never be true).
I hope that INSN can also never be true, that's why I abort in that case.

I'm just a bit surprised that this bug never showed up before. It can't
be that exotic to have a floating point instruction as the last one in
a basic block, can it?

Bernd

	* reg-stack.c (convert_regs): Make sure that the insn we use when
	processing the block exit is always the last one in the block.

*** ./reg-stack.c.orig-1	Wed Apr 22 10:15:16 1998
--- ./reg-stack.c	Wed Apr 22 10:19:49 1998
*************** convert_regs ()
*** 3097,3102 ****
--- 3097,3114 ----
  	    subst_stack_regs (insn, &regstack);
  
  	} while (insn != block_end[block]);
+       
+       /* For all further actions, INSN needs to be the last insn in
+          this basic block.  If subst_stack_regs inserted additional
+          instructions after INSN, it is no longer the last one at
+          this point.  */
+       next = PREV_INSN (next);
+ 
+       /* If subst_stack_regs inserted something after a JUMP_INSN, that
+          is almost certainly a bug.  */
+       if (GET_CODE (insn) == JUMP_INSN && insn != next)
+ 	abort ();
+       insn = next;
  
        /* Something failed if the stack life doesn't match.  */