[PATCH] workqueue: fix enum type for gcc-13

[PATCH] workqueue: fix enum type for gcc-13

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

In gcc-13, the WORK_STRUCT_WQ_DATA_MASK constant is a signed 64-bit
type on 32-bit architectures because the enum definition has both
negative numbers and numbers above LONG_MAX in it:

kernel/workqueue.c: In function 'get_work_pwq':
kernel/workqueue.c:709:24: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  709 |                 return (void *)(data & WORK_STRUCT_WQ_DATA_MASK);
      |                        ^
kernel/workqueue.c: In function 'get_work_pool':
kernel/workqueue.c:737:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  737 |                 return ((struct pool_workqueue *)
      |                         ^
kernel/workqueue.c: In function 'get_work_pool_id':
kernel/workqueue.c:759:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  759 |                 return ((struct pool_workqueue *)
      |                         ^

Change the enum definition to ensure all values can fit into
the range of 'unsigned long' on all architectures.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/workqueue.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index ac551b8ee7d9..fba8d0154a1e 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -83,7 +83,7 @@ enum {
 
 	/* convenience constants */
 	WORK_STRUCT_FLAG_MASK	= (1UL << WORK_STRUCT_FLAG_BITS) - 1,
-	WORK_STRUCT_WQ_DATA_MASK = ~WORK_STRUCT_FLAG_MASK,
+	WORK_STRUCT_WQ_DATA_MASK = (unsigned long)~WORK_STRUCT_FLAG_MASK,
 	WORK_STRUCT_NO_POOL	= (unsigned long)WORK_OFFQ_POOL_NONE << WORK_OFFQ_POOL_SHIFT,
 
 	/* bit mask for work_busy() return values */
-- 
2.39.0

RE: [PATCH] workqueue: fix enum type for gcc-13

[David Laight]

From: Arnd Bergmann
> Sent: 17 January 2023 16:41
> 
> In gcc-13, the WORK_STRUCT_WQ_DATA_MASK constant is a signed 64-bit
> type on 32-bit architectures because the enum definition has both
> negative numbers and numbers above LONG_MAX in it:
> 
...
>  	/* convenience constants */
>  	WORK_STRUCT_FLAG_MASK	= (1UL << WORK_STRUCT_FLAG_BITS) - 1,
> -	WORK_STRUCT_WQ_DATA_MASK = ~WORK_STRUCT_FLAG_MASK,
> +	WORK_STRUCT_WQ_DATA_MASK = (unsigned long)~WORK_STRUCT_FLAG_MASK,
>  	WORK_STRUCT_NO_POOL	= (unsigned long)WORK_OFFQ_POOL_NONE << WORK_OFFQ_POOL_SHIFT,

How can that make any difference?
You aren't changing the value or type (which makes no difference)
of WORK_STRUCT_WQ_DATA_MASK.
Indeed you require it to have the high bit set.

So if the enum contains a -1 somewhere gcc-13 will promote
everything to s64.

Either declare gcc-13 as 'BROKEN' or change the:
	return (void *)(data & WORK_STRUCT_WQ_DATA_MASK);
to
	return (void *)(data & ~WORK_STRUCT_FLAG_MASK);

or use #defines.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] workqueue: fix enum type for gcc-13

[Tejun Heo]

Hello,

On Wed, Jan 18, 2023 at 09:31:18AM +0000, David Laight wrote:
> From: Arnd Bergmann
> > Sent: 17 January 2023 16:41
> > 
> > In gcc-13, the WORK_STRUCT_WQ_DATA_MASK constant is a signed 64-bit
> > type on 32-bit architectures because the enum definition has both
> > negative numbers and numbers above LONG_MAX in it:
> > 
> ...
> >  	/* convenience constants */
> >  	WORK_STRUCT_FLAG_MASK	= (1UL << WORK_STRUCT_FLAG_BITS) - 1,
> > -	WORK_STRUCT_WQ_DATA_MASK = ~WORK_STRUCT_FLAG_MASK,
> > +	WORK_STRUCT_WQ_DATA_MASK = (unsigned long)~WORK_STRUCT_FLAG_MASK,
> >  	WORK_STRUCT_NO_POOL	= (unsigned long)WORK_OFFQ_POOL_NONE << WORK_OFFQ_POOL_SHIFT,
> 
> How can that make any difference?
> You aren't changing the value or type (which makes no difference)
> of WORK_STRUCT_WQ_DATA_MASK.
> Indeed you require it to have the high bit set.
> 
> So if the enum contains a -1 somewhere gcc-13 will promote
> everything to s64.
> 
> Either declare gcc-13 as 'BROKEN' or change the:

I have a hard time understanding why gcc would change its behavior so that
there's no way to compile the same code in a consistent manner across two
adjacent compiler versions. The new behavior is fine but it makes no sense
to introduce it like this. If at all possible, marking gcc13 broken sounds
about right to me.

Thanks.

-- 
tejun

Re: [PATCH] workqueue: fix enum type for gcc-13

[Arnd Bergmann]

On Wed, Jan 18, 2023, at 17:38, Tejun Heo wrote:
>> From: Arnd Bergmann
>> > Sent: 17 January 2023 16:41
>> > 
>> > In gcc-13, the WORK_STRUCT_WQ_DATA_MASK constant is a signed 64-bit
>> > type on 32-bit architectures because the enum definition has both
>> > negative numbers and numbers above LONG_MAX in it:
>> > 
>> ...
>> >  	/* convenience constants */
>> >  	WORK_STRUCT_FLAG_MASK	= (1UL << WORK_STRUCT_FLAG_BITS) - 1,
>> > -	WORK_STRUCT_WQ_DATA_MASK = ~WORK_STRUCT_FLAG_MASK,
>> > +	WORK_STRUCT_WQ_DATA_MASK = (unsigned long)~WORK_STRUCT_FLAG_MASK,
>> >  	WORK_STRUCT_NO_POOL	= (unsigned long)WORK_OFFQ_POOL_NONE << WORK_OFFQ_POOL_SHIFT,
>
> I have a hard time understanding why gcc would change its behavior so that
> there's no way to compile the same code in a consistent manner across two
> adjacent compiler versions. The new behavior is fine but it makes no sense
> to introduce it like this. If at all possible, marking gcc13 broken sounds
> about right to me.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107405 has some more
information on the change. In short, the old behavior was a gcc
extension that was somewhat surprising and not well documented,
while the new behavior is consistent with C23 and C++ as well
as easier to understand: Any constant that is defined as part of
an enum now has the same type as the enum itself, even if it fits
within a shorter type. In a definition like

enum e {
   A = -1,
   B = -1u,
};

the enum type has to be compatible with 'long long' because
anything shorter would not fit both -1 and -1u (UINT_MAX).
A and B were both signed types to match the signedness of the
enum type, but A was actually a 32-bit integer since that is
sufficient, while B was also a 64-bit type since it exceeds
INT_MAX. Now they are both the same type.

I don't think there is a chance they will revert to the old behavior,
though we could try asking for an command line flag to warn about
cases where this changes code generation.

     Arnd

Re: [PATCH] workqueue: fix enum type for gcc-13

[Tejun Heo]

On Wed, Jan 18, 2023 at 09:32:45PM +0100, Arnd Bergmann wrote:
> the enum type has to be compatible with 'long long' because
> anything shorter would not fit both -1 and -1u (UINT_MAX).
> A and B were both signed types to match the signedness of the
> enum type, but A was actually a 32-bit integer since that is
> sufficient, while B was also a 64-bit type since it exceeds
> INT_MAX. Now they are both the same type.

Yeah, the new behavior makes total sense.

> I don't think there is a chance they will revert to the old behavior,
> though we could try asking for an command line flag to warn about
> cases where this changes code generation.

but the way that it's transitioning doesn't really make sense to me. We do
phase out support for old compilers, so it isn't that difficult to manage
this transition smoothly - add a compat option in the new versions, switch
code later once old compilers are phased out and drop the compat flag.

The way it's currently done causes situations which can't be handled
logically - we end up having to regroup enums based on their value range
rather than the logical type they need to be because there's no way to
override the enum type on older compilers.

Thanks.

-- 
tejun

RE: [PATCH] workqueue: fix enum type for gcc-13

[David Laight]

From: Tejun Heo
> Sent: 19 January 2023 01:42
> 
> On Wed, Jan 18, 2023 at 09:32:45PM +0100, Arnd Bergmann wrote:
> > the enum type has to be compatible with 'long long' because
> > anything shorter would not fit both -1 and -1u (UINT_MAX).
> > A and B were both signed types to match the signedness of the
> > enum type, but A was actually a 32-bit integer since that is
> > sufficient, while B was also a 64-bit type since it exceeds
> > INT_MAX. Now they are both the same type.
> 
> Yeah, the new behavior makes total sense.
> 
> > I don't think there is a chance they will revert to the old behavior,
> > though we could try asking for an command line flag to warn about
> > cases where this changes code generation.
> 
> but the way that it's transitioning doesn't really make sense to me. We do
> phase out support for old compilers, so it isn't that difficult to manage
> this transition smoothly - add a compat option in the new versions, switch
> code later once old compilers are phased out and drop the compat flag.
> 
> The way it's currently done causes situations which can't be handled
> logically - we end up having to regroup enums based on their value range
> rather than the logical type they need to be because there's no way to
> override the enum type on older compilers.

Didn't someone say that either C++ or C23 defines a syntax to explicitly
set the type of the enum.
IIRC gcc-13 doesn't support that.

If gcc-13 supported explicit setting of the type and also generated a
warn/error enum with 'large' values when the type wasn't specified
(ie where the gcc extension had been used) then at least there would
be no obscure surprises.
Getting an old gcc to ignore the type is easy.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] workqueue: fix enum type for gcc-13

[Thierry Reding]

On Tue, Jan 17, 2023 at 6:00 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> In gcc-13, the WORK_STRUCT_WQ_DATA_MASK constant is a signed 64-bit
> type on 32-bit architectures because the enum definition has both
> negative numbers and numbers above LONG_MAX in it:
>
> kernel/workqueue.c: In function 'get_work_pwq':
> kernel/workqueue.c:709:24: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>   709 |                 return (void *)(data & WORK_STRUCT_WQ_DATA_MASK);
>       |                        ^
> kernel/workqueue.c: In function 'get_work_pool':
> kernel/workqueue.c:737:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>   737 |                 return ((struct pool_workqueue *)
>       |                         ^
> kernel/workqueue.c: In function 'get_work_pool_id':
> kernel/workqueue.c:759:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>   759 |                 return ((struct pool_workqueue *)
>       |                         ^
>
> Change the enum definition to ensure all values can fit into
> the range of 'unsigned long' on all architectures.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/linux/workqueue.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Tested-by: Thierry Reding <treding@nvidia.com>

Re: [PATCH] workqueue: fix enum type for gcc-13

[Lai Jiangshan]

On Wed, Jan 18, 2023 at 12:40 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> In gcc-13, the WORK_STRUCT_WQ_DATA_MASK constant is a signed 64-bit
> type on 32-bit architectures because the enum definition has both
> negative numbers and numbers above LONG_MAX in it:
>
> kernel/workqueue.c: In function 'get_work_pwq':
> kernel/workqueue.c:709:24: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>   709 |                 return (void *)(data & WORK_STRUCT_WQ_DATA_MASK);
>       |                        ^
> kernel/workqueue.c: In function 'get_work_pool':
> kernel/workqueue.c:737:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>   737 |                 return ((struct pool_workqueue *)
>       |                         ^
> kernel/workqueue.c: In function 'get_work_pool_id':
> kernel/workqueue.c:759:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>   759 |                 return ((struct pool_workqueue *)
>       |                         ^
>
> Change the enum definition to ensure all values can fit into
> the range of 'unsigned long' on all architectures.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/linux/workqueue.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index ac551b8ee7d9..fba8d0154a1e 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -83,7 +83,7 @@ enum {
>
>         /* convenience constants */
>         WORK_STRUCT_FLAG_MASK   = (1UL << WORK_STRUCT_FLAG_BITS) - 1,
> -       WORK_STRUCT_WQ_DATA_MASK = ~WORK_STRUCT_FLAG_MASK,
> +       WORK_STRUCT_WQ_DATA_MASK = (unsigned long)~WORK_STRUCT_FLAG_MASK,
>         WORK_STRUCT_NO_POOL     = (unsigned long)WORK_OFFQ_POOL_NONE << WORK_OFFQ_POOL_SHIFT,

Tested with "old" gcc: 7.3.1

Tested-by: Lai Jiangshan<jiangshanlai@gmail.com>

But I personally prefer to redefine them as non-enum.

>
>         /* bit mask for work_busy() return values */
> --
> 2.39.0
>

Re: [PATCH] workqueue: fix enum type for gcc-13

[Tejun Heo]

Applied to wq/for-6.5.

Thanks.

-- 
tejun