| Message ID | 20241113172939.747686-5-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add const_true() to simplify GENMASK_INPUT_CHECK() | expand |
On Thu, Nov 14, 2024 at 02:18:32AM +0900, Vincent Mailhol wrote: > __builtin_constant_p() is known for not always being able to produce > constant expression [1] which led to the introduction of > __is_constexpr() [2]. Because of its dependency on > __builtin_constant_p(), statically_true() suffers from the same > issues. > > For example: > > void foo(int a) > { > /* fail on GCC */ > BUILD_BUG_ON_ZERO(statically_true(a)); > > /* fail on both clang and GCC */ > static char arr[statically_true(a) ? 1 : 2]; > } > > For the same reasons why __is_constexpr() was created to cover > __builtin_constant_p() edge cases, __is_constexpr() can be used to > resolve statically_true() limitations. > > Note that, somehow, GCC is not always able to fold this: > > __is_constexpr(x) && (x) > > It is OK in BUILD_BUG_ON_ZERO() but not in array declarations nor in > static_assert(): > > void bar(int a) > { > /* success */ > BUILD_BUG_ON_ZERO(__is_constexpr(a) && (a)); > > /* fail on GCC */ > static char arr[__is_constexpr(a) && (a) ? 1 : 2]; > > /* fail on GCC */ > static_assert(__is_constexpr(a) && (a)); > } > > Encapsulating the expression in a __builtin_choose_expr() switch > resolves all these failed tests. > > Define a new const_true() macro which, by making use of the > __builtin_choose_expr() and __is_constexpr(x) combo, always produces a > constant expression. > > It should be noted that statically_true() is the only one able to fold > tautologic expressions in which at least one on the operands is not a > constant expression. For example: > > statically_true(true || var) > statically_true(var == var) > statically_true(var * 0 + 1) > statically_true(!(var * 8 % 4)) > > always evaluates to true, whereas all of these would be false under > const_true() if var is not a constant expression [3]. > > For this reason, usage of const_true() be should the exception. > Reflect in the documentation that const_true() is less powerful and > that statically_true() is the overall preferred solution. > > [1] __builtin_constant_p cannot resolve to const when optimizing > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449 > > [2] commit 3c8ba0d61d04 ("kernel.h: Retain constant expression output for max()/min()") > Link: https://git.kernel.org/torvalds/c/3c8ba0d61d04 > > [3] https://godbolt.org/z/c61PMxqbK > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> For the series: Reviewed-by: Yury Norov <yury.norov@gmail.com> If no objections, I'll move it with my tree. Thanks, Yury > --- > Above examples, and a bit more: > > https://godbolt.org/z/11xnxfx3P > --- > include/linux/compiler.h | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index 4d4e23b6e3e7..f9d660b63765 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -308,6 +308,28 @@ static inline void *offset_to_ptr(const int *off) > */ > #define statically_true(x) (__builtin_constant_p(x) && (x)) > > +/* > + * Similar to statically_true() but produces a constant expression > + * > + * To be used in conjunction with macros, such as BUILD_BUG_ON_ZERO(), > + * which require their input to be a constant expression and for which > + * statically_true() would otherwise fail. > + * > + * This is a trade-off: const_true() requires all its operands to be > + * compile time constants. Else, it would always returns false even on > + * the most trivial cases like: > + * > + * true || non_const_var > + * > + * On the opposite, statically_true() is able to fold more complex > + * tautologies and will return true on expressions such as: > + * > + * !(non_const_var * 8 % 4) > + * > + * For the general case, statically_true() is better. > + */ > +#define const_true(x) __builtin_choose_expr(__is_constexpr(x), x, false) > + > /* > * This is needed in functions which generate the stack canary, see > * arch/x86/kernel/smpboot.c::start_secondary() for an example. > -- > 2.45.2
From: Vincent Mailhol > Sent: 13 November 2024 17:19 > > __builtin_constant_p() is known for not always being able to produce > constant expression [1] which led to the introduction of > __is_constexpr() [2]. Because of its dependency on > __builtin_constant_p(), statically_true() suffers from the same > issues. Chalk and cheese. Personally I don't think any of the text below is really needed. You might want the final short form - but it is a short form. OTOH the implementation is horrid. You probably want to start with this (posted a while back in a minmax patch set: diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 2594553bb30b..35d5b2fa4786 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -242,6 +242,23 @@ static inline void *offset_to_ptr(const int *off) /* &a[0] degrades to a pointer: a different type from an array */ #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) +/** + * __if_constexpr - Check whether an expression is an 'integer + * constant expression' + * @expr: Expression to test, not evaluated, can be a pointer + * @if_const: return value if constant + * @if_not_const: return value if not constant + * + * The return values @if_const and @if_not_const can have different types. + * + * Relies on typeof(x ? NULL : ptr_type) being ptr_type and + * typeof(x ? (void *)y : ptr_type) being 'void *'. + */ +#define __if_constexpr(expr, if_const, if_not_const) \ + _Generic(0 ? ((void *)((long)(expr) * 0l)) : (char *)0, \ + char *: (if_const), \ + void *: (if_not_const)) + /* * This returns a constant expression while determining if an argument is * a constant expression, most importantly without evaluating the argument. -- Then have: #define __is_constexpr(x) __if_constexpr(x, 1, 0) #define const_true(x) __if_constexpr(x, x, 0) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, 17 Nov 2024 at 09:42, David Laight <David.Laight@aculab.com> wrote: > > #define const_true(x) __if_constexpr(x, x, 0) No, let's not do this "double expansion" thing again. If we actually want to make things smarter, the trick to use is to know that only a constant _zero_ turns into a void pointer (aka NULL). IOW, something like this: /* * iff 'x' is a non-zero constant integer expression, * then '!(x)' will be a zero constant integer expression, * and casting that to 'void *' will result in a NULL * pointer. Otherwise casting it to 'void *' will be just * a regular 'void *'. * * The type of '0 ? NULL : (char *)' is 'char *' * The type of '0 ? (void *) : (char *) is 'void *' */ #define const_true(x) \ _Generic(0 ? (void *)((long)!(x)) : (char *)0, char *: 1, void *: 0) should work, and doesn't do any double expansion of complex arguments. Linus
On Sun, 17 Nov 2024 at 10:00, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > IOW, something like this: > > /* > * iff 'x' is a non-zero constant integer expression, > * then '!(x)' will be a zero constant integer expression, > * and casting that to 'void *' will result in a NULL > * pointer. Otherwise casting it to 'void *' will be just > * a regular 'void *'. > * > * The type of '0 ? NULL : (char *)' is 'char *' > * The type of '0 ? (void *) : (char *) is 'void *' > */ > #define const_true(x) \ > _Generic(0 ? (void *)((long)!(x)) : (char *)0, char *: 1, void *: 0) > > should work, and doesn't do any double expansion of complex arguments. Always good to test things, and it does seem to actually work. Interestingly, while testing it, I found what looks like a (harmless) bug in gcc. Gcc seems to think that "!(void *)1" is an integer constant expression. But technically, only *integer* casts can be part of an integer constant expression. Both sparse and clang get that odd case right. Practically speaking, this doesn't matter, but I'll claim that my test coverage was at least interesting since it seems to have found a compiler issue. Maybe it's a documented gcc thing, I'm not sure. Regardless, I think I actually prefer the gcc behavior, but I don't see that it really makes much of a difference. Linus
From: Linus Torvalds > Sent: 17 November 2024 18:00 > > On Sun, 17 Nov 2024 at 09:42, David Laight <David.Laight@aculab.com> wrote: > > > > #define const_true(x) __if_constexpr(x, x, 0) > > No, let's not do this "double expansion" thing again. It would be better that the proposed define :-) > If we actually want to make things smarter, the trick to use is to > know that only a constant _zero_ turns into a void pointer (aka NULL). > > IOW, something like this: > > /* > * iff 'x' is a non-zero constant integer expression, > * then '!(x)' will be a zero constant integer expression, > * and casting that to 'void *' will result in a NULL > * pointer. Otherwise casting it to 'void *' will be just > * a regular 'void *'. > * > * The type of '0 ? NULL : (char *)' is 'char *' > * The type of '0 ? (void *) : (char *) is 'void *' > */ > #define const_true(x) \ > _Generic(0 ? (void *)((long)!(x)) : (char *)0, char *: 1, void *: 0) > > should work, and doesn't do any double expansion of complex arguments. I'm sure I have one place where I did want other than 1 or 0. I do remember moving the '* 0' into the wrapper for __is_constexpr(). Now than min/max don't use __is_constexpr() I wonder if it still has to be sane for pointers? Supporting pointers just makes life hard - especially since (void *)1 isn't constant. I think everything can be built on a base if_const_zero(x, if_z, if_nz) #define const_true(x) if_const_zero(!(x), 1, 0) #define is_constexpr(x) if_const_zero((x) * 0), 1, 0) which gives a bit more flexibility. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, 17 Nov 2024 at 11:05, David Laight <David.Laight@aculab.com> wrote: > > I think everything can be built on a base if_const_zero(x, if_z, if_nz) > #define const_true(x) if_const_zero(!(x), 1, 0) > #define is_constexpr(x) if_const_zero((x) * 0), 1, 0) > which gives a bit more flexibility. The is_constexpr() should probably be if_const_zero(0*!(x)),1,0) to be ok with pointers and with "long long" constants. And the "1,0" arguments should have a real reason for existing. I'm not entirely convinced any other cases make much sense. Linus
From: Linus Torvalds > Sent: 17 November 2024 19:10 > > On Sun, 17 Nov 2024 at 11:05, David Laight <David.Laight@aculab.com> wrote: > > > > I think everything can be built on a base if_const_zero(x, if_z, if_nz) > > #define const_true(x) if_const_zero(!(x), 1, 0) > > #define is_constexpr(x) if_const_zero((x) * 0, 1, 0) > > which gives a bit more flexibility. > > The is_constexpr() should probably be if_const_zero(0*!(x),1,0) to be > ok with pointers and with "long long" constants. > > And the "1,0" arguments should have a real reason for existing. I'm > not entirely convinced any other cases make much sense. I might have used them when trying to get (high >= 0u) through a -W1 build. (for example in GENMASK()). Can't quite remember the horrid solution though. Since 99% will be 1,0 maybe saving the extra expansion is best anyway. So have is_const_zero(x) and add if_const_zero(x, if_z, if_nz) later. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, 17 Nov 2024 at 11:23, David Laight <David.Laight@aculab.com> wrote: > > Since 99% will be 1,0 maybe saving the extra expansion is best anyway. > So have is_const_zero(x) and add if_const_zero(x, if_z, if_nz) later. Ok. So something like this seems to give us the relevant cases: #define __is_const_zero(x) \ _Generic(0?(void *)(long)(x):(char *)0, char *:1, void *:0) #define is_const_zero(x) __is_const_zero(!!(x)) #define is_const_true(x) __is_const_zero(!(x)) #define is_const(x) __is_const_zero(0*!(x)) and should work with all scalar expressions that I can think of (ok, technically 'void' is a scalar type and it obviously won't work with that). And should work in all contexts. It does want a comment (in addition to the comment about how NULL is special for the ternary op that makes it work): the '(long)' cast is so that there are no warnings for casting to 'void *' when it's *not* a constant zero expression, and the '!' pattern is to turn pointers and huge constants into 'int' without loss of information and without warnings. Compound types obviously will generate a warning. As they should. The above looks reasonable to me, but I didn't actually test any of it in the actual kernel build. Linus
From: Linus Torvalds > Sent: 17 November 2024 20:12 > > On Sun, 17 Nov 2024 at 11:23, David Laight <David.Laight@aculab.com> wrote: > > > > Since 99% will be 1,0 maybe saving the extra expansion is best anyway. > > So have is_const_zero(x) and add if_const_zero(x, if_z, if_nz) later. > > Ok. So something like this seems to give us the relevant cases: > > #define __is_const_zero(x) \ > _Generic(0?(void *)(long)(x):(char *)0, char *:1, void *:0) > > #define is_const_zero(x) __is_const_zero(!!(x)) > #define is_const_true(x) __is_const_zero(!(x)) > #define is_const(x) __is_const_zero(0*!(x)) > > and should work with all scalar expressions that I can think of (ok, > technically 'void' is a scalar type and it obviously won't work with > that). And should work in all contexts. Seems a reasonable set. Maybe they need a set that are paired with __BUILD_BUG_ON_ZERO_MSG() to generate an error message on failure. Although I would add a few more ' ' characters for readability. > It does want a comment (in addition to the comment about how NULL is > special for the ternary op that makes it work): the '(long)' cast is > so that there are no warnings for casting to 'void *' when it's *not* > a constant zero expression, and the '!' pattern is to turn pointers > and huge constants into 'int' without loss of information and without > warnings. The comments would need to be terse one-liners. I wonder if it reads better (and without extra comments) if the (long) cast is removed and the 'callers' are required to generate 'long' args. So you have: #define __is_const_zero(x) \ _Generic(0 ? (void *)(x) : (char *)0, char *: 1, void *: 0) #define is_const_zero(x) __is_const_zero((x) ? 1L : 0L) #define is_const_true(x) __is_const_zero((x) ? 0L : 1L) #define is_const(x) __is_const_zero((x) ? 0L : 0L) I've done a quick test of the last one in godbolt. David > > Compound types obviously will generate a warning. As they should. > > The above looks reasonable to me, but I didn't actually test any of it > in the actual kernel build. > > Linus - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, 17 Nov 2024 at 14:38, David Laight <David.Laight@aculab.com> wrote: > > I wonder if it reads better (and without extra comments) if the (long) > cast is removed and the 'callers' are required to generate 'long' args. I think that's much less obvious, actually. You'd have to explain why it has those odd long constants now. Also, technically it's not even really about "long", but "intptr_t", which doesn't have a simple constant representation. We're using "long" in this context because we don't want to have even more dependencies in compiler.h - but I do think that means that the cast is at least conceptually the proper way to do things: it's how you'd do it in some user-mode header if you do this (as opposed to our kernel model where we generate all these things from scratch anyway). The "0*!(x)" is admittedly kind of ugly, and might be prettier as "0&&(x)". Same number of characters, but technically one op less and not mixing booleans and integer ops. But honestly, nobody is ever going to look at the internals anyway once it's all in there and works. Linus
On Mon. 18 nov. 2024 à 07:58, Linus Torvalds <torvalds@linux-foundation.org> wrote: > The "0*!(x)" is admittedly kind of ugly, and might be prettier as > "0&&(x)". Same number of characters, but technically one op less and > not mixing booleans and integer ops. I did a tree wide replacement of __is_constexpr() with is_const() and did an allyesconfig build test. It yields a -Wint-in-bool-context warning in GCC for both the "0*!(x)" and the "0&&(x)" each time the expression contains non-boolean operators, for example: * or <<. I reproduced it in godbolt here: https://godbolt.org/z/5Wcbvanq3 Aside from this warning, the allyesconfig build succeeded. Yours sincerely, Vincent Mailhol
From: Vincent Mailhol > Sent: 18 November 2024 03:22 > > On Mon. 18 nov. 2024 à 07:58, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > The "0*!(x)" is admittedly kind of ugly, and might be prettier as > > "0&&(x)". Same number of characters, but technically one op less and > > not mixing booleans and integer ops. > > I did a tree wide replacement of __is_constexpr() with is_const() and > did an allyesconfig build test. It yields a -Wint-in-bool-context > warning in GCC for both the "0*!(x)" and the "0&&(x)" each time the > expression contains non-boolean operators, for example: * or <<. > > I reproduced it in godbolt here: > > https://godbolt.org/z/5Wcbvanq3 Applies to pretty much all the variants. Needs to be (x) == 0 (or (x) != 0) rather than !(x) Fortunately comparison operators (and ?:) are all valid in constant integer expressions. Oh, one advantage of statically_const() is that you can give it a 'local' variable that contains the value. So this works: #define check_lo_ho(lo, hi) do { \ __auto_type _lo = lo; \ __auto_type _hi = hi; \ BUILD_BUG_ON_MSG(_lo > _hi, "inverted bounds"); \ } while (0) I'm trying to finalise a patch for min() and max(). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sun, 17 Nov 2024 at 19:22, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > > I did a tree wide replacement of __is_constexpr() with is_const() and > did an allyesconfig build test. It yields a -Wint-in-bool-context > warning in GCC for both the "0*!(x)" and the "0&&(x)" each time the > expression contains non-boolean operators, for example: * or <<. Grr. Annoying. But yeah, replace the "!" with "!= 0" and I guess it should be ok. Linus
diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 4d4e23b6e3e7..f9d660b63765 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -308,6 +308,28 @@ static inline void *offset_to_ptr(const int *off) */ #define statically_true(x) (__builtin_constant_p(x) && (x)) +/* + * Similar to statically_true() but produces a constant expression + * + * To be used in conjunction with macros, such as BUILD_BUG_ON_ZERO(), + * which require their input to be a constant expression and for which + * statically_true() would otherwise fail. + * + * This is a trade-off: const_true() requires all its operands to be + * compile time constants. Else, it would always returns false even on + * the most trivial cases like: + * + * true || non_const_var + * + * On the opposite, statically_true() is able to fold more complex + * tautologies and will return true on expressions such as: + * + * !(non_const_var * 8 % 4) + * + * For the general case, statically_true() is better. + */ +#define const_true(x) __builtin_choose_expr(__is_constexpr(x), x, false) + /* * This is needed in functions which generate the stack canary, see * arch/x86/kernel/smpboot.c::start_secondary() for an example.
__builtin_constant_p() is known for not always being able to produce constant expression [1] which led to the introduction of __is_constexpr() [2]. Because of its dependency on __builtin_constant_p(), statically_true() suffers from the same issues. For example: void foo(int a) { /* fail on GCC */ BUILD_BUG_ON_ZERO(statically_true(a)); /* fail on both clang and GCC */ static char arr[statically_true(a) ? 1 : 2]; } For the same reasons why __is_constexpr() was created to cover __builtin_constant_p() edge cases, __is_constexpr() can be used to resolve statically_true() limitations. Note that, somehow, GCC is not always able to fold this: __is_constexpr(x) && (x) It is OK in BUILD_BUG_ON_ZERO() but not in array declarations nor in static_assert(): void bar(int a) { /* success */ BUILD_BUG_ON_ZERO(__is_constexpr(a) && (a)); /* fail on GCC */ static char arr[__is_constexpr(a) && (a) ? 1 : 2]; /* fail on GCC */ static_assert(__is_constexpr(a) && (a)); } Encapsulating the expression in a __builtin_choose_expr() switch resolves all these failed tests. Define a new const_true() macro which, by making use of the __builtin_choose_expr() and __is_constexpr(x) combo, always produces a constant expression. It should be noted that statically_true() is the only one able to fold tautologic expressions in which at least one on the operands is not a constant expression. For example: statically_true(true || var) statically_true(var == var) statically_true(var * 0 + 1) statically_true(!(var * 8 % 4)) always evaluates to true, whereas all of these would be false under const_true() if var is not a constant expression [3]. For this reason, usage of const_true() be should the exception. Reflect in the documentation that const_true() is less powerful and that statically_true() is the overall preferred solution. [1] __builtin_constant_p cannot resolve to const when optimizing Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449 [2] commit 3c8ba0d61d04 ("kernel.h: Retain constant expression output for max()/min()") Link: https://git.kernel.org/torvalds/c/3c8ba0d61d04 [3] https://godbolt.org/z/c61PMxqbK Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- Above examples, and a bit more: https://godbolt.org/z/11xnxfx3P --- include/linux/compiler.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)