Re: [PATCH] MIPS: Fix GCC `noreorder' for undefined R5900 short loops

[Fredrik Noring <noring@nocrew.org>]

Hi Maciej,

> > > > How can one reasonably prevent the bug when compiling a whole Linux
> > > > distribution with thousands of packages if GAS merely warns and proceeds
> > > > in many cases?
> > > 
> > >  I think the tools must not set a policy.  By using `.set noreorder' the 
> > > user told the toolchain he knows better and asked to keep its hands away.
> > > 
> > >  As I say you can use `-Werror' for code auditing.  And in most cases 
> > > manually scheduled delay slots in handcoded assembly are a sign of a 
> > > problem with the piece of code affected, regardless of the R5900 erratum.
> > 
> > Well, it seems practical to use unofficial tools and a patched GAS to fix
> > this assembly bug, then. It's a grave problem for the R5900 and it needs to
> > be reliably detected.
> 
>  Why?  What is wrong with using `-Werror'?
> 
>  Or you could use `-Wa,-fatal-warnings' to limit making warnings fatal to 
> the assembly stage only if you are concerned about bad high-level language 
> code quality interfering with your goal.

It appears unfeasible to fix thousands of build scripts to work like that.
Although the numbers with actual MIPS code in them likely are very small,
some may source macros and other stuff via various libraries. Some
distributions, such as Gentoo, do have global CFLAGS but such mechanism are
unreliable.

I understand that one may want to take a principled stance with "hands away"
and "author asked for trouble", but fact is that whomever put a given
noreorder directive into a piece of code most likely didn't have R5900
errata in mind, and R5900 users most likely don't want the assembler to
generate code that is known to cause subtle bugs of all imaginable kinds,
including data corruption.

Hence my recommendation. It's not really an argument because I'm not going
to ask that the official GAS changes behaviour on this.

> >  3:
> > +	short_loop_war(3b)
> >  	b	3b
> >  	 nop
> 
>  Is it needed here given that the delay slot instruction is a NOP anyway?  

Ah, no. That is a left-over from the note that "a branch delay slot of the
loop is not NOP (EE 2.9 or later)", as it appears a NOP is not enough for
EE before 2.9.

Perhaps the short_loop_war macro can be improved to give errors when
misused. For example, it could potentially give an error if it is placed
outside of noreorder, but it doesn't seem like GAS can inform the macro
whether it is inside or outside.

>  Also this source does not need `.set noreorder', except for the loop 
> around `1:' where a data dependency is present with the delay slot 
> instruction.

That data dependency can surely be reworked too, so as to make the whole
piece reorderable?

> And I am surprised that it even assembles as it uses 
> `.cprestore' without the required offset argument (not that this pseudo-op 
> makes any sense here).  And it's weird overall, e.g. it loads $ra 
> explicitly rather than using JALR (or JAL even).  But these are unrelated 
> problems.

Good to know, thanks. :)

Fredrik