| Message ID | 20221031114520.10518-1-jirislaby@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/blk-iocost (gcc13): cast enum members to int in prints | expand |
On Mon, Oct 31, 2022 at 12:45:20PM +0100, Jiri Slaby (SUSE) wrote: > Cast the enum members to int when printing them. > > Alternatively, we can cast them to ulong (to silence gcc < 12) and use %lu. > Alternatively, we can move VTIME_PER_SEC away from the enum. Yes, either split the enum or just use a define. But casts are a big code smell and should be avoided if there is a reasonable alternative.
On Mon, Oct 31, 2022 at 05:24:28AM -0700, Christoph Hellwig wrote: > On Mon, Oct 31, 2022 at 12:45:20PM +0100, Jiri Slaby (SUSE) wrote: > > Cast the enum members to int when printing them. > > > > Alternatively, we can cast them to ulong (to silence gcc < 12) and use %lu. > > Alternatively, we can move VTIME_PER_SEC away from the enum. > > Yes, either split the enum or just use a define. But casts are a big > code smell and should be avoided if there is a reasonable alternative. enums are so much better for debugging and other instrumentation stuff. The only requirement for the enum types is that they're big enough to express all the members and we can use whatever printf format letter which matches the type in use. The problem here is that the compiler behavior is different depending on the compiler version, which kinda sucks. I suppose the most reasonable thing to do here is just splitting them into separate enum definitions. Does anyone know how this behavior change came to be? Do we know whether clang is gonna be changed the same way? Thanks.
On 31. 10. 22, 18:57, Tejun Heo wrote: > On Mon, Oct 31, 2022 at 05:24:28AM -0700, Christoph Hellwig wrote: >> On Mon, Oct 31, 2022 at 12:45:20PM +0100, Jiri Slaby (SUSE) wrote: >>> Cast the enum members to int when printing them. >>> >>> Alternatively, we can cast them to ulong (to silence gcc < 12) and use %lu. >>> Alternatively, we can move VTIME_PER_SEC away from the enum. >> >> Yes, either split the enum or just use a define. But casts are a big >> code smell and should be avoided if there is a reasonable alternative. > > enums are so much better for debugging and other instrumentation stuff. The > only requirement for the enum types is that they're big enough to express > all the members and we can use whatever printf format letter which matches > the type in use. The problem here is that the compiler behavior is different > depending on the compiler version, which kinda sucks. Yes. The real problem is that using anything else then an INT_MIN <= x <= INT_MAX _constant_ in an enum is undefined in ANSI C < 2x (in particular, 1 << x is undefined too). gcc manual defines unsigned int on the top of that as defined too (so this holds for our -std=g*). > I suppose the most reasonable thing to do here is just splitting them into > separate enum definitions. Does anyone know how this behavior change came to > be? C2x which introduces un/signed long enums. See the bug I linked in the commit log: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36113 The change is also turned on in < C2x on purpose. AIUI, unless there is too much breakage. So we'd need to sort it out in (rather distant) future anyway (when we come up to -std=g2x). > Do we know whether clang is gonna be changed the same way? In C2x, Likely. In < C2x, dunno what'd be the default. thanks,
Hello, On Tue, Nov 01, 2022 at 06:46:56AM +0100, Jiri Slaby wrote: > Yes. The real problem is that using anything else then an INT_MIN <= x <= > INT_MAX _constant_ in an enum is undefined in ANSI C < 2x (in particular, 1 > << x is undefined too). gcc manual defines unsigned int on the top of that > as defined too (so this holds for our -std=g*). > > > I suppose the most reasonable thing to do here is just splitting them into > > separate enum definitions. Does anyone know how this behavior change came to > > be? > > C2x which introduces un/signed long enums. See the bug I linked in the > commit log: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36113 I see. So, it was an extension but the new standard is defined differently and we're gonna end up with that behavior. > The change is also turned on in < C2x on purpose. AIUI, unless there is too > much breakage. So we'd need to sort it out in (rather distant) future anyway > (when we come up to -std=g2x). The part that the new behavior applying to <C2x feels like an odd decision. I'm having a hard time seeing the upsides in doing so but maybe that's just me not knowing the area well enough. > > Do we know whether clang is gonna be changed the same way? > > In C2x, Likely. In < C2x, dunno what'd be the default. It looks like we can do one of the following two: * If gcc actually changes the behavior for <c2x, split the enums according to their sizes. This feels rather silly but I can't think of a better way to cater to divergent compiler behaviors. * If gcc doesn't change the behavior for <c2x, there's nothing to do for the time being. Later when we switch to -std=g2x, we can just change the format strings to use the now larger types. Does the above make sense? Thanks.
From: Tejun Heo > Sent: 01 November 2022 16:47 > > Hello, > > On Tue, Nov 01, 2022 at 06:46:56AM +0100, Jiri Slaby wrote: > > Yes. The real problem is that using anything else then an INT_MIN <= x <= > > INT_MAX _constant_ in an enum is undefined in ANSI C < 2x (in particular, 1 > > << x is undefined too). gcc manual defines unsigned int on the top of that > > as defined too (so this holds for our -std=g*). > > > > > I suppose the most reasonable thing to do here is just splitting them into > > > separate enum definitions. Does anyone know how this behavior change came to > > > be? > > > > C2x which introduces un/signed long enums. See the bug I linked in the > > commit log: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36113 > > I see. So, it was an extension but the new standard is defined differently > and we're gonna end up with that behavior. > > > The change is also turned on in < C2x on purpose. AIUI, unless there is too > > much breakage. So we'd need to sort it out in (rather distant) future anyway > > (when we come up to -std=g2x). > > The part that the new behavior applying to <C2x feels like an odd decision. > I'm having a hard time seeing the upsides in doing so but maybe that's just > me not knowing the area well enough. > > > > Do we know whether clang is gonna be changed the same way? > > > > In C2x, Likely. In < C2x, dunno what'd be the default. > > It looks like we can do one of the following two: > > * If gcc actually changes the behavior for <c2x, split the enums according > to their sizes. This feels rather silly but I can't think of a better way > to cater to divergent compiler behaviors. > > * If gcc doesn't change the behavior for <c2x, there's nothing to do for the > time being. Later when we switch to -std=g2x, we can just change the > format strings to use the now larger types. > > Does the above make sense? I think the enums have to be split. There will be other side effects of promoting the constants to 64bit that are much more difficult to detect than the warnings from printf. I'm also not sure whether the type is even consistent for 32bit and 64bit builds. Casts are (sort of) horrid. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Nov 02, 2022 at 08:35:34AM +0000, David Laight wrote: > I think the enums have to be split. > There will be other side effects of promoting the constants to 64bit > that are much more difficult to detect than the warnings from printf. idk, I think I can just add LLU to everything and it should be fine. > I'm also not sure whether the type is even consistent for 32bit > and 64bit builds. > Casts are (sort of) horrid. Yeah, I don't think casts are the solution either. Lemme add LLU to everything and see how it works. Thanks.
On Wed, Nov 02, 2022 at 06:27:46AM -1000, 'Tejun Heo' wrote: > On Wed, Nov 02, 2022 at 08:35:34AM +0000, David Laight wrote: > > I think the enums have to be split. > > There will be other side effects of promoting the constants to 64bit > > that are much more difficult to detect than the warnings from printf. > > idk, I think I can just add LLU to everything and it should be fine. > > > I'm also not sure whether the type is even consistent for 32bit > > and 64bit builds. > > Casts are (sort of) horrid. > > Yeah, I don't think casts are the solution either. Lemme add LLU to > everything and see how it works. So adding LLU to initializers don't make the specific enum's type follow suit. I guess type determination is really based on the value range. Oh man, what a mess. If we end up having to split the enum defs, that's what we'll do but this doesn't sense to me. It's one thing to make one time adjustment when we adopt -std=g2x. That's fine, but it makes no sense for the compiler to change type behavior underneath existing code bases in a way that prevents the same code to mean the same thing in adjacent and recent compiler versions. Even if gcc goes for that for whatever reason, there gotta be an option to keep the original behavior, right? If so, my suggestion is just sticking with the old behavior until we switch to --std=g2x and then make one time adjustment at that point. Thanks.
On 02. 11. 22, 17:43, 'Tejun Heo' wrote: > On Wed, Nov 02, 2022 at 06:27:46AM -1000, 'Tejun Heo' wrote: >> On Wed, Nov 02, 2022 at 08:35:34AM +0000, David Laight wrote: >>> I think the enums have to be split. >>> There will be other side effects of promoting the constants to 64bit >>> that are much more difficult to detect than the warnings from printf. >> >> idk, I think I can just add LLU to everything and it should be fine. >> >>> I'm also not sure whether the type is even consistent for 32bit >>> and 64bit builds. >>> Casts are (sort of) horrid. >> >> Yeah, I don't think casts are the solution either. Lemme add LLU to >> everything and see how it works. > > So adding LLU to initializers don't make the specific enum's type follow > suit. I guess type determination is really based on the value range. Oh man, > what a mess. > > If we end up having to split the enum defs, that's what we'll do but this > doesn't sense to me. It's one thing to make one time adjustment when we > adopt -std=g2x. That's fine, but it makes no sense for the compiler to > change type behavior underneath existing code bases in a way that prevents > the same code to mean the same thing in adjacent and recent compiler > versions. Even if gcc goes for that for whatever reason, there gotta be an > option to keep the original behavior, right? Unfortunately not, see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107405#c8 (linked also from the commit log). We'd use such an option if there were one. > If so, my suggestion is just sticking with the old behavior until we switch > to --std=g2x and then make one time adjustment at that point. So is the enum split OK under these circumstances? thanks,
On Mon, Dec 12, 2022 at 01:14:31PM +0100, Jiri Slaby wrote: > > If so, my suggestion is just sticking with the old behavior until we switch > > to --std=g2x and then make one time adjustment at that point. > > So is the enum split OK under these circumstances? Oh man, it's kinda crazy that the compiler is changing in a way that the same piece of code can't be compiled the same way across two adjoining versions of the same compiler. But, yeah, if that's what gcc is gonna do and splitting enums is the only way to be okay across the compiler versions, there isn't any other choice we can make. Thanks.
From: Tejun Heo <htejun@gmail.com> On Behalf Of 'Tejun Heo' > Sent: 12 December 2022 21:47 > To: Jiri Slaby <jirislaby@kernel.org> > Cc: David Laight <David.Laight@ACULAB.COM>; Christoph Hellwig <hch@infradead.org>; linux- > kernel@vger.kernel.org; Martin Liska <mliska@suse.cz>; Josef Bacik <josef@toxicpanda.com>; Jens Axboe > <axboe@kernel.dk>; cgroups@vger.kernel.org; linux-block@vger.kernel.org > Subject: Re: [PATCH] block/blk-iocost (gcc13): cast enum members to int in prints > > On Mon, Dec 12, 2022 at 01:14:31PM +0100, Jiri Slaby wrote: > > > If so, my suggestion is just sticking with the old behavior until we switch > > > to --std=g2x and then make one time adjustment at that point. > > > > So is the enum split OK under these circumstances? > > Oh man, it's kinda crazy that the compiler is changing in a way that the > same piece of code can't be compiled the same way across two adjoining > versions of the same compiler. But, yeah, if that's what gcc is gonna do and > splitting enums is the only way to be okay across the compiler versions, > there isn't any other choice we can make. It is also a silent code-breaker. Compile this for 32bit x86: enum { a = 1, b = ~0ull}; extern int foo(int, ...); int f(void) { return foo(0, a, 2); } gcc13 pushes an extra zero onto the stack between the 1 and 2. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 13. 12. 22, 9:30, David Laight wrote: > From: Tejun Heo <htejun@gmail.com> On Behalf Of 'Tejun Heo' >> Sent: 12 December 2022 21:47 >> To: Jiri Slaby <jirislaby@kernel.org> >> Cc: David Laight <David.Laight@ACULAB.COM>; Christoph Hellwig <hch@infradead.org>; linux- >> kernel@vger.kernel.org; Martin Liska <mliska@suse.cz>; Josef Bacik <josef@toxicpanda.com>; Jens Axboe >> <axboe@kernel.dk>; cgroups@vger.kernel.org; linux-block@vger.kernel.org >> Subject: Re: [PATCH] block/blk-iocost (gcc13): cast enum members to int in prints >> >> On Mon, Dec 12, 2022 at 01:14:31PM +0100, Jiri Slaby wrote: >>>> If so, my suggestion is just sticking with the old behavior until we switch >>>> to --std=g2x and then make one time adjustment at that point. >>> >>> So is the enum split OK under these circumstances? >> >> Oh man, it's kinda crazy that the compiler is changing in a way that the >> same piece of code can't be compiled the same way across two adjoining >> versions of the same compiler. But, yeah, if that's what gcc is gonna do and >> splitting enums is the only way to be okay across the compiler versions, >> there isn't any other choice we can make. > > It is also a silent code-breaker. > Compile this for 32bit x86: > > enum { a = 1, b = ~0ull}; But having ull in an enum is undefined anyway. C99 allows only int constants. gnuC supports ulong expressions (IIRC). > extern int foo(int, ...); > int f(void) > { > return foo(0, a, 2); > } > > gcc13 pushes an extra zero onto the stack between the 1 and 2. So this is sort of "expected". thanks,
From: Jiri Slaby > Sent: 13 December 2022 11:15 > > On 13. 12. 22, 9:30, David Laight wrote: > > From: Tejun Heo <htejun@gmail.com> On Behalf Of 'Tejun Heo' > >> Sent: 12 December 2022 21:47 > >> To: Jiri Slaby <jirislaby@kernel.org> > >> Cc: David Laight <David.Laight@ACULAB.COM>; Christoph Hellwig <hch@infradead.org>; linux- > >> kernel@vger.kernel.org; Martin Liska <mliska@suse.cz>; Josef Bacik <josef@toxicpanda.com>; Jens > Axboe > >> <axboe@kernel.dk>; cgroups@vger.kernel.org; linux-block@vger.kernel.org > >> Subject: Re: [PATCH] block/blk-iocost (gcc13): cast enum members to int in prints > >> > >> On Mon, Dec 12, 2022 at 01:14:31PM +0100, Jiri Slaby wrote: > >>>> If so, my suggestion is just sticking with the old behavior until we switch > >>>> to --std=g2x and then make one time adjustment at that point. > >>> > >>> So is the enum split OK under these circumstances? > >> > >> Oh man, it's kinda crazy that the compiler is changing in a way that the > >> same piece of code can't be compiled the same way across two adjoining > >> versions of the same compiler. But, yeah, if that's what gcc is gonna do and > >> splitting enums is the only way to be okay across the compiler versions, > >> there isn't any other choice we can make. > > > > It is also a silent code-breaker. > > Compile this for 32bit x86: > > > > enum { a = 1, b = ~0ull}; > > But having ull in an enum is undefined anyway. C99 allows only int > constants. gnuC supports ulong expressions (IIRC). gcc supports 'long long' as well - 64bit on 32bit systems. In practical terms it really doesn't matter what C99 (or any other version) says, the important thing is that the compiler accepted it. > > extern int foo(int, ...); > > int f(void) > > { > > return foo(0, a, 2); > > } > > > > gcc13 pushes an extra zero onto the stack between the 1 and 2. > > So this is sort of "expected". For some definitions of "expected" :-) Note that it (probably) makes no actual difference to some architectures (like 64bit x86) where all varargs parameters are passed as 64bit. Extending a value to 64bits just makes the high bits well defined. (The high bits of stacked 32bit args are undefined.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 13. 12. 22, 12:50, David Laight wrote: > From: Jiri Slaby >> Sent: 13 December 2022 11:15 >> >> On 13. 12. 22, 9:30, David Laight wrote: >>> From: Tejun Heo <htejun@gmail.com> On Behalf Of 'Tejun Heo' >>>> Sent: 12 December 2022 21:47 >>>> To: Jiri Slaby <jirislaby@kernel.org> >>>> Cc: David Laight <David.Laight@ACULAB.COM>; Christoph Hellwig <hch@infradead.org>; linux- >>>> kernel@vger.kernel.org; Martin Liska <mliska@suse.cz>; Josef Bacik <josef@toxicpanda.com>; Jens >> Axboe >>>> <axboe@kernel.dk>; cgroups@vger.kernel.org; linux-block@vger.kernel.org >>>> Subject: Re: [PATCH] block/blk-iocost (gcc13): cast enum members to int in prints >>>> >>>> On Mon, Dec 12, 2022 at 01:14:31PM +0100, Jiri Slaby wrote: >>>>>> If so, my suggestion is just sticking with the old behavior until we switch >>>>>> to --std=g2x and then make one time adjustment at that point. >>>>> >>>>> So is the enum split OK under these circumstances? >>>> >>>> Oh man, it's kinda crazy that the compiler is changing in a way that the >>>> same piece of code can't be compiled the same way across two adjoining >>>> versions of the same compiler. But, yeah, if that's what gcc is gonna do and >>>> splitting enums is the only way to be okay across the compiler versions, >>>> there isn't any other choice we can make. >>> >>> It is also a silent code-breaker. >>> Compile this for 32bit x86: >>> >>> enum { a = 1, b = ~0ull}; >> >> But having ull in an enum is undefined anyway. C99 allows only int >> constants. gnuC supports ulong expressions (IIRC). > > gcc supports 'long long' as well - 64bit on 32bit systems. Can you elaborate what's source of this? Gcc manual says this about enum values: The integer type compatible with each enumerated type (C90 6.5.2.2, C99 and C11 6.7.2.2). Normally, the type is unsigned int if there are no negative values in the enumeration, otherwise int. If ‘-fshort-enums’ is specified, ..., otherwise it is the first of unsigned char, unsigned short and unsigned int that can represent all the values. I.e. the documentation says uint is the highest possible enum value. C2x/g2x also supports ulong (that's what it is all about). But we don't do c2x quite yet. thanks,
From: Jiri Slaby <jirislaby@kernel.org> > Sent: 13 December 2022 12:05 > > On 13. 12. 22, 12:50, David Laight wrote: > > From: Jiri Slaby > >> Sent: 13 December 2022 11:15 > >> ... > >>>> Oh man, it's kinda crazy that the compiler is changing in a way that the > >>>> same piece of code can't be compiled the same way across two adjoining > >>>> versions of the same compiler. But, yeah, if that's what gcc is gonna do and > >>>> splitting enums is the only way to be okay across the compiler versions, > >>>> there isn't any other choice we can make. > >>> > >>> It is also a silent code-breaker. > >>> Compile this for 32bit x86: > >>> > >>> enum { a = 1, b = ~0ull}; > >> > >> But having ull in an enum is undefined anyway. C99 allows only int > >> constants. gnuC supports ulong expressions (IIRC). > > > > gcc supports 'long long' as well - 64bit on 32bit systems. > > Can you elaborate what's source of this? ... Experimentation, for example: https://godbolt.org/z/n4rnc7cKG David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/block/blk-iocost.c b/block/blk-iocost.c index f01359906c83..a257ba17183b 100644 --- a/block/blk-iocost.c +++ b/block/blk-iocost.c @@ -3034,7 +3034,8 @@ static u64 ioc_weight_prfill(struct seq_file *sf, struct blkg_policy_data *pd, struct ioc_gq *iocg = pd_to_iocg(pd); if (dname && iocg->cfg_weight) - seq_printf(sf, "%s %u\n", dname, iocg->cfg_weight / WEIGHT_ONE); + seq_printf(sf, "%s %d\n", dname, + iocg->cfg_weight / (int)WEIGHT_ONE); return 0; } @@ -3044,7 +3045,8 @@ static int ioc_weight_show(struct seq_file *sf, void *v) struct blkcg *blkcg = css_to_blkcg(seq_css(sf)); struct ioc_cgrp *iocc = blkcg_to_iocc(blkcg); - seq_printf(sf, "default %u\n", iocc->dfl_weight / WEIGHT_ONE); + seq_printf(sf, "default %d\n", + iocc->dfl_weight / (int)WEIGHT_ONE); blkcg_print_blkgs(sf, blkcg, ioc_weight_prfill, &blkcg_policy_iocost, seq_cft(sf)->private, false); return 0;
Since gcc13, each member of an enum has the same type as the enum [1]. And that is inherited from its members. Provided: VTIME_PER_SEC_SHIFT = 37, VTIME_PER_SEC = 1LLU << VTIME_PER_SEC_SHIFT, the named type is unsigned long. This generates warnings with gcc-13: block/blk-iocost.c: In function 'ioc_weight_prfill': block/blk-iocost.c:3037:37: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' block/blk-iocost.c: In function 'ioc_weight_show': block/blk-iocost.c:3047:34: error: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'long unsigned int' Cast the enum members to int when printing them. Alternatively, we can cast them to ulong (to silence gcc < 12) and use %lu. Alternatively, we can move VTIME_PER_SEC away from the enum. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36113 Cc: Martin Liska <mliska@suse.cz> Cc: Tejun Heo <tj@kernel.org> Cc: Josef Bacik <josef@toxicpanda.com> Cc: Jens Axboe <axboe@kernel.dk> Cc: cgroups@vger.kernel.org Cc: linux-block@vger.kernel.org Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org> --- block/blk-iocost.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)