[03/10] compiler.h: add is_const_true() and is_const_false()

Commit Message

Vincent Mailhol via B4 Relay Dec. 2, 2024, 5:33 p.m. UTC

From: Vincent Mailhol <[e-mail]>

__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];
  }

Define a new is_const_true() and is_const_false() pair of macros
which, by making use of __is_const_zero(), always produces a constant
expression.

Note that is_const_false() can not be directly defined as an alias to
__is_const_zero(). Otherwise, it could yield some false positives on
huge numbers because of a lost of precision when doing the (long) cast
in __is_const_zero(). Example:

  is_const_false((u128)ULONG_MAX << BITS_PER_LONG)

Furthermore, using the ! operator like this:

  #define is_const_true(x) __is_const_zero(!(x))
  #define is_const_false(x) __is_const_zero(!!(x))

would yield a -Wint-in-bool-context compiler warning if the argument
is not a boolean. Use the == and != operators instead.

It should be noted that statically_true/false() are the only ones
capable of folding 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_false(var * 0)
  statically_false(var * 8 % 4)

always evaluate to true, whereas all of these would be false under
is_const_true/false() if var is not a constant expression [3].

For this reason, usage of const_true/false() should be 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/E4r7EaxW9

Signed-off-by: Vincent Mailhol <[e-mail]>
---
 include/linux/compiler.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

David Laight Dec. 4, 2024, 6:48 p.m. UTC | #1

From: Vincent Mailhol
> Sent: 02 December 2024 17:33
>
> __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.

No, they are testing different things.


> 
> 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];
>   }
> 
> Define a new is_const_true() and is_const_false() pair of macros
> which, by making use of __is_const_zero(), always produces a constant
> expression.
> 
> Note that is_const_false() can not be directly defined as an alias to
> __is_const_zero(). Otherwise, it could yield some false positives on
> huge numbers because of a lost of precision when doing the (long) cast
> in __is_const_zero(). Example:
> 
>   is_const_false((u128)ULONG_MAX << BITS_PER_LONG)
> 
> Furthermore, using the ! operator like this:
> 
>   #define is_const_true(x) __is_const_zero(!(x))
>   #define is_const_false(x) __is_const_zero(!!(x))
> 
> would yield a -Wint-in-bool-context compiler warning if the argument
> is not a boolean. Use the == and != operators instead.
> 
> It should be noted that statically_true/false() are the only ones
> capable of folding 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_false(var * 0)
>   statically_false(var * 8 % 4)
> 
> always evaluate to true, whereas all of these would be false under
> is_const_true/false() if var is not a constant expression [3].
> 
> For this reason, usage of const_true/false() should be 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/E4r7EaxW9
> 
> Signed-off-by: Vincent Mailhol <[e-mail]>
> ---
>  include/linux/compiler.h | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 30ce06df4153cfdc0fad9bc7bffab9097f8b0450..165aa5b9bc484376087a130a1ac1f3edb50c983d 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -357,6 +357,29 @@ static inline void *offset_to_ptr(const int *off)
>   */
>  #define is_const(x) __is_const_zero(0 * (x))
> 
> +/*
> + * Similar to statically_true() but produces a constant expression

No.
It tests whether a value is a 'constant integer expression' and
the result is a 'constant integer expression'.
statically_true() checks for the value being a 'compile time constant'.

Most code really doesn't care, it all got added to min() so that
a very few places could do:
	char foo[min(16, sizeof (type))];
without triggering the 'variable length array' warning.
But that just bloated everywhere else and (IIRC) Linus replaced
them with a MIN() that was just an 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.

Use a different BUILD_BUG macro instead.
Look at the current definition of min().

	David

> + *
> + * This is a trade-off: is_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_expr
> + *
> + * On the opposite, statically_true() is able to fold more complex
> + * tautologies and will return true on expressions such as:
> + *
> + *   !(non_const_expr * 8 % 4)
> + *
> + * For the general case, statically_true() is better.
> + */
> +#define is_const_true(x) __is_const_zero((x) == 0)
> +#define is_const_false(x) __is_const_zero((x) != 0)
> +
>  /*
>   * This is needed in functions which generate the stack canary, see
>   * arch/x86/kernel/smpboot.c::start_secondary() for an example.
> 
> --
> 2.45.2
> 
> 

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

Vincent Mailhol Dec. 5, 2024, 3:48 p.m. UTC | #2

On Thu. 5 Dec 2024 at 03:48, David Laight <[e-mail]> wrote:
> From: Vincent Mailhol
> > Sent: 02 December 2024 17:33
> >
> > __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.
>
> No, they are testing different things.

OK, I will remove this paragraph.

> > 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];
> >   }
> >
> > Define a new is_const_true() and is_const_false() pair of macros
> > which, by making use of __is_const_zero(), always produces a constant
> > expression.
> >
> > Note that is_const_false() can not be directly defined as an alias to
> > __is_const_zero(). Otherwise, it could yield some false positives on
> > huge numbers because of a lost of precision when doing the (long) cast
> > in __is_const_zero(). Example:
> >
> >   is_const_false((u128)ULONG_MAX << BITS_PER_LONG)
> >
> > Furthermore, using the ! operator like this:
> >
> >   #define is_const_true(x) __is_const_zero(!(x))
> >   #define is_const_false(x) __is_const_zero(!!(x))
> >
> > would yield a -Wint-in-bool-context compiler warning if the argument
> > is not a boolean. Use the == and != operators instead.
> >
> > It should be noted that statically_true/false() are the only ones
> > capable of folding 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_false(var * 0)
> >   statically_false(var * 8 % 4)
> >
> > always evaluate to true, whereas all of these would be false under
> > is_const_true/false() if var is not a constant expression [3].
> >
> > For this reason, usage of const_true/false() should be 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/E4r7EaxW9

D'oh, I used some old versions of the macros in that link. The link
will be updated to this in v2:

  https://godbolt.org/z/E4r7EaxW9

> > Signed-off-by: Vincent Mailhol <[e-mail]>
> > ---
> >  include/linux/compiler.h | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index 30ce06df4153cfdc0fad9bc7bffab9097f8b0450..165aa5b9bc484376087a130a1ac1f3edb50c983d 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -357,6 +357,29 @@ static inline void *offset_to_ptr(const int *off)
> >   */
> >  #define is_const(x) __is_const_zero(0 * (x))
> >
> > +/*
> > + * Similar to statically_true() but produces a constant expression
>
> No.
> It tests whether a value is a 'constant integer expression' and
> the result is a 'constant integer expression'.
> statically_true() checks for the value being a 'compile time constant'.

I still would argue that ’constant integer expressions’ and ’compile
time constants’ are *similar*. Not the same, agreed, but not
drastically different either. I picked the term *similar* for that
reason.

> Most code really doesn't care, it all got added to min() so that
> a very few places could do:
>         char foo[min(16, sizeof (type))];
> without triggering the 'variable length array' warning.
> But that just bloated everywhere else and (IIRC) Linus replaced
> them with a MIN() that was just an expression.

What about:

  Return an integer constant expression while evaluating if the
  argument is a true (non zero) integer 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.
>
> Use a different BUILD_BUG macro instead.
> Look at the current definition of min().

Do you mean BUILD_BUG_ON_MSG()? That one, at the end, relies on the
error attribute:

  https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-error-function-attribute

And the error attribute logic relies on compiler optimization. So
BUILD_BUG_ON_MSG() is not a valid example here because it does not
require its argument to be an integer constant expression. It works
well with other compile time constants.

Another valid example would be _Static_assert() but as a matter of
fact, it is more common to use __is_constexpr() together with
BUILD_BUG_ON_ZERO() than it is with _Static_assert(). So I think that
BUILD_BUG_ON_ZERO() is best here.


Yours sincerely,
Vincent Mailhol

Patch

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 30ce06df4153cfdc0fad9bc7bffab9097f8b0450..165aa5b9bc484376087a130a1ac1f3edb50c983d 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -357,6 +357,29 @@  static inline void *offset_to_ptr(const int *off)
  */
 #define is_const(x) __is_const_zero(0 * (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: is_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_expr
+ *
+ * On the opposite, statically_true() is able to fold more complex
+ * tautologies and will return true on expressions such as:
+ *
+ *   !(non_const_expr * 8 % 4)
+ *
+ * For the general case, statically_true() is better.
+ */
+#define is_const_true(x) __is_const_zero((x) == 0)
+#define is_const_false(x) __is_const_zero((x) != 0)
+
 /*
  * This is needed in functions which generate the stack canary, see
  * arch/x86/kernel/smpboot.c::start_secondary() for an example.