gcc miscompiles csum_tcpudp_magic() on ARMv5

[Måns Rullgård]

Russell King - ARM Linux <linux at arm.linux.org.uk> writes:

> On Thu, Dec 12, 2013 at 04:04:17PM +0100, Maxime Bizon wrote:
>> 
>> On Thu, 2013-12-12 at 14:48 +0000, Russell King - ARM Linux wrote:
>> 
>> > Even so, the code _is_ buggy, because if the protocol value had bits
>> > 15-8 set, then this would go wrong for all the same reasons that
>> > you've found.  GCC is definitely ignoring the outter (uint16_t) cast
>> > in the above.
>> 
>> ok I'll file a gcc bug report to get their input on this.
>
> Actually, it may not be a gcc bug at all.
>
> One of the issues here is that internally, GCC tracks the "machine mode"
> of a register, where "mode" basically means the type of register it is.
> In this case, the register will be in "half integer" mode when it is
> passed into the assembler, and gcc does _not_ promote it.
>
> We can see this in the RTL:
>
> (insn 11 10 12 3 ../t.c:9 (set (reg:SI 143 [ sum ])
>         (asm_operands:SI ("mov  %0, %1") ("=&r") 0 [
>                 (subreg:HI (reg:SI 148) 0)
>             ]
>              [
>                 (asm_input:HI ("r") (null):0)
>             ]
>              [] ../t.c:12)) -1 (nil))
>
> Note that "HI" for the input register 148.  What this means is that the
> compiler "knows" that it's supposed to be a 16-bit quantity, and has
> recorded that fact, and _will_ truncate it down to 16-bit when it needs
> to be promoted.
>
> Since the asm() only asks for a "register", that's what we get - a
> register which _happens_ to be in HI mode containing the value.  However,
> there's no way to tell that from the assembly code itself (GCC doesn't
> have the facility to do conditional assembly generation depending on
> the argument type passed into it.)
>
> What this means is that passing "unsigned short"s into an asm is dodgy.

If that's the intended behaviour, the compiler really ought to error or
warn when this happens.  It's far too easy to accidentally fall foul of
this.

> What's more, I've just had this confirmed by compiler people in ARM Ltd -
> we can't rely on the state of the upper 16 bits when passing a u16 into
> an asm().  So, it seems that it _is_ a kernel bug after all.

With that being the official stance, the proper fix appears to be to
explicitly cast all 8/16-bit asm operands to a 32-bit type.  Good luck
finding them.

-- 
Måns Rullgård
mans at mansr.com