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

[H.J. Lu]

> 
> > 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 like your patch. Jeff, can we use Bernd's version?

> 
> 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?
> 

We have seen/fixed 2 long standing x86 fp bugs so far :-(.

Thanks.


H.J.