[PATCH v4 0/2] add const_true() to simplify GENMASK_INPUT_CHECK()

[PATCH v4 0/2] add const_true() to simplify GENMASK_INPUT_CHECK()

[Vincent Mailhol]

The first patch introduces a new variant of statically_true() named
const_true() which rely on __is_constexpr() to produce a constant
expression result which can be used in BUILD_BUG_ON_ZERO() and other
macros which expect a constant expression as input.

The second patch applies this newly created const_true() to
GENMASK_INPUT_CHECK().
---

** Changelog **

v3 -> v4:

   - rename _statically_true() into const_true().

   - fix incorrect subject in the first patch.

   - s/Declare/Define in first patch description: macro are defined,
     not declared.

   - move the godbolt link with the tautology folding examples as a
     footnote into the patch description (was after the --- scissors
     in v3).

   - add more examples of tautologically true expressions in the first
     patch description.

   - Rewrite the paragraph in the macro comment which compares
     statically_true() with const_true() and add code examples
     directly into that comment.

Link: https://lore.kernel.org/all/20241112190840.601378-4-mailhol.vincent@wanadoo.fr/

v3 -> v3 RESEND:

   - send email using the smtp.wanadoo.fr gateway. Note that this may
     appear as smtp.orange.fr which is an alias (both have the same IP).

Link: https://lore.kernel.org/all/20241112140454.518823-4-mailhol.vincent@wanadoo.fr/

v2 -> v3:

   - split the single patch into a series of two patches.

   - add explanation of why _statically_true() is needed in addition
     to the existing statically_true(). Explain the pros and cons of
     each.

   - use __builtin_choose_expr() in _statically_true(). The
     _statically_true() of the v2 works perfectly fine when used in
     conjunction with BUILD_BUG_ON_ZERO() but fails if used, for
     example, in arrays or in static_assert().

Link: https://lore.kernel.org/all/20241111164743.339117-2-mailhol.vincent@wanadoo.fr/

v1 -> v2:

   - introduce _statically_true(), taking inspiration from
     statically_true() as introduced in commit 22f546873149 ("minmax:
     improve macro expansion and type checking").

Link: https://lore.kernel.org/all/20240609073513.256179-1-mailhol.vincent@wanadoo.fr/

Vincent Mailhol (2):
  compiler.h: add const_true()
  linux/bits.h: simplify GENMASK_INPUT_CHECK()

 include/linux/bits.h     |  5 ++---
 include/linux/compiler.h | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.45.2

[PATCH v4 1/2] compiler.h: add const_true()

[Vincent Mailhol]

__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(+)

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

Re: [PATCH v4 1/2] compiler.h: add const_true()

[Yury Norov]

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

Re: [PATCH v4 1/2] compiler.h: add const_true()

[Yury Norov]

On Wed, Nov 13, 2024 at 10:53:55AM -0800, Yury Norov wrote:
> 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.

This is already in my branch, but there was a discussion after I pulled
it. Can you guys tell me what is your conclusion on that? Should I
keep it in the branch, or drop?

Thanks,
Yury

Re: [PATCH v4 1/2] compiler.h: add const_true()

[Vincent Mailhol]

On 31/12/2024 at 03:32, Yury Norov wrote:
> On Wed, Nov 13, 2024 at 10:53:55AM -0800, Yury Norov wrote:
>> 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.
> 
> This is already in my branch, but there was a discussion after I pulled
> it. Can you guys tell me what is your conclusion on that? Should I
> keep it in the branch, or drop?

I see... Thanks for asking!

After receiving criticism on this series, I was assuming that I had to
rework it. But if given the option, I definitely prefer if you keep it
in your tree.

The new series [1] I sent depends on this patch from David:

  https://git.kernel.org/akpm/mm/c/c108f4c2947a

which is causing build failure in linux-next. Because of that, I put my
new series of hiatus. And the merge windows approaches, so I would
rather like that we just keep this series of two patches for 6.13 and
that I continue the bigger refactor of is_const() in the 6.14 cycle (by
then, the dependencies on David patch will hopefully be fixed).

Note that the new series does not conflict with this one. So if this
series gets merged first, I see only benefit: it will offload some work
and make the new series a bit smaller.

[1]
https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-0-4e4cbaecc216@wanadoo.fr/


Yours sincerely,
Vincent Mailhol

RE: [PATCH v4 1/2] compiler.h: add const_true()

[David Laight]

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)

Re: [PATCH v4 1/2] compiler.h: add const_true()

[Linus Torvalds]

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

Re: [PATCH v4 1/2] compiler.h: add const_true()

[Linus Torvalds]

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

RE: [PATCH v4 1/2] compiler.h: add const_true()

[David Laight]

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)

Re: [PATCH v4 1/2] compiler.h: add const_true()

[Linus Torvalds]

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

RE: [PATCH v4 1/2] compiler.h: add const_true()

[David Laight]

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)

Re: [PATCH v4 1/2] compiler.h: add const_true()

[Linus Torvalds]

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

RE: [PATCH v4 1/2] compiler.h: add const_true()

[David Laight]

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)

Re: [PATCH v4 1/2] compiler.h: add const_true()

[Linus Torvalds]

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

Re: [PATCH v4 1/2] compiler.h: add const_true()

[Vincent Mailhol]

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

RE: [PATCH v4 1/2] compiler.h: add const_true()

[David Laight]

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)

Re: [PATCH v4 1/2] compiler.h: add const_true()

[Linus Torvalds]

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

[PATCH v4 2/2] linux/bits.h: simplify GENMASK_INPUT_CHECK()

[Vincent Mailhol]

In GENMASK_INPUT_CHECK(),

  __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0)

is the exact expansion of:

  const_true((l) > (h))

Apply const_true() to simplify GENMASK_INPUT_CHECK().

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
This change passes the unit tests from CONFIG_BITS_TEST, including the
extra negative tests provided under #ifdef TEST_GENMASK_FAILURES [1].

[1] commit 6d511020e13d ("lib/test_bits.c: add tests of GENMASK")
Link: https://git.kernel.org/torvalds/c/6d511020e13d
---
 include/linux/bits.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 60044b608817..61a75d3f294b 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -20,9 +20,8 @@
  */
 #if !defined(__ASSEMBLY__)
 #include <linux/build_bug.h>
-#define GENMASK_INPUT_CHECK(h, l) \
-	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
-		__is_constexpr((l) > (h)), (l) > (h), 0)))
+#include <linux/compiler.h>
+#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
 #else
 /*
  * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
-- 
2.45.2

RE: [PATCH v4 2/2] linux/bits.h: simplify GENMASK_INPUT_CHECK()

[David Laight]

From: Vincent Mailhol
> Sent: 13 November 2024 17:19
> 
> In GENMASK_INPUT_CHECK(),
> 
>   __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0)
> 
> is the exact expansion of:
> 
>   const_true((l) > (h))
> 
> Apply const_true() to simplify GENMASK_INPUT_CHECK().

Wouldn't statically_true() give better coverage ?
I wouldn't have though that GENMASK() got used anywhere where a constant
integer expression was needed.

More interesting would be to get it to pass a W=1 build for
any place where 'l' is 0u.

	David

> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> This change passes the unit tests from CONFIG_BITS_TEST, including the
> extra negative tests provided under #ifdef TEST_GENMASK_FAILURES [1].
> 
> [1] commit 6d511020e13d ("lib/test_bits.c: add tests of GENMASK")
> Link: https://git.kernel.org/torvalds/c/6d511020e13d
> ---
>  include/linux/bits.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 60044b608817..61a75d3f294b 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -20,9 +20,8 @@
>   */
>  #if !defined(__ASSEMBLY__)
>  #include <linux/build_bug.h>
> -#define GENMASK_INPUT_CHECK(h, l) \
> -	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> -		__is_constexpr((l) > (h)), (l) > (h), 0)))
> +#include <linux/compiler.h>
> +#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
>  #else
>  /*
>   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> --
> 2.45.2
> 

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

RE: [PATCH v4 2/2] linux/bits.h: simplify GENMASK_INPUT_CHECK()

[David Laight]

From: David Laight
> Sent: 17 November 2024 17:25
> 
> From: Vincent Mailhol
> > Sent: 13 November 2024 17:19
> >
> > In GENMASK_INPUT_CHECK(),
> >
> >   __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0)
> >
> > is the exact expansion of:
> >
> >   const_true((l) > (h))
> >
> > Apply const_true() to simplify GENMASK_INPUT_CHECK().
> 
> Wouldn't statically_true() give better coverage ?
> I wouldn't have though that GENMASK() got used anywhere where a constant
> integer expression was needed.

If it is, maybe add a GENMASK_CONST() that uses BUILD_BUG_ON_ZERO_MSG()
(recently proposed) and so validates that the values are constants.
And then use statically_true() in the normal case to pick up more errors.

(Or just remove the check because coders really aren't that stupid!)

The interesting cases are the ones using variables.
And they'd need run-time checks of some form.

	David

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

Re: [PATCH v4 2/2] linux/bits.h: simplify GENMASK_INPUT_CHECK()

[Vincent Mailhol]

On Mon. 18 Nov. 2024 at 04:45, David Laight <David.Laight@aculab.com> wrote:
> From: David Laight
> > Sent: 17 November 2024 17:25
> >
> > From: Vincent Mailhol
> > > Sent: 13 November 2024 17:19
> > >
> > > In GENMASK_INPUT_CHECK(),
> > >
> > >   __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0)
> > >
> > > is the exact expansion of:
> > >
> > >   const_true((l) > (h))
> > >
> > > Apply const_true() to simplify GENMASK_INPUT_CHECK().
> >
> > Wouldn't statically_true() give better coverage ?

Yes, it would.

> > I wouldn't have though that GENMASK() got used anywhere where a constant
> > integer expression was needed.

It is used in many places, including some inline functions such as bitmap_set():

  https://elixir.bootlin.com/linux/v6.11/source/include/linux/bitmap.h#L469

where the input is not an integer constant expression (thus preventing
the use of statically_true()).

> If it is, maybe add a GENMASK_CONST() that uses BUILD_BUG_ON_ZERO_MSG()
> (recently proposed) and so validates that the values are constants.
> And then use statically_true() in the normal case to pick up more errors.

The issue if you introduce GENMASK_CONST() is that there is no gain.

The only advantage of const_true(x) is that it works on cases where
statically_true(x) would cause a compilation error. But for
statically_true(x) to cause a compilation error, x has to be a non
constant expression. And if x is non constant, then const_true(x)
returns false.

Regardless, considering the number of times where GENMASK() is used
with integer literals, I do not think it would be worth it to replace
all of these with GENMASK_CONST() tree wide.

Trying to go in your direction, we already have two genmasks:

   - GENMASK(): which uses GENMASK_INPUT_CHECK()

  - __GENMASK(): no check, used in uapi

What would make more sense to me would be to:

  1. replace const_true() by statically_true() in GENMASK_INPUT_CHECK()

  2. replace all the instances where GENMASK() breaks compilation with
     __GENMASK()

But this idea was already proposed in the past and was rejected here:

  https://lore.kernel.org/lkml/YJm5Dpo+RspbAtye@rikard/

> (Or just remove the check because coders really aren't that stupid!)

I think that this check exists in the first place *because* such a
mistake was made in the past.

> The interesting cases are the ones using variables.
> And they'd need run-time checks of some form.

Then, instead of introducing GENMASK_CONST(), maybe introduce
GENMASK_NON_CONST()? But then, the number of instances in which
GENMASK() is used with something other than literal integers is pretty
rare. So probably not worth it.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 2/2] linux/bits.h: simplify GENMASK_INPUT_CHECK()

[Vincent Mailhol]

On Mon. 18 Nov. 2024 at 02:24, David Laight <David.Laight@aculab.com> wrote:
> From: Vincent Mailhol
> > Sent: 13 November 2024 17:19
> >
> > In GENMASK_INPUT_CHECK(),
> >
> >   __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0)
> >
> > is the exact expansion of:
> >
> >   const_true((l) > (h))
> >
> > Apply const_true() to simplify GENMASK_INPUT_CHECK().
>
> Wouldn't statically_true() give better coverage ?
> I wouldn't have though that GENMASK() got used anywhere where a constant
> integer expression was needed.
>
> More interesting would be to get it to pass a W=1 build for
> any place where 'l' is 0u.

Are you referring to -Wtype-limits? If yes, this warning was moved to
W=2. I suggested in the past to add a cast to silence this warning,
but got rejected:

  https://lore.kernel.org/lkml/CAHk-=whvGWbpsTa538CvQ9e=VF+m8WPQmES2y6-=0=-64uGkgg@mail.gmail.com/

Yours sincerely,
Vincent Mailhol