Re: RFC: MIPS: Workaround the "daddi" and "daddiu" errata

[Richard Sandiford <rsandifo at redhat dot com>]

"Maciej W. Rozycki" <macro@ds2.pg.gda.pl> writes:
>  I'm not so sure.  I wanted to make absolutely sure the output is
> verifiable for correctness and this achieved.  You can run e.g. `objdump
> -S <file> | grep 'daddi[u]*' on a binary and see a problem immediately.

I don't agree that that's a good enough reason.  You can't generally
expect to verify compiler changes with a simple check like this.  For
one thing, even if the output is free of daddius, how do you know the
replacements work correctly?  You still have to test the binary at the
end of the day.

> It can also be handled quite easily by the kernel or the dynamic linker
> when mmap()ping executable pages -- simply by scanning for the bad
> opcodes.  I'd say your proposal is more fragile -- you have to study
> generated code thoroughly for every "daddiu" instruction to check if the
> erratum can be triggered.  And run-time validation is essentially
> impossible (we don't want to do static analysis in the kernel, do we?).

Are you talking about the kernel refusing to run suspect binaries?
Or at least warning about them somehow?  If so, ELF offers far better
ways of detecting a "safe" binary than scanning all executable pages.

>>   (2) Adding %lo(X) to the high part of X can only trigger case (-)
>>       since yyyy (the low 16 bits of src) will be 0.  In this case,
>>       X must be of the form 0x7fff_ffff_ffff_8???, which is an invalid
>>       address, right?  So there's no way this bug could trigger if X
>>       refers to a valid in-memory object.
>> 
>>       Of course, X could still be some kind of constant defined in a
>>       linker script, but, well... don't do that if you care about
>>       targets with a buggy daddiu.  It hardly seems to justify the
>>       kind of code bloat it causes in the common case, nor the big
>>       maintenance burden.
>> 
>>       The same argument applies when "src" contains the high part of
>>       X plus an index.  If X refers to an in-memory object, no valid
>>       index can cause the daddiu to overflow.
>> 
>>       Unfortunately, I don't know of any specific code in gcc to stop:
>> 
>>           &foo[y - x]
>> 
>>       from being expanded as:
>> 
>>           ...load high part of foo into dst...
>>           daddu dst,dst,y
>>           daddiu dst,dst,%lo(foo)
>>           dsubu dst,dst,x
>> 
>>       so perhaps we could trip over the errata when x and y are both
>>       humungous values.  But I don't think this case justifies treating
>>       "daddiu ...,%lo(...)" as a complete no-no.  Again, it leads to
>>       large code bloat and has a high maintenance burden.
>> 
>>       If you can demonstrate that &foo[y - x] is indeed a problem then
>>       I'd prefer a patch that addresses it head-on.
>
>  Note that even if the address is invalid, code should work correctly --
> the address may be further offsetted later.  Or code may expect a bad
> address and deal with it somehow.  Or there may be a real bug in code --
> it should really get an exception then, instead of silently using a 
> different address.

But you seem to be missing my point.  Within GCC, I think all symbolic
constants are associated with pointers.  I don't know of any well-defined
way of creating a legitimate address by using an offset from an invalid
pointer.  And besides, in the normal run of things, an invalid constant
address could only be defined explicitly by the user, perhaps using a
linker script.

>>   (3) In the case of ASM_OUTPUT_REG_PUSH, you're catering for case (-);
>>       i.e. the case where the stack pointer goes from 0x8000... to
>>       0x7fff....  Since this represents the stack pointer moving from
>>       xkphys to an invalid address, it surely represents a program gone
>>       wrong.  Even the original stack pointer value is a bit suspect ;)
>> 
>>       Perhaps you could argue that, since 0xffff... is a valid address,
>>       the behaviour of the broken daddiu is less friendly for error-detection
>>       and debugging purposes.  But I don't think that's enough to justify
>>       the extra gcc maintenance burden, nor the cost of using two instructions
>>       to allocate small chunks of stack.
>> 
>>       Similar arguments apply for the POP case.
>> 
>> Your patch also includes changes to patterns like ffsdi.  This seems
>> a bit over-the-top; there's no way counting from 0 to 64 can trigger
>> overflow.
>
>  It's just the consequence of getting rid of the instructions universally, 
> which was my goal.

But this for me is the main example of why this goal seems wrong.
We know full well that stack pointer allocations can safely be done
with daddiu.  Why double the number of instructions needed just so
that a simple grep will yield no matches?

>  If you still worry about maintenance hassle of the change, then feel free
> to offload it to me.  Though with an easy detectability of a breakage, as 
> demonstrated above, I'll probably be first to notice it and provide an 
> update.

That isn't really the point though.  The patch you posted would have made
it harder to maintain all targets, not just the ones with the bug.

Richard