All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
@ 2016-05-02 21:48 Arnd Bergmann
  2016-05-02 23:02 ` Andrew Morton
  0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2016-05-02 21:48 UTC (permalink / raw)
  To: akpm
  Cc: mm-commits, linux-kernel, linux-scsi, jpoimboe, Martin Jambor,
	Martin K. Petersen, James Bottomley, Denys Vlasenko, Thomas Graf,
	Peter Zijlstra, David Rientjes, Ingo Molnar, Himanshu Madhani,
	Dept-Eng QLA2xxx Upstream, Jan Hubicka

This is another attempt to avoid a regression in wwn_to_u64() after
that started using get_unaligned_be64(), which in turn ran into a
bug on gcc-4.9 through 6.1.

The regression got introduced due to the combination of two
separate workarounds (e3bde9568d99 and ef3fb2422ffe) that each
try to sidestep distinct problems with gcc behavior (code growth
and increased stack usage). Unfortunately after both have been
applied, a more series gcc bug has been uncovered, leading to
incorrect object code that discards part of a function and
causes undefined behavior.

As part of this problem is how __builtin_constant_p gets evaluated
on an argument passed by reference into an inline function, this
avoids the use of __builtin_constant_p() for all architectures
that set CONFIG_ARCH_USE_BUILTIN_BSWAP. Most architectures do not
set ARCH_SUPPORTS_OPTIMIZED_INLINING, which means they probably
do not suffer from the problem in the qla2xxx driver, but they
might still run into it elsewhere.

Both of the original workarounds were only merged in the 4.6 kernel,
and the bug that is fixed by this patch should only appear if
both are there, so we probably don't need to backport the fix.
On the other hand, it works by simplifying the code path and
should not have any negative effects.

Link: https://lkml.org/lkml/headers/2016/4/12/1103
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70232
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
Fixes: e3bde9568d99 ("include/linux/unaligned: force inlining of byteswap operations")
Fixes: ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
Tested-by: Josh Poimboeuf <jpoimboe@redhat.com> # on gcc-5.3
Tested-by: Quinn Tran <quinn.tran@qlogic.com>
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
----
This contains the extra cast to fix up 64-bit builds, and has
an expanded changelog, compared to the original version.

diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
index 3f10e5317b46..de56fd54428d 100644
--- a/include/uapi/linux/swab.h
+++ b/include/uapi/linux/swab.h
@@ -45,9 +45,7 @@
 
 static inline __attribute_const__ __u16 __fswab16(__u16 val)
 {
-#ifdef __HAVE_BUILTIN_BSWAP16__
-	return __builtin_bswap16(val);
-#elif defined (__arch_swab16)
+#if defined (__arch_swab16)
 	return __arch_swab16(val);
 #else
 	return ___constant_swab16(val);
@@ -56,9 +54,7 @@ static inline __attribute_const__ __u16 __fswab16(__u16 val)
 
 static inline __attribute_const__ __u32 __fswab32(__u32 val)
 {
-#ifdef __HAVE_BUILTIN_BSWAP32__
-	return __builtin_bswap32(val);
-#elif defined(__arch_swab32)
+#if defined(__arch_swab32)
 	return __arch_swab32(val);
 #else
 	return ___constant_swab32(val);
@@ -67,9 +63,7 @@ static inline __attribute_const__ __u32 __fswab32(__u32 val)
 
 static inline __attribute_const__ __u64 __fswab64(__u64 val)
 {
-#ifdef __HAVE_BUILTIN_BSWAP64__
-	return __builtin_bswap64(val);
-#elif defined (__arch_swab64)
+#if defined (__arch_swab64)
 	return __arch_swab64(val);
 #elif defined(__SWAB_64_THRU_32__)
 	__u32 h = val >> 32;
@@ -102,28 +96,40 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
  * __swab16 - return a byteswapped 16-bit value
  * @x: value to byteswap
  */
+#ifdef __HAVE_BUILTIN_BSWAP16__
+#define __swab16(x) __builtin_bswap16((__u16)(x))
+#else
 #define __swab16(x)				\
 	(__builtin_constant_p((__u16)(x)) ?	\
 	___constant_swab16(x) :			\
 	__fswab16(x))
+#endif
 
 /**

  * __swab32 - return a byteswapped 32-bit value
  * @x: value to byteswap
  */
+#ifdef __HAVE_BUILTIN_BSWAP32__
+#define __swab32(x) __builtin_bswap32((__u32)(x))
+#else
 #define __swab32(x)				\
 	(__builtin_constant_p((__u32)(x)) ?	\
 	___constant_swab32(x) :			\
 	__fswab32(x))
+#endif
 
 /**
  * __swab64 - return a byteswapped 64-bit value
  * @x: value to byteswap
  */
+#ifdef __HAVE_BUILTIN_BSWAP64__
+#define __swab64(x) (__u64)__builtin_bswap64((__u64)(x))
+#else
 #define __swab64(x)				\
 	(__builtin_constant_p((__u64)(x)) ?	\
 	___constant_swab64(x) :			\
 	__fswab64(x))
+#endif
 
 /**
  * __swahw32 - return a word-swapped 32-bit value

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-05-02 21:48 [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug Arnd Bergmann
@ 2016-05-02 23:02 ` Andrew Morton
  2016-05-02 23:10   ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: Andrew Morton @ 2016-05-02 23:02 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: mm-commits, linux-kernel, linux-scsi, jpoimboe, Martin Jambor,
	Martin K. Petersen, James Bottomley, Denys Vlasenko, Thomas Graf,
	Peter Zijlstra, David Rientjes, Ingo Molnar, Himanshu Madhani,
	Dept-Eng QLA2xxx Upstream, Jan Hubicka

On Mon, 02 May 2016 23:48:19 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> This is another attempt to avoid a regression in wwn_to_u64() after
> that started using get_unaligned_be64(), which in turn ran into a
> bug on gcc-4.9 through 6.1.

I'm still getting a couple screenfuls of things like

net/tipc/name_distr.c: In function 'tipc_named_process_backlog':
net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 3 has type 'unsigned int'
net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 4 has type 'unsigned int'
net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 5 has type 'unsigned int'
net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 7 has type 'unsigned int'

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-05-02 23:02 ` Andrew Morton
@ 2016-05-02 23:10   ` Arnd Bergmann
  2016-05-02 23:32     ` Andrew Morton
  2016-05-02 23:32     ` Andrew Morton
  0 siblings, 2 replies; 16+ messages in thread
From: Arnd Bergmann @ 2016-05-02 23:10 UTC (permalink / raw)
  To: Andrew Morton
  Cc: mm-commits, linux-kernel, linux-scsi, jpoimboe, Martin Jambor,
	Martin K. Petersen, James Bottomley, Denys Vlasenko, Thomas Graf,
	Peter Zijlstra, David Rientjes, Ingo Molnar, Himanshu Madhani,
	Dept-Eng QLA2xxx Upstream, Jan Hubicka

On Monday 02 May 2016 16:02:18 Andrew Morton wrote:
> On Mon, 02 May 2016 23:48:19 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > This is another attempt to avoid a regression in wwn_to_u64() after
> > that started using get_unaligned_be64(), which in turn ran into a
> > bug on gcc-4.9 through 6.1.
> 
> I'm still getting a couple screenfuls of things like
> 
> net/tipc/name_distr.c: In function 'tipc_named_process_backlog':
> net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 3 has type 'unsigned int'
> net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 4 has type 'unsigned int'
> net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 5 has type 'unsigned int'
> net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 7 has type 'unsigned int'

I've built a few thousand kernels (arm32 with gcc-6.1) with the patch applied,
but didn't see this one. What target architecture and compiler version produced
this? Does it go away if you add a (__u32) cast? I don't even know what the
warning is trying to tell me.

	Arnd

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-05-02 23:10   ` Arnd Bergmann
  2016-05-02 23:32     ` Andrew Morton
@ 2016-05-02 23:32     ` Andrew Morton
  2016-05-03  6:36       ` Arnd Bergmann
  1 sibling, 1 reply; 16+ messages in thread
From: Andrew Morton @ 2016-05-02 23:32 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: mm-commits, linux-kernel, linux-scsi, jpoimboe, Martin Jambor,
	Martin K. Petersen, James Bottomley, Denys Vlasenko, Thomas Graf,
	Peter Zijlstra, David Rientjes, Ingo Molnar, Himanshu Madhani,
	Dept-Eng QLA2xxx Upstream, Jan Hubicka

On Tue, 03 May 2016 01:10:16 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 02 May 2016 16:02:18 Andrew Morton wrote:
> > On Mon, 02 May 2016 23:48:19 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> > 
> > > This is another attempt to avoid a regression in wwn_to_u64() after
> > > that started using get_unaligned_be64(), which in turn ran into a
> > > bug on gcc-4.9 through 6.1.
> > 
> > I'm still getting a couple screenfuls of things like
> > 
> > net/tipc/name_distr.c: In function 'tipc_named_process_backlog':
> > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 3 has type 'unsigned int'
> > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 4 has type 'unsigned int'
> > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 5 has type 'unsigned int'
> > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 7 has type 'unsigned int'
> 
> I've built a few thousand kernels (arm32 with gcc-6.1) with the patch applied,
> but didn't see this one. What target architecture and compiler version produced
> this? Does it go away if you add a (__u32) cast? I don't even know what the
> warning is trying to tell me.

heh, I didn't actually read it.

Hopefully we can write this off as a gcc-4.4.4 glitch. 4.8.4 is OK.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-05-02 23:10   ` Arnd Bergmann
@ 2016-05-02 23:32     ` Andrew Morton
  2016-05-02 23:32     ` Andrew Morton
  1 sibling, 0 replies; 16+ messages in thread
From: Andrew Morton @ 2016-05-02 23:32 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: mm-commits, linux-kernel, linux-scsi, jpoimboe, Martin Jambor,
	Martin K. Petersen, James Bottomley, Denys Vlasenko, Thomas Graf,
	Peter Zijlstra, David Rientjes, Ingo Molnar, Himanshu Madhani,
	Dept-Eng QLA2xxx Upstream, Jan Hubicka

On Tue, 03 May 2016 01:10:16 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 02 May 2016 16:02:18 Andrew Morton wrote:
> > On Mon, 02 May 2016 23:48:19 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> > 
> > > This is another attempt to avoid a regression in wwn_to_u64() after
> > > that started using get_unaligned_be64(), which in turn ran into a
> > > bug on gcc-4.9 through 6.1.
> > 
> > I'm still getting a couple screenfuls of things like
> > 
> > net/tipc/name_distr.c: In function 'tipc_named_process_backlog':
> > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 3 has type 'unsigned int'
> > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 4 has type 'unsigned int'
> > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 5 has type 'unsigned int'
> > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 7 has type 'unsigned int'
> 
> I've built a few thousand kernels (arm32 with gcc-6.1) with the patch applied,
> but didn't see this one. What target architecture and compiler version produced
> this? Does it go away if you add a (__u32) cast? I don't even know what the
> warning is trying to tell me.

heh, I didn't actually read it.

Hopefully we can write this off as a gcc-4.4.4 glitch. 4.8.4 is OK.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-05-02 23:32     ` Andrew Morton
@ 2016-05-03  6:36       ` Arnd Bergmann
  2016-06-21  9:02           ` Tomas Winkler
  0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2016-05-03  6:36 UTC (permalink / raw)
  To: Andrew Morton
  Cc: mm-commits, linux-kernel, linux-scsi, jpoimboe, Martin Jambor,
	Martin K. Petersen, James Bottomley, Denys Vlasenko, Thomas Graf,
	Peter Zijlstra, David Rientjes, Ingo Molnar, Himanshu Madhani,
	Dept-Eng QLA2xxx Upstream, Jan Hubicka

On Monday 02 May 2016 16:32:25 Andrew Morton wrote:
> On Tue, 03 May 2016 01:10:16 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Monday 02 May 2016 16:02:18 Andrew Morton wrote:
> > > On Mon, 02 May 2016 23:48:19 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> > > 
> > > > This is another attempt to avoid a regression in wwn_to_u64() after
> > > > that started using get_unaligned_be64(), which in turn ran into a
> > > > bug on gcc-4.9 through 6.1.
> > > 
> > > I'm still getting a couple screenfuls of things like
> > > 
> > > net/tipc/name_distr.c: In function 'tipc_named_process_backlog':
> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 3 has type 'unsigned int'
> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 4 has type 'unsigned int'
> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 5 has type 'unsigned int'
> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 7 has type 'unsigned int'
> > 
> > I've built a few thousand kernels (arm32 with gcc-6.1) with the patch applied,
> > but didn't see this one. What target architecture and compiler version produced
> > this? Does it go away if you add a (__u32) cast? I don't even know what the
> > warning is trying to tell me.
> 
> heh, I didn't actually read it.
> 
> Hopefully we can write this off as a gcc-4.4.4 glitch. 4.8.4 is OK.

Ah, old compiler. I've tried gcc-4.3 now on ARM, and I don't get this warning
(just a lot "may be used uninitialized"), but unlike gcc-4.4, my version doesn't
actually get into the code path I have changed because __builtin_bswap32 was only
introduced with 4.4.

I don't have gcc-4.4 and 4.5 here, but the warning does show up with 4.6, 4.7
and 4.8:

drivers/soc/sunxi/sunxi_sram.c: In function ‘sunxi_sram_show’:
drivers/soc/sunxi/sunxi_sram.c:103:7: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=]

4.8 is probably still common enough that we should try to address this.
This change addresses the problem for me with ARM gcc-4.8, but adding
two more type casts. This also makes the 16/32/64 bit swaps all
look the same. I would expect this to also have the same effect on 4.4.

Please fold into the previous patch.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
index d737804af181..8f3a8f606fd9 100644
--- a/include/uapi/linux/swab.h
+++ b/include/uapi/linux/swab.h
@@ -97,7 +97,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
  * @x: value to byteswap
  */
 #ifdef __HAVE_BUILTIN_BSWAP16__
-#define __swab16(x) __builtin_bswap16((__u16)(x))
+#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
 #else
 #define __swab16(x)				\
 	(__builtin_constant_p((__u16)(x)) ?	\
@@ -110,7 +110,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
  * @x: value to byteswap
  */
 #ifdef __HAVE_BUILTIN_BSWAP32__
-#define __swab32(x) __builtin_bswap32((__u32)(x))
+#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
 #else
 #define __swab32(x)				\
 	(__builtin_constant_p((__u32)(x)) ?	\

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-05-03  6:36       ` Arnd Bergmann
@ 2016-06-21  9:02           ` Tomas Winkler
  0 siblings, 0 replies; 16+ messages in thread
From: Tomas Winkler @ 2016-06-21  9:02 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Morton, mm-commits, linux-kernel, linux-scsi, jpoimboe,
	Martin Jambor, Martin K. Petersen, James Bottomley,
	Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
	Ingo Molnar, Himanshu Madhani, Dept-Eng QLA2xxx Upstream,
	Jan Hubicka, Amir (Jer)

On Tue, May 3, 2016 at 9:36 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 02 May 2016 16:32:25 Andrew Morton wrote:
>> On Tue, 03 May 2016 01:10:16 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> > On Monday 02 May 2016 16:02:18 Andrew Morton wrote:
>> > > On Mon, 02 May 2016 23:48:19 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>> > >
>> > > > This is another attempt to avoid a regression in wwn_to_u64() after
>> > > > that started using get_unaligned_be64(), which in turn ran into a
>> > > > bug on gcc-4.9 through 6.1.
>> > >
>> > > I'm still getting a couple screenfuls of things like
>> > >
>> > > net/tipc/name_distr.c: In function 'tipc_named_process_backlog':
>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 3 has type 'unsigned int'
>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 4 has type 'unsigned int'
>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 5 has type 'unsigned int'
>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 7 has type 'unsigned int'
>> >
>> > I've built a few thousand kernels (arm32 with gcc-6.1) with the patch applied,
>> > but didn't see this one. What target architecture and compiler version produced
>> > this? Does it go away if you add a (__u32) cast? I don't even know what the
>> > warning is trying to tell me.
>>
>> heh, I didn't actually read it.
>>
>> Hopefully we can write this off as a gcc-4.4.4 glitch. 4.8.4 is OK.
>
> Ah, old compiler. I've tried gcc-4.3 now on ARM, and I don't get this warning
> (just a lot "may be used uninitialized"), but unlike gcc-4.4, my version doesn't
> actually get into the code path I have changed because __builtin_bswap32 was only
> introduced with 4.4.
>
> I don't have gcc-4.4 and 4.5 here, but the warning does show up with 4.6, 4.7
> and 4.8:
>
> drivers/soc/sunxi/sunxi_sram.c: In function ‘sunxi_sram_show’:
> drivers/soc/sunxi/sunxi_sram.c:103:7: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=]
>
> 4.8 is probably still common enough that we should try to address this.
> This change addresses the problem for me with ARM gcc-4.8, but adding
> two more type casts. This also makes the 16/32/64 bit swaps all
> look the same. I would expect this to also have the same effect on 4.4.
>
> Please fold into the previous patch.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
> index d737804af181..8f3a8f606fd9 100644
> --- a/include/uapi/linux/swab.h
> +++ b/include/uapi/linux/swab.h
> @@ -97,7 +97,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
>   * @x: value to byteswap
>   */
>  #ifdef __HAVE_BUILTIN_BSWAP16__
> -#define __swab16(x) __builtin_bswap16((__u16)(x))
> +#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>  #else
>  #define __swab16(x)                            \
>         (__builtin_constant_p((__u16)(x)) ?     \
> @@ -110,7 +110,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
>   * @x: value to byteswap
>   */
>  #ifdef __HAVE_BUILTIN_BSWAP32__
> -#define __swab32(x) __builtin_bswap32((__u32)(x))
> +#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
>  #else
>  #define __swab32(x)                            \
>         (__builtin_constant_p((__u32)(x)) ?     \

>

I wonder if this doesn't break switch statement that requires a
constant expression, there few cases like this over the kernel.

switch(val) {
case cpu_to_le32(IXGBE_RXDADV_STAT_FCSTAT_FCPRSP):

http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c#L458


Thanks
Tomas

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
@ 2016-06-21  9:02           ` Tomas Winkler
  0 siblings, 0 replies; 16+ messages in thread
From: Tomas Winkler @ 2016-06-21  9:02 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Morton, mm-commits, linux-kernel, linux-scsi, jpoimboe,
	Martin Jambor, Martin K. Petersen, James Bottomley,
	Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
	Ingo Molnar, Himanshu Madhani, Dept-Eng QLA2xxx Upstream,
	Jan Hubicka, Amir (Jer)

On Tue, May 3, 2016 at 9:36 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 02 May 2016 16:32:25 Andrew Morton wrote:
>> On Tue, 03 May 2016 01:10:16 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> > On Monday 02 May 2016 16:02:18 Andrew Morton wrote:
>> > > On Mon, 02 May 2016 23:48:19 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>> > >
>> > > > This is another attempt to avoid a regression in wwn_to_u64() after
>> > > > that started using get_unaligned_be64(), which in turn ran into a
>> > > > bug on gcc-4.9 through 6.1.
>> > >
>> > > I'm still getting a couple screenfuls of things like
>> > >
>> > > net/tipc/name_distr.c: In function 'tipc_named_process_backlog':
>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 3 has type 'unsigned int'
>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 4 has type 'unsigned int'
>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 5 has type 'unsigned int'
>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 7 has type 'unsigned int'
>> >
>> > I've built a few thousand kernels (arm32 with gcc-6.1) with the patch applied,
>> > but didn't see this one. What target architecture and compiler version produced
>> > this? Does it go away if you add a (__u32) cast? I don't even know what the
>> > warning is trying to tell me.
>>
>> heh, I didn't actually read it.
>>
>> Hopefully we can write this off as a gcc-4.4.4 glitch. 4.8.4 is OK.
>
> Ah, old compiler. I've tried gcc-4.3 now on ARM, and I don't get this warning
> (just a lot "may be used uninitialized"), but unlike gcc-4.4, my version doesn't
> actually get into the code path I have changed because __builtin_bswap32 was only
> introduced with 4.4.
>
> I don't have gcc-4.4 and 4.5 here, but the warning does show up with 4.6, 4.7
> and 4.8:
>
> drivers/soc/sunxi/sunxi_sram.c: In function ‘sunxi_sram_show’:
> drivers/soc/sunxi/sunxi_sram.c:103:7: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=]
>
> 4.8 is probably still common enough that we should try to address this.
> This change addresses the problem for me with ARM gcc-4.8, but adding
> two more type casts. This also makes the 16/32/64 bit swaps all
> look the same. I would expect this to also have the same effect on 4.4.
>
> Please fold into the previous patch.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
> index d737804af181..8f3a8f606fd9 100644
> --- a/include/uapi/linux/swab.h
> +++ b/include/uapi/linux/swab.h
> @@ -97,7 +97,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
>   * @x: value to byteswap
>   */
>  #ifdef __HAVE_BUILTIN_BSWAP16__
> -#define __swab16(x) __builtin_bswap16((__u16)(x))
> +#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>  #else
>  #define __swab16(x)                            \
>         (__builtin_constant_p((__u16)(x)) ?     \
> @@ -110,7 +110,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
>   * @x: value to byteswap
>   */
>  #ifdef __HAVE_BUILTIN_BSWAP32__
> -#define __swab32(x) __builtin_bswap32((__u32)(x))
> +#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
>  #else
>  #define __swab32(x)                            \
>         (__builtin_constant_p((__u32)(x)) ?     \

>

I wonder if this doesn't break switch statement that requires a
constant expression, there few cases like this over the kernel.

switch(val) {
case cpu_to_le32(IXGBE_RXDADV_STAT_FCSTAT_FCPRSP):

http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c#L458


Thanks
Tomas
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-06-21  9:02           ` Tomas Winkler
@ 2016-06-22  8:24             ` Tomas Winkler
  -1 siblings, 0 replies; 16+ messages in thread
From: Tomas Winkler @ 2016-06-22  8:24 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Morton, mm-commits, linux-kernel, linux-scsi, jpoimboe,
	Martin Jambor, Martin K. Petersen, James Bottomley,
	Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
	Ingo Molnar, Himanshu Madhani, Dept-Eng QLA2xxx Upstream,
	Jan Hubicka, Amir (Jer)

On Tue, Jun 21, 2016 at 12:02 PM, Tomas Winkler <tomasw@gmail.com> wrote:
> On Tue, May 3, 2016 at 9:36 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Monday 02 May 2016 16:32:25 Andrew Morton wrote:
>>> On Tue, 03 May 2016 01:10:16 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>> > On Monday 02 May 2016 16:02:18 Andrew Morton wrote:
>>> > > On Mon, 02 May 2016 23:48:19 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>>> > >
>>> > > > This is another attempt to avoid a regression in wwn_to_u64() after
>>> > > > that started using get_unaligned_be64(), which in turn ran into a
>>> > > > bug on gcc-4.9 through 6.1.
>>> > >
>>> > > I'm still getting a couple screenfuls of things like
>>> > >
>>> > > net/tipc/name_distr.c: In function 'tipc_named_process_backlog':
>>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 3 has type 'unsigned int'
>>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 4 has type 'unsigned int'
>>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 5 has type 'unsigned int'
>>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 7 has type 'unsigned int'
>>> >
>>> > I've built a few thousand kernels (arm32 with gcc-6.1) with the patch applied,
>>> > but didn't see this one. What target architecture and compiler version produced
>>> > this? Does it go away if you add a (__u32) cast? I don't even know what the
>>> > warning is trying to tell me.
>>>
>>> heh, I didn't actually read it.
>>>
>>> Hopefully we can write this off as a gcc-4.4.4 glitch. 4.8.4 is OK.
>>
>> Ah, old compiler. I've tried gcc-4.3 now on ARM, and I don't get this warning
>> (just a lot "may be used uninitialized"), but unlike gcc-4.4, my version doesn't
>> actually get into the code path I have changed because __builtin_bswap32 was only
>> introduced with 4.4.
>>
>> I don't have gcc-4.4 and 4.5 here, but the warning does show up with 4.6, 4.7
>> and 4.8:
>>
>> drivers/soc/sunxi/sunxi_sram.c: In function ‘sunxi_sram_show’:
>> drivers/soc/sunxi/sunxi_sram.c:103:7: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=]
>>
>> 4.8 is probably still common enough that we should try to address this.
>> This change addresses the problem for me with ARM gcc-4.8, but adding
>> two more type casts. This also makes the 16/32/64 bit swaps all
>> look the same. I would expect this to also have the same effect on 4.4.
>>
>> Please fold into the previous patch.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>> diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
>> index d737804af181..8f3a8f606fd9 100644
>> --- a/include/uapi/linux/swab.h
>> +++ b/include/uapi/linux/swab.h
>> @@ -97,7 +97,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
>>   * @x: value to byteswap
>>   */
>>  #ifdef __HAVE_BUILTIN_BSWAP16__
>> -#define __swab16(x) __builtin_bswap16((__u16)(x))
>> +#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>  #else
>>  #define __swab16(x)                            \
>>         (__builtin_constant_p((__u16)(x)) ?     \
>> @@ -110,7 +110,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
>>   * @x: value to byteswap
>>   */
>>  #ifdef __HAVE_BUILTIN_BSWAP32__
>> -#define __swab32(x) __builtin_bswap32((__u32)(x))
>> +#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
>>  #else
>>  #define __swab32(x)                            \
>>         (__builtin_constant_p((__u32)(x)) ?     \
>
>>
>
> I wonder if this doesn't break switch statement that requires a
> constant expression, there few cases like this over the kernel.
>
> switch(val) {
> case cpu_to_le32(IXGBE_RXDADV_STAT_FCSTAT_FCPRSP):
>
> http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c#L458
>

I'm asking because sparse and checkpatch doesn't agree on that ping
sparse issues
'error: bad constant expression'
When changing to __constant_cpu_to_le32 sparse is happy but
checkpatch.ps is complaining
__constant_cpu_to_le32 should be cpu_to_le32



 Thanks
 Tomas

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
@ 2016-06-22  8:24             ` Tomas Winkler
  0 siblings, 0 replies; 16+ messages in thread
From: Tomas Winkler @ 2016-06-22  8:24 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Morton, mm-commits, linux-kernel, linux-scsi, jpoimboe,
	Martin Jambor, Martin K. Petersen, James Bottomley,
	Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
	Ingo Molnar, Himanshu Madhani, Dept-Eng QLA2xxx Upstream,
	Jan Hubicka, Amir (Jer)

On Tue, Jun 21, 2016 at 12:02 PM, Tomas Winkler <tomasw@gmail.com> wrote:
> On Tue, May 3, 2016 at 9:36 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Monday 02 May 2016 16:32:25 Andrew Morton wrote:
>>> On Tue, 03 May 2016 01:10:16 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>> > On Monday 02 May 2016 16:02:18 Andrew Morton wrote:
>>> > > On Mon, 02 May 2016 23:48:19 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>>> > >
>>> > > > This is another attempt to avoid a regression in wwn_to_u64() after
>>> > > > that started using get_unaligned_be64(), which in turn ran into a
>>> > > > bug on gcc-4.9 through 6.1.
>>> > >
>>> > > I'm still getting a couple screenfuls of things like
>>> > >
>>> > > net/tipc/name_distr.c: In function 'tipc_named_process_backlog':
>>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 3 has type 'unsigned int'
>>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 4 has type 'unsigned int'
>>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 5 has type 'unsigned int'
>>> > > net/tipc/name_distr.c:330: warning: format '%u' expects type 'unsigned int', but argument 7 has type 'unsigned int'
>>> >
>>> > I've built a few thousand kernels (arm32 with gcc-6.1) with the patch applied,
>>> > but didn't see this one. What target architecture and compiler version produced
>>> > this? Does it go away if you add a (__u32) cast? I don't even know what the
>>> > warning is trying to tell me.
>>>
>>> heh, I didn't actually read it.
>>>
>>> Hopefully we can write this off as a gcc-4.4.4 glitch. 4.8.4 is OK.
>>
>> Ah, old compiler. I've tried gcc-4.3 now on ARM, and I don't get this warning
>> (just a lot "may be used uninitialized"), but unlike gcc-4.4, my version doesn't
>> actually get into the code path I have changed because __builtin_bswap32 was only
>> introduced with 4.4.
>>
>> I don't have gcc-4.4 and 4.5 here, but the warning does show up with 4.6, 4.7
>> and 4.8:
>>
>> drivers/soc/sunxi/sunxi_sram.c: In function ‘sunxi_sram_show’:
>> drivers/soc/sunxi/sunxi_sram.c:103:7: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=]
>>
>> 4.8 is probably still common enough that we should try to address this.
>> This change addresses the problem for me with ARM gcc-4.8, but adding
>> two more type casts. This also makes the 16/32/64 bit swaps all
>> look the same. I would expect this to also have the same effect on 4.4.
>>
>> Please fold into the previous patch.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>> diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
>> index d737804af181..8f3a8f606fd9 100644
>> --- a/include/uapi/linux/swab.h
>> +++ b/include/uapi/linux/swab.h
>> @@ -97,7 +97,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
>>   * @x: value to byteswap
>>   */
>>  #ifdef __HAVE_BUILTIN_BSWAP16__
>> -#define __swab16(x) __builtin_bswap16((__u16)(x))
>> +#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
>>  #else
>>  #define __swab16(x)                            \
>>         (__builtin_constant_p((__u16)(x)) ?     \
>> @@ -110,7 +110,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
>>   * @x: value to byteswap
>>   */
>>  #ifdef __HAVE_BUILTIN_BSWAP32__
>> -#define __swab32(x) __builtin_bswap32((__u32)(x))
>> +#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
>>  #else
>>  #define __swab32(x)                            \
>>         (__builtin_constant_p((__u32)(x)) ?     \
>
>>
>
> I wonder if this doesn't break switch statement that requires a
> constant expression, there few cases like this over the kernel.
>
> switch(val) {
> case cpu_to_le32(IXGBE_RXDADV_STAT_FCSTAT_FCPRSP):
>
> http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c#L458
>

I'm asking because sparse and checkpatch doesn't agree on that ping
sparse issues
'error: bad constant expression'
When changing to __constant_cpu_to_le32 sparse is happy but
checkpatch.ps is complaining
__constant_cpu_to_le32 should be cpu_to_le32



 Thanks
 Tomas
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-06-22  8:24             ` Tomas Winkler
  (?)
@ 2016-06-22  9:59             ` Arnd Bergmann
  2016-06-22 10:25               ` Levy, Amir (Jer)
  -1 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2016-06-22  9:59 UTC (permalink / raw)
  To: Tomas Winkler
  Cc: Andrew Morton, mm-commits, linux-kernel, linux-scsi, jpoimboe,
	Martin Jambor, Martin K. Petersen, James Bottomley,
	Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
	Ingo Molnar, Himanshu Madhani, Dept-Eng QLA2xxx Upstream,
	Jan Hubicka, Amir (Jer)

On Wednesday, June 22, 2016 11:24:50 AM CEST Tomas Winkler wrote:
> On Tue, Jun 21, 2016 at 12:02 PM, Tomas Winkler <tomasw@gmail.com> wrote:
> > On Tue, May 3, 2016 at 9:36 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Monday 02 May 2016 16:32:25 Andrew Morton wrote:

> >>  #ifdef __HAVE_BUILTIN_BSWAP32__
> >> -#define __swab32(x) __builtin_bswap32((__u32)(x))
> >> +#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
> >>  #else
> >>  #define __swab32(x)                            \
> >>         (__builtin_constant_p((__u32)(x)) ?     \
> >
> >>
> >
> > I wonder if this doesn't break switch statement that requires a
> > constant expression, there few cases like this over the kernel.
> >
> > switch(val) {
> > case cpu_to_le32(IXGBE_RXDADV_STAT_FCSTAT_FCPRSP):
> >
> > http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c#L458
> >
> 
> I'm asking because sparse and checkpatch doesn't agree on that ping
> sparse issues
> 'error: bad constant expression'
> When changing to __constant_cpu_to_le32 sparse is happy but
> checkpatch.ps is complaining
> __constant_cpu_to_le32 should be cpu_to_le32
> 

That is an interesting problem, as both seem to be reasonable:
sparse probably doesn't understand __builtin_constant_p() enough,
while avoiding __constant_cpu_to_le32() is a good recommendation
in general.

How many instances of this do you see in the kernel? If ixgbe is the
only one, I'd just move the byteswap up into the switch statement:

	switch (le32_to_cpu(val)) {
	case IXGBE_RXDADV_STAT_FCSTAT_FCPRSP:

which may cost one or two cycles for the non-constant byteswap,
but is also easier to read than the current code.

	Arnd

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-06-22  9:59             ` Arnd Bergmann
@ 2016-06-22 10:25               ` Levy, Amir (Jer)
  2016-06-22 11:44                 ` Tomas Winkler
  0 siblings, 1 reply; 16+ messages in thread
From: Levy, Amir (Jer) @ 2016-06-22 10:25 UTC (permalink / raw)
  To: Arnd Bergmann, Tomas Winkler
  Cc: Andrew Morton, mm-commits, linux-kernel, linux-scsi, jpoimboe,
	Martin Jambor, Martin K. Petersen, James Bottomley,
	Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
	Ingo Molnar, Himanshu Madhani, Dept-Eng QLA2xxx Upstream,
	Jan Hubicka

On 2016-06-22 Arnd Bergmann wrote:
> On Wednesday, June 22, 2016 11:24:50 AM CEST Tomas Winkler wrote:
> > On Tue, Jun 21, 2016 at 12:02 PM, Tomas Winkler <tomasw@gmail.com>
> wrote:
> > > On Tue, May 3, 2016 at 9:36 AM, Arnd Bergmann <arnd@arndb.de>
> wrote:
> > >> On Monday 02 May 2016 16:32:25 Andrew Morton wrote:
> 
> > >>  #ifdef __HAVE_BUILTIN_BSWAP32__
> > >> -#define __swab32(x) __builtin_bswap32((__u32)(x))
> > >> +#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
> > >>  #else
> > >>  #define __swab32(x)                            \
> > >>         (__builtin_constant_p((__u32)(x)) ?     \
> > >
> > >>
> > >
> > > I wonder if this doesn't break switch statement that requires a
> > > constant expression, there few cases like this over the kernel.
> > >
> > > switch(val) {
> > > case cpu_to_le32(IXGBE_RXDADV_STAT_FCSTAT_FCPRSP):
> > >
> > > http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/ixgb
> > > e/ixgbe_fcoe.c#L458
> > >
> >
> > I'm asking because sparse and checkpatch doesn't agree on that ping
> > sparse issues
> > 'error: bad constant expression'
> > When changing to __constant_cpu_to_le32 sparse is happy but
> > checkpatch.ps is complaining
> > __constant_cpu_to_le32 should be cpu_to_le32
> >
> 
> That is an interesting problem, as both seem to be reasonable:
> sparse probably doesn't understand __builtin_constant_p() enough, while
> avoiding __constant_cpu_to_le32() is a good recommendation in general.
> 
> How many instances of this do you see in the kernel? If ixgbe is the only one,
> I'd just move the byteswap up into the switch statement:
> 
> 	switch (le32_to_cpu(val)) {
> 	case IXGBE_RXDADV_STAT_FCSTAT_FCPRSP:
> 
> which may cost one or two cycles for the non-constant byteswap, but is also
> easier to read than the current code.
> 
> 	Arnd

There are more than 20 files that have the statement: case cpu_to_...
Sparse complains about: case __builtin_bswap, not about __builtin_constant_p.

	Amir

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-06-22 10:25               ` Levy, Amir (Jer)
@ 2016-06-22 11:44                 ` Tomas Winkler
  2016-06-22 12:25                   ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: Tomas Winkler @ 2016-06-22 11:44 UTC (permalink / raw)
  To: Levy, Amir (Jer)
  Cc: Arnd Bergmann, Andrew Morton, mm-commits, linux-kernel,
	linux-scsi, jpoimboe, Martin Jambor, Martin K. Petersen,
	James Bottomley, Denys Vlasenko, Thomas Graf, Peter Zijlstra,
	David Rientjes, Ingo Molnar, Himanshu Madhani,
	Dept-Eng QLA2xxx Upstream, Jan Hubicka

On Wed, Jun 22, 2016 at 1:25 PM, Levy, Amir (Jer)
<amir.jer.levy@intel.com> wrote:
> On 2016-06-22 Arnd Bergmann wrote:
>> On Wednesday, June 22, 2016 11:24:50 AM CEST Tomas Winkler wrote:
>> > On Tue, Jun 21, 2016 at 12:02 PM, Tomas Winkler <tomasw@gmail.com>
>> wrote:
>> > > On Tue, May 3, 2016 at 9:36 AM, Arnd Bergmann <arnd@arndb.de>
>> wrote:
>> > >> On Monday 02 May 2016 16:32:25 Andrew Morton wrote:
>>
>> > >>  #ifdef __HAVE_BUILTIN_BSWAP32__
>> > >> -#define __swab32(x) __builtin_bswap32((__u32)(x))
>> > >> +#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
>> > >>  #else
>> > >>  #define __swab32(x)                            \
>> > >>         (__builtin_constant_p((__u32)(x)) ?     \
>> > >
>> > >>
>> > >
>> > > I wonder if this doesn't break switch statement that requires a
>> > > constant expression, there few cases like this over the kernel.
>> > >
>> > > switch(val) {
>> > > case cpu_to_le32(IXGBE_RXDADV_STAT_FCSTAT_FCPRSP):
>> > >
>> > > http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/ixgb
>> > > e/ixgbe_fcoe.c#L458
>> > >
>> >
>> > I'm asking because sparse and checkpatch doesn't agree on that ping
>> > sparse issues
>> > 'error: bad constant expression'
>> > When changing to __constant_cpu_to_le32 sparse is happy but
>> > checkpatch.ps is complaining
>> > __constant_cpu_to_le32 should be cpu_to_le32
>> >
>>
>> That is an interesting problem, as both seem to be reasonable:
>> sparse probably doesn't understand __builtin_constant_p() enough, while
>> avoiding __constant_cpu_to_le32() is a good recommendation in general.
>>
>> How many instances of this do you see in the kernel? If ixgbe is the only one,
>> I'd just move the byteswap up into the switch statement:
>>
>>       switch (le32_to_cpu(val)) {
>>       case IXGBE_RXDADV_STAT_FCSTAT_FCPRSP:
>>
>> which may cost one or two cycles for the non-constant byteswap, but is also
>> easier to read than the current code.
>>
>>       Arnd
>
> There are more than 20 files that have the statement: case cpu_to_...
> Sparse complains about: case __builtin_bswap, not about __builtin_constant_p.

There is even much more in the header files used in  initializers,
which also require constants.  I wonder if __builtin_bswap produces
constant expression correctly under gcc?

Thanks
Tomas

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-06-22 11:44                 ` Tomas Winkler
@ 2016-06-22 12:25                   ` Arnd Bergmann
  2016-06-23  6:27                     ` Tomas Winkler
  0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2016-06-22 12:25 UTC (permalink / raw)
  To: Tomas Winkler
  Cc: Levy, Amir (Jer),
	Andrew Morton, mm-commits, linux-kernel, linux-scsi, jpoimboe,
	Martin Jambor, Martin K. Petersen, James Bottomley,
	Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
	Ingo Molnar, Himanshu Madhani, Dept-Eng QLA2xxx Upstream,
	Jan Hubicka

On Wednesday, June 22, 2016 2:44:21 PM CEST Tomas Winkler wrote:
> >
> > There are more than 20 files that have the statement: case cpu_to_...
> > Sparse complains about: case __builtin_bswap, not about __builtin_constant_p.
> 
> There is even much more in the header files used in  initializers,
> which also require constants.  I wonder if __builtin_bswap produces
> constant expression correctly under gcc?

In gcc-4.8 or later yes. gcc-4.6/4.7 on powerpc was a special case that we
have worked around now, as the 16-bit byteswap there was not a constant
expression, unlike the 32-bit and 64-bit ones.

	Arnd

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-06-22 12:25                   ` Arnd Bergmann
@ 2016-06-23  6:27                     ` Tomas Winkler
  2016-06-23  9:29                       ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: Tomas Winkler @ 2016-06-23  6:27 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Levy, Amir (Jer),
	Andrew Morton, mm-commits, linux-kernel, linux-scsi, jpoimboe,
	Martin Jambor, Martin K. Petersen, James Bottomley,
	Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
	Ingo Molnar, Himanshu Madhani, Dept-Eng QLA2xxx Upstream,
	Jan Hubicka

On Wed, Jun 22, 2016 at 3:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, June 22, 2016 2:44:21 PM CEST Tomas Winkler wrote:
>> >
>> > There are more than 20 files that have the statement: case cpu_to_...
>> > Sparse complains about: case __builtin_bswap, not about __builtin_constant_p.
>>
>> There is even much more in the header files used in  initializers,
>> which also require constants.  I wonder if __builtin_bswap produces
>> constant expression correctly under gcc?
>
> In gcc-4.8 or later yes. gcc-4.6/4.7 on powerpc was a special case that we
> have worked around now, as the 16-bit byteswap there was not a constant
> expression, unlike the 32-bit and 64-bit ones.

Can you please give any references to that?
Just to make sure we are on the same line , the  bottom line is that
sparse has to be adjusted.

Thanks
Tomas

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug
  2016-06-23  6:27                     ` Tomas Winkler
@ 2016-06-23  9:29                       ` Arnd Bergmann
  0 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2016-06-23  9:29 UTC (permalink / raw)
  To: Tomas Winkler
  Cc: Levy, Amir (Jer),
	Andrew Morton, mm-commits, linux-kernel, linux-scsi, jpoimboe,
	Martin Jambor, Martin K. Petersen, James Bottomley,
	Denys Vlasenko, Thomas Graf, Peter Zijlstra, David Rientjes,
	Ingo Molnar, Himanshu Madhani, Dept-Eng QLA2xxx Upstream,
	Jan Hubicka

On Thursday, June 23, 2016 9:27:30 AM CEST Tomas Winkler wrote:
> On Wed, Jun 22, 2016 at 3:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday, June 22, 2016 2:44:21 PM CEST Tomas Winkler wrote:
> >> >
> >> > There are more than 20 files that have the statement: case cpu_to_...
> >> > Sparse complains about: case __builtin_bswap, not about __builtin_constant_p.
> >>
> >> There is even much more in the header files used in  initializers,
> >> which also require constants.  I wonder if __builtin_bswap produces
> >> constant expression correctly under gcc?
> >
> > In gcc-4.8 or later yes. gcc-4.6/4.7 on powerpc was a special case that we
> > have worked around now, as the 16-bit byteswap there was not a constant
> > expression, unlike the 32-bit and 64-bit ones.
> 
> Can you please give any references to that?

The patch description for the last change has a good explanation:

commit 8634de6d254462e6953b7dac772a1df4f44c8030
Author: Josh Poimboeuf <jpoimboe@redhat.com>
Date:   Fri May 6 09:22:25 2016 -0500

    compiler-gcc: require gcc 4.8 for powerpc __builtin_bswap16()
    
    gcc support for __builtin_bswap16() was supposedly added for powerpc in
    gcc 4.6, and was then later added for other architectures in gcc 4.8.
    
    However, Stephen Rothwell reported that attempting to use it on powerpc
    in gcc 4.6 fails with:
    
      lib/vsprintf.c:160:2: error: initializer element is not constant
      lib/vsprintf.c:160:2: error: (near initialization for 'decpair[0]')
      lib/vsprintf.c:160:2: error: initializer element is not constant
      lib/vsprintf.c:160:2: error: (near initialization for 'decpair[1]')
      ...
    
    I'm not entirely sure what those errors mean, but I don't see them on
    gcc 4.8.  So let's consider gcc 4.8 to be the official starting point
    for __builtin_bswap16().
    
    Arnd Bergmann adds:
     "I found the commit in gcc-4.8 that replaced the powerpc-specific
      implementation of __builtin_bswap16 with an architecture-independent
      one.  Apparently the powerpc version (gcc-4.6 and 4.7) just mapped to
      the lhbrx/sthbrx instructions, so it ended up not being a constant,
      though the intent of the patch was mainly to add support for the
      builtin to x86:
    
        https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52624
    
      has the patch that went into gcc-4.8 and more information."
    
    Fixes: 7322dd755e7d ("byteswap: try to avoid __builtin_constant_p gcc bug")
    Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
    Tested-by: Stephen Rothwell <sfr@canb.auug.org.au>
    Acked-by: Arnd Bergmann <arnd@arndb.de>
    Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
    Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

 include/linux/compiler-gcc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


> Just to make sure we are on the same line , the  bottom line is that
> sparse has to be adjusted.

Yes, that sounds right. I don't know if sparse actually tracks the
value of the constant, or if it's good enough for sparse to know that
a constant input to __builtin_bswap results in a constant output.
If that is the case, we could just #define __builtin_bswap32(x) (x)
in the kernel headers when building with sparse, and have it work
for older sparse versions too.

	Arnd

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-06-23  9:28 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-02 21:48 [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug Arnd Bergmann
2016-05-02 23:02 ` Andrew Morton
2016-05-02 23:10   ` Arnd Bergmann
2016-05-02 23:32     ` Andrew Morton
2016-05-02 23:32     ` Andrew Morton
2016-05-03  6:36       ` Arnd Bergmann
2016-06-21  9:02         ` Tomas Winkler
2016-06-21  9:02           ` Tomas Winkler
2016-06-22  8:24           ` Tomas Winkler
2016-06-22  8:24             ` Tomas Winkler
2016-06-22  9:59             ` Arnd Bergmann
2016-06-22 10:25               ` Levy, Amir (Jer)
2016-06-22 11:44                 ` Tomas Winkler
2016-06-22 12:25                   ` Arnd Bergmann
2016-06-23  6:27                     ` Tomas Winkler
2016-06-23  9:29                       ` Arnd Bergmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.