2023-07-17 19:13:39

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] blk-iocost: fix seq_printf compile type mismatch error

On Mon, Jul 17, 2023 at 09:18:52AM -0500, Carlos Bilbao wrote:
> From: amd <[email protected]>
>
> Fix two type mismatch errors encountered while compiling blk-iocost.c with
> GCC version 13.1.1 that involved constant operator WEIGHT_ONE. Cast the
> result of the division operation to (unsigned int) to match the expected
> format specifier %u in two seq_printf invocations.

Can you detail the warnings? Was that on 32bit compiles?

Thanks.

--
tejun


2023-07-18 16:20:46

by Bilbao, Carlos

[permalink] [raw]
Subject: Re: [PATCH] blk-iocost: fix seq_printf compile type mismatch error

On 7/17/23 13:49, Tejun Heo wrote:
> On Mon, Jul 17, 2023 at 09:18:52AM -0500, Carlos Bilbao wrote:
>> From: amd <[email protected]>
>>
>> Fix two type mismatch errors encountered while compiling blk-iocost.c with
>> GCC version 13.1.1 that involved constant operator WEIGHT_ONE. Cast the
>> result of the division operation to (unsigned int) to match the expected
>> format specifier %u in two seq_printf invocations.
>
> Can you detail the warnings? Was that on 32bit compiles?

The concrete error was: "format ‘%u’ expects argument of type ‘unsigned
int’, but argument 3 has type ‘long unsigned int’". If I run:

$ echo | cpp -dM | grep __LP64__
#define __LP64__ 1

which makes me believe it is not 32 bits.

>
> Thanks.
>

Thanks,
Carlos

2023-07-19 09:04:10

by David Laight

[permalink] [raw]
Subject: RE: [PATCH] blk-iocost: fix seq_printf compile type mismatch error

From: Tejun Heo
> Sent: 17 July 2023 19:49
>
> On Mon, Jul 17, 2023 at 09:18:52AM -0500, Carlos Bilbao wrote:
> > From: amd <[email protected]>
> >
> > Fix two type mismatch errors encountered while compiling blk-iocost.c with
> > GCC version 13.1.1 that involved constant operator WEIGHT_ONE. Cast the
> > result of the division operation to (unsigned int) to match the expected
> > format specifier %u in two seq_printf invocations.
>
> Can you detail the warnings? Was that on 32bit compiles?

The problem is caused by gcc 13 changing the types of the
constants inside an enum to be all the same.

The best fix is (probably) to replace all the enum used to
define unrelated constants with #defines.

David

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


2023-07-20 20:21:54

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] blk-iocost: fix seq_printf compile type mismatch error

On Tue, Jul 18, 2023 at 10:37:23AM -0500, Carlos Bilbao wrote:
> On 7/17/23 13:49, Tejun Heo wrote:
> > On Mon, Jul 17, 2023 at 09:18:52AM -0500, Carlos Bilbao wrote:
> > > From: amd <[email protected]>
> > >
> > > Fix two type mismatch errors encountered while compiling blk-iocost.c with
> > > GCC version 13.1.1 that involved constant operator WEIGHT_ONE. Cast the
> > > result of the division operation to (unsigned int) to match the expected
> > > format specifier %u in two seq_printf invocations.
> >
> > Can you detail the warnings? Was that on 32bit compiles?
>
> The concrete error was: "format ‘%u’ expects argument of type ‘unsigned
> int’, but argument 3 has type ‘long unsigned int’". If I run:
>
> $ echo | cpp -dM | grep __LP64__
> #define __LP64__ 1
>
> which makes me believe it is not 32 bits.

So, we broke up the enum definitions so that WEIGHT_ONE doesn't end up being
a ulong. Which kernel are you building? Can you plesae try the current
linus#master?

Thanks.

--
tejun

2023-07-20 21:01:41

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] blk-iocost: fix seq_printf compile type mismatch error

On Wed, Jul 19, 2023 at 08:57:32AM +0000, David Laight wrote:
> From: Tejun Heo
> > Sent: 17 July 2023 19:49
> >
> > On Mon, Jul 17, 2023 at 09:18:52AM -0500, Carlos Bilbao wrote:
> > > From: amd <[email protected]>
> > >
> > > Fix two type mismatch errors encountered while compiling blk-iocost.c with
> > > GCC version 13.1.1 that involved constant operator WEIGHT_ONE. Cast the
> > > result of the division operation to (unsigned int) to match the expected
> > > format specifier %u in two seq_printf invocations.
> >
> > Can you detail the warnings? Was that on 32bit compiles?
>
> The problem is caused by gcc 13 changing the types of the
> constants inside an enum to be all the same.
>
> The best fix is (probably) to replace all the enum used to
> define unrelated constants with #defines.

Yeah, but then you end up without any way to read that value from outside
the kernel for BPF, drgn or any other tools which use debug info. That
actually matters.

Thanks.

--
tejun

2023-07-21 08:20:14

by David Laight

[permalink] [raw]
Subject: RE: [PATCH] blk-iocost: fix seq_printf compile type mismatch error

From: Tejun Heo
> Sent: 20 July 2023 21:07
>
> On Wed, Jul 19, 2023 at 08:57:32AM +0000, David Laight wrote:
> > From: Tejun Heo
> > > Sent: 17 July 2023 19:49
> > >
> > > On Mon, Jul 17, 2023 at 09:18:52AM -0500, Carlos Bilbao wrote:
> > > > From: amd <[email protected]>
> > > >
> > > > Fix two type mismatch errors encountered while compiling blk-iocost.c with
> > > > GCC version 13.1.1 that involved constant operator WEIGHT_ONE. Cast the
> > > > result of the division operation to (unsigned int) to match the expected
> > > > format specifier %u in two seq_printf invocations.
> > >
> > > Can you detail the warnings? Was that on 32bit compiles?
> >
> > The problem is caused by gcc 13 changing the types of the
> > constants inside an enum to be all the same.
> >
> > The best fix is (probably) to replace all the enum used to
> > define unrelated constants with #defines.
>
> Yeah, but then you end up without any way to read that value from outside
> the kernel for BPF, drgn or any other tools which use debug info. That
> actually matters.

Some of those constants (probably including the one that forces
the enum to 'long' are very boring.
I don't remember which one caused the change, but some were
similar to 'microseconds in a second'.

In any case it is enough to split the enum.
If you really need unrelated constants to be defined in an enum
them maybe use a separate enum for each.
Using (on one line):
enum { name = constant };
may work best.

David

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


2023-07-21 18:39:33

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] blk-iocost: fix seq_printf compile type mismatch error

Hello,

On Fri, Jul 21, 2023 at 08:05:59AM +0000, David Laight wrote:
> In any case it is enough to split the enum.
> If you really need unrelated constants to be defined in an enum
> them maybe use a separate enum for each.
> Using (on one line):
> enum { name = constant };

Yeah, I'm hoping it won't come down to that. Hopefully, we can limp along
like this until we can always assume the new behavior. Right now, the
problem is that both gcc<13 and gcc=13 have to supported when the two assign
different types to the same enum definitions.

Thanks.

--
tejun