| Message ID | 1611722507-12017-1-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 198688edbf77c6fc0e65f5d062f810d83d090166 |
| Headers | show |
| Series | MIPS: Fix inline asm input/output type mismatch in checksum.h used with Clang | expand |
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.
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
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
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
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(-)