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@xxxxxxxxxxxxxx/
> > >
> > > Signed-off-by: Tiezhu Yang <yangtiezhu@xxxxxxxxxxx>
> > > ---
> > >   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