Re: [PATCH 3/5] MIPS: LLVMLinux: Fix an 'inline asm input/output type mismatch' error.

[Maciej W. Rozycki]

On Tue, 3 Feb 2015, Daniel Sanders wrote:

> From: Toma Tabacu <toma.tabacu@xxxxxxxxxx>
> 
> Change the type of csum_ipv6_magic's 'proto' argument from unsigned
> short to __u32.
> 
> This fixes a type mismatch between the 'htonl(proto)' inline asm
> input, which is __u32, and the 'proto' output, which is unsigned
> short.
> 
> This is the error message reported by clang:
> arch/mips/include/asm/checksum.h:285:27: error: unsupported inline asm: input with type '__be32' (aka 'unsigned int') matching output with type 'unsigned short'
>           "0" (htonl(len)), "1" (htonl(proto)), "r" (sum));
>                                  ^~~~~~~~~~~~
> 
> The changed code can be compiled successfully by both gcc and clang.

 This definitely looks like a bug in clang to me.  What this construct 
means is both input #5 and output #1 live in the same register, and that 
an `__u32' value is taken on input (from the result of the `htonl(proto)' 
calculation) and an `unsigned short' value produced in the same register 
on output, that'll be the value of the `proto' variable from there on.  A 
perfectly valid arrangement.  This would be the right arrangement to use 
with the MIPS16 SEH instruction for example.  Has this bug been reported 
to clang maintainers?

 And I'd prefer to leave the declaration of `proto' alone as IPv6 network 
protocol numbers are 16-bit quantities.

 That said this code is indeed weird if not wrong, which is probably why 
this arrangement resulted, in an attempt to prevent GCC from messing up 
the registers used.

 First and foremost both outputs, and especially #1, lack an earlyclobber.  
This I imagine may have prompted GCC to overwrite one of the inputs, which 
in turn is why whoever poked at this code decided to alias input #5 to 
output #1.  But as you can see in the asm there's no real aliasing between 
input #5 and output #1.  Input #5 is consumed early on (and even referred 
to with `%5' rather than `%1', which would be the norm in the case of 
actual aliasing), and the containing register reused for something else.  
So the two operands can be separated.  This is unlike input #4 vs output 
#0, that is both read and written right away (and just as one'd expect 
there's no reference to `%4' anywhere).

 Output #0 can do without an earlyclobber as it is aliased to input #4 and 
therefore cannot be assigned by GCC to another input.  But it won't hurt 
to have one too and it will set a good practice and serve a documentation 
purpose.

 I suggest a fix like this then:

static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
					  const struct in6_addr *daddr,
					  __u32 len, unsigned short proto,
					  __wsum sum)
{
	__wsum tmp;

	__asm__(
[...]
	: "=&r" (sum), "=&r" (tmp)
	: "r" (saddr), "r" (daddr),
	  "0" (htonl(len)), "r" (htonl(proto)), "r" (sum));

        return csum_fold(sum);
}

Try and see if it works for you.

 I wonder why this is an asm in the first place though.  There's no rocket 
science here that GCC couldn't handle.  I guess it must have been very bad 
at optimising a C equivalent then.

  Maciej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/