Message-ID: <e2b21bdc-e847-cfe8-2c97-e99fcafcb4e8@loongson.cn>
Date:   Thu, 4 Mar 2021 15:18:22 +0800
From:   Tiezhu Yang <yangtiezhu@...ngson.cn>
To:     "Maciej W. Rozycki" <macro@...am.me.uk>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Cc:     linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        Xuefeng Li <lixuefeng@...ngson.cn>,
        Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH] MIPS: Fix inline asm input/output type mismatch in
 checksum.h used with Clang

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@...ngson.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

Powered by blists - more mailing lists