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
