[PATCH] MIPS: Fix inline asm input/output type mismatch in checksum.h used with Clang

[PATCH] MIPS: Fix inline asm input/output type mismatch in checksum.h used with Clang

[Tiezhu Yang]

Fix the following build error when make M=samples/bpf used with Clang:

  CLANG-bpf  samples/bpf/sockex2_kern.o
In file included from samples/bpf/sockex2_kern.c:7:
In file included from ./include/uapi/linux/if_tunnel.h:7:
In file included from ./include/linux/ip.h:16:
In file included from ./include/linux/skbuff.h:28:
In file included from ./include/net/checksum.h:22:
./arch/mips/include/asm/checksum.h:161:9: error: unsupported inline asm: input with type 'unsigned long' matching output with type '__wsum' (aka 'unsigned int')
        : "0" ((__force unsigned long)daddr),
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

This is a known issue on MIPS [1], the changed code can be compiled
successfully by both GCC and Clang.

[1] https://lore.kernel.org/linux-mips/CAG_fn=W0JHf8QyUX==+rQMp8PoULHrsQCa9Htffws31ga8k-iw@mail.gmail.com/

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/include/asm/checksum.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/checksum.h b/arch/mips/include/asm/checksum.h
index 5f80c28..1e6c135 100644
--- a/arch/mips/include/asm/checksum.h
+++ b/arch/mips/include/asm/checksum.h
@@ -130,6 +130,8 @@ static inline __wsum csum_tcpudp_nofold(__be32 saddr, __be32 daddr,
 					__u32 len, __u8 proto,
 					__wsum sum)
 {
+	unsigned long tmp = (__force unsigned long)sum;
+
 	__asm__(
 	"	.set	push		# csum_tcpudp_nofold\n"
 	"	.set	noat		\n"
@@ -157,7 +159,7 @@ static inline __wsum csum_tcpudp_nofold(__be32 saddr, __be32 daddr,
 	"	addu	%0, $1		\n"
 #endif
 	"	.set	pop"
-	: "=r" (sum)
+	: "=r" (tmp)
 	: "0" ((__force unsigned long)daddr),
 	  "r" ((__force unsigned long)saddr),
 #ifdef __MIPSEL__
@@ -167,7 +169,7 @@ static inline __wsum csum_tcpudp_nofold(__be32 saddr, __be32 daddr,
 #endif
 	  "r" ((__force unsigned long)sum));
 
-	return sum;
+	return (__force __wsum)tmp;
 }
 #define csum_tcpudp_nofold csum_tcpudp_nofold
 
-- 
2.1.0

Re: [PATCH] MIPS: Fix inline asm input/output type mismatch in checksum.h used with Clang

[Thomas Bogendoerfer]

On Wed, Jan 27, 2021 at 12:41:47PM +0800, Tiezhu Yang wrote:
> Fix the following build error when make M=samples/bpf used with Clang:
> 
>   CLANG-bpf  samples/bpf/sockex2_kern.o
> In file included from samples/bpf/sockex2_kern.c:7:
> In file included from ./include/uapi/linux/if_tunnel.h:7:
> In file included from ./include/linux/ip.h:16:
> In file included from ./include/linux/skbuff.h:28:
> In file included from ./include/net/checksum.h:22:
> ./arch/mips/include/asm/checksum.h:161:9: error: unsupported inline asm: input with type 'unsigned long' matching output with type '__wsum' (aka 'unsigned int')
>         : "0" ((__force unsigned long)daddr),
>                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
> 
> This is a known issue on MIPS [1], the changed code can be compiled
> successfully by both GCC and Clang.
> 
> [1] https://lore.kernel.org/linux-mips/CAG_fn=W0JHf8QyUX==+rQMp8PoULHrsQCa9Htffws31ga8k-iw@mail.gmail.com/
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  arch/mips/include/asm/checksum.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

applied to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: Fix inline asm input/output type mismatch in checksum.h used with Clang

[Maciej W. Rozycki]

On Wed, 27 Jan 2021, Thomas Bogendoerfer wrote:

> > Fix the following build error when make M=samples/bpf used with Clang:
> > 
> >   CLANG-bpf  samples/bpf/sockex2_kern.o
> > In file included from samples/bpf/sockex2_kern.c:7:
> > In file included from ./include/uapi/linux/if_tunnel.h:7:
> > In file included from ./include/linux/ip.h:16:
> > In file included from ./include/linux/skbuff.h:28:
> > In file included from ./include/net/checksum.h:22:
> > ./arch/mips/include/asm/checksum.h:161:9: error: unsupported inline asm: input with type 'unsigned long' matching output with type '__wsum' (aka 'unsigned int')
> >         : "0" ((__force unsigned long)daddr),
> >                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 1 error generated.
> > 
> > This is a known issue on MIPS [1], the changed code can be compiled
> > successfully by both GCC and Clang.
> > 
> > [1] https://lore.kernel.org/linux-mips/CAG_fn=W0JHf8QyUX==+rQMp8PoULHrsQCa9Htffws31ga8k-iw@mail.gmail.com/
> > 
> > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> > ---
> >  arch/mips/include/asm/checksum.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> applied to mips-next.

 This is in a performance-critical path (otherwise it wouldn't have been 
in the form of inline assembly).  Has it been verified that it does not 
regress code quality with GCC?

 The semantics is clear here: output is in the same register as in input, 
but the register holds a different local variable in each case.  There's 
nothing odd about that and the variables can obviously be of a different 
type each; that's no different to register usage with code produced by the 
compiler directly itself from a high-level language.

 I seem to remember discussing the issue before, but I can't remember what 
the outcome has been WRT filing this as a Clang bug, and archives are not 
easily available at the moment (I know a mirror exists, but any old links 
are not relevant there).  Would someone be able to fill me in?

 I think ultimately with any critical piece where a Clang workaround does 
regress code produced with GCC we do want to go with `#ifdef __clang__' so 
that good use with GCC is not penalised on one hand and we know the places 
to revert changes at should Clang ever get fixed.

 Otherwise I'll start suspecting that Clang supporters try some kind of an 
unfair game to gain advantage over GCC, by modifying projects such that 
the competing compiler produces worse code than it could if Clang was not 
actively supported.

  Maciej

Re: [PATCH] MIPS: Fix inline asm input/output type mismatch in checksum.h used with Clang

[Tiezhu Yang]

On 02/13/2021 04:36 AM, Maciej W. Rozycki wrote:
> On Wed, 27 Jan 2021, Thomas Bogendoerfer wrote:
>
>>> Fix the following build error when make M=samples/bpf used with Clang:
>>>
>>>    CLANG-bpf  samples/bpf/sockex2_kern.o
>>> In file included from samples/bpf/sockex2_kern.c:7:
>>> In file included from ./include/uapi/linux/if_tunnel.h:7:
>>> In file included from ./include/linux/ip.h:16:
>>> In file included from ./include/linux/skbuff.h:28:
>>> In file included from ./include/net/checksum.h:22:
>>> ./arch/mips/include/asm/checksum.h:161:9: error: unsupported inline asm: input with type 'unsigned long' matching output with type '__wsum' (aka 'unsigned int')
>>>          : "0" ((__force unsigned long)daddr),
>>>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> 1 error generated.
>>>
>>> This is a known issue on MIPS [1], the changed code can be compiled
>>> successfully by both GCC and Clang.
>>>
>>> [1] https://lore.kernel.org/linux-mips/CAG_fn=W0JHf8QyUX==+rQMp8PoULHrsQCa9Htffws31ga8k-iw@mail.gmail.com/
>>>
>>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>>> ---
>>>   arch/mips/include/asm/checksum.h | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>> applied to mips-next.
>   This is in a performance-critical path (otherwise it wouldn't have been
> in the form of inline assembly).  Has it been verified that it does not
> regress code quality with GCC?
>
>   The semantics is clear here: output is in the same register as in input,
> but the register holds a different local variable in each case.  There's
> nothing odd about that and the variables can obviously be of a different
> type each; that's no different to register usage with code produced by the
> compiler directly itself from a high-level language.
>
>   I seem to remember discussing the issue before, but I can't remember what
> the outcome has been WRT filing this as a Clang bug, and archives are not
> easily available at the moment (I know a mirror exists, but any old links
> are not relevant there).  Would someone be able to fill me in?
>
>   I think ultimately with any critical piece where a Clang workaround does
> regress code produced with GCC we do want to go with `#ifdef __clang__' so
> that good use with GCC is not penalised on one hand and we know the places
> to revert changes at should Clang ever get fixed.
>
>   Otherwise I'll start suspecting that Clang supporters try some kind of an
> unfair game to gain advantage over GCC, by modifying projects such that
> the competing compiler produces worse code than it could if Clang was not
> actively supported.
>
>    Maciej

Hi Maciej,

Thank you very much for your detailed explanation, sorry for the
late response and the poorly considered implementation about the
performance influence with GCC.

I think you are right, so are you OK with the following changes?
If yes, I will send a new patch later.

With the new patch, we can build successfully by both GCC and Clang,
at the same time, we can avoid the potential performance influence
with GCC.

diff --git a/arch/mips/include/asm/checksum.h 
b/arch/mips/include/asm/checksum.h
index 1e6c135..0079a8e 100644
--- a/arch/mips/include/asm/checksum.h
+++ b/arch/mips/include/asm/checksum.h
@@ -130,7 +130,9 @@ static inline __wsum csum_tcpudp_nofold(__be32 
saddr, __be32 daddr,
                                         __u32 len, __u8 proto,
                                         __wsum sum)
  {
+#ifdef CONFIG_CC_IS_CLANG
         unsigned long tmp = (__force unsigned long)sum;
+#endif

         __asm__(
         "       .set    push            # csum_tcpudp_nofold\n"
@@ -159,7 +161,11 @@ static inline __wsum csum_tcpudp_nofold(__be32 
saddr, __be32 daddr,
         "       addu    %0, $1          \n"
  #endif
         "       .set    pop"
+#ifdef CONFIG_CC_IS_CLANG
         : "=r" (tmp)
+#else
+       : "=r" (sum)
+#endif
         : "0" ((__force unsigned long)daddr),
           "r" ((__force unsigned long)saddr),
  #ifdef __MIPSEL__
@@ -169,7 +175,11 @@ static inline __wsum csum_tcpudp_nofold(__be32 
saddr, __be32 daddr,
  #endif
           "r" ((__force unsigned long)sum));

+#ifdef CONFIG_CC_IS_CLANG
         return (__force __wsum)tmp;
+#else
+       return sum;
+#endif
  }
  #define csum_tcpudp_nofold csum_tcpudp_nofold

Thanks,
Tiezhu