gcc miscompiles csum_tcpudp_magic() on ARMv5

[Måns Rullgård]

Willy Tarreau <w at 1wt.eu> writes:

>> >> Hmmm aren't you passing a 16-bit register directly to the ASM for
>> >> being used as a 32-bit one ? This seems hasardous to me since
>> >> nowhere you tell gcc how you're going to use the register.
>> >
>> > this is exactly what I'm complaining about, the arm code for
>> > csum_tcpudp_nofold() in the kernel does exactly that.
>> >
>> >> Could you check if that fixes it :
>> >> 
>> >>  static inline uint32_t asm_add(uint16_t len, uint32_t sum)
>> >>  {
>> >>          uint32_t len32 = len;
>> >
>> > or change the asm_add() proto to take an "uint32_t len" instead, and yes
>> > of course that fixes it.
>> 
>> It's a bug.  Please report it to the gcc developers.
>
> Here I don't agree with the generalization (and believe me I swear all the
> day about gcc's bugs). It's a matter of ABI and availability or not of 16
> bit registers or not. If the ASM supports 16-bit regs and the compiler is
> allowed to emit 16-bit regs, then gcc will have no way to know whether it
> must zero-extend the value first. If it's specified that "r" is necessarily
> a 32-bit register then it should.

ARM has *only* 32-bit registers.

> Maybe the issue is in the ABI itself, I don't know if 16-bit values
> are supposed to be zero-extended only when they're converted to 32-bit
> or also when they're passed as arguments. The fact that it works
> without inline may simply be a side effect of the different code (eg:
> 16 lower bits of the register copied into another one).
>
> So one needs to look at the specs of the ABI to know where the 16-bit value
> is supposed to be converted to 32-bit, then the faulty component must be
> fixed (gcc or kernel code).

The ABI for function calls sign/zero-extends all arguments prior to the
call.

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