[PATCH 00/10] compiler.h: refactor __is_constexpr() into is_const{,_true,_false}()

[PATCH 00/10] compiler.h: refactor __is_constexpr() into is_const{,_true,_false}()

[Vincent Mailhol via B4 Relay]

This series is the spiritual successor of [1] which introduced
const_true(). In [1], following a comment from David Laight, Linus
came with a suggestion to simplify __is_constexpr() and its derived
macros using a _Generic() selection. Because of the total change of
scope, I am starting a new series.

The goal is to introduce a set of three macros:

  - is_const(): a one to one replacement of __is_constexpr() in term
    of features but written in a less hacky way thanks to _Generic().

  - is_const_true(): tells whether or not the argument is a true
    integer constant expression.

  - is_const_false(): tells whether or not the argument is a false
    integer constant expression.

Once defined, apply them tree-wide.

All those three macros will rely on a single building block:
__is_const_zero().

Patch 1 adds statically_false(). This is just done so that at the end
of this series, the full set of statically_true/false() and
is_const_true/false() is present.

Patch 2 adds __is_const_zero() and is_const().

Patch 3 adds is_const_true() and is_const_false().

Patch 4 to 9 do a tree-wide replacement to remove all the usages of
__is_constexpr() and replace them by is_const_true() or
is_const_false() whenever feasible, or by is_const() otherwise.

Patch 10 finally remove __is_constexpr(). RIP!

[1] add const_true() to simplify GENMASK_INPUT_CHECK()
Link: https://lore.kernel.org/all/20241113172939.747686-4-mailhol.vincent@wanadoo.fr/

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Vincent Mailhol (10):
      compiler.h: add statically_false()
      compiler.h: add is_const() as a replacement of __is_constexpr()
      compiler.h: add is_const_true() and is_const_false()
      linux/bits.h: simplify GENMASK_INPUT_CHECK() by using is_const_true()
      minmax: simplify __clamp_once() by using is_const_false()
      fortify: replace __is_constexpr() by is_const() in strlen()
      overflow: replace __is_constexpr() by is_const()
      drm/i915/reg: replace __is_const_expr() by is_const_true() or is_const()
      coresight: etm4x: replace __is_const_expr() by is_const()
      compiler.h: remove __is_constexpr()

 drivers/gpu/drm/i915/i915_reg_defs.h          |  47 +++++------
 drivers/hwtracing/coresight/coresight-etm4x.h |   2 +-
 include/linux/bits.h                          |   5 +-
 include/linux/compiler.h                      | 112 +++++++++++++++-----------
 include/linux/fortify-string.h                |   4 +-
 include/linux/minmax.h                        |   3 +-
 include/linux/overflow.h                      |   8 +-
 7 files changed, 97 insertions(+), 84 deletions(-)
---
base-commit: e70140ba0d2b1a30467d4af6bcfe761327b9ec95
change-id: 20241129-is_constexpr-refactor-19460adedc48

Best regards,
-- 
Vincent Mailhol <mailhol.vincent@wanadoo.fr>

[PATCH 01/10] compiler.h: add statically_false()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

For completion, add statically_false() which is the equivalent of
statically_true() except that it will return true only if the input is
known to be false at compile time.

The == operator is used instead of the ! negation to prevent a
-Wint-in-bool-context compiler warning when the argument is not a
boolean. For example:

  statically_false(var * 0)

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/compiler.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 469a64dd6495fefab2c85ffc279568a657b72660..a2a56a50dd85227a4fdc62236a2710ca37c5ba52 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -314,6 +314,7 @@ static inline void *offset_to_ptr(const int *off)
  * values to determine that the condition is statically true.
  */
 #define statically_true(x) (__builtin_constant_p(x) && (x))
+#define statically_false(x) (__builtin_constant_p(x) && (x) == 0)
 
 /*
  * This is needed in functions which generate the stack canary, see

-- 
2.45.2

RE: [PATCH 01/10] compiler.h: add statically_false()

[David Laight]

From: Vincent Mailhol
> Sent: 02 December 2024 17:33
> 
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> For completion, add statically_false() which is the equivalent of
> statically_true() except that it will return true only if the input is
> known to be false at compile time.

This is pretty much pointless.
It is just as easy to invert the condition at the call site.

	David

> 
> The == operator is used instead of the ! negation to prevent a
> -Wint-in-bool-context compiler warning when the argument is not a
> boolean. For example:
> 
>   statically_false(var * 0)
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  include/linux/compiler.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 469a64dd6495fefab2c85ffc279568a657b72660..a2a56a50dd85227a4fdc62236a2710ca37c5ba52 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -314,6 +314,7 @@ static inline void *offset_to_ptr(const int *off)
>   * values to determine that the condition is statically true.
>   */
>  #define statically_true(x) (__builtin_constant_p(x) && (x))
> +#define statically_false(x) (__builtin_constant_p(x) && (x) == 0)
> 
>  /*
>   * This is needed in functions which generate the stack canary, see
> 
> --
> 2.45.2
> 
> 

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

Re: [PATCH 01/10] compiler.h: add statically_false()

[Vincent Mailhol]

On Thu. 5 Dec 2024 at 03:30, David Laight <David.Laight@aculab.com> wrote:
> From: Vincent Mailhol
> > Sent: 02 December 2024 17:33
> >
> > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >
> > For completion, add statically_false() which is the equivalent of
> > statically_true() except that it will return true only if the input is
> > known to be false at compile time.
>
> This is pretty much pointless.
> It is just as easy to invert the condition at the call site.

To start with, I will argue that:

  statically_false(foo)

is more pretty than

  statically_true(!(foo))

In addition, the simple negation !(foo) only works if foo is a
boolean. If it is an integer, you would get the -Wint-in-bool-context
warning. Thus you would have to write:

  statically_true((foo) == 0)

Anyone using this in a global header basically does not know what type
of argument they are getting. So, the (foo) == 0 trick is a must. But
because it is ugly, better to encapsulate it once for all. The
statically_false() is just cleaner and less error prone.

That said, I am not strongly opposed to removing statically_false(),
but in that case, I would also remove is_const_false(). For me, these
come as a pair, either we have both or we have none. Only having one
of these looked unbalanced.


Yours sincerely,
Vincent Mailhol

RE: [PATCH 01/10] compiler.h: add statically_false()

[David Laight]

From: Vincent Mailhol
> Sent: 05 December 2024 15:26
> 
> On Thu. 5 Dec 2024 at 03:30, David Laight <David.Laight@aculab.com> wrote:
> > From: Vincent Mailhol
> > > Sent: 02 December 2024 17:33
> > >
> > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > >
> > > For completion, add statically_false() which is the equivalent of
> > > statically_true() except that it will return true only if the input is
> > > known to be false at compile time.
> >
> > This is pretty much pointless.
> > It is just as easy to invert the condition at the call site.
> 
> To start with, I will argue that:
> 
>   statically_false(foo)
> 
> is more pretty than
> 
>   statically_true(!(foo))

Except that the test is more likely to be:
	statically_false(x > y)
and the invert is then
	statically_true(x <= y)

No different from C itself, there is no 'ifnot (condition) {...}'
(don't talk to me about perl...)

I suspect you need to pretty much remove all the comments that
cross-refer to statically_true() from the other patches.

So is_const_true() is just 'return true if the expression
is a 'non-zero constant integer expression'.

	David

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

Re: [PATCH 01/10] compiler.h: add statically_false()

[Vincent Mailhol]

On Fri. 6 Dec. 2024 at 12:39, David Laight <David.Laight@aculab.com> wrote:
> From: Vincent Mailhol
> > Sent: 05 December 2024 15:26
> >
> > On Thu. 5 Dec 2024 at 03:30, David Laight <David.Laight@aculab.com> wrote:
> > > From: Vincent Mailhol
> > > > Sent: 02 December 2024 17:33
> > > >
> > > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > >
> > > > For completion, add statically_false() which is the equivalent of
> > > > statically_true() except that it will return true only if the input is
> > > > known to be false at compile time.
> > >
> > > This is pretty much pointless.
> > > It is just as easy to invert the condition at the call site.
> >
> > To start with, I will argue that:
> >
> >   statically_false(foo)
> >
> > is more pretty than
> >
> >   statically_true(!(foo))
>
> Except that the test is more likely to be:
>         statically_false(x > y)
> and the invert is then
>         statically_true(x <= y)
>
> No different from C itself, there is no 'ifnot (condition) {...}'
> (don't talk to me about perl...)

No need to talk about perl, just staying in C, it has both the #ifdef
and the #ifndef directives (and since C23, it even has the #elifndef).

Regardless, this was just a nitpick. You convinced me, I will remove
both statically_false() and is_const_false() in v2.

> I suspect you need to pretty much remove all the comments that
> cross-refer to statically_true() from the other patches.
>
> So is_const_true() is just 'return true if the expression
> is a 'non-zero constant integer expression'.

No. Linus made it clear in

  https://lore.kernel.org/all/CAHk-=wh5SNYdgx8-X+ggHP+ojbG2F7oyt3TLmMgqejYd5zn0Aw@mail.gmail.com/

that we need an explanation of why statically_true() may not work in
some contexts and I agree with this.

As far as I like using proper nuances between the terms 'integer
constant expression' and 'compile time constant', treating
statically_true() and is_const_true() as completely different things
as you are suggesting is not constructive. At the end, what matters
the most in a comment, is that the final user properly understands how
to use the thing correctly.

Removing the cross reference would increase the risk of people using
is_const_true() in places where it is not needed. I will keep the
cross reference to statically_true().


Yours sincerely,
Vincent Mailhol

[PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

__is_constexpr(), while being one of the most glorious one liner hack
ever witnessed by mankind, is overly complex. Following the adoption
of C11 in the kernel, this macro can be simplified through the use of
a _Generic() selection.

First, split the macro in two:

  - __is_const_zero(x): an helper macro; tells whether x is the
    integer constant expression 0 or something else.

  - is_const(x): replacement of __is_constexpr(); tells whether x is a
    integer constant expression.

The split serves two purposes: first make it easier to understand;
second, __is_const_zero() will be reused as a building block for other
is_const_*() macros that will be introduced later on.

The core principle of __is_constexpr() to abuse the return type of the
ternary operator remains, but all the surrounding sizeof() hack
disappear.

On a side note, while not relevant to the kernel, __is_constexpr()
relied on the GNU extension that sizeof(void) is 1. const_expr() does
not use any GNU extensions, making it ISO C compliant.

__is_constexpr() is temporarily kept and will be removed once all its
users get migrated to is_const() (or one of its friend).

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/compiler.h | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index a2a56a50dd85227a4fdc62236a2710ca37c5ba52..30ce06df4153cfdc0fad9bc7bffab9097f8b0450 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -316,6 +316,47 @@ static inline void *offset_to_ptr(const int *off)
 #define statically_true(x) (__builtin_constant_p(x) && (x))
 #define statically_false(x) (__builtin_constant_p(x) && (x) == 0)
 
+/*
+ * Whether x is the integer constant expression 0 or something else.
+ *
+ * Details:
+ *   - The C11 standard defines in §6.3.2.3.3
+ *       (void *)<integer constant expression with the value 0>
+ *     as a null pointer constant (c.f. the NULL macro).
+ *   - If x evaluates to the integer constant expression 0,
+ *       (void *)(x)
+ *     is a null pointer constant. Else, it is a void * expression.
+ *   - In a ternary expression:
+ *       condition ? operand1 : operand2
+ *     if one of the two operands is of type void * and the other one
+ *     some other pointer type, the C11 standard defines in §6.5.15.6
+ *     the resulting type as below:
+ *       if one operand is a null pointer constant, the result has the
+ *       type of the other operand; otherwise [...] the result type is
+ *       a pointer to an appropriately qualified version of void.
+ *   - As such, in
+ *       0 ? (void *)(x) : (char *)0
+ *     if x is the integer constant expression 0, operand1 is a null
+ *     pointer constant and the resulting type is that of operand2:
+ *     char *. If x is anything else, the type is void *.
+ *   - The (long) cast silences a compiler warning for when x is not 0.
+ *   - Finally, the _Generic() dispatches the resulting type into a
+ *     Boolean.
+ *
+ * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
+ */
+#define __is_const_zero(x) \
+	_Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)
+
+/*
+ * Returns a constant expression while determining if its argument is a
+ * constant expression, most importantly without evaluating the argument.
+ *
+ * If getting a constant expression is not relevant to you, use the more
+ * powerful __builtin_constant_p() instead.
+ */
+#define is_const(x) __is_const_zero(0 * (x))
+
 /*
  * 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 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Vincent Mailhol
> Sent: 02 December 2024 17:33
> 
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> __is_constexpr(), while being one of the most glorious one liner hack
> ever witnessed by mankind, is overly complex. Following the adoption
> of C11 in the kernel, this macro can be simplified through the use of
> a _Generic() selection.

You should give credit to some of the earlier patches that do the same.
I'm sure there were some related ones from Linus - not applied yet.

> 
> First, split the macro in two:
> 
>   - __is_const_zero(x): an helper macro; tells whether x is the
>     integer constant expression 0 or something else.
> 
>   - is_const(x): replacement of __is_constexpr(); tells whether x is a
>     integer constant expression.
> 
> The split serves two purposes: first make it easier to understand;
> second, __is_const_zero() will be reused as a building block for other
> is_const_*() macros that will be introduced later on.
> 
> The core principle of __is_constexpr() to abuse the return type of the
> ternary operator remains, but all the surrounding sizeof() hack
> disappear.
> 
> On a side note, while not relevant to the kernel, __is_constexpr()
> relied on the GNU extension that sizeof(void) is 1. const_expr() does
> not use any GNU extensions, making it ISO C compliant.
> 
> __is_constexpr() is temporarily kept and will be removed once all its
> users get migrated to is_const() (or one of its friend).
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  include/linux/compiler.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index a2a56a50dd85227a4fdc62236a2710ca37c5ba52..30ce06df4153cfdc0fad9bc7bffab9097f8b0450 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -316,6 +316,47 @@ static inline void *offset_to_ptr(const int *off)
>  #define statically_true(x) (__builtin_constant_p(x) && (x))
>  #define statically_false(x) (__builtin_constant_p(x) && (x) == 0)
> 
> +/*
> + * Whether x is the integer constant expression 0 or something else.
> + *
> + * Details:
> + *   - The C11 standard defines in §6.3.2.3.3
> + *       (void *)<integer constant expression with the value 0>
> + *     as a null pointer constant (c.f. the NULL macro).
> + *   - If x evaluates to the integer constant expression 0,
> + *       (void *)(x)
> + *     is a null pointer constant. Else, it is a void * expression.
> + *   - In a ternary expression:
> + *       condition ? operand1 : operand2
> + *     if one of the two operands is of type void * and the other one
> + *     some other pointer type, the C11 standard defines in §6.5.15.6
> + *     the resulting type as below:
> + *       if one operand is a null pointer constant, the result has the
> + *       type of the other operand; otherwise [...] the result type is
> + *       a pointer to an appropriately qualified version of void.
> + *   - As such, in
> + *       0 ? (void *)(x) : (char *)0
> + *     if x is the integer constant expression 0, operand1 is a null
> + *     pointer constant and the resulting type is that of operand2:
> + *     char *. If x is anything else, the type is void *.
> + *   - The (long) cast silences a compiler warning for when x is not 0.
> + *   - Finally, the _Generic() dispatches the resulting type into a
> + *     Boolean.

The comment is absolutely excessive.
I'm sure I managed about 2 lines in one of the patches I did.

> + *
> + * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>

IIRC Martin has agreed in the past that the accreditation can
be removed - especially since it refers to the 'sizeof (void)' trick.

> + */
> +#define __is_const_zero(x) \
> +	_Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)
> +
> +/*
> + * Returns a constant expression while determining if its argument is a
> + * constant expression, most importantly without evaluating the argument.

You need to differentiate between a 'constant integer expression'
and a 'compile time constant'.
 
> + *
> + * If getting a constant expression is not relevant to you, use the more
> + * powerful __builtin_constant_p() instead.

__builtin_constant_p() is not 'more powerful' it is testing for
something different.

	David

> + */
> +#define is_const(x) __is_const_zero(0 * (x))
> +
>  /*
>   * 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)

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Yury Norov]

> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index a2a56a50dd85227a4fdc62236a2710ca37c5ba52..30ce06df4153cfdc0fad9bc7bffab9097f8b0450 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -316,6 +316,47 @@ static inline void *offset_to_ptr(const int *off)
> >  #define statically_true(x) (__builtin_constant_p(x) && (x))
> >  #define statically_false(x) (__builtin_constant_p(x) && (x) == 0)
> > 
> > +/*
> > + * Whether x is the integer constant expression 0 or something else.
> > + *
> > + * Details:
> > + *   - The C11 standard defines in §6.3.2.3.3
> > + *       (void *)<integer constant expression with the value 0>
> > + *     as a null pointer constant (c.f. the NULL macro).
> > + *   - If x evaluates to the integer constant expression 0,
> > + *       (void *)(x)
> > + *     is a null pointer constant. Else, it is a void * expression.
> > + *   - In a ternary expression:
> > + *       condition ? operand1 : operand2
> > + *     if one of the two operands is of type void * and the other one
> > + *     some other pointer type, the C11 standard defines in §6.5.15.6
> > + *     the resulting type as below:
> > + *       if one operand is a null pointer constant, the result has the
> > + *       type of the other operand; otherwise [...] the result type is
> > + *       a pointer to an appropriately qualified version of void.
> > + *   - As such, in
> > + *       0 ? (void *)(x) : (char *)0
> > + *     if x is the integer constant expression 0, operand1 is a null
> > + *     pointer constant and the resulting type is that of operand2:
> > + *     char *. If x is anything else, the type is void *.
> > + *   - The (long) cast silences a compiler warning for when x is not 0.
> > + *   - Finally, the _Generic() dispatches the resulting type into a
> > + *     Boolean.
> 
> The comment is absolutely excessive.

I like this comment. Particularly I like the references to the standard
followed by a step-by-step explanation of how the macro is built.

> I'm sure I managed about 2 lines in one of the patches I did.

Sorry, don't understand this.

Thanks,
Yury

> > + *
> > + * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
> 
> IIRC Martin has agreed in the past that the accreditation can
> be removed - especially since it refers to the 'sizeof (void)' trick.
> 
> > + */
> > +#define __is_const_zero(x) \
> > +	_Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)
> > +
> > +/*
> > + * Returns a constant expression while determining if its argument is a
> > + * constant expression, most importantly without evaluating the argument.
> 
> You need to differentiate between a 'constant integer expression'
> and a 'compile time constant'.
>  
> > + *
> > + * If getting a constant expression is not relevant to you, use the more
> > + * powerful __builtin_constant_p() instead.
> 
> __builtin_constant_p() is not 'more powerful' it is testing for
> something different.
> 
> 	David
> 
> > + */
> > +#define is_const(x) __is_const_zero(0 * (x))
> > +
> >  /*
> >   * 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)

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol]

-CC: Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
+CC: Martin Uecker <muecker@gwdg.de>
(seems that Martin changed his address)

On Thu. 5 Dec. 2024 at 03:39, David Laight <David.Laight@aculab.com> wrote:
> > Sent: 02 December 2024 17:33
> >
> > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >
> > __is_constexpr(), while being one of the most glorious one liner hack
> > ever witnessed by mankind, is overly complex. Following the adoption
> > of C11 in the kernel, this macro can be simplified through the use of
> > a _Generic() selection.
>
> You should give credit to some of the earlier patches that do the same.
> I'm sure there were some related ones from Linus - not applied yet.

ACK. Would adding a suggested--by Linus tag solve your concern?

> > First, split the macro in two:
> >
> >   - __is_const_zero(x): an helper macro; tells whether x is the
> >     integer constant expression 0 or something else.
> >
> >   - is_const(x): replacement of __is_constexpr(); tells whether x is a
> >     integer constant expression.
> >
> > The split serves two purposes: first make it easier to understand;
> > second, __is_const_zero() will be reused as a building block for other
> > is_const_*() macros that will be introduced later on.
> >
> > The core principle of __is_constexpr() to abuse the return type of the
> > ternary operator remains, but all the surrounding sizeof() hack
> > disappear.
> >
> > On a side note, while not relevant to the kernel, __is_constexpr()
> > relied on the GNU extension that sizeof(void) is 1. const_expr() does
> > not use any GNU extensions, making it ISO C compliant.
> >
> > __is_constexpr() is temporarily kept and will be removed once all its
> > users get migrated to is_const() (or one of its friend).
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> >  include/linux/compiler.h | 41 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index a2a56a50dd85227a4fdc62236a2710ca37c5ba52..30ce06df4153cfdc0fad9bc7bffab9097f8b0450 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -316,6 +316,47 @@ static inline void *offset_to_ptr(const int *off)
> >  #define statically_true(x) (__builtin_constant_p(x) && (x))
> >  #define statically_false(x) (__builtin_constant_p(x) && (x) == 0)
> >
> > +/*
> > + * Whether x is the integer constant expression 0 or something else.
> > + *
> > + * Details:
> > + *   - The C11 standard defines in §6.3.2.3.3
> > + *       (void *)<integer constant expression with the value 0>
> > + *     as a null pointer constant (c.f. the NULL macro).
> > + *   - If x evaluates to the integer constant expression 0,
> > + *       (void *)(x)
> > + *     is a null pointer constant. Else, it is a void * expression.
> > + *   - In a ternary expression:
> > + *       condition ? operand1 : operand2
> > + *     if one of the two operands is of type void * and the other one
> > + *     some other pointer type, the C11 standard defines in §6.5.15.6
> > + *     the resulting type as below:
> > + *       if one operand is a null pointer constant, the result has the
> > + *       type of the other operand; otherwise [...] the result type is
> > + *       a pointer to an appropriately qualified version of void.
> > + *   - As such, in
> > + *       0 ? (void *)(x) : (char *)0
> > + *     if x is the integer constant expression 0, operand1 is a null
> > + *     pointer constant and the resulting type is that of operand2:
> > + *     char *. If x is anything else, the type is void *.
> > + *   - The (long) cast silences a compiler warning for when x is not 0.
> > + *   - Finally, the _Generic() dispatches the resulting type into a
> > + *     Boolean.
>
> The comment is absolutely excessive.
> I'm sure I managed about 2 lines in one of the patches I did.

I think that Linus made it  clear in:

  https://lore.kernel.org/all/CAHk-=wgfpLdt7SFFGcByTfHdkvv7AEa3MDu_s_W1kfOxQs49pw@mail.gmail.com/

that this deserves a detailed comment.

The details block in the current __is_constexpr() is 37 lines long,
the details block in __is_const_zero() takes 22 lines. So I would
argue that I made things better.

Unless more people share your concern, I am planning to keep this comment as-is.

> > + *
> > + * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
>
> IIRC Martin has agreed in the past that the accreditation can
> be removed - especially since it refers to the 'sizeof (void)' trick.

I tried to look for such message:

  https://lore.kernel.org/all/?q=f%3A%22martin+uecker%22+__is_constexpr

but couldn't find it. Do you have the link?

@Martin, do you agree that I remove the accreditation?

> > + */
> > +#define __is_const_zero(x) \
> > +     _Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)
> > +
> > +/*
> > + * Returns a constant expression while determining if its argument is a
> > + * constant expression, most importantly without evaluating the argument.
>
> You need to differentiate between a 'constant integer expression'
> and a 'compile time constant'.

OK. This one was just copied from the previous __is_constexpr(). I will apply
"s/constant expression/constant integer expression/g" in v2.

> > + *
> > + * If getting a constant expression is not relevant to you, use the more
> > + * powerful __builtin_constant_p() instead.
>
> __builtin_constant_p() is not 'more powerful' it is testing for
> something different.

I meant to say that __builtin_constant_p() is more powerful at
constant folding. But I agree that the comment is not clear.

What about this?

  If getting a constant integer expression is not relevant to you, use
  __builtin_constant_p() which not only returns true if the argument
  is an integer constant expression, but also if it is a compile time
  constant.


Yours sincerely,
Vincent Mailhol

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Vincent Mailhol
> Sent: 05 December 2024 15:31
> 
> -CC: Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
> +CC: Martin Uecker <muecker@gwdg.de>
> (seems that Martin changed his address)
> 
> On Thu. 5 Dec. 2024 at 03:39, David Laight <David.Laight@aculab.com> wrote:
> > > Sent: 02 December 2024 17:33
> > >
> > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > >
> > > __is_constexpr(), while being one of the most glorious one liner hack
> > > ever witnessed by mankind, is overly complex. Following the adoption
> > > of C11 in the kernel, this macro can be simplified through the use of
> > > a _Generic() selection.
> >
> > You should give credit to some of the earlier patches that do the same.
> > I'm sure there were some related ones from Linus - not applied yet.
> 
> ACK. Would adding a suggested--by Linus tag solve your concern?

I actually suspect the first patches to change __is_constexpr() to
use _Generic were from myself.
I've found a patch I send in November 2023.

> 
> > > First, split the macro in two:
> > >
> > >   - __is_const_zero(x): an helper macro; tells whether x is the
> > >     integer constant expression 0 or something else.
> > >
> > >   - is_const(x): replacement of __is_constexpr(); tells whether x is a
> > >     integer constant expression.
> > >
> > > The split serves two purposes: first make it easier to understand;
> > > second, __is_const_zero() will be reused as a building block for other
> > > is_const_*() macros that will be introduced later on.
> > >
> > > The core principle of __is_constexpr() to abuse the return type of the
> > > ternary operator remains, but all the surrounding sizeof() hack
> > > disappear.
> > >
> > > On a side note, while not relevant to the kernel, __is_constexpr()
> > > relied on the GNU extension that sizeof(void) is 1. const_expr() does
> > > not use any GNU extensions, making it ISO C compliant.
> > >
> > > __is_constexpr() is temporarily kept and will be removed once all its
> > > users get migrated to is_const() (or one of its friend).
> > >
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > ---
> > >  include/linux/compiler.h | 41 +++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 41 insertions(+)
> > >
> > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > > index a2a56a50dd85227a4fdc62236a2710ca37c5ba52..30ce06df4153cfdc0fad9bc7bffab9097f8b0450 100644
> > > --- a/include/linux/compiler.h
> > > +++ b/include/linux/compiler.h
> > > @@ -316,6 +316,47 @@ static inline void *offset_to_ptr(const int *off)
> > >  #define statically_true(x) (__builtin_constant_p(x) && (x))
> > >  #define statically_false(x) (__builtin_constant_p(x) && (x) == 0)
> > >
> > > +/*
> > > + * Whether x is the integer constant expression 0 or something else.
> > > + *
> > > + * Details:
> > > + *   - The C11 standard defines in §6.3.2.3.3
> > > + *       (void *)<integer constant expression with the value 0>
> > > + *     as a null pointer constant (c.f. the NULL macro).
> > > + *   - If x evaluates to the integer constant expression 0,
> > > + *       (void *)(x)
> > > + *     is a null pointer constant. Else, it is a void * expression.
> > > + *   - In a ternary expression:
> > > + *       condition ? operand1 : operand2
> > > + *     if one of the two operands is of type void * and the other one
> > > + *     some other pointer type, the C11 standard defines in §6.5.15.6
> > > + *     the resulting type as below:
> > > + *       if one operand is a null pointer constant, the result has the
> > > + *       type of the other operand; otherwise [...] the result type is
> > > + *       a pointer to an appropriately qualified version of void.
> > > + *   - As such, in
> > > + *       0 ? (void *)(x) : (char *)0
> > > + *     if x is the integer constant expression 0, operand1 is a null
> > > + *     pointer constant and the resulting type is that of operand2:
> > > + *     char *. If x is anything else, the type is void *.
> > > + *   - The (long) cast silences a compiler warning for when x is not 0.
> > > + *   - Finally, the _Generic() dispatches the resulting type into a
> > > + *     Boolean.
> >
> > The comment is absolutely excessive.
> > I'm sure I managed about 2 lines in one of the patches I did.
> 
> I think that Linus made it  clear in:
> 
>   https://lore.kernel.org/all/CAHk-=wgfpLdt7SFFGcByTfHdkvv7AEa3MDu_s_W1kfOxQs49pw@mail.gmail.com/
> 
> that this deserves a detailed comment.

And he wrote one in https://lore.kernel.org/all/CAHk-=wiq=GUNWJwWh1CRAYchW73UmOaSkaCovLatfDKeveZctA@mail.gmail.com/

   /*
    * 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)



> 
> The details block in the current __is_constexpr() is 37 lines long,
> the details block in __is_const_zero() takes 22 lines. So I would
> argue that I made things better.

The old block was too long :-)

> 
> Unless more people share your concern, I am planning to keep this comment as-is.
> 
> > > + *
> > > + * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
> >
> > IIRC Martin has agreed in the past that the accreditation can
> > be removed - especially since it refers to the 'sizeof (void)' trick.
> 
> I tried to look for such message:
> 
>   https://lore.kernel.org/all/?q=f%3A%22martin+uecker%22+__is_constexpr
> 
> but couldn't find it. Do you have the link?
> 
> @Martin, do you agree that I remove the accreditation?
> 
> > > + */
> > > +#define __is_const_zero(x) \
> > > +     _Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)
> > > +
> > > +/*
> > > + * Returns a constant expression while determining if its argument is a
> > > + * constant expression, most importantly without evaluating the argument.
> >
> > You need to differentiate between a 'constant integer expression'
> > and a 'compile time constant'.
> 
> OK. This one was just copied from the previous __is_constexpr(). I will apply
> "s/constant expression/constant integer expression/g" in v2.
> 
> > > + *
> > > + * If getting a constant expression is not relevant to you, use the more
> > > + * powerful __builtin_constant_p() instead.
> >
> > __builtin_constant_p() is not 'more powerful' it is testing for
> > something different.
> 
> I meant to say that __builtin_constant_p() is more powerful at
> constant folding. But I agree that the comment is not clear.
> 
> What about this?
> 
>   If getting a constant integer expression is not relevant to you, use
>   __builtin_constant_p() which not only returns true if the argument
>   is an integer constant expression, but also if it is a compile time
>   constant.

Complete f***ed tense.

It's not about 'constant folding' and 'powerful' isn't the correct word.
They are checking for two different things.

A 'constant integer expression' is defined by the C language, and is
basically something that is constant when first parsed by the compiler
(my definition) so it can pretty much only contain constants, sizeof()
and offsetof().

__builtin_constant_p() is true if the compiler decides that an expression is
constant. This can track values through inlined function calls and can
change from 'unknown' to 'true' late in the compilation.

	David

> 
> 
> Yours sincerely,
> Vincent Mailhol

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

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Linus Torvalds]

On Thu, 5 Dec 2024 at 18:26, David Laight <David.Laight@aculab.com> wrote:
>
> From: Vincent Mailhol
> > ACK. Would adding a suggested--by Linus tag solve your concern?

I'm genberally the one person who doesn't need any more credit ;)

> I actually suspect the first patches to change __is_constexpr() to
> use _Generic were from myself.

Yes. And David was also I think the one who suggested something else
than "!!" originally too.

I may have liked "!!" for being very idiomatic and traditional C, but
there were those pesky compilers that warn about "integer in bool
context" or whatever the annoying warning was when then doing the
"multiply by zero" to turn a constant expression into a constant zero
expression.

So that

  #define is_const(x) __is_const_zero(0 * (x))

causes issues when 'x' is not an integer expression (think
"is_const(NULL)" or "is_const(1 == 2)".

Side note: I think "(x) == 0" will make sparse unhappy when 'x' is a
pointer, because it results that horrid "use integer zero as NULL
without a cast" thing when the plain zero gets implicitly cast to a
pointer. Which is a really nasty and broken C pattern and should never
have been silent.

I think David suggested using ((x)?0:0) at some point. Silly
nonsensical and complex expression, but maybe that finally gets rid of
all the warnings:

     #define is_const(x) __is_const_zero((x)?0:0)

might work regardless of the type of 'x'.

Or does that trigger some odd case too?

            Linus

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol]

On Fri. 6 Dec. 2024 at 15:14, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, 5 Dec 2024 at 18:26, David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Vincent Mailhol
> > > ACK. Would adding a suggested--by Linus tag solve your concern?
>
> I'm genberally the one person who doesn't need any more credit ;)
>
> > I actually suspect the first patches to change __is_constexpr() to
> > use _Generic were from myself.
>
> Yes. And David was also I think the one who suggested something else
> than "!!" originally too.

Ack. Then, I will add a suggested-by tag to credit David!

> I may have liked "!!" for being very idiomatic and traditional C, but
> there were those pesky compilers that warn about "integer in bool
> context" or whatever the annoying warning was when then doing the
> "multiply by zero" to turn a constant expression into a constant zero
> expression.
>
> So that
>
>   #define is_const(x) __is_const_zero(0 * (x))
>
> causes issues when 'x' is not an integer expression (think
> "is_const(NULL)" or "is_const(1 == 2)".

But 1 == 2 has already an integer type as proven by:

  #define is_int(x) _Generic(x, int: 1, default: 0)
  static_assert(is_int(1 == 2));

So, it leaves us with the case is_const(pointer). To which I would
question if we really want to support this. By definition, an
expression with a pointer type can not be an *integer* constant
expression. So one part of me tells me that it is a sane thing to
*not* support this case and throw a warning if the user feeds
is_cont() with a pointer.

If we just what to accept pointer arguments but still return false
(because those are not integers), one solution is:

  #define is_const(x) __is_const_zero((long)(x) * 0l)

This would be consistent with __is_constexpr(): it does accept NULL
(i.e. no warnings), but does not recognize it as an integer constant
expression, e.g.:

  is_const(NULL);

returns false with no warnings.

> Side note: I think "(x) == 0" will make sparse unhappy when 'x' is a
> pointer, because it results that horrid "use integer zero as NULL
> without a cast" thing when the plain zero gets implicitly cast to a
> pointer. Which is a really nasty and broken C pattern and should never
> have been silent.
>
> I think David suggested using ((x)?0:0) at some point. Silly
> nonsensical and complex expression, but maybe that finally gets rid of
> all the warnings:
>
>      #define is_const(x) __is_const_zero((x)?0:0)
>
> might work regardless of the type of 'x'.
>
> Or does that trigger some odd case too?

Following a quick test, this seems to work and to return true if given
NULL as an argument (contrary to the current __is_const_expr()). So if
we want to go beyond the C standard and extend the meaning of integer
constant expression in the kernel to also include constant pointers, I
agree that this is the way to go!

Side question, Linus, what do you think about the __is_const_zero()
documentation in

  https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-2-4e4cbaecc216@wanadoo.fr/

Do you think I am too verbose as pointed out by David? Some people
(including me and Yuri) like it that way, but if you also think this
is too much, I will make it shorter.

Thanks,


Yours sincerely,
Vincent Mailhol

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol]

On Fri. 6 Dec. 2024 at 16:19, Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
> On Fri. 6 Dec. 2024 at 15:14, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Thu, 5 Dec 2024 at 18:26, David Laight <David.Laight@aculab.com> wrote:

(...)

> > I may have liked "!!" for being very idiomatic and traditional C, but
> > there were those pesky compilers that warn about "integer in bool
> > context" or whatever the annoying warning was when then doing the
> > "multiply by zero" to turn a constant expression into a constant zero
> > expression.
> >
> > So that
> >
> >   #define is_const(x) __is_const_zero(0 * (x))
> >
> > causes issues when 'x' is not an integer expression (think
> > "is_const(NULL)" or "is_const(1 == 2)".
>
> But 1 == 2 has already an integer type as proven by:
>
>   #define is_int(x) _Generic(x, int: 1, default: 0)
>   static_assert(is_int(1 == 2));
>
> So, it leaves us with the case is_const(pointer). To which I would
> question if we really want to support this. By definition, an
> expression with a pointer type can not be an *integer* constant
> expression. So one part of me tells me that it is a sane thing to
> *not* support this case and throw a warning if the user feeds
> is_cont() with a pointer.
>
> If we just what to accept pointer arguments but still return false
> (because those are not integers), one solution is:
>
>   #define is_const(x) __is_const_zero((long)(x) * 0l)
>
> This would be consistent with __is_constexpr(): it does accept NULL
> (i.e. no warnings), but does not recognize it as an integer constant
> expression, e.g.:
>
>   is_const(NULL);
>
> returns false with no warnings.
>
> > Side note: I think "(x) == 0" will make sparse unhappy when 'x' is a
> > pointer, because it results that horrid "use integer zero as NULL
> > without a cast" thing when the plain zero gets implicitly cast to a
> > pointer. Which is a really nasty and broken C pattern and should never
> > have been silent.
> >
> > I think David suggested using ((x)?0:0) at some point. Silly
> > nonsensical and complex expression, but maybe that finally gets rid of
> > all the warnings:
> >
> >      #define is_const(x) __is_const_zero((x)?0:0)
> >
> > might work regardless of the type of 'x'.
> >
> > Or does that trigger some odd case too?
>
> Following a quick test, this seems to work and to return true if given
> NULL as an argument (contrary to the current __is_const_expr()). So if
> we want to go beyond the C standard and extend the meaning of integer
> constant expression in the kernel to also include constant pointers, I
> agree that this is the way to go!

I just came up with a new idea:

  #define is_const(x) __is_const_zero((x) != (x))

Similarly to ((x)?0:0), this seems to work with everything (including
with NULL), but arguably a bit less ugly.

> Side question, Linus, what do you think about the __is_const_zero()
> documentation in
>
>   https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-2-4e4cbaecc216@wanadoo.fr/
>
> Do you think I am too verbose as pointed out by David? Some people
> (including me and Yuri) like it that way, but if you also think this
> is too much, I will make it shorter.
>
> Thanks,
>
>
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Donnerstag, dem 05.12.2024 um 22:14 -0800 schrieb Linus Torvalds:
> On Thu, 5 Dec 2024 at 18:26, David Laight <David.Laight@aculab.com> wrote:
> > 
> > From: Vincent Mailhol
> > > ACK. Would adding a suggested--by Linus tag solve your concern?
> 
> I'm genberally the one person who doesn't need any more credit ;)
> 
> > I actually suspect the first patches to change __is_constexpr() to
> > use _Generic were from myself.
> 
> Yes. And David was also I think the one who suggested something else
> than "!!" originally too.
> 
> I may have liked "!!" for being very idiomatic and traditional C, but
> there were those pesky compilers that warn about "integer in bool
> context" or whatever the annoying warning was when then doing the
> "multiply by zero" to turn a constant expression into a constant zero
> expression.
> 
> So that
> 
>   #define is_const(x) __is_const_zero(0 * (x))
> 
> causes issues when 'x' is not an integer expression (think
> "is_const(NULL)" or "is_const(1 == 2)".
> 
> Side note: I think "(x) == 0" will make sparse unhappy when 'x' is a
> pointer, because it results that horrid "use integer zero as NULL
> without a cast" thing when the plain zero gets implicitly cast to a
> pointer. Which is a really nasty and broken C pattern and should never
> have been silent.


BTW: I added '-Wzero-as-null-pointer-constant' to GCC 15.

Hopefully we can also remove / deprecate this for C2Y.

Martin

> 
> I think David suggested using ((x)?0:0) at some point. Silly
> nonsensical and complex expression, but maybe that finally gets rid of
> all the warnings:
> 
>      #define is_const(x) __is_const_zero((x)?0:0)
> 
> might work regardless of the type of 'x'.
> 
> Or does that trigger some odd case too?

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol]

Grrr, I just realized that my emails are bouncing… Didn't happen
before, I guess this is because my smtp server is unhappy with the
many people in CC.

Resending using another address (and, while at it, redacted two
messages to combine them in one and added more details).

My deep apologies for that, really sorry. Also, maybe the bouncing
messages will finally find their way out? In that case, please just
ignore them, and sorry for the noise.

On Fri. 6 Dec. 2024 at 15:14, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, 5 Dec 2024 at 18:26, David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Vincent Mailhol
> > > ACK. Would adding a suggested--by Linus tag solve your concern?
>
> I'm genberally the one person who doesn't need any more credit ;)
>
> > I actually suspect the first patches to change __is_constexpr() to
> > use _Generic were from myself.
>
> Yes. And David was also I think the one who suggested something else
> than "!!" originally too.

Ack. Then, I will add a suggested-by tag to credit David!

> I may have liked "!!" for being very idiomatic and traditional C, but
> there were those pesky compilers that warn about "integer in bool
> context" or whatever the annoying warning was when then doing the
> "multiply by zero" to turn a constant expression into a constant zero
> expression.
>
> So that
>
>   #define is_const(x) __is_const_zero(0 * (x))
>
> causes issues when 'x' is not an integer expression (think
> "is_const(NULL)" or "is_const(1 == 2)".

But 1 == 2 already has an integer type as proven by:

  #define is_int(x) _Generic(x, int: 1, default: 0)
  static_assert(is_int(1 == 2));

And regardless, __is_const_zero() performs a (long) cast, so
__is_const_zero() works for any other scalar types. Even the u128
works because after the (0 * (x)) multiplication, you are left with
zero, preventing any loss of precision when casting to (long).

Here are some tests to prove my claims:

  https://godbolt.org/z/beGs841sz

So, it leaves us with the is_const(pointer) case. To which I would
question if we really want to support this. By definition, an
expression with a pointer type can not be an *integer* constant
expression. So one part of me tells me that it is a sane thing to
*not* support this case and throw a warning if the user feeds
is_const() with a pointer.

By the way, currently, __is_constexpr(NULL) returns false:

  https://godbolt.org/z/f5P9oP9n5

and the reason this went unnotified in the kernel is because no one is
using it that way. So instead of complicating the macro for a use case
which currently does not exist (and which goes against the definition
of an ICE in the C standard), why not say instead that this is not
supported by just leaving the constraint violation in is_const()?

> Side note: I think "(x) == 0" will make sparse unhappy when 'x' is a
> pointer, because it results that horrid "use integer zero as NULL
> without a cast" thing when the plain zero gets implicitly cast to a
> pointer. Which is a really nasty and broken C pattern and should never
> have been silent.
>
> I think David suggested using ((x)?0:0) at some point. Silly
> nonsensical and complex expression, but maybe that finally gets rid of
> all the warnings:
>
>      #define is_const(x) __is_const_zero((x)?0:0)
>
> might work regardless of the type of 'x'.

If we *really* want to go beyond the C standard and extend the meaning
of integer constant expressions in the kernel to also include constant
pointers, then what about:

  #define is_const(x) __is_const_zero((x) != (x))

As established above, comparaisons return an int, thus making this
pass all tests:

  https://godbolt.org/z/f5zrzf44h

And it is slightly more pretty than the ((x)?0:0).

(...)

Side question, Linus, what do you think about the __is_const_zero()
documentation:

  https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-2-4e4cbaecc216@wanadoo.fr/

Do you think I am too verbose as pointed out by David? Some people
(including me and Yuri) like it that way. But if you also think this
is too much, I will make it shorter.

Thanks,


Yours sincerely,
Vincent Mailhol

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Linus Torvalds]

On Fri, 6 Dec 2024 at 10:31, Vincent Mailhol <vincent.mailhol@gmail.com> wrote:
>
> > causes issues when 'x' is not an integer expression (think
> > "is_const(NULL)" or "is_const(1 == 2)".
>
> But 1 == 2 already has an integer type as proven by:

Yeah, I was confused about exactly what triggers that odd
'-Wint-in-bool-context'.

It's not about some actual bool type, it's literally a random
collection of integer operations used with logical ops.

So it's things like "!(var<<2)" that generate that warning, because
some compiler person at some point went "maybe that left shift should
have been just a comparison instead '<'".

But it turns out that "(var <<2)?0:0" _also_ triggers that warning.

End result: I have *no* idea how to shut that crazy warning up for
this case, if we want to have some generic macro that says "is this
constant". Because it damn well is perfectly sane to ask "is (a << 3)
a constant expression".

How very annoying.

This may be a case of "we just need to disable that incorrect compiler
warning". Or does anybody see a workaround?

             Linus

              Linus

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Linus Torvalds]

On Fri, 6 Dec 2024 at 10:52, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> This may be a case of "we just need to disable that incorrect compiler
> warning". Or does anybody see a workaround?

Hmm. The "== 0" thing does work, but as mentioned, that causes (more
valid, imho) warnings with pointers.

And it's not necessarily require that a pointer expression actually be
marked as a constant, as for the fact that these macros often get used
in various arbitrary contexts where things *might* be pointers, even
if "not constant" is a perfectly fine answer.

We do actually consciously use __builtin_constant_p() on pointers.
It's very convenient for format strings in particular, where
__builtin_constant_p() is a good test for a constant string, which
sometimes gets treated differently.

And in fact, dealing with NULL pointers statically might be worth it
too, so I do think it's worth keeping in mind.

               Linus

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Linus Torvalds
> Sent: 06 December 2024 18:53
> 
> On Fri, 6 Dec 2024 at 10:31, Vincent Mailhol <vincent.mailhol@gmail.com> wrote:
> >
> > > causes issues when 'x' is not an integer expression (think
> > > "is_const(NULL)" or "is_const(1 == 2)".
> >
> > But 1 == 2 already has an integer type as proven by:
> 
> Yeah, I was confused about exactly what triggers that odd
> '-Wint-in-bool-context'.
> 
> It's not about some actual bool type, it's literally a random
> collection of integer operations used with logical ops.
> 
> So it's things like "!(var<<2)" that generate that warning, because
> some compiler person at some point went "maybe that left shift should
> have been just a comparison instead '<'".
> 
> But it turns out that "(var <<2)?0:0" _also_ triggers that warning.
> 
> End result: I have *no* idea how to shut that crazy warning up for
> this case, if we want to have some generic macro that says "is this
> constant". Because it damn well is perfectly sane to ask "is (a << 3)
> a constant expression".

I'm missing the compiler version and options to generate the error.
Does a '+ 0' help?  "(var << 2) + 0 ? 0 : 0"

I realised the:
#define const_NULL(x) _Generic(0 ? (x) : (char *)0, char *: 1, void *: 0)
#define const_true(x) const_NULL((x) ? NULL : (void *)1L))
#define const_expr(x) const_NULL((x) ? NULL : NULL))
I send this morning.
Needs 's/char/struct kjkjkjkjui/' applied.

	David

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

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Linus Torvalds]

On Fri, 6 Dec 2024 at 11:07, David Laight <David.Laight@aculab.com> wrote:
>
> I'm missing the compiler version and options to generate the error.

Just -Wall with most recent gcc versions seems to do it. At least I
can repro it with gcc-14.2.1 and something silly like this:

  $ cat t.c
  int fn(int a) { return (a<<2)?1:2; }
  $ gcc -Wall -S t.c
  t.c: In function ‘fn’:
  t.c:1:26: warning: ‘<<’ in boolean context, did you mean ‘<’?
[-Wint-in-bool-context]

> Does a '+ 0' help?  "(var << 2) + 0 ? 0 : 0"

Yeah, that actually works.

And "+0" is nice in that it should work in any context.

> #define const_NULL(x) _Generic(0 ? (x) : (char *)0, char *: 1, void *: 0)
> #define const_true(x) const_NULL((x) ? NULL : (void *)1L))
> #define const_expr(x) const_NULL((x) ? NULL : NULL))
> I send this morning.
> Needs 's/char/struct kjkjkjkjui/' applied.

Oh Christ. You really are taking this whole ugly to another level.

           Linus

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Willy Tarreau]

On Fri, Dec 06, 2024 at 11:15:20AM -0800, Linus Torvalds wrote:
> On Fri, 6 Dec 2024 at 11:07, David Laight <David.Laight@aculab.com> wrote:
> >
> > I'm missing the compiler version and options to generate the error.
> 
> Just -Wall with most recent gcc versions seems to do it. At least I
> can repro it with gcc-14.2.1 and something silly like this:
> 
>   $ cat t.c
>   int fn(int a) { return (a<<2)?1:2; }
>   $ gcc -Wall -S t.c
>   t.c: In function 'fn':
>   t.c:1:26: warning: '<<' in boolean context, did you mean '<'?
> [-Wint-in-bool-context]
> 
> > Does a '+ 0' help?  "(var << 2) + 0 ? 0 : 0"
> 
> Yeah, that actually works.
> 
> And "+0" is nice in that it should work in any context.

I've already used "+0" to shut certain warnings, I don't really remember
which one, but also remember it was OK everywhere I needed.

Another trick I've been using to shut up the compiler is a cast via typeof
and an intermediary variable:

  #define shut_up(expr)                           \
          ({                                      \
                  typeof(expr) _expr_ = expr;     \
                  _expr_;                         \
                  })
  
  int fn1(int a)
  {
          return shut_up(a << 2) ? 1 : 2;
  }
  
  int fn2(int a)
  {
          return (a << 2) ? 1 : 2;
  }

  $ gcc -Wall -S t2.c
  t2.c: In function 'fn2':
  t2.c:15:19: warning: '<<' in boolean context, did you mean '<'? [-Wint-in-bool-context]
     15 |         return (a << 2) ? 1 : 2;
        |                ~~~^~~~~

See ? It only complains on fn2() but not fn1(). Similarly I found it
to be quite portable (at least I don't remember seeing it fail on me).
It produces the exact same code, except at -O0 where it does really
create a local variable.

Willy

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Willy Tarreau
> Sent: 06 December 2024 19:39
> On Fri, Dec 06, 2024 at 11:15:20AM -0800, Linus Torvalds wrote:
> > On Fri, 6 Dec 2024 at 11:07, David Laight <David.Laight@aculab.com> wrote:
> > >
> > > I'm missing the compiler version and options to generate the error.
> >
> > Just -Wall with most recent gcc versions seems to do it. At least I
> > can repro it with gcc-14.2.1 and something silly like this:
> >
> >   $ cat t.c
> >   int fn(int a) { return (a<<2)?1:2; }
> >   $ gcc -Wall -S t.c
> >   t.c: In function 'fn':
> >   t.c:1:26: warning: '<<' in boolean context, did you mean '<'?
> > [-Wint-in-bool-context]
> >
> > > Does a '+ 0' help?  "(var << 2) + 0 ? 0 : 0"
> >
> > Yeah, that actually works.
> >
> > And "+0" is nice in that it should work in any context.
> 
> I've already used "+0" to shut certain warnings, I don't really remember
> which one, but also remember it was OK everywhere I needed.

I've often used +0u when -Wsign-compare is enabled.
Much safer than a cast.

> 
> Another trick I've been using to shut up the compiler is a cast via typeof
> and an intermediary variable:
> 
>   #define shut_up(expr)                           \
>           ({                                      \
>                   typeof(expr) _expr_ = expr;     \
>                   _expr_;                         \
>                   })

That is like OPTIMISER_HIDE_VAR() and can't be used in a 'constant integer expression'.

I suspect it also has the same nasty habit of adding an extra register move.

	David

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

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Linus Torvalds
> Sent: 06 December 2024 19:15
> On Fri, 6 Dec 2024 at 11:07, David Laight <David.Laight@aculab.com> wrote:
> >
> > I'm missing the compiler version and options to generate the error.
> 
> Just -Wall with most recent gcc versions seems to do it. At least I
> can repro it with gcc-14.2.1 and something silly like this:

I may have just missed golbolt returning a warning.
...
> > Does a '+ 0' help?  "(var << 2) + 0 ? 0 : 0"
> 
> Yeah, that actually works.
> 
> And "+0" is nice in that it should work in any context.

Unless it falls foul of the clang test for arthmetic on NULL pointers.
(I'm sure that is only a problem if NULL isn't the all-zero bit pattern.
And pretty much no C code has ever been 'that portable'.)

Adding 0 can also help when compliers are being picky about enums.
Since none have (yet) made them more like a 32bit pointer to a one
byte structure (or the Pascal enum).
In case the relevant people are reading this, maybe only do any
non-integer warnings for named enums?

> > #define const_NULL(x) _Generic(0 ? (x) : (char *)0, char *: 1, void *: 0)
> > #define const_true(x) const_NULL((x) ? NULL : (void *)1L))
> > #define const_expr(x) const_NULL((x) ? NULL : NULL))
> > I send this morning.
> > Needs 's/char/struct kjkjkjkjui/' applied.
> 
> Oh Christ. You really are taking this whole ugly to another level.

I sort of liked that version in a perverse sort of way.
It does give you a simple test for NULL (unless you've used 'struct kjkjkjkjui').

	David

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

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

...
> > > #define const_NULL(x) _Generic(0 ? (x) : (char *)0, char *: 1, void *: 0)
> > > #define const_true(x) const_NULL((x) ? NULL : (void *)1L))
> > > #define const_expr(x) const_NULL((x) ? NULL : NULL))
> > > I send this morning.
> > > Needs 's/char/struct kjkjkjkjui/' applied.
> >
> > Oh Christ. You really are taking this whole ugly to another level.
> 
> I sort of liked that version in a perverse sort of way.
> It does give you a simple test for NULL (unless you've used 'struct kjkjkjkjui').

Except const_NULL() really doesn't work at all - so you are lucky :-)

So maybe the slightly long lines:
#define const_true(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 1L) : (char *)0, char *: 1, void *: 0)
#define const_expr(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 0L) : (char *)0, char *: 1, void *: 0)

I make that 98 characters.
Of course, you can remove all the spaces, only one of the constants need the L suffix
and 'int' is a shorter type name.
That cuts is down to 76:
#define const_expr(x) _Generic(0?(void*)((x)+0?0L:0):(int*)0,int*:1,void*:0)
which starts looking like the TECO commands to parse its command line!

	David

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

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol]

On Sat. 7 Dec. 2024 at 05:24, David Laight <David.Laight@aculab.com> wrote:
> > > > #define const_NULL(x) _Generic(0 ? (x) : (char *)0, char *: 1, void *: 0)
> > > > #define const_true(x) const_NULL((x) ? NULL : (void *)1L))
> > > > #define const_expr(x) const_NULL((x) ? NULL : NULL))
> > > > I send this morning.
> > > > Needs 's/char/struct kjkjkjkjui/' applied.
> > >
> > > Oh Christ. You really are taking this whole ugly to another level.
> >
> > I sort of liked that version in a perverse sort of way.
> > It does give you a simple test for NULL (unless you've used 'struct kjkjkjkjui').
>
> Except const_NULL() really doesn't work at all - so you are lucky :-)
>
> So maybe the slightly long lines:
> #define const_true(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 1L) : (char *)0, char *: 1, void *: 0)
> #define const_expr(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 0L) : (char *)0, char *: 1, void *: 0)

This still throws a -Wnull-pointer-arithmetic on clang on const_expr(NULL):

  https://godbolt.org/z/vo5W7efdE

I just do not see a method to silence that one. So three options:

  1. is_const() does not accept pointers and throws a constraint
     violation:

       #define is_const(x) __is_const_zero(0 * (x))

     This is my current patch.

  2. is_const() accept pointers but is_const(NULL) returns false:

       #define is_const(x) __is_const_zero((x) != (x))

     This keeps the current __is_constexpr() behaviour.

  3. is_const() accepts pointers and is_const(NULL) return true:

       #define const_expr(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 0L)
: (char *)0, char *: 1, void *: 0)

     David's latest proposal, it requires to remove the
     -Wnull-pointer-arithmetic clang warning.

I vote for 1. or 2. (with a preference for 1.). IMHO, we are just
adding an unreasonable level of complexity for making the macro treat
NULL as an integer. Would someone find a solution for 3. that does not
yield a warning, then why not. But if we have to remove a compiler
check for a theoretical use case that does not even exist in the
kernel, then it is not worth the trade off.

Concerning is_const(var << 2), the patch I submitted works fine as-is
with all scalars including that (var << 2):

  https://godbolt.org/z/xer4aMees

And can we ignore the case (!(var << 2))? This is not a warning
because of the macro, but because of the caller! If I do any of:

          if (!(var << 2)) {}
          (void)__builtin_constant_p(!(var << 2));

I also got the warning. The point is that the macro should not
generate *new* warnings. If the given argument already raises a
warning, it is the caller's responsibility to fix.


Yours sincerely,
Vincent Mailhol

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Vincent Mailhol
> Sent: 07 December 2024 07:43
...
> > So maybe the slightly long lines:
> > #define const_true(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 1L) : (char *)0, char *: 1, void *: 0)
> > #define const_expr(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 0L) : (char *)0, char *: 1, void *: 0)

Clearly they can be implemented in terms of a common define.
But I don't see a need for a const_zero() and nested expansions make extra
work for the compiler.

> 
> This still throws a -Wnull-pointer-arithmetic on clang on const_expr(NULL):
>   https://godbolt.org/z/vo5W7efdE

I was worried about that one.

> I just do not see a method to silence that one. So three options:
> 
>   1. is_const() does not accept pointers and throws a constraint violation:
>        #define is_const(x) __is_const_zero(0 * (x))
>      This is my current patch.

Is that going to affect things like const_true(x << y)?
Disallowing that seems counter-productive.
(Remember it might be passed into a #define that is then
checking its argument for being constant.)

>   2. is_const() accept pointers but is_const(NULL) returns false:
>        #define is_const(x) __is_const_zero((x) != (x))
>      This keeps the current __is_constexpr() behaviour.

No good - expands everything twice.

>   3. is_const() accepts pointers and is_const(NULL) return true:
> 
>        #define const_expr(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 0L)
> : (char *)0, char *: 1, void *: 0)
> 
>      David's latest proposal, it requires to remove the
>      -Wnull-pointer-arithmetic clang warning.

Only for const_expr(NULL) - and since clang gets that wrong
maybe the warning is a good thing.

You can just add:
#define const_NULL(ptr) const_true(!(ptr))
Probably the only place where you actually want to test for zero.

> 
> I vote for 1. or 2. (with a preference for 1.). IMHO, we are just
> adding an unreasonable level of complexity for making the macro treat
> NULL as an integer. Would someone find a solution for 3. that does not
> yield a warning, then why not. But if we have to remove a compiler
> check for a theoretical use case that does not even exist in the
> kernel, then it is not worth the trade off.
> 
> Concerning is_const(var << 2), the patch I submitted works fine as-is
> with all scalars including that (var << 2):
> 
>   https://godbolt.org/z/xer4aMees
> 
> And can we ignore the case (!(var << 2))? This is not a warning
> because of the macro, but because of the caller! If I do any of:
> 
>           if (!(var << 2)) {}
>           (void)__builtin_constant_p(!(var << 2));
> 
> I also got the warning. The point is that the macro should not
> generate *new* warnings. If the given argument already raises a
> warning, it is the caller's responsibility to fix.

Except it could easily happen way inside some other expansion.
Perhaps someone optimises frobnicate(x) for constant input.
Suddenly frobnicate(!(var << 2)) generates a compile error.

	David

> 
> 
> Yours sincerely,
> Vincent Mailhol

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

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol]

On Sat. 7 Dec. 2024 at 20:19, David Laight <David.Laight@aculab.com> wrote:
> From: Vincent Mailhol
> > Sent: 07 December 2024 07:43
> ...
> > > So maybe the slightly long lines:
> > > #define const_true(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 1L) : (char *)0, char *: 1, void *: 0)
> > > #define const_expr(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 0L) : (char *)0, char *: 1, void *: 0)
>
> Clearly they can be implemented in terms of a common define.
> But I don't see a need for a const_zero() and nested expansions make extra
> work for the compiler.
>
> >
> > This still throws a -Wnull-pointer-arithmetic on clang on const_expr(NULL):
> >   https://godbolt.org/z/vo5W7efdE
>
> I was worried about that one.
>
> > I just do not see a method to silence that one. So three options:
> >
> >   1. is_const() does not accept pointers and throws a constraint violation:
> >        #define is_const(x) __is_const_zero(0 * (x))
> >      This is my current patch.
>
> Is that going to affect things like const_true(x << y)?

No warnings for const_true(x << y). See by yourself:

  https://godbolt.org/z/v79x3dnor

> Disallowing that seems counter-productive.
> (Remember it might be passed into a #define that is then
> checking its argument for being constant.)

I understand how a static inline may use __builtin_constant_p() to
check if its argument is NULL. But I am having a hard time
understanding how this would look like with macros and with
is_const().

Do you know one real life example where you have to pass NULL to a
function like macro which can not be written as a static inline
function?

And if that real life use case does not exist, then disallowing it
looks sane: it will push the developper to do the right thing: write a
static inline with __builting_constant_p().

> >   2. is_const() accept pointers but is_const(NULL) returns false:
> >        #define is_const(x) __is_const_zero((x) != (x))
> >      This keeps the current __is_constexpr() behaviour.
>
> No good - expands everything twice.

And? __is_const_zero() does not evaluate its arguments, so no side effect:

  https://godbolt.org/z/W988P4v9e

Or am I missing something else?

> >   3. is_const() accepts pointers and is_const(NULL) return true:
> >
> >        #define const_expr(x) _Generic(0 ? (void *)((x) + 0 ? 0L : 0L)
> > : (char *)0, char *: 1, void *: 0)
> >
> >      David's latest proposal, it requires to remove the
> >      -Wnull-pointer-arithmetic clang warning.
>
> Only for const_expr(NULL) - and since clang gets that wrong
> maybe the warning is a good thing.
>
> You can just add:
> #define const_NULL(ptr) const_true(!(ptr))
> Probably the only place where you actually want to test for zero.

Which goes back to my first point: if we have to declare const_NULL(),
then we probably do not want is_const() to accept NULL so that the
user sees the mismatch (and I am not acknowledging that we need
const_NULL(): as long as no use case arize, no need for it).

> > I vote for 1. or 2. (with a preference for 1.). IMHO, we are just
> > adding an unreasonable level of complexity for making the macro treat
> > NULL as an integer. Would someone find a solution for 3. that does not
> > yield a warning, then why not. But if we have to remove a compiler
> > check for a theoretical use case that does not even exist in the
> > kernel, then it is not worth the trade off.
> >
> > Concerning is_const(var << 2), the patch I submitted works fine as-is
> > with all scalars including that (var << 2):
> >
> >   https://godbolt.org/z/xer4aMees
> >
> > And can we ignore the case (!(var << 2))? This is not a warning
> > because of the macro, but because of the caller! If I do any of:
> >
> >           if (!(var << 2)) {}
> >           (void)__builtin_constant_p(!(var << 2));
> >
> > I also got the warning. The point is that the macro should not
> > generate *new* warnings. If the given argument already raises a
> > warning, it is the caller's responsibility to fix.
>
> Except it could easily happen way inside some other expansion.
> Perhaps someone optimises frobnicate(x) for constant input.
> Suddenly frobnicate(!(var << 2)) generates a compile error.

Then, that person will see that compile error and will modify
frobnicate() to remove the compile error. End of the story.

My point is simple: if the use case you are mentioning is valid, why
did it never collide with __builtin_constant_p() that we have been
using for many years?

And I am sure that there are hundreds of examples of macro that will
either pass their argument as-is to an if() or to the ternary operator
without accounting for if their input is (!(var << 2)). So why did not
these collide?


Yours sincerely,
Vincent Mailhol

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Linus Torvalds]

On Sat, 7 Dec 2024 at 04:24, Vincent Mailhol <vincent.mailhol@gmail.com> wrote:
>
> > No good - expands everything twice.
>
> And? __is_const_zero() does not evaluate its arguments, so no side effect:

No, the problem is literally the expansion.

Double expansion of these fundamental helpers gets exponential,
because they are used in various nested ways in other fundamental
helpers.

That's why we then spent so much effort on trying to clean up the
min/max macros, because a single line of code would expand to
literally tens of megabytes of horrific expansions.

And the problem with these things is that you can't make them inline
functions, so they have to be macros, and then you build up other
macros using them (like that "clamp()" macro), and it really gets
horrendous and affects the build time.

And yes, it is very sad. Particularly since a compiler would have a
really easy time with some nice helper builtins.

Of course, often the compiler *does* have helper builtins, but we
can't use them, because they aren't *quite* the right thing. Like that
"__builtin_constant_p()" not actually working for some situations
where we absolutely need not just a constant value, but a constant
_expression_ due to C parsing rules.

Quite a lot of the pain we tend to have with these things is directly
related to the fact that we often want to do these tests in contexts
like global array initializers etc.

If there is one feature of C I would have liked it is "allow inline
functions and statement expressions with constant arguments as
constant expressions". Other languages have done that, and it really
does help. And yes, it means that you have to basically have a
language interpreter in the compiler (you do want to allow loop
constructions etc), but it really is very useful.

Oh well. Even if compilers added that today, it would be years until
we could take advantage of it.

At one point I literally was thinking I'd do 'sparse' as a
pre-processor for kernel code, in order to have extended language
facilities like that.

            Linus

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Samstag, dem 07.12.2024 um 10:19 -0800 schrieb Linus Torvalds:
> On Sat, 7 Dec 2024 at 04:24, Vincent Mailhol <vincent.mailhol@gmail.com> wrote:
> > 
> > > No good - expands everything twice.
> > 
> > And? __is_const_zero() does not evaluate its arguments, so no side effect:
> 
> No, the problem is literally the expansion.
> 
> Double expansion of these fundamental helpers gets exponential,
> because they are used in various nested ways in other fundamental
> helpers.
> 
> That's why we then spent so much effort on trying to clean up the
> min/max macros, because a single line of code would expand to
> literally tens of megabytes of horrific expansions.
> 
> And the problem with these things is that you can't make them inline
> functions, so they have to be macros, and then you build up other
> macros using them (like that "clamp()" macro), and it really gets
> horrendous and affects the build time.
> 
> And yes, it is very sad. Particularly since a compiler would have a
> really easy time with some nice helper builtins.
> 
> Of course, often the compiler *does* have helper builtins, but we
> can't use them, because they aren't *quite* the right thing. Like that
> "__builtin_constant_p()" not actually working for some situations
> where we absolutely need not just a constant value, but a constant
> _expression_ due to C parsing rules.
> 
> Quite a lot of the pain we tend to have with these things is directly
> related to the fact that we often want to do these tests in contexts
> like global array initializers etc.
> 
> If there is one feature of C I would have liked it is "allow inline
> functions and statement expressions with constant arguments as
> constant expressions". Other languages have done that, and it really
> does help. And yes, it means that you have to basically have a
> language interpreter in the compiler (you do want to allow loop
> constructions etc), but it really is very useful.
> 
> Oh well. Even if compilers added that today, it would be years until
> we could take advantage of it.
> 
> At one point I literally was thinking I'd do 'sparse' as a
> pre-processor for kernel code, in order to have extended language
> facilities like that.

There exist proposals along those lines for C2Y.

From a more near-term solution, I wonder if making it possible (or
easier) to return integer constant expressions from statement
expressions and allowing a restricted form of statement expressions 
at file scope would help?


Martin

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Linus Torvalds]

On Sat, 7 Dec 2024 at 11:51, Martin Uecker <muecker@gwdg.de> wrote:
>
> Am Samstag, dem 07.12.2024 um 10:19 -0800 schrieb Linus Torvalds:
> >
> > If there is one feature of C I would have liked it is "allow inline
> > functions and statement expressions with constant arguments as
> > constant expressions".
>
> There exist proposals along those lines for C2Y.
>
> From a more near-term solution, I wonder if making it possible (or
> easier) to return integer constant expressions from statement
> expressions and allowing a restricted form of statement expressions
> at file scope would help?

Even a more limited form of this would have been useful several times,
but as mentioned, the problem tends to be that we end up supporting
compilers for many years.

So then we end up having to work with older compilers without that
feature, and can't actually clean stuff up until many years after the
fact.

We're currently still accepting gcc-5.1 as a compiler, although it's
time to look at that and probably (judging by what stable distros use)
upgrade to something like gcc-8.1 as the minimum supported compiler
version.

            Linus

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Linus Torvalds
> Sent: 07 December 2024 20:31
...
> We're currently still accepting gcc-5.1 as a compiler, although it's
> time to look at that and probably (judging by what stable distros use)
> upgrade to something like gcc-8.1 as the minimum supported compiler
> version.

That's going to annoy me.
The system disk in the system I test build kernel on is actually older
than the machine! (not by much).
And Ubuntu 18.04 (still getting some fixes) has gcc 7.5.0.

It isn't as though the 8.1 update is anything really major.
Disabling stack canaries would let an older compiler be used.
(and I might change the tests...)

Much more useful would be mandating 'asm go with outputs' which
would cut out a whole load horrid alternatives.

But that would make it pretty common that a kernel build would
need a later compiler than the one the distribution installed.

It maybe time to consider directly supporting downloading and
building the required compiler as part of a normal kernel build.
That would allow the minimum version to be set to a very recent
build and also make cross architecture build easier.
(In effect all builds become cross builds.)

NetBSD used to (may still do) import gcc into its CVS repository.
So that everything was built with a 'known' compiler.

It is (probably) less of a problem with clang.
People using clang are likely to have explicitly downloaded it.

	David

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

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Martin Uecker
> Sent: 07 December 2024 19:52
...
> > Of course, often the compiler *does* have helper builtins, but we
> > can't use them, because they aren't *quite* the right thing. Like that
> > "__builtin_constant_p()" not actually working for some situations
> > where we absolutely need not just a constant value, but a constant
> > _expression_ due to C parsing rules.

In many cases the C language says 'constant integer expression'
but is reality something that passes __builtin_constant_p()
is likely to be fine - because the actual value isn't needed
until late on in the compilation.

...
> > If there is one feature of C I would have liked it is "allow inline
> > functions and statement expressions with constant arguments as
> > constant expressions". Other languages have done that, and it really
> > does help. And yes, it means that you have to basically have a
> > language interpreter in the compiler (you do want to allow loop
> > constructions etc), but it really is very useful.

It the output needs to be a constant the loop would have to be unrollable.
In those cases the result will be 'constant enough' for __builtin_constant_p()
and the compiler is actually already capable doing it.

...
> > At one point I literally was thinking I'd do 'sparse' as a
> > pre-processor for kernel code, in order to have extended language
> > facilities like that.

You could use m4 :-) and make it entirely unreadable.

> There exist proposals along those lines for C2Y.
> 
> From a more near-term solution, I wonder if making it possible (or
> easier) to return integer constant expressions from statement
> expressions and allowing a restricted form of statement expressions
> at file scope would help?

It would help a lot if a #define that just used local variables
to avoid arguments being re-expanded and for CSE could still
generate a constant value.
Does need to be a #define - to get token pasting and 'stringify'.
Although you would need something for reporting detected errors,
and builtin compiler support for const_true() for the detection
itself.



	David

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

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Samstag, dem 07.12.2024 um 21:00 +0000 schrieb David Laight:
> From: Martin Uecker
> > Sent: 07 December 2024 19:52
...

> 
> > There exist proposals along those lines for C2Y.
> > 
> > From a more near-term solution, I wonder if making it possible (or
> > easier) to return integer constant expressions from statement
> > expressions and allowing a restricted form of statement expressions
> > at file scope would help?
> 
> It would help a lot if a #define that just used local variables
> to avoid arguments being re-expanded and for CSE could still
> generate a constant value.
> Does need to be a #define - to get token pasting and 'stringify'.
> Although you would need something for reporting detected errors,
> and builtin compiler support for const_true() for the detection
> itself.

We are super close:

https://godbolt.org/z/Tarq89bha

(if we ignore the grotesque hacks to get there, but this would go
away if the compiler does it internally)

Martin

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Martin Uecker
> Sent: 07 December 2024 21:06
> 
> Am Samstag, dem 07.12.2024 um 21:00 +0000 schrieb David Laight:
> > From: Martin Uecker
> > > Sent: 07 December 2024 19:52
> ...
> 
> >
> > > There exist proposals along those lines for C2Y.
> > >
> > > From a more near-term solution, I wonder if making it possible (or
> > > easier) to return integer constant expressions from statement
> > > expressions and allowing a restricted form of statement expressions
> > > at file scope would help?
> >
> > It would help a lot if a #define that just used local variables
> > to avoid arguments being re-expanded and for CSE could still
> > generate a constant value.
> > Does need to be a #define - to get token pasting and 'stringify'.
> > Although you would need something for reporting detected errors,
> > and builtin compiler support for const_true() for the detection
> > itself.
> 
> We are super close:
> 
> https://godbolt.org/z/Tarq89bha

(The preprocess output is about 500 bytes for each line.)

> (if we ignore the grotesque hacks to get there, but this would go
> away if the compiler does it internally)

Some of those hacks look excessive.
Isn't IF_CONST(x, y, z) just
	_Generic(0 ? (void *)((x) ? 0L : 0L) : (char *)0, char *: y, void *: z)
and that gets rid some of the grossness.
Just having that as a builtin would simplify some things.
Although you could use:
	__builtin_choose_expr(IS_CONST(x), y, z)
if you need y and z to have different types, and just:
	IS_CONST(x) ? y : z
otherwise.
Since AFAICT they are otherwise equivalent.

	David

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

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Rasmus Villemoes]

On Sat, Dec 07 2024, Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 7 Dec 2024 at 04:24, Vincent Mailhol <vincent.mailhol@gmail.com> wrote:
>>
>> > No good - expands everything twice.
>>
>> And? __is_const_zero() does not evaluate its arguments, so no side effect:
>
> No, the problem is literally the expansion.
>
> Double expansion of these fundamental helpers gets exponential,
> because they are used in various nested ways in other fundamental
> helpers.
>
> That's why we then spent so much effort on trying to clean up the
> min/max macros, because a single line of code would expand to
> literally tens of megabytes of horrific expansions.
>
> And the problem with these things is that you can't make them inline
> functions, so they have to be macros, and then you build up other
> macros using them (like that "clamp()" macro), and it really gets
> horrendous and affects the build time.
>
> And yes, it is very sad. Particularly since a compiler would have a
> really easy time with some nice helper builtins.
>
> Of course, often the compiler *does* have helper builtins, but we
> can't use them, because they aren't *quite* the right thing.

One thing I've been thinking about when all this comes up is: What if
the compilers gave us (and the same for _min):

  __builtin_max(T, e1, e2, ...)
  __builtin_max(e1, e2, ...)

with T being a type, e1... expressions, the latter being the former with
T being the result of usual promotion on the types of the expressions,
and the former having these semantics:

(1) If all the expressions are ICE, so is the whole thing.

(2) It's a compile-time error if the values of the expressions are not
    guaranteed to fit in T (that also applies in case (1)), but this
    should not be thrown by the front-end but only after optimizations
    have had a chance.

(3) Obviously: Every expression is evaluated exactly once and the result
    is the maximum of those, of type T.

For (2), I'd expect trivial value-range analysis to allow something like

  int x;

  ...
  if (x < 0)
    bail;
  size_t y = max(x, sizeof(foo));

Of course, specifying exactly which optimizations one can rely on having
been applied is impossible, but it's the same with our current
BUILD_BUG_ON() - many of them would trigger at -O0.

Then we could just have _one_ simple #define max __builtin_max , which
would work at file-scope, automatically have max3 etc. (because I'd
imagine it would not be much harder for the compiler to just provide the
variadic version if it has code to compute the max of two already), and
none of the preprocessor issues would apply.

Dear Santa: Pretty please?

Rasmus

Footnotes:

This is of course very kernel-centric. A compiler developer
doing this would probably have to think about "what if floating point
types are in the mix". I wouldn't mind if that was just disallowed, but
I can see how that might be a bit odd. I don't think it's hard to amend
the rules to that case - rule 2 could probably be used as-is, and (3)
could say "if any expr are NaN, so is the whole thing" (and if one cares
which NaN, just the first among the expressions); inf values don't need
special treatment wrt. min/max.

With my math hat on, I'd want the zero-expressions variant
__builtin_max(int) to evaluate to INT_MIN ('cause that's the neutral
element for the binary max of two ints) and similarly for other types,
but it's probably better to just require at least two expressions.

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Freitag, dem 06.12.2024 um 02:25 +0000 schrieb David Laight:
> From: Vincent Mailhol
> > Sent: 05 December 2024 15:31
> > 
> > -CC: Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
> > +CC: Martin Uecker <muecker@gwdg.de>
> > (seems that Martin changed his address)

My current one is this: uecker@tugraz.at

Martin

> > 
> > On Thu. 5 Dec. 2024 at 03:39, David Laight <David.Laight@aculab.com> wrote:
> > > > Sent: 02 December 2024 17:33
> > > > 
> > > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > 
> > > > __is_constexpr(), while being one of the most glorious one liner hack
> > > > ever witnessed by mankind, is overly complex. Following the adoption
> > > > of C11 in the kernel, this macro can be simplified through the use of
> > > > a _Generic() selection.
> > > 
> > > You should give credit to some of the earlier patches that do the same.
> > > I'm sure there were some related ones from Linus - not applied yet.
> > 
> > ACK. Would adding a suggested--by Linus tag solve your concern?
> 
> I actually suspect the first patches to change __is_constexpr() to
> use _Generic were from myself.
> I've found a patch I send in November 2023.
> 
> > 
> > > > First, split the macro in two:
> > > > 
> > > >   - __is_const_zero(x): an helper macro; tells whether x is the
> > > >     integer constant expression 0 or something else.
> > > > 
> > > >   - is_const(x): replacement of __is_constexpr(); tells whether x is a
> > > >     integer constant expression.
> > > > 
> > > > The split serves two purposes: first make it easier to understand;
> > > > second, __is_const_zero() will be reused as a building block for other
> > > > is_const_*() macros that will be introduced later on.
> > > > 
> > > > The core principle of __is_constexpr() to abuse the return type of the
> > > > ternary operator remains, but all the surrounding sizeof() hack
> > > > disappear.
> > > > 
> > > > On a side note, while not relevant to the kernel, __is_constexpr()
> > > > relied on the GNU extension that sizeof(void) is 1. const_expr() does
> > > > not use any GNU extensions, making it ISO C compliant.
> > > > 
> > > > __is_constexpr() is temporarily kept and will be removed once all its
> > > > users get migrated to is_const() (or one of its friend).
> > > > 
> > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > ---
> > > >  include/linux/compiler.h | 41 +++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 41 insertions(+)
> > > > 
> > > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > > > index a2a56a50dd85227a4fdc62236a2710ca37c5ba52..30ce06df4153cfdc0fad9bc7bffab9097f8b0450 100644
> > > > --- a/include/linux/compiler.h
> > > > +++ b/include/linux/compiler.h
> > > > @@ -316,6 +316,47 @@ static inline void *offset_to_ptr(const int *off)
> > > >  #define statically_true(x) (__builtin_constant_p(x) && (x))
> > > >  #define statically_false(x) (__builtin_constant_p(x) && (x) == 0)
> > > > 
> > > > +/*
> > > > + * Whether x is the integer constant expression 0 or something else.
> > > > + *
> > > > + * Details:
> > > > + *   - The C11 standard defines in §6.3.2.3.3
> > > > + *       (void *)<integer constant expression with the value 0>
> > > > + *     as a null pointer constant (c.f. the NULL macro).
> > > > + *   - If x evaluates to the integer constant expression 0,
> > > > + *       (void *)(x)
> > > > + *     is a null pointer constant. Else, it is a void * expression.
> > > > + *   - In a ternary expression:
> > > > + *       condition ? operand1 : operand2
> > > > + *     if one of the two operands is of type void * and the other one
> > > > + *     some other pointer type, the C11 standard defines in §6.5.15.6
> > > > + *     the resulting type as below:
> > > > + *       if one operand is a null pointer constant, the result has the
> > > > + *       type of the other operand; otherwise [...] the result type is
> > > > + *       a pointer to an appropriately qualified version of void.
> > > > + *   - As such, in
> > > > + *       0 ? (void *)(x) : (char *)0
> > > > + *     if x is the integer constant expression 0, operand1 is a null
> > > > + *     pointer constant and the resulting type is that of operand2:
> > > > + *     char *. If x is anything else, the type is void *.
> > > > + *   - The (long) cast silences a compiler warning for when x is not 0.
> > > > + *   - Finally, the _Generic() dispatches the resulting type into a
> > > > + *     Boolean.
> > > 
> > > The comment is absolutely excessive.
> > > I'm sure I managed about 2 lines in one of the patches I did.
> > 
> > I think that Linus made it  clear in:
> > 
> >   https://lore.kernel.org/all/CAHk-=wgfpLdt7SFFGcByTfHdkvv7AEa3MDu_s_W1kfOxQs49pw@mail.gmail.com/
> > 
> > that this deserves a detailed comment.
> 
> And he wrote one in https://lore.kernel.org/all/CAHk-=wiq=GUNWJwWh1CRAYchW73UmOaSkaCovLatfDKeveZctA@mail.gmail.com/
> 
>    /*
>     * 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)
> 
> 
> 
> > 
> > The details block in the current __is_constexpr() is 37 lines long,
> > the details block in __is_const_zero() takes 22 lines. So I would
> > argue that I made things better.
> 
> The old block was too long :-)
> 
> > 
> > Unless more people share your concern, I am planning to keep this comment as-is.
> > 
> > > > + *
> > > > + * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
> > > 
> > > IIRC Martin has agreed in the past that the accreditation can
> > > be removed - especially since it refers to the 'sizeof (void)' trick.
> > 
> > I tried to look for such message:
> > 
> >   https://lore.kernel.org/all/?q=f%3A%22martin+uecker%22+__is_constexpr
> > 
> > but couldn't find it. Do you have the link?
> > 
> > @Martin, do you agree that I remove the accreditation?
> > 
> > > > + */
> > > > +#define __is_const_zero(x) \
> > > > +     _Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)
> > > > +
> > > > +/*
> > > > + * Returns a constant expression while determining if its argument is a
> > > > + * constant expression, most importantly without evaluating the argument.
> > > 
> > > You need to differentiate between a 'constant integer expression'
> > > and a 'compile time constant'.
> > 
> > OK. This one was just copied from the previous __is_constexpr(). I will apply
> > "s/constant expression/constant integer expression/g" in v2.
> > 
> > > > + *
> > > > + * If getting a constant expression is not relevant to you, use the more
> > > > + * powerful __builtin_constant_p() instead.
> > > 
> > > __builtin_constant_p() is not 'more powerful' it is testing for
> > > something different.
> > 
> > I meant to say that __builtin_constant_p() is more powerful at
> > constant folding. But I agree that the comment is not clear.
> > 
> > What about this?
> > 
> >   If getting a constant integer expression is not relevant to you, use
> >   __builtin_constant_p() which not only returns true if the argument
> >   is an integer constant expression, but also if it is a compile time
> >   constant.
> 
> Complete f***ed tense.
> 
> It's not about 'constant folding' and 'powerful' isn't the correct word.
> They are checking for two different things.
> 
> A 'constant integer expression' is defined by the C language, and is
> basically something that is constant when first parsed by the compiler
> (my definition) so it can pretty much only contain constants, sizeof()
> and offsetof().
> 
> __builtin_constant_p() is true if the compiler decides that an expression is
> constant. This can track values through inlined function calls and can
> change from 'unknown' to 'true' late in the compilation.
> 
> 	David
> 
> > 
> > 
> > Yours sincerely,
> > Vincent Mailhol
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol]

On Fri. 6 Dec. 2024 at 15:40, Martin Uecker <muecker@gwdg.de> wrote:
> Am Freitag, dem 06.12.2024 um 02:25 +0000 schrieb David Laight:
> > From: Vincent Mailhol
> > > Sent: 05 December 2024 15:31
> > >
> > > -CC: Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
> > > +CC: Martin Uecker <muecker@gwdg.de>
> > > (seems that Martin changed his address)
>
> My current one is this: uecker@tugraz.at

Ack

(...)

> > > > > + *
> > > > > + * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
> > > >
> > > > IIRC Martin has agreed in the past that the accreditation can
> > > > be removed - especially since it refers to the 'sizeof (void)' trick.
> > >
> > > I tried to look for such message:
> > >
> > >   https://lore.kernel.org/all/?q=f%3A%22martin+uecker%22+__is_constexpr
> > >
> > > but couldn't find it. Do you have the link?
> > >
> > > @Martin, do you agree that I remove the accreditation?

So, do you agree to have the accreditation removed in compiler.h?
Personally, I do not mind. I am also OK to remove you from the
documentation and add you to the CREDITS file if you'd like to.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Freitag, dem 06.12.2024 um 16:26 +0900 schrieb Vincent Mailhol:
> > On Fri. 6 Dec. 2024 at 15:40, Martin Uecker <muecker@gwdg.de> wrote:
> > > > Am Freitag, dem 06.12.2024 um 02:25 +0000 schrieb David Laight:
> > > > > > From: Vincent Mailhol
> > > > > > > > Sent: 05 December 2024 15:31
> > > > > > > > 
> > > > > > > > -CC: Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
> > > > > > > > +CC: Martin Uecker <muecker@gwdg.de>
> > > > > > > > (seems that Martin changed his address)
> > > > 
> > > > My current one is this: uecker@tugraz.at
> > 
> > Ack
> > 
> > (...)
> > 
> > > > > > > > > > > > + *
> > > > > > > > > > > > + * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
> > > > > > > > > > 
> > > > > > > > > > IIRC Martin has agreed in the past that the accreditation can
> > > > > > > > > > be removed - especially since it refers to the 'sizeof (void)' trick.
> > > > > > > > 
> > > > > > > > I tried to look for such message:
> > > > > > > > 
> > > > > > > >   https://lore.kernel.org/all/?q=f%3A%22martin+uecker%22+__is_constexpr
> > > > > > > > 
> > > > > > > > but couldn't find it. Do you have the link?
> > > > > > > > 
> > > > > > > > @Martin, do you agree that I remove the accreditation?
> > 
> > So, do you agree to have the accreditation removed in compiler.h?
> > Personally, I do not mind. I am also OK to remove you from the
> > documentation and add you to the CREDITS file if you'd like to.

Sorry, I somehow didn't read this part. Please do whatever you think is
most appropriate (but please update my email to the new above if it
still appears anywhere).


I find it amazing how much time the Linux kernel community spends
revising code to make it work perfectly.

Still, I am wondering whether some of this time and effort should not
be targeted at C compilers and language work to make these macro
hacks unnecessary?

I already found the original motivation for these macros very questionable.
Removing VLAs at the cost having imprecise worst-case bounds strikes
me as fundamentally misguided - at least if security is the motivation.

So maybe there are other good reasons for this, e.g. bad code
for VLAs or risk of jumping the guard page if the attacker can somehow
influence its size (but for this there is -Wvla-larger-than). But even then,
wouldn't it be a more worthwhile and interesting investment of engineering
resources to improving code generation / warnings at the compiler level?

Also the fortification of strlen and co seems something which could be
much better solved with annotations and proper compiler support. 

Martin

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Martin Uecker
> Sent: 07 December 2024 08:40
...
> I find it amazing how much time the Linux kernel community spends
> revising code to make it work perfectly.
> 
> Still, I am wondering whether some of this time and effort should not
> be targeted at C compilers and language work to make these macro
> hacks unnecessary?

I'm probably not alone in thinking that sometimes the compiler writers
are doing their hardest to make life hard for people writing low level code.

> I already found the original motivation for these macros very questionable.
> Removing VLAs at the cost having imprecise worst-case bounds strikes
> me as fundamentally misguided - at least if security is the motivation.

VLA basically cannot be allowed because of the very limited stack space.
Even the per-frame limits aren't a real solution - they just catch the
places that most likely to cause issues. Very deep call chains and any
recursion (that isn't tightly bounded) can cause grief.

> So maybe there are other good reasons for this, e.g. bad code
> for VLAs or risk of jumping the guard page if the attacker can somehow
> influence its size (but for this there is -Wvla-larger-than). But even then,
> wouldn't it be a more worthwhile and interesting investment of engineering
> resources to improving code generation / warnings at the compiler level?

This is kernel code, any access into a stack guard page is basically
unrecoverable for the entire system - a kernel lock/mutex could be held.

With a list of (calling_fn, called_fn, stack_offset) it is possible
calculate an accurate maximum stack usage.
Indirect calls would need to use the (IIRC) FINE_IBT hashes to identify
the possible functions (and I'm not sure than has an attribute for a 'seed'
so that 'int (*)(void *)' functions can be separated into groups.
I've not looked at whether objtool could generate the output - but is has
to be easier for the compiler to do it.

I have done that calculation in the past (parsing a compiler listing file)
and basically discovered the system didn't actually have enough memory
to allocate 'safe' stacks! The max stack was pretty much always (the
equivalent of) printf() inside an error path that never happens.
It might be interesting to see how bad linux is (after sorting out
how to handle recursive calls - hopefully there won't be too many
unexpected ones.

> Also the fortification of strlen and co seems something which could be
> much better solved with annotations and proper compiler support.

That might be nice, but kernel have to be buildable with relatively
old compilers.

Some things might need language/ABI changes to better handle ptr+size.
The ability to return such a pair in registers would probably be useful
(without doing horrid games with a union and __int128).

	David

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

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Samstag, dem 07.12.2024 um 10:33 +0000 schrieb David Laight:
> From: Martin Uecker
> > Sent: 07 December 2024 08:40
> ...
> > I find it amazing how much time the Linux kernel community spends
> > revising code to make it work perfectly.
> > 
> > Still, I am wondering whether some of this time and effort should not
> > be targeted at C compilers and language work to make these macro
> > hacks unnecessary?
> 
> I'm probably not alone in thinking that sometimes the compiler writers
> are doing their hardest to make life hard for people writing low level code.

GCC and Clang are open-source projects just like the kernel. One can 
go there and contribute.  I am not saying that it is always easy to
find consensus and there also projects that have other requirements
than the kernel. But I started to contribute to GCC (with very limited
time) to address some of my own issues, and I found the community very
welcoming.

> 
> > I already found the original motivation for these macros very questionable.
> > Removing VLAs at the cost having imprecise worst-case bounds strikes
> > me as fundamentally misguided - at least if security is the motivation.
> 
> VLA basically cannot be allowed because of the very limited stack space.
> Even the per-frame limits aren't a real solution - they just catch the
> places that most likely to cause issues. Very deep call chains and any
> recursion (that isn't tightly bounded) can cause grief.

VLA use *less* stack than a fixed size arrays with fixed bound.
> 
> > So maybe there are other good reasons for this, e.g. bad code
> > for VLAs or risk of jumping the guard page if the attacker can somehow
> > influence its size (but for this there is -Wvla-larger-than). But even then,
> > wouldn't it be a more worthwhile and interesting investment of engineering
> > resources to improving code generation / warnings at the compiler level?
> 
> This is kernel code, any access into a stack guard page is basically
> unrecoverable for the entire system - a kernel lock/mutex could be held.
>
> With a list of (calling_fn, called_fn, stack_offset) it is possible
> calculate an accurate maximum stack usage.
> Indirect calls would need to use the (IIRC) FINE_IBT hashes to identify
> the possible functions (and I'm not sure than has an attribute for a 'seed'
> so that 'int (*)(void *)' functions can be separated into groups.
> I've not looked at whether objtool could generate the output - but is has
> to be easier for the compiler to do it.
> 
> I have done that calculation in the past (parsing a compiler listing file)
> and basically discovered the system didn't actually have enough memory
> to allocate 'safe' stacks! The max stack was pretty much always (the
> equivalent of) printf() inside an error path that never happens.
> It might be interesting to see how bad linux is (after sorting out
> how to handle recursive calls - hopefully there won't be too many
> unexpected ones.

Compiler and ISO C language support to guarantee bounded stack usage
would indeed be a very interesting feature.

> 
> > Also the fortification of strlen and co seems something which could be
> > much better solved with annotations and proper compiler support.
> 
> That might be nice, but kernel have to be buildable with relatively
> old compilers.

Yes, but it could make use of it at some point in the future (or
optionally).

> Some things might need language/ABI changes to better handle ptr+size.
> The ability to return such a pair in registers would probably be useful
> (without doing horrid games with a union and __int128).

ptr + size is something we are looking into.

You can already do quite a bit by using C99's syntax for variably modified
types. For example, you would get UBSan trap for the following OOB access:

int foo(int n, char (*buf)[n])
{
  (*buf)[n] = 1;
}

This does not require an ABI change. 

Martin

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Linus Torvalds]

On Sat, 7 Dec 2024 at 05:07, Martin Uecker <muecker@gwdg.de> wrote:
>
> VLA use *less* stack than a fixed size arrays with fixed bound.

Not really. You end up with tons of problems, not the least of which
is how to actually analyze the stack size. It also gets *very* nasty
to have code that declares the VLA size using an argument that is then
checked afterwards - and if you have a strong preference for
"declarations before code", you end up with *horrific* issues.

And even if you are super-careful, and you solved the analysis
problem, in practice VLAs will cause huge stack issues simply due to
code generation issues.  The compiler will end up doing extra
alignment and extra frame handling and saving, to the point where any
advantages the VLA would bring is completely dwarfed by all the
disadvantages.

We went through this. We are so *much* better off without VLAs that
it's not even funny.

Now when the compiler says "your stack size is big", you just look
"Oh, that struct should be allocated with kmalloc, not on the stack".
Boom. Done.

            Linus

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Samstag, dem 07.12.2024 um 10:26 -0800 schrieb Linus Torvalds:
> On Sat, 7 Dec 2024 at 05:07, Martin Uecker <muecker@gwdg.de> wrote:
> > 
> > VLA use *less* stack than a fixed size arrays with fixed bound.
> 
> Not really. You end up with tons of problems, not the least of which
> is how to actually analyze the stack size. It also gets *very* nasty
> to have code that declares the VLA size using an argument that is then
> checked afterwards - and if you have a strong preference for
> "declarations before code", you end up with *horrific* issues.
> 
> And even if you are super-careful, and you solved the analysis
> problem, in practice VLAs will cause huge stack issues simply due to
> code generation issues.  The compiler will end up doing extra
> alignment and extra frame handling and saving, to the point where any
> advantages the VLA would bring is completely dwarfed by all the
> disadvantages.

But that all seem solvable issues on the compiler side.  If you 
said the maximum stack size for arrays we tolerate is X,
then a compiler could tell you if 

a) this is not guaranteed in a specific situation (-Wvla-larher-than)
and 
b) transform the array automatically to fixed size array
of size X *or* something smaller when it can show this.

Because now you do the exact same thing manually while losing
precise bounds checking.

Martin

> 
> We went through this. We are so *much* better off without VLAs that
> it's not even funny.
> 
> Now when the compiler says "your stack size is big", you just look
> "Oh, that struct should be allocated with kmalloc, not on the stack".
> Boom. Done.
> 
>             Linus

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Linus Torvalds]

On Sat, 7 Dec 2024 at 11:19, Martin Uecker <muecker@gwdg.de> wrote:
>
> But that all seem solvable issues on the compiler side.

You know, there was a whole *architecture* that was designed and
predicated on "it's all solvable on the compiler side".

That architecture was pure and utter *shit*.

Because no, it's not solvable on the compiler side.

Getting things like value range analysis right on the compiler side is
fundamentally hard.

It's hard to the point where either you only get it for the simple
cases - yes, I can show you a ton of real code where it's trivial - or
you need to have explicit markings in the source code to help the
compiler, and then you end up having to have the compiler (or some
other tool) validate those with asserts or whatever.

And random asserts ARE NOT ACCEPTABLE in the kernel outside of pure
debug builds.

They may be acceptable in most other situations, but in a kernel you
can't just say "we stop now because it turns out the analysis was
broken and the manual notes could be fooled by an attacker".

Now, would we want to have proper value *static* range analysis in the
kernel for other reasons? Oh yes. It would often be very nice to have
the ability to state "this value is trusted and is in this range", and
have it percolate all the way down, both for optimization purposes but
also for various sanity check purposes.

But it's simply not sanely available in the generic case.

> a) this is not guaranteed in a specific situation (-Wvla-larher-than)

We'd either get horrendous numbers of false positives that we then
have to manually add special code for, or

> b) transform the array automatically to fixed size array
> of size X *or* something smaller when it can show this.

we'd just do this by hand *once* and for all, and say "VLA's didn't work out".

So yeah. We did (b) by hand.

We used to have VLA's in the kernel. It was a disaster. We got rid of
them, because the (big) pain wasn't worth the (few) places it was
actually useful.

So we have been VLA-free for the last five years, and it's been good.

Simplify.

             Linus

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Samstag, dem 07.12.2024 um 12:28 -0800 schrieb Linus Torvalds:
> On Sat, 7 Dec 2024 at 11:19, Martin Uecker <muecker@gwdg.de> wrote:
> > 
> > But that all seem solvable issues on the compiler side.
> 
[... Itanium, value range analysis, no assertions in kernel...]


> Now, would we want to have proper value *static* range analysis in the
> kernel for other reasons? Oh yes. It would often be very nice to have
> the ability to state "this value is trusted and is in this range", and
> have it percolate all the way down, both for optimization purposes but
> also for various sanity check purposes.

__bos should give you some access to it, but it seems not as
good as it could be (relative to what the optimizer knows)
and then the information is also not available in the front-end. 

> 
> But it's simply not sanely available in the generic case.

I was not talking about future still-to-be-invented compiler
technology, but about things that work today.

While the compiler can not automatically prove every use
of VLA bounded, it can reliably diagnose the cases where it
can *not* see that it is bounded. Consider this example:

void oob(int n, char p[n]);
void f(unsigned int n)
{
    char buf[MIN(n, 100)]; // bounded
    oob(n + 10, buf); // warning
}

void h(unsigned int n)
{
    char buf[100];  // static with worst case size
    oob(n + 10, buf); // no warning
}

void i(unsigned int n)
{
    char buf[n]; // warning!
    oob(n + 10, buf);
}

void test_f() { f(50); }
void test_h() { h(50); }
void test_i() { i(50); }

https://godbolt.org/z/b15ajTPda

For both 'f' and 'h' stack is bounded and generally smaller in 'f'.
Also any worst-case stack size analysis that applies to 'h' also
applies to 'f' (and could potentially be improved).

In 'f' one gets a warning because 'oob' will try to do an OOB
access.  This is only possibly because the compiler sees the true
size of the array.  (One can also get precise information
about the size with __bdos.)

In 'h' the error can not be detected. The information is lost.
One can get a warning only when the static worst-case size is
exceeded, but otherwise the error remains hidden.

In 'i' one gets the warning about the OOB access and with 
-Wvla-larget-than=100  one gets a warning that the VLA size may
be unbounded.  These case should then not be allowed of course.

Note that this works today in GCC and seems to have better
information than __bos available (not sure why though).

> 
> > a) this is not guaranteed in a specific situation (-Wvla-larher-than)
> 
> We'd either get horrendous numbers of false positives that we then
> have to manually add special code for, or
>

> > b) transform the array automatically to fixed size array
> > of size X *or* something smaller when it can show this.
> 
> we'd just do this by hand *once* and for all, and say "VLA's didn't work out".
> 
> So yeah. We did (b) by hand.
> 
> We used to have VLA's in the kernel. It was a disaster. We got rid of
> them, because the (big) pain wasn't worth the (few) places it was
> actually useful.

Can you point me to some horror stories? 


> So we have been VLA-free for the last five years, and it's been good.
> Simplify.

These macro discussions I get CC-ed on sometimes leave a different 
impression ;-)


Martin

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Linus Torvalds]

On Sat, 7 Dec 2024 at 15:52, Martin Uecker <muecker@gwdg.de> wrote:
>
> Can you point me to some horror stories?

So the main issues tended to be about various static verification tools.

Ranging from things like the stackleak plugin for gcc, where handling
VLA's and alloca() (which are pretty much the same thing with
different syntax) was just very much added complexity, to perhaps
overly simplistic tools that literally just check the stack usage by
parsing "objdump --disassemble" output and then building up
approximate "this is the combined deepest stack" call chain
approximations.

And even in the *basic* infrastructure like gcc itself, VLA's simply
made -Wframe-larger-than= just simply not work.

I also have this memory of bad code generation (again, this is 5=
years ago, so take this with a pinch of salt: dim memories), where gcc
wouldn't end up re-using VLA stack slots, so VLA's made the frame
bigger for that reason or something like that.

We explicitly use "-fconserve-stack" to get gcc to reuse spill slots,
because gcc has been known to sometimes makes insanely piggish stack
frames when it just creates a spill slot for *everything*, even if the
spills aren't live at the same time (think big functions with lots of
case statements).

We also had several cases of the VLA's just being silly, when a simple
constant-sized allocation just worked fine and didn't generate
pointless extra code.

Pretty much none of the code core actually ever wanted VLA's, so the
end result was that we had these bad patterns mainly in random drivers
etc. Don't do that.

                Linus

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Samstag, dem 07.12.2024 um 17:58 -0800 schrieb Linus Torvalds:
> On Sat, 7 Dec 2024 at 15:52, Martin Uecker <muecker@gwdg.de> wrote:
> > 
> > Can you point me to some horror stories?
> 
> So the main issues tended to be about various static verification tools.
> 
> Ranging from things like the stackleak plugin for gcc, where handling
> VLA's and alloca() (which are pretty much the same thing with
> different syntax) was just very much added complexity, to perhaps
> overly simplistic tools that literally just check the stack usage by
> parsing "objdump --disassemble" output and then building up
> approximate "this is the combined deepest stack" call chain
> approximations.
> 
> And even in the *basic* infrastructure like gcc itself, VLA's simply
> made -Wframe-larger-than= just simply not work.
> 
> I also have this memory of bad code generation (again, this is 5=
> years ago, so take this with a pinch of salt: dim memories), where gcc
> wouldn't end up re-using VLA stack slots, so VLA's made the frame
> bigger for that reason or something like that.
> 
> We explicitly use "-fconserve-stack" to get gcc to reuse spill slots,
> because gcc has been known to sometimes makes insanely piggish stack
> frames when it just creates a spill slot for *everything*, even if the
> spills aren't live at the same time (think big functions with lots of
> case statements).
> 
> We also had several cases of the VLA's just being silly, when a simple
> constant-sized allocation just worked fine and didn't generate
> pointless extra code.
> 
> Pretty much none of the code core actually ever wanted VLA's, so the
> end result was that we had these bad patterns mainly in random drivers
> etc. Don't do that.

Thanks. This confirms that the tooling around VLAs is rather poor.


Martin

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Martin Uecker
> Sent: 07 December 2024 23:52
...
> While the compiler can not automatically prove every use
> of VLA bounded, it can reliably diagnose the cases where it
> can *not* see that it is bounded. Consider this example:
> 
> void oob(int n, char p[n]);
> void f(unsigned int n)
> {
>     char buf[MIN(n, 100)]; // bounded
>     oob(n + 10, buf); // warning
> }
...

The kernel stack has to have enough space for the [100]
so the full amount might as well always be allocated.
The chance of 'trading off' stack usage with another function
in the same call stack that is guaranteed to use less than
its maximum is about zero.

The VLA code also adds an extra stack frame, this pretty much
pessimises everything.
This happened for 'constant' sizes from min(16, sizeof (struct))
because min() needs to be a statement function to avoid re-evaluating
its arguments.
(The version of min() that managed to return constant from constant
input just exploded in cpp, partially responsible for 18MB lines
being fed into the compiler part.)

	David

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

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Sonntag, dem 08.12.2024 um 11:26 +0000 schrieb David Laight:
> From: Martin Uecker
> > Sent: 07 December 2024 23:52
> ...
> > While the compiler can not automatically prove every use
> > of VLA bounded, it can reliably diagnose the cases where it
> > can *not* see that it is bounded. Consider this example:
> > 
> > void oob(int n, char p[n]);
> > void f(unsigned int n)
> > {
> >     char buf[MIN(n, 100)]; // bounded
> >     oob(n + 10, buf); // warning
> > }
> ...
> 
> The kernel stack has to have enough space for the [100]
> so the full amount might as well always be allocated.
> The chance of 'trading off' stack usage with another function
> in the same call stack that is guaranteed to use less than
> its maximum is about zero.

In numerical computing this is a big motivation because
you can reduce stack usage in recursive divide-and-conquer
algorithms.  For the kernel, I agree this is not a
compelling use case, and the better motivation would be
precise bounds checking and clearer semantics for buffer
management.  

But don't get me wrong, if the kernel is happier without VLA
this is fine with me, I am just trying to understand the
underlying issues better and the "VLAs are security problem"
or "VLA use more stack"  arguments do not convince me, while
the points Linus raises make much more sense to me.

> 
> The VLA code also adds an extra stack frame, this pretty much
> pessimises everything.

Yes, but this is something which seems could be improved
on the compiler side, e.g. by simply transforming
small VLAs automatically to a fixed size array while
preserving their semantics for bound checking.


> This happened for 'constant' sizes from min(16, sizeof (struct))
> because min() needs to be a statement function to avoid re-evaluating
> its arguments.

Can you clarify this?  If the VLA size is constant, even when
it is not an integer constant expression according to ISO C,
the compiler should not produce worse code.  For example,

void g(void*);

void foo()
{
    int n = 10;
    char buf[n];
    g(buf);
}

void bar()
{
    char buf[10];
    g(buf);
}

So a lot of this macro business seems to be necessary
to avoid creating warnings for ISO VLAs when instead you really
care about the created code not having a dynamic allocation on
the stack.

So one might wonder whether a compiler warning that warns more
specifically about this would help.

> (The version of min() that managed to return constant from constant
> input just exploded in cpp, partially responsible for 18MB lines
> being fed into the compiler part.)

The issue here is that we miss a language feature in C to
introduce local variables that help avoid multiple expansion
of macro arguments.  GCC's statement expressions and __auto_type
are a solution

#define foo(x) ({ __auto_type __x = (x); ... })

but this runs into the current limitations that ({ }) can not be used
at file-scope and can not return constant expressions.


For other reasons I was thinking about adding names to _Generic,
as in

_Generic(x, int i: (i + 1));

because one design issues with _Generic is that it typechecks 
also the untaken associations and there the 'x' then has the wrong
type.  Having an 'i' with the right type which is set to the value
of 'x' when the branch is taken would fix this issue.

But this feature might also allow writing macros that avoid
doublel expansion without requiring statement expressions (which
are more difficult to fix):

#define foo(x) _Generic(x, int i: (i + i));


Martin

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Martin Uecker
> Sent: 08 December 2024 12:38
> 
> Am Sonntag, dem 08.12.2024 um 11:26 +0000 schrieb David Laight:
> > From: Martin Uecker
> > > Sent: 07 December 2024 23:52
> > ...
> > > While the compiler can not automatically prove every use
> > > of VLA bounded, it can reliably diagnose the cases where it
> > > can *not* see that it is bounded. Consider this example:
> > >
> > > void oob(int n, char p[n]);
> > > void f(unsigned int n)
> > > {
> > >     char buf[MIN(n, 100)]; // bounded
> > >     oob(n + 10, buf); // warning
> > > }
> > ...
> >
> > The kernel stack has to have enough space for the [100]
> > so the full amount might as well always be allocated.
> > The chance of 'trading off' stack usage with another function
> > in the same call stack that is guaranteed to use less than
> > its maximum is about zero.
> 
> In numerical computing this is a big motivation because
> you can reduce stack usage in recursive divide-and-conquer
> algorithms.  For the kernel, I agree this is not a
> compelling use case, and the better motivation would be
> precise bounds checking and clearer semantics for buffer
> management.

Except that changing the size of the on-stack array makes
absolutely no difference.
Ideally the kernel stack would be a single 4k page, but too
much code uses on-stack buffers so it has been increased and
might be 16k (or more!).
Remember this is physical memory allocated to every user thread.
On Linux it is not swappable.

...
> > This happened for 'constant' sizes from min(16, sizeof (struct))
> > because min() needs to be a statement function to avoid re-evaluating
> > its arguments.
> 
> Can you clarify this?  If the VLA size is constant, even when
> it is not an integer constant expression according to ISO C,
> the compiler should not produce worse code.  For example,

I just tried to reproduce the failing case - and failed.
It was similar to __builtin_constant_p() initially returning 'don't know'
so the 'variable sized' array code got added, then much later
after further optimisation passes the expression became constant.
So you ended up with a 'fixed size' VLA.

Compile with -Wno-vla (and -Werror) and the compile failed.

...
> So a lot of this macro business seems to be necessary
> to avoid creating warnings for ISO VLAs when instead you really
> care about the created code not having a dynamic allocation on
> the stack.

A lot of the 'macro business' for min/max is avoiding unexpected
conversion of negative values to very large unsigned ones.
And no, -Wsign-compare is spectacularly useless.

..
> The issue here is that we miss a language feature in C to
> introduce local variables that help avoid multiple expansion
> of macro arguments.  GCC's statement expressions and __auto_type
> are a solution

or historically 'typeof(x) _x = x'

> #define foo(x) ({ __auto_type __x = (x); ... })
> 
> but this runs into the current limitations that ({ }) can not be used
> at file-scope and can not return constant expressions.
> 
> 
> For other reasons I was thinking about adding names to _Generic,
> as in
> 
> _Generic(x, int i: (i + 1));
> 
> because one design issues with _Generic is that it typechecks
> also the untaken associations and there the 'x' then has the wrong
> type.  Having an 'i' with the right type which is set to the value
> of 'x' when the branch is taken would fix this issue.

That looks even more syntactically obscure than _Generic itself.
Why does it need to do more than very simple syntax analysis of
the unwanted branches - or they could automatically be analysed
with the named variable have the specified type?

> But this feature might also allow writing macros that avoid
> double expansion without requiring statement expressions (which
> are more difficult to fix):
> 
> #define foo(x) _Generic(x, int i: (i + i));

How can that work for things like min() that have multiple arguments?
Not going to work if you need __auto_type either.

	David

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

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Sonntag, dem 08.12.2024 um 16:48 +0000 schrieb David Laight:
> From: Martin Uecker
> > Sent: 08 December 2024 12:38

...
> ...
> > So a lot of this macro business seems to be necessary
> > to avoid creating warnings for ISO VLAs when instead you really
> > care about the created code not having a dynamic allocation on
> > the stack.
> 
> A lot of the 'macro business' for min/max is avoiding unexpected
> conversion of negative values to very large unsigned ones.
> And no, -Wsign-compare is spectacularly useless.

This is a different topic, but what would be needed here?
> 
> ..
> > The issue here is that we miss a language feature in C to
> > introduce local variables that help avoid multiple expansion
> > of macro arguments.  GCC's statement expressions and __auto_type
> > are a solution
> 
> or historically 'typeof(x) _x = x'
> 
> > #define foo(x) ({ __auto_type __x = (x); ... })
> > 
> > but this runs into the current limitations that ({ }) can not be used
> > at file-scope and can not return constant expressions.
> > 
> > 
> > For other reasons I was thinking about adding names to _Generic,
> > as in
> > 
> > _Generic(x, int i: (i + 1));
> > 
> > because one design issues with _Generic is that it typechecks
> > also the untaken associations and there the 'x' then has the wrong
> > type.  Having an 'i' with the right type which is set to the value
> > of 'x' when the branch is taken would fix this issue.
> 
> That looks even more syntactically obscure than _Generic itself.
> Why does it need to do more than very simple syntax analysis of
> the unwanted branches 

This would be possible and GCC does turn of some warnings in
the unwanted branches.  I added this to GCC 14 I think.

But so far, ISO C requires that all branches are valid and this
was an intentional design decision to detect errors.

> - or they could automatically be analysed
> with the named variable have the specified type?

Inside a macro there is no variable 'x' but
the macro argument 'x' is replaced by some expression.

Also there is the general problem of multiple expansion which
can only be addressed by introducing an identifier.

> 
> > But this feature might also allow writing macros that avoid
> > double expansion without requiring statement expressions (which
> > are more difficult to fix):
> > 
> > #define foo(x) _Generic(x, int i: (i + i));
> 
> How can that work for things like min() that have multiple arguments?

You would need to nest it:

#define foo(x, y) _Generic(x, int i: _Generic(y, int j: i + j))

Otherwise one could invent syntax for matching multiple arguments
at the same time.

There is still the problem of name collision, but this is already
a problem with 

({ int i = (x); int j = (x); i + j; }) 

> Not going to work if you need __auto_type either.

If we allowed an identifier for the default branch too, this
would work:  _Generic(x, default i: (2 * i))


But hey, I am not saying  this is perfect, it is just
a possible improvement I was thinking about and which could be
implemented easily, would automatically return constant expressions,
and could be used at file scope without further changes.

There are certainly better long-term solutions.

Martin

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Linus Torvalds]

On Sun, 8 Dec 2024 at 10:11, Martin Uecker <muecker@gwdg.de> wrote:
> >
> > A lot of the 'macro business' for min/max is avoiding unexpected
> > conversion of negative values to very large unsigned ones.
> > And no, -Wsign-compare is spectacularly useless.
>
> This is a different topic, but what would be needed here?

Dan Carpenter actually wrote up some of the issues in:

   https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/

but the basic issue is that -Wsign-compare has over the years been
truly spectacularly bad.

It has literally started out from the completely nonsensical and
incorrect assumption that the types of a comparison have to match in
signedness, and it shows in the name itself, but it also showed in
early implementations.

The very first versions of gcc that did -Wsign-compare literally
complained about code like

     sizeof(x) < 5

because obviously one side is an unsigned 'size_t', and the other side
is a signed 'int'. So comparing the two is clearly invalid, right?

No.

It's obviously *not* invalid, and any compiler that complains about
different signedness of that compare is just complete useless garbage.
It's literally checking two constants against each other, and the
result doesn't depend on the signedness or the silent C implicit type
conversion.

And no, gcc doesn't complain about that particular code any more.
*That* particular problem was I think only visible in a gcc
pre-release that sadly did actually ship as part of a SUSE release, so
we saw it in the wild even if it was never in an official gcc release.

I'm pointing out the history because it's relevant due to explaining
*why* the whole concept of looking at just the type is so broken, and
how the whole background to the warning was broken from the very
beginning. The very name of the warning is a sign of the problem.

Because gcc still *does* complain about entirely valid code, where
"fixing" the warning just means you have to write worse code.

I think Dan's example from the link above is a good one: if

        for (int i = 0; i < sizeof(x); i++)

causes a warning, the compiler got things entirely wrong.

And yes, modern gcc very much warns about that:

  t.c:4:27: warning: comparison of integer expressions of different
signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
      4 |         for (int i = 0; i < sizeof(b); i++)
        |                           ^

So if you want a general-purpose "Warn about dangerous comparisons",
you need to get away from the mindset that it's about different signs.

A compiler needs to do proper value range analysis before warning
about comparing said values. Not just mindlessly say "different types
bad" like some marsupial that has been dropped on its head a few too
many times.

End result: calling it "Warn about sign compare" is a disease. It
shows a lack of understanding of how complex the warning logic needs
to be.

Now, I'm not claiming that our min/max type warnings are great either:
they *do* end up basically being the same silly "just check signs, but
at least don't complain about signed positive constants being used for
unsigned comparisons".

So our min/max macros most definitely are *not* doing that "value
range analysis" that I claim is required for a *general* comparison
thing.

But our min//max macros aren't some general thing. They are very
specific, and so it's a lot easier to accept the not-great-analysis
for those specific cases where we then may have to change types
explicitly or do some other massaging to avoid the warning.

Put another way: a warning that triggers on really basic C absolutely
*must*not* have silly easily triggerable false positives for good and
idiomatic source code.

Such a warning is worse than useless, and gets disabled.

But a warning that is overly restrictive and gives silly false
positives can still be entirely acceptable when the context of that
warning is very limited.

So this is why in the kernel we disable '-Wsign-compare' in the
general case, but *do* basically manually then implement that very
same logic in the very _specific_ case of the min/max() macros.

What is unacceptable nonsense in one case may be acceptable "good
enough" in another. Life is not fair, I'm afraid.

                Linus

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol]

On Sat. 7 Dec. 2024 at 17:39, Martin Uecker <muecker@gwdg.de> wrote:
> Am Freitag, dem 06.12.2024 um 16:26 +0900 schrieb Vincent Mailhol:
> > > On Fri. 6 Dec. 2024 at 15:40, Martin Uecker <muecker@gwdg.de> wrote:
> > > > > Am Freitag, dem 06.12.2024 um 02:25 +0000 schrieb David Laight:
> > > > > > > From: Vincent Mailhol
> > > > > > > > > Sent: 05 December 2024 15:31

(...)

> > > > > > > > > @Martin, do you agree that I remove the accreditation?
> > >
> > > So, do you agree to have the accreditation removed in compiler.h?
> > > Personally, I do not mind. I am also OK to remove you from the
> > > documentation and add you to the CREDITS file if you'd like to.
>
> Sorry, I somehow didn't read this part. Please do whatever you think is
> most appropriate (but please update my email to the new above if it
> still appears anywhere).

OK. Then I will remove the accreditation from the  compiler.h
documentation and instead add an entry in the CREDITS file in a
separate patch. I think this is the most appropriate way.

> I find it amazing how much time the Linux kernel community spends
> revising code to make it work perfectly.
>
> Still, I am wondering whether some of this time and effort should not
> be targeted at C compilers and language work to make these macro
> hacks unnecessary?

It seems to me that the long term solution to this problem are the
constexpr functions.

But the core issue is that before getting this support in Linux, we
have to wait for this to be added to the C2Y draft, then implemented
in the compilers (probably just reusing the C++ constexpr functions)
and finally wait maybe one more decade for the C2Y support to reach
the kernel. For reference the kernel supports C11 only from 2022… So
maybe we will see those in the kernel around 2037? Meanwhile, we have
to deal with those hacks.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Samstag, dem 07.12.2024 um 21:45 +0900 schrieb Vincent Mailhol:
> On Sat. 7 Dec. 2024 at 17:39, Martin Uecker <muecker@gwdg.de> wrote:
> > Am Freitag, dem 06.12.2024 um 16:26 +0900 schrieb Vincent Mailhol:

...

> > I find it amazing how much time the Linux kernel community spends
> > revising code to make it work perfectly.
> > 
> > Still, I am wondering whether some of this time and effort should not
> > be targeted at C compilers and language work to make these macro
> > hacks unnecessary?
> 
> It seems to me that the long term solution to this problem are the
> constexpr functions.

How would constexpr functions help here?  (I am a bit sceptical about
constexpr functions.)

> 
> But the core issue is that before getting this support in Linux, we
> have to wait for this to be added to the C2Y draft, then implemented
> in the compilers (probably just reusing the C++ constexpr functions)
> and finally wait maybe one more decade for the C2Y support to reach
> the kernel. For reference the kernel supports C11 only from 2022… So
> maybe we will see those in the kernel around 2037? Meanwhile, we have
> to deal with those hacks.

If we do not collaborate on proper solutions, then you might have 
to wait much longer.

Martin


> 
> 
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol]

On Sat. 7 Dec. 2024 à 22:19, Martin Uecker <muecker@gwdg.de> wrote:
> Am Samstag, dem 07.12.2024 um 21:45 +0900 schrieb Vincent Mailhol:
> > On Sat. 7 Dec. 2024 at 17:39, Martin Uecker <muecker@gwdg.de> wrote:
> > > Am Freitag, dem 06.12.2024 um 16:26 +0900 schrieb Vincent Mailhol:
>
> ...
>
> > > I find it amazing how much time the Linux kernel community spends
> > > revising code to make it work perfectly.
> > >
> > > Still, I am wondering whether some of this time and effort should not
> > > be targeted at C compilers and language work to make these macro
> > > hacks unnecessary?
> >
> > It seems to me that the long term solution to this problem are the
> > constexpr functions.
>
> How would constexpr functions help here?  (I am a bit sceptical about
> constexpr functions.)

I was thinking of some of the "side features" of constexpr functions. Namely:

  - std::is_constant_evaluated
  Link: https://en.cppreference.com/w/cpp/types/is_constant_evaluated

  - if consteval
  Link: https://en.cppreference.com/w/cpp/language/if#Consteval_if

I did not try it, but looking at these, I believe that this would
allow us to rewrite most of our macros into some constexpr functions.

> > But the core issue is that before getting this support in Linux, we
> > have to wait for this to be added to the C2Y draft, then implemented
> > in the compilers (probably just reusing the C++ constexpr functions)
> > and finally wait maybe one more decade for the C2Y support to reach
> > the kernel. For reference the kernel supports C11 only from 2022… So
> > maybe we will see those in the kernel around 2037? Meanwhile, we have
> > to deal with those hacks.
>
> If we do not collaborate on proper solutions, then you might have
> to wait much longer.

I was invited to WG14 this September. For now, I am only lurking. The
thing I have in mind right now is to write a paper to allow the use of
static_assert() in expressions (i.e. make it return 0 on success).
That should be a relatively small change, but would bring a nice
quality of life improvement.

For context, look at this:

  https://lore.kernel.org/all/CAHk-=wjLSEcZ5LdW+3C+9rtjvNPHZT6zdk0POj67T5k2ZpDbgA@mail.gmail.com/T/#m1ba33a804b4041154b72a1d0333f90ec7204c461

And I will definitely follow the progress of constexpr functions in C2Y.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Samstag, dem 07.12.2024 um 22:50 +0900 schrieb Vincent Mailhol:
> On Sat. 7 Dec. 2024 à 22:19, Martin Uecker <muecker@gwdg.de> wrote:
> > Am Samstag, dem 07.12.2024 um 21:45 +0900 schrieb Vincent Mailhol:
> > > On Sat. 7 Dec. 2024 at 17:39, Martin Uecker <muecker@gwdg.de> wrote:
> > > > Am Freitag, dem 06.12.2024 um 16:26 +0900 schrieb Vincent Mailhol:

...


> > > But the core issue is that before getting this support in Linux, we
> > > have to wait for this to be added to the C2Y draft, then implemented
> > > in the compilers (probably just reusing the C++ constexpr functions)
> > > and finally wait maybe one more decade for the C2Y support to reach
> > > the kernel. For reference the kernel supports C11 only from 2022… So
> > > maybe we will see those in the kernel around 2037? Meanwhile, we have
> > > to deal with those hacks.
> > 
> > If we do not collaborate on proper solutions, then you might have
> > to wait much longer.
> 
> I was invited to WG14 this September. For now, I am only lurking. The
> thing I have in mind right now is to write a paper to allow the use of
> static_assert() in expressions (i.e. make it return 0 on success).
> That should be a relatively small change, but would bring a nice
> quality of life improvement.

Excellent, then I was complaining to the wrong person. 


Martin

> 
> For context, look at this:
> 
>   https://lore.kernel.org/all/CAHk-=wjLSEcZ5LdW+3C+9rtjvNPHZT6zdk0POj67T5k2ZpDbgA@mail.gmail.com/T/#m1ba33a804b4041154b72a1d0333f90ec7204c461
> 
> And I will definitely follow the progress of constexpr functions in C2Y.
> 
> 
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Martin Uecker]

Am Samstag, dem 07.12.2024 um 22:50 +0900 schrieb Vincent Mailhol:
> On Sat. 7 Dec. 2024 à 22:19, Martin Uecker <muecker@gwdg.de> wrote:
> > 
...

> 
> I was invited to WG14 this September. For now, I am only lurking. The
> thing I have in mind right now is to write a paper to allow the use of
> static_assert() in expressions (i.e. make it return 0 on success).
> That should be a relatively small change, but would bring a nice
> quality of life improvement.
> 
> For context, look at this:
> 
>   https://lore.kernel.org/all/CAHk-=wjLSEcZ5LdW+3C+9rtjvNPHZT6zdk0POj67T5k2ZpDbgA@mail.gmail.com/T/#m1ba33a804b4041154b72a1d0333f90ec7204c461

What one can do is put it into a structure.

#define const_assert(x) \
    (sizeof(struct { _Static_assert(x, ""); }))

but yeah, also a hack to work around a limitation of the standard
feature.

Martin

Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[Vincent Mailhol]

On Sun. 8 Dec. 2024 at 00:10, Martin Uecker <muecker@gwdg.de> wrote:
> Am Samstag, dem 07.12.2024 um 22:50 +0900 schrieb Vincent Mailhol:
> > On Sat. 7 Dec. 2024 à 22:19, Martin Uecker <muecker@gwdg.de> wrote:
> > >
> ...
>
> >
> > I was invited to WG14 this September. For now, I am only lurking. The
> > thing I have in mind right now is to write a paper to allow the use of
> > static_assert() in expressions (i.e. make it return 0 on success).
> > That should be a relatively small change, but would bring a nice
> > quality of life improvement.
> >
> > For context, look at this:
> >
> >   https://lore.kernel.org/all/CAHk-=wjLSEcZ5LdW+3C+9rtjvNPHZT6zdk0POj67T5k2ZpDbgA@mail.gmail.com/T/#m1ba33a804b4041154b72a1d0333f90ec7204c461
>
> What one can do is put it into a structure.
>
> #define const_assert(x) \
>     (sizeof(struct { _Static_assert(x, ""); }))
>
> but yeah, also a hack to work around a limitation of the standard
> feature.

If you scroll down a couple more messages, you can see that Linus came
up with that exact same hack :-)

It is now upstreamed in:

  https://git.kernel.org/torvalds/c/d7a516c6eeae

And yes, this solves the problem for the kernel, but I would still
like to change the standard to solve it for everyone else.


Yours sincerely,
Vincent Mailhol

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: Vincent Mailhol
> Sent: 07 December 2024 13:51
...
> > > It seems to me that the long term solution to this problem are the
> > > constexpr functions.
> >
> > How would constexpr functions help here?  (I am a bit sceptical about
> > constexpr functions.)
> 
> I was thinking of some of the "side features" of constexpr functions. Namely:
> 
>   - std::is_constant_evaluated
>   Link: https://en.cppreference.com/w/cpp/types/is_constant_evaluated
> 
>   - if consteval
>   Link: https://en.cppreference.com/w/cpp/language/if#Consteval_if
> 
> I did not try it, but looking at these, I believe that this would
> allow us to rewrite most of our macros into some constexpr functions.

IIRC (and trying to understand the definitions) they are backwards
from what the kernel tests are trying to achieve.
The kernel wans to add additional compile-time tests where possible.
This is often restricted to checking values that are compile time
constants (for some definition of constant).
The C++ 'constexpr' is about determining the context in which a
function is called.

Remember in C 'static int x = expr;' requires that 'expr' is a constant
so that the asm can contain 'x: .word value', but C++ is perfectly willing
to add an entry to the 'global constructors' and call a function for you.
This is not useful default behaviour.
The 'constexpr' stuff seems to be about detecting some of these cases
so the function can return a different value - and then possibly be
optimised away.
The kernel is trying to get some test coverage at compile time
without affecting run-time.

The compile-time checks all get more complicated because things like
initialisers have to be 'integer constant expressions' rather than the
more relaxed 'compile time constant' (array bounds probably do need
to be the former).
This is (probably) what stops ({ expr; }) being used for initialisers
even when the value is a compile time constant.
Relaxing that rule would be useful.

Then there is the brain-dead definition of _Static_assert() that makes
it pretty near completely useless (I can't remember Linus's words) for
anything non-trivial.
To be useful it would need to be deferred until after the optimiser
has done all its processing an only trigger if the call hasn't been
optimised away and the condition is either non-constant or false.
clang's 'infinite wisdom' decided to unconditionally output the cpp
output of the expression in the error message (even when there was
a caller provided message). When min() was using it that could be
a few hundred bytes of impenetrable text in a normal call - never
mind the nested ones that hit 10+MB because of a requirement to return
'constant integer expressions'.

It would also be useful to have a 'warning' form (or even an 'info'
that isn't an error even with -Werror).

Then you get _Generic().
WTF do the unselected cases not only have to be valid C but also get
checked for some warnings (like -Wsign-compare and 'unsigned >= 0').

...
> I was invited to WG14 this September. For now, I am only lurking. The
> thing I have in mind right now is to write a paper to allow the use of
> static_assert() in expressions (i.e. make it return 0 on success).
> That should be a relatively small change, but would bring a nice
> quality of life improvement.

Adding gcc's ({ expr; }) to the standard and allowing its output to
be as 'constant' as anything in 'expr' would solve a lot of issues.

You need to be able to have:
	if (x)
		static_assert(y)
for static_assert() to be usable as the main method of reporting
these sort on messages.
The best one in the kernel (ab)uses the message for calling a deprecated
function.

There are other things that get annoying.
I understand why offsetof() needs to be a 'compile time constant',
but that should only be for constant input.
There is no reason why offsetof(x, y[expression]) should be invalid
C for a non-constant expression.
(Although I've had issues even with a constant expression with a
certain compiler I'm forced to use sometimes.)

	David

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

RE: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()

[David Laight]

From: David Laight
> Sent: 06 December 2024 02:26

(now it is no longer 2am...)

Linus suggested this a while back:

>  in https://lore.kernel.org/all/CAHk-=wiq=GUNWJwWh1CRAYchW73UmOaSkaCovLatfDKeveZctA@mail.gmail.com/
> 
>    /*
>     * 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)

But some of the things built on it has issues with compiler warnings.
I think there have several variations before and since with subtle differences.
Probably const_zero() const_true() const_false() and const_expr().

While the 'base' define is really const_zero() with just (long)(x) that
will mask high bits off 'long long'. 
A const_false() could 'fix' that using (long)!!(x) but even !(x) starts
giving compile errors.

If called for pointers (long)((x) == 0) is also problematic.
(Perhaps this is less likely now that min/max don't use it.)

So we may end up with (long)((x) ? 0 : 1) which really might as
well be written ((x) ? 0L : 1L).

The use is likely to be const_true(x > y) so perhaps there isn't a
reason to have const_false() since the boolean operator can be inverted.

const_expr() has relied on '* 0' to make all constant expressions zero.
But it has to handle pointers - so needs a conditional.
Since it is only one line, maybe just replicate the whole thing as:

#define const_true(x) _Generic(0 ? (void *)((x) ? 0L : 1L) : (char *)0, char *: 1, void *: 0)
#define const_expr(x) _Generic(0 ? (void *)((x) ? 0L : 0L) : (char *)0, char *: 1, void *: 0)

Or maybe (mostly because the lines are shorter):
#define const_NULL(x) _Generic(0 ? (x) : (char *)0, char *: 1, void *: 0)
#define const_true(x) const_NULL((void *)(x) ? 0L : 1L))
#define const_expr(x) const_NULL((void *)(x) ? 0L : 0L))

Or even:
#define const_true(x) const_NULL((x) ? NULL : (void *)1L))
#define const_expr(x) const_NULL((x) ? NULL : NULL))

	David

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

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

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

__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 <mailhol.vincent@wanadoo.fr>
---
 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
+ *
+ * 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.

-- 
2.45.2

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

[David Laight]

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 <mailhol.vincent@wanadoo.fr>
> ---
>  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)

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

[Vincent Mailhol]

On Thu. 5 Dec 2024 at 03:48, David Laight <David.Laight@aculab.com> 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 <mailhol.vincent@wanadoo.fr>
> > ---
> >  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 04/10] linux/bits.h: simplify GENMASK_INPUT_CHECK() by using is_const_true()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

In GENMASK_INPUT_CHECK(),

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

is equivalent to:

  is_const_true((l) > (h))

Apply is_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 60044b6088172b3f26aa3f17cdaede9786863dae..ef0119e6179e1ca95345a3d4d3327ba19633028e 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(is_const_true((l) > (h)))
 #else
 /*
  * BUILD_BUG_ON_ZERO is not available in h files included from asm files,

-- 
2.45.2

RE: [PATCH 04/10] linux/bits.h: simplify GENMASK_INPUT_CHECK() by using is_const_true()

[David Laight]

From: Vincent Mailhol
> Sent: 02 December 2024 17:33
> 
>   __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0)
> 
> is equivalent to:
> 
>   is_const_true((l) > (h))

Change it to BUILD_BUG_ON_MSG(statically_true((l) < (h)), "error message")

and then fix all the fallout :-)

	David

> 
> Apply is_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 60044b6088172b3f26aa3f17cdaede9786863dae..ef0119e6179e1ca95345a3d4d3327ba19633028e 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(is_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 04/10] linux/bits.h: simplify GENMASK_INPUT_CHECK() by using is_const_true()

[Vincent Mailhol]

On Thu. 5 Dec 2024 at 03:52, David Laight <David.Laight@aculab.com> wrote:
> From: Vincent Mailhol
> > Sent: 02 December 2024 17:33
> >
> >   __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0)
> >
> > is equivalent to:
> >
> >   is_const_true((l) > (h))
>
> Change it to BUILD_BUG_ON_MSG(statically_true((l) < (h)), "error message")
>
> and then fix all the fallout :-)

BUILD_BUG_ON_MSG() is not suitable for GENMASK_INPUT_CHECK. We need
one of the variants that returns zero here.

But I agree that it is good to have a more meaningful error message
here. I will use __BUILD_BUG_ON_ZERO_MSG() in v2.


Yours sincerely,
Vincent Mailhol

[PATCH 05/10] minmax: simplify __clamp_once() by using is_const_false()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

In __clamp_once(),

  __builtin_choose_expr(__is_constexpr((lo) > (hi)), (lo) <= (hi), true)

is equivalent to:

  !is_const_false((lo) <= (hi))

Apply is_const_false() to simplify __clamp_once().

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/minmax.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 98008dd92153db10c672155bca93201ffabee994..431bf76ac460a11a2e4af23acd90c0d26e99c862 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -111,8 +111,7 @@
 	__auto_type uval = (val);						\
 	__auto_type ulo = (lo);							\
 	__auto_type uhi = (hi);							\
-	static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), 	\
-			(lo) <= (hi), true),					\
+	static_assert(!is_const_false((lo) <= (hi)),				\
 		"clamp() low limit " #lo " greater than high limit " #hi);	\
 	BUILD_BUG_ON_MSG(!__types_ok3(val,lo,hi,uval,ulo,uhi),			\
 		"clamp("#val", "#lo", "#hi") signedness error");		\

-- 
2.45.2

RE: [PATCH 05/10] minmax: simplify __clamp_once() by using is_const_false()

[David Laight]

From: Vincent Mailhol
> Sent: 02 December 2024 17:33
> 
> In __clamp_once(),
> 
>   __builtin_choose_expr(__is_constexpr((lo) > (hi)), (lo) <= (hi), true)
> 
> is equivalent to:
> 
>   !is_const_false((lo) <= (hi))
> 
> Apply is_const_false() to simplify __clamp_once().

There is already a patch 'for next' that changes it use BUILD_BUG_ON_MSG()
and statically_true().

It has found some 'interesting' code.

	David

> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  include/linux/minmax.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index 98008dd92153db10c672155bca93201ffabee994..431bf76ac460a11a2e4af23acd90c0d26e99c862 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -111,8 +111,7 @@
>  	__auto_type uval = (val);						\
>  	__auto_type ulo = (lo);							\
>  	__auto_type uhi = (hi);							\
> -	static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), 	\
> -			(lo) <= (hi), true),					\
> +	static_assert(!is_const_false((lo) <= (hi)),				\
>  		"clamp() low limit " #lo " greater than high limit " #hi);	\
>  	BUILD_BUG_ON_MSG(!__types_ok3(val,lo,hi,uval,ulo,uhi),			\
>  		"clamp("#val", "#lo", "#hi") signedness error");		\
> 
> --
> 2.45.2
> 

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

Re: [PATCH 05/10] minmax: simplify __clamp_once() by using is_const_false()

[Vincent Mailhol]

On Thu. 5 Dec. 2024 at 03:54, David Laight <David.Laight@aculab.com> wrote:
> From: Vincent Mailhol
> > Sent: 02 December 2024 17:33
> >
> > In __clamp_once(),
> >
> >   __builtin_choose_expr(__is_constexpr((lo) > (hi)), (lo) <= (hi), true)
> >
> > is equivalent to:
> >
> >   !is_const_false((lo) <= (hi))
> >
> > Apply is_const_false() to simplify __clamp_once().
>
> There is already a patch 'for next' that changes it use BUILD_BUG_ON_MSG()
> and statically_true().

Found it!

  https://lore.kernel.org/all/34d53778977747f19cce2abb287bb3e6@AcuMS.aculab.com/

I think the easiest would be for me to cherry pick this patch. So that
regardless which series is merged first, no conflict will occur, the
patch will just be skipped the second time it is encountered.

Does this work for you?


Yours sincerely,
Vincent Mailhol

Re: [PATCH 05/10] minmax: simplify __clamp_once() by using is_const_false()

[Vincent Mailhol]

On Fri. 6 Dec. 2024 at 00:52, Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
> On Thu. 5 Dec. 2024 at 03:54, David Laight <David.Laight@aculab.com> wrote:
> > From: Vincent Mailhol
> > > Sent: 02 December 2024 17:33
> > >
> > > In __clamp_once(),
> > >
> > >   __builtin_choose_expr(__is_constexpr((lo) > (hi)), (lo) <= (hi), true)
> > >
> > > is equivalent to:
> > >
> > >   !is_const_false((lo) <= (hi))
> > >
> > > Apply is_const_false() to simplify __clamp_once().
> >
> > There is already a patch 'for next' that changes it use BUILD_BUG_ON_MSG()
> > and statically_true().
>
> Found it!
>
>   https://lore.kernel.org/all/34d53778977747f19cce2abb287bb3e6@AcuMS.aculab.com/

I picked up your patch and got two build errors on an allyesconfig.

You already sent a patch for the first one:

  https://lore.kernel.org/all/33893212b1cc4a418cec09aeeed0a9fc@AcuMS.aculab.com/

For the second one, I submitted a patch here:

  https://lore.kernel.org/all/20241209-nfs4state_fix-v1-1-7a66819c60f0@wanadoo.fr/

I will wait for those two to appear in Andrew's mm tree first, and
only then, I will send a v2 (that will be rebased on the mm tree to
get your change).

Meanwhile, I think this series will be on hiatus.


Yours sincerely,
Vincent Mailhol

[PATCH 06/10] fortify: replace __is_constexpr() by is_const() in strlen()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

is_const() is a one to one replacement of __is_constexpr(). Do the
replacement so that __is_constexpr() can be removed.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/fortify-string.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 0d99bf11d260a3482bbe46e35c7553c0ccfb8b94..e3f2f772c5439ef71eb4a904b4ce27956bc69743 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -254,8 +254,8 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
  * Returns number of characters in @p (NOT including the final NUL).
  *
  */
-#define strlen(p)							\
-	__builtin_choose_expr(__is_constexpr(__builtin_strlen(p)),	\
+#define strlen(p)						\
+	__builtin_choose_expr(is_const(__builtin_strlen(p)),	\
 		__builtin_strlen(p), __fortify_strlen(p))
 __FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1)
 __kernel_size_t __fortify_strlen(const char * const POS p)

-- 
2.45.2

RE: [PATCH 06/10] fortify: replace __is_constexpr() by is_const() in strlen()

[David Laight]

From: Vincent Mailhol
> Sent: 02 December 2024 17:33
> 
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> is_const() is a one to one replacement of __is_constexpr(). Do the
> replacement so that __is_constexpr() can be removed.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  include/linux/fortify-string.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index 0d99bf11d260a3482bbe46e35c7553c0ccfb8b94..e3f2f772c5439ef71eb4a904b4ce27956bc69743 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -254,8 +254,8 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
>   * Returns number of characters in @p (NOT including the final NUL).
>   *
>   */
> -#define strlen(p)							\
> -	__builtin_choose_expr(__is_constexpr(__builtin_strlen(p)),	\
> +#define strlen(p)						\
> +	__builtin_choose_expr(is_const(__builtin_strlen(p)),	\
>  		__builtin_strlen(p), __fortify_strlen(p))

I'm sure Linus suggested a way of doing that without replicating
the __builtin_strlen().

Indeed it may be valid to do:
	len = __builtin_strlen(p);
	__builtin_constant_p(len) ? len : __fortify_strlen(p);

	David

>  __FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1)
>  __kernel_size_t __fortify_strlen(const char * const POS p)
> 
> --
> 2.45.2
> 
> 

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

Re: [PATCH 06/10] fortify: replace __is_constexpr() by is_const() in strlen()

[Vincent Mailhol]

On Thu. 5 Dec. 2024 at 03:58, David Laight <David.Laight@aculab.com> wrote:
> From: Vincent Mailhol
> > Sent: 02 December 2024 17:33
> >
> > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >
> > is_const() is a one to one replacement of __is_constexpr(). Do the
> > replacement so that __is_constexpr() can be removed.
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> >  include/linux/fortify-string.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> > index 0d99bf11d260a3482bbe46e35c7553c0ccfb8b94..e3f2f772c5439ef71eb4a904b4ce27956bc69743 100644
> > --- a/include/linux/fortify-string.h
> > +++ b/include/linux/fortify-string.h
> > @@ -254,8 +254,8 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
> >   * Returns number of characters in @p (NOT including the final NUL).
> >   *
> >   */
> > -#define strlen(p)                                                    \
> > -     __builtin_choose_expr(__is_constexpr(__builtin_strlen(p)),      \
> > +#define strlen(p)                                            \
> > +     __builtin_choose_expr(is_const(__builtin_strlen(p)),    \
> >               __builtin_strlen(p), __fortify_strlen(p))
>
> I'm sure Linus suggested a way of doing that without replicating
> the __builtin_strlen().
>
> Indeed it may be valid to do:
>         len = __builtin_strlen(p);
>         __builtin_constant_p(len) ? len : __fortify_strlen(p);

Then, wouldn't it be better for strlen() to be an inline function
instead of a macro?

  __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
  {
          __kernel_size_t ret = __builtin_strlen(p);

          if (__builtin_constant_p(ret))
                  return ret;
          return __fortify_strlen(p);
  }

I tested it and it worked on an allyesconfig. So if I receive no
objections, strlen() will become an inline function in v2.


Yours sincerely,
Vincent Mailhol

[PATCH 07/10] overflow: replace __is_constexpr() by is_const()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

is_const() is a one to one replacement of __is_constexpr(). Do the
replacement so that __is_constexpr() can be removed.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/overflow.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 0c7e3dcfe8670cb95f371a6f7d41bb8b63b1786c..d69361a3128897d493b5e2ec471f077ca2b9861d 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -234,7 +234,7 @@ static inline bool __must_check __must_check_overflow(bool overflow)
  * Returns: true if overflow can occur, false otherwise.
  */
 #define overflows_type(n, T)					\
-	__builtin_choose_expr(__is_constexpr(n),		\
+	__builtin_choose_expr(is_const(n),			\
 			      __overflows_type_constexpr(n, T),	\
 			      __overflows_type(n, T))
 
@@ -250,7 +250,7 @@ static inline bool __must_check __must_check_overflow(bool overflow)
  * back to __same_type().
  */
 #define castable_to_type(n, T)						\
-	__builtin_choose_expr(__is_constexpr(n),			\
+	__builtin_choose_expr(is_const(n),				\
 			      !__overflows_type_constexpr(n, T),	\
 			      __same_type(n, T))
 
@@ -352,7 +352,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
  * Return: number of bytes needed or SIZE_MAX on overflow.
  */
 #define flex_array_size(p, member, count)				\
-	__builtin_choose_expr(__is_constexpr(count),			\
+	__builtin_choose_expr(is_const(count),				\
 		(count) * sizeof(*(p)->member) + __must_be_array((p)->member),	\
 		size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
 
@@ -368,7 +368,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
  * Return: number of bytes needed or SIZE_MAX on overflow.
  */
 #define struct_size(p, member, count)					\
-	__builtin_choose_expr(__is_constexpr(count),			\
+	__builtin_choose_expr(is_const(count),				\
 		sizeof(*(p)) + flex_array_size(p, member, count),	\
 		size_add(sizeof(*(p)), flex_array_size(p, member, count)))
 

-- 
2.45.2

[PATCH 08/10] drm/i915/reg: replace __is_const_expr() by is_const_true() or is_const()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Most of the use of __is_const_expr() in i915_reg_defs.h are just to
test whether an expression is known to be true. Because those checks
are all done in a BUILD_BUG_ON_ZERO(), replace those with
is_const_true().

Replace the few other occurrences of __is_const_expr() with is_const().

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/gpu/drm/i915/i915_reg_defs.h | 47 +++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
index e251bcc0c89f5710125bc70f07851b2cb978c89c..6ed2fb9cf506a3bd6467ba30f9d0e863d62762f3 100644
--- a/drivers/gpu/drm/i915/i915_reg_defs.h
+++ b/drivers/gpu/drm/i915/i915_reg_defs.h
@@ -19,8 +19,7 @@
  */
 #define REG_BIT(__n)							\
 	((u32)(BIT(__n) +						\
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&		\
-				 ((__n) < 0 || (__n) > 31))))
+	       BUILD_BUG_ON_ZERO(is_const_true((__n) < 0 || (__n) > 31))))
 
 /**
  * REG_BIT8() - Prepare a u8 bit value
@@ -32,8 +31,7 @@
  */
 #define REG_BIT8(__n)                                                   \
 	((u8)(BIT(__n) +                                                \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&         \
-				 ((__n) < 0 || (__n) > 7))))
+	      BUILD_BUG_ON_ZERO(is_const_true((__n) < 0 || (__n) > 7))))
 
 /**
  * REG_GENMASK() - Prepare a continuous u32 bitmask
@@ -46,9 +44,9 @@
  */
 #define REG_GENMASK(__high, __low)					\
 	((u32)(GENMASK(__high, __low) +					\
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&	\
-				 __is_constexpr(__low) &&		\
-				 ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
+	       BUILD_BUG_ON_ZERO(is_const_true((__low) < 0 ||		\
+					       (__high) > 31 ||		\
+					       (__low) > (__high)))))
 
 /**
  * REG_GENMASK64() - Prepare a continuous u64 bitmask
@@ -61,9 +59,9 @@
  */
 #define REG_GENMASK64(__high, __low)					\
 	((u64)(GENMASK_ULL(__high, __low) +				\
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&		\
-				 __is_constexpr(__low) &&		\
-				 ((__low) < 0 || (__high) > 63 || (__low) > (__high)))))
+	       BUILD_BUG_ON_ZERO(is_const_true((__low) < 0 ||		\
+					       (__high) > 63 ||		\
+					       (__low) > (__high)))))
 
 /**
  * REG_GENMASK8() - Prepare a continuous u8 bitmask
@@ -76,9 +74,9 @@
  */
 #define REG_GENMASK8(__high, __low)                                     \
 	((u8)(GENMASK(__high, __low) +                                  \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&      \
-				 __is_constexpr(__low) &&               \
-				 ((__low) < 0 || (__high) > 7 || (__low) > (__high)))))
+	      BUILD_BUG_ON_ZERO(is_const_true((__low) < 0 ||            \
+					      (__high) > 7 ||           \
+					      (__low) > (__high)))))
 
 /*
  * Local integer constant expression version of is_power_of_2().
@@ -97,10 +95,10 @@
  */
 #define REG_FIELD_PREP(__mask, __val)						\
 	((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) +	\
-	       BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) +		\
+	       BUILD_BUG_ON_ZERO(!is_const(__mask)) +				\
 	       BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) +		\
 	       BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \
-	       BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))))
+	       BUILD_BUG_ON_ZERO(is_const_true(~((__mask) >> __bf_shf(__mask)) & (__val)))))
 
 /**
  * REG_FIELD_PREP8() - Prepare a u8 bitfield value
@@ -114,10 +112,10 @@
  */
 #define REG_FIELD_PREP8(__mask, __val)                                          \
 	((u8)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) +      \
-	       BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) +             \
+	       BUILD_BUG_ON_ZERO(!is_const(__mask)) +                           \
 	       BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U8_MAX) +          \
 	       BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \
-	       BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))))
+	       BUILD_BUG_ON_ZERO(is_const_true(~((__mask) >> __bf_shf(__mask)) & (__val)))))
 
 /**
  * REG_FIELD_GET() - Extract a u32 bitfield value
@@ -154,8 +152,7 @@
  */
 #define REG_BIT16(__n)                                                   \
 	((u16)(BIT(__n) +                                                \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&         \
-				 ((__n) < 0 || (__n) > 15))))
+	       BUILD_BUG_ON_ZERO(is_const_true((__n) < 0 || (__n) > 15))))
 
 /**
  * REG_GENMASK16() - Prepare a continuous u8 bitmask
@@ -169,9 +166,9 @@
  */
 #define REG_GENMASK16(__high, __low)                                     \
 	((u16)(GENMASK(__high, __low) +                                  \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&      \
-				 __is_constexpr(__low) &&               \
-				 ((__low) < 0 || (__high) > 15 || (__low) > (__high)))))
+	       BUILD_BUG_ON_ZERO(is_const_true((__low) < 0 ||            \
+					       (__high) > 15 ||          \
+					       (__low) > (__high)))))
 
 /**
  * REG_FIELD_PREP16() - Prepare a u16 bitfield value
@@ -186,10 +183,10 @@
  */
 #define REG_FIELD_PREP16(__mask, __val)                                          \
 	((u16)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) +      \
-	       BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) +             \
+	       BUILD_BUG_ON_ZERO(!is_const(__mask)) +                            \
 	       BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U16_MAX) +          \
 	       BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \
-	       BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))))
+	       BUILD_BUG_ON_ZERO(is_const_true(~((__mask) >> __bf_shf(__mask)) & (__val)))))
 
 #define __MASKED_FIELD(mask, value) ((mask) << 16 | (value))
 #define _MASKED_FIELD(mask, value) ({					   \
@@ -237,7 +234,7 @@
  *	...
  */
 #define _PICK_EVEN_2RANGES(__index, __c_index, __a, __b, __c, __d)		\
-	(BUILD_BUG_ON_ZERO(!__is_constexpr(__c_index)) +			\
+	(BUILD_BUG_ON_ZERO(!is_const(__c_index)) +				\
 	 ((__index) < (__c_index) ? _PICK_EVEN(__index, __a, __b) :		\
 				   _PICK_EVEN((__index) - (__c_index), __c, __d)))
 

-- 
2.45.2

RE: [PATCH 08/10] drm/i915/reg: replace __is_const_expr() by is_const_true() or is_const()

[David Laight]

From: Vincent Mailhol
> Sent: 02 December 2024 17:34
> 
> Most of the use of __is_const_expr() in i915_reg_defs.h are just to
> test whether an expression is known to be true. Because those checks
> are all done in a BUILD_BUG_ON_ZERO(), replace those with
> is_const_true().

Another place that could use statically_true() and BUILD_BUG_ON_MSG().

	David

> 
> Replace the few other occurrences of __is_const_expr() with is_const().
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  drivers/gpu/drm/i915/i915_reg_defs.h | 47 +++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
> index e251bcc0c89f5710125bc70f07851b2cb978c89c..6ed2fb9cf506a3bd6467ba30f9d0e863d62762f3 100644
> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
> @@ -19,8 +19,7 @@
>   */
>  #define REG_BIT(__n)							\
>  	((u32)(BIT(__n) +						\
> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&		\
> -				 ((__n) < 0 || (__n) > 31))))
> +	       BUILD_BUG_ON_ZERO(is_const_true((__n) < 0 || (__n) > 31))))
> 
>  /**
>   * REG_BIT8() - Prepare a u8 bit value
> @@ -32,8 +31,7 @@
>   */
>  #define REG_BIT8(__n)                                                   \
>  	((u8)(BIT(__n) +                                                \
> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&         \
> -				 ((__n) < 0 || (__n) > 7))))
> +	      BUILD_BUG_ON_ZERO(is_const_true((__n) < 0 || (__n) > 7))))
> 
>  /**
>   * REG_GENMASK() - Prepare a continuous u32 bitmask
> @@ -46,9 +44,9 @@
>   */
>  #define REG_GENMASK(__high, __low)					\
>  	((u32)(GENMASK(__high, __low) +					\
> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&	\
> -				 __is_constexpr(__low) &&		\
> -				 ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
> +	       BUILD_BUG_ON_ZERO(is_const_true((__low) < 0 ||		\
> +					       (__high) > 31 ||		\
> +					       (__low) > (__high)))))
> 
>  /**
>   * REG_GENMASK64() - Prepare a continuous u64 bitmask
> @@ -61,9 +59,9 @@
>   */
>  #define REG_GENMASK64(__high, __low)					\
>  	((u64)(GENMASK_ULL(__high, __low) +				\
> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&		\
> -				 __is_constexpr(__low) &&		\
> -				 ((__low) < 0 || (__high) > 63 || (__low) > (__high)))))
> +	       BUILD_BUG_ON_ZERO(is_const_true((__low) < 0 ||		\
> +					       (__high) > 63 ||		\
> +					       (__low) > (__high)))))
> 
>  /**
>   * REG_GENMASK8() - Prepare a continuous u8 bitmask
> @@ -76,9 +74,9 @@
>   */
>  #define REG_GENMASK8(__high, __low)                                     \
>  	((u8)(GENMASK(__high, __low) +                                  \
> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&      \
> -				 __is_constexpr(__low) &&               \
> -				 ((__low) < 0 || (__high) > 7 || (__low) > (__high)))))
> +	      BUILD_BUG_ON_ZERO(is_const_true((__low) < 0 ||            \
> +					      (__high) > 7 ||           \
> +					      (__low) > (__high)))))
> 
>  /*
>   * Local integer constant expression version of is_power_of_2().
> @@ -97,10 +95,10 @@
>   */
>  #define REG_FIELD_PREP(__mask, __val)						\
>  	((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) +	\
> -	       BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) +		\
> +	       BUILD_BUG_ON_ZERO(!is_const(__mask)) +				\
>  	       BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) +		\
>  	       BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \
> -	       BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >>
> __bf_shf(__mask)) & (__val)), 0))))
> +	       BUILD_BUG_ON_ZERO(is_const_true(~((__mask) >> __bf_shf(__mask)) & (__val)))))
> 
>  /**
>   * REG_FIELD_PREP8() - Prepare a u8 bitfield value
> @@ -114,10 +112,10 @@
>   */
>  #define REG_FIELD_PREP8(__mask, __val)                                          \
>  	((u8)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) +      \
> -	       BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) +             \
> +	       BUILD_BUG_ON_ZERO(!is_const(__mask)) +                           \
>  	       BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U8_MAX) +          \
>  	       BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \
> -	       BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >>
> __bf_shf(__mask)) & (__val)), 0))))
> +	       BUILD_BUG_ON_ZERO(is_const_true(~((__mask) >> __bf_shf(__mask)) & (__val)))))
> 
>  /**
>   * REG_FIELD_GET() - Extract a u32 bitfield value
> @@ -154,8 +152,7 @@
>   */
>  #define REG_BIT16(__n)                                                   \
>  	((u16)(BIT(__n) +                                                \
> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&         \
> -				 ((__n) < 0 || (__n) > 15))))
> +	       BUILD_BUG_ON_ZERO(is_const_true((__n) < 0 || (__n) > 15))))
> 
>  /**
>   * REG_GENMASK16() - Prepare a continuous u8 bitmask
> @@ -169,9 +166,9 @@
>   */
>  #define REG_GENMASK16(__high, __low)                                     \
>  	((u16)(GENMASK(__high, __low) +                                  \
> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&      \
> -				 __is_constexpr(__low) &&               \
> -				 ((__low) < 0 || (__high) > 15 || (__low) > (__high)))))
> +	       BUILD_BUG_ON_ZERO(is_const_true((__low) < 0 ||            \
> +					       (__high) > 15 ||          \
> +					       (__low) > (__high)))))
> 
>  /**
>   * REG_FIELD_PREP16() - Prepare a u16 bitfield value
> @@ -186,10 +183,10 @@
>   */
>  #define REG_FIELD_PREP16(__mask, __val)                                          \
>  	((u16)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) +      \
> -	       BUILD_BUG_ON_ZERO(!__is_constexpr(__mask)) +             \
> +	       BUILD_BUG_ON_ZERO(!is_const(__mask)) +                            \
>  	       BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U16_MAX) +          \
>  	       BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \
> -	       BUILD_BUG_ON_ZERO(__builtin_choose_expr(__is_constexpr(__val), (~((__mask) >>
> __bf_shf(__mask)) & (__val)), 0))))
> +	       BUILD_BUG_ON_ZERO(is_const_true(~((__mask) >> __bf_shf(__mask)) & (__val)))))
> 
>  #define __MASKED_FIELD(mask, value) ((mask) << 16 | (value))
>  #define _MASKED_FIELD(mask, value) ({					   \
> @@ -237,7 +234,7 @@
>   *	...
>   */
>  #define _PICK_EVEN_2RANGES(__index, __c_index, __a, __b, __c, __d)		\
> -	(BUILD_BUG_ON_ZERO(!__is_constexpr(__c_index)) +			\
> +	(BUILD_BUG_ON_ZERO(!is_const(__c_index)) +				\
>  	 ((__index) < (__c_index) ? _PICK_EVEN(__index, __a, __b) :		\
>  				   _PICK_EVEN((__index) - (__c_index), __c, __d)))
> 
> 
> --
> 2.45.2
> 
> 

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

Re: [PATCH 08/10] drm/i915/reg: replace __is_const_expr() by is_const_true() or is_const()

[Vincent Mailhol]

On Thu. 5 Dec 2024 at 04:00, David Laight <David.Laight@aculab.com> wrote:
> From: Vincent Mailhol
> > Sent: 02 December 2024 17:34
> >
> > Most of the use of __is_const_expr() in i915_reg_defs.h are just to
> > test whether an expression is known to be true. Because those checks
> > are all done in a BUILD_BUG_ON_ZERO(), replace those with
> > is_const_true().
>
> Another place that could use statically_true() and BUILD_BUG_ON_MSG().

Here also, BUILD_BUG_ON_MSG() is not suitable because it does not
return a value.

__BUILD_BUG_ON_ZERO_MSG() could be used; but there is less benefit to
do this at a driver scope. In this i915_reg_defs.h,
BUILD_BUG_ON_ZERO() is used 20 times. Adding an error message each
time will just make things ugly.

If we want more readable error messages here, the solution for me is
just to redefine BUILD_BUG_ON_ZERO() to print a more meaningful error
message by default. But this is not the scope of this series. I sent a
separate patch for this:

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

Concerning statically_true() instead of is_const_true(), let me test,
and if this works, then I will replace these in v2.


Yours sincerely,
Vincent Mailhol

[PATCH 09/10] coresight: etm4x: replace __is_const_expr() by is_const()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

is_const() is a one to one replacement of __is_constexpr(). Do the
replacement so that __is_constexpr() can be removed.

Refer to [1] for an explaination of why __builtin_constant_p() can not
be used as a replacement here.

[1] commit 4d45bc82df66 ("coresight: etm4x: avoid build failure with unrolled loops")
Link: https://git.kernel.org/torvalds/c/4d45bc82df66

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/hwtracing/coresight/coresight-etm4x.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 9e9165f62e81fe5a87d35b4e30bc23f93cb211ec..91cf81ac46f44418f76783c102477d08f9abc70f 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -492,7 +492,7 @@
 	({									\
 		u64 __val;							\
 										\
-		if (__is_constexpr((offset)))					\
+		if (is_const(offset))						\
 			__val = read_etm4x_sysreg_const_offset((offset));	\
 		else								\
 			__val = etm4x_sysreg_read((offset), true, (_64bit));	\

-- 
2.45.2

[PATCH 10/10] compiler.h: remove __is_constexpr()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Now that all the users of __is_constexpr() have been migrated to
is_const() or one of its friends, remove it.

The homage to Martin Uecker's genius hack remains in the documentation
of __is_const_zero().

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/compiler.h | 47 -----------------------------------------------
 1 file changed, 47 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 165aa5b9bc484376087a130a1ac1f3edb50c983d..7ba75044828129cf1f81f458ade695786dbf132a 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -252,53 +252,6 @@ static inline void *offset_to_ptr(const int *off)
 #define __must_be_cstr(p) \
 	__BUILD_BUG_ON_ZERO_MSG(__annotated(p, nonstring), "must be cstr (NUL-terminated)")
 
-/*
- * This returns a constant expression while determining if an argument is
- * a constant expression, most importantly without evaluating the argument.
- * Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
- *
- * Details:
- * - sizeof() return an integer constant expression, and does not evaluate
- *   the value of its operand; it only examines the type of its operand.
- * - The results of comparing two integer constant expressions is also
- *   an integer constant expression.
- * - The first literal "8" isn't important. It could be any literal value.
- * - The second literal "8" is to avoid warnings about unaligned pointers;
- *   this could otherwise just be "1".
- * - (long)(x) is used to avoid warnings about 64-bit types on 32-bit
- *   architectures.
- * - The C Standard defines "null pointer constant", "(void *)0", as
- *   distinct from other void pointers.
- * - If (x) is an integer constant expression, then the "* 0l" resolves
- *   it into an integer constant expression of value 0. Since it is cast to
- *   "void *", this makes the second operand a null pointer constant.
- * - If (x) is not an integer constant expression, then the second operand
- *   resolves to a void pointer (but not a null pointer constant: the value
- *   is not an integer constant 0).
- * - The conditional operator's third operand, "(int *)8", is an object
- *   pointer (to type "int").
- * - The behavior (including the return type) of the conditional operator
- *   ("operand1 ? operand2 : operand3") depends on the kind of expressions
- *   given for the second and third operands. This is the central mechanism
- *   of the macro:
- *   - When one operand is a null pointer constant (i.e. when x is an integer
- *     constant expression) and the other is an object pointer (i.e. our
- *     third operand), the conditional operator returns the type of the
- *     object pointer operand (i.e. "int *"). Here, within the sizeof(), we
- *     would then get:
- *       sizeof(*((int *)(...))  == sizeof(int)  == 4
- *   - When one operand is a void pointer (i.e. when x is not an integer
- *     constant expression) and the other is an object pointer (i.e. our
- *     third operand), the conditional operator returns a "void *" type.
- *     Here, within the sizeof(), we would then get:
- *       sizeof(*((void *)(...)) == sizeof(void) == 1
- * - The equality comparison to "sizeof(int)" therefore depends on (x):
- *     sizeof(int) == sizeof(int)     (x) was a constant expression
- *     sizeof(int) != sizeof(void)    (x) was not a constant expression
- */
-#define __is_constexpr(x) \
-	(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
-
 /*
  * Whether 'type' is a signed type or an unsigned type. Supports scalar types,
  * bool and also pointer types.

-- 
2.45.2

Re: [PATCH 00/10] compiler.h: refactor __is_constexpr() into is_const{,_true,_false}()

[Kees Cook]

On December 3, 2024 3:33:22 AM GMT+10:00, Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> wrote:
>This series is the spiritual successor of [1] which introduced
>const_true(). In [1], following a comment from David Laight, Linus
>came with a suggestion to simplify __is_constexpr() and its derived
>macros using a _Generic() selection. Because of the total change of
>scope, I am starting a new series.
>
>The goal is to introduce a set of three macros:
>
>  - is_const(): a one to one replacement of __is_constexpr() in term
>    of features but written in a less hacky way thanks to _Generic().
>
>  - is_const_true(): tells whether or not the argument is a true
>    integer constant expression.
>
>  - is_const_false(): tells whether or not the argument is a false
>    integer constant expression.

But why make this change? Is something broken? Does it make builds faster?

> 7 files changed, 97 insertions(+), 84 deletions(-)

It makes the code larger too. I don't see what the benefit is, and given how much time has been spent making sure the existing stuff works correctly, I feel like we should have a clear benefit to replacing it all.

-Kees

-- 
Kees Cook

Re: [PATCH 00/10] compiler.h: refactor __is_constexpr() into is_const{,_true,_false}()

[Vincent Mailhol]

On Thu. 5 Dec. 2024 at 08:58, Kees Cook <kees@kernel.org> wrote:
> On December 3, 2024 3:33:22 AM GMT+10:00, Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> wrote:
> >This series is the spiritual successor of [1] which introduced
> >const_true(). In [1], following a comment from David Laight, Linus
> >came with a suggestion to simplify __is_constexpr() and its derived
> >macros using a _Generic() selection. Because of the total change of
> >scope, I am starting a new series.
> >
> >The goal is to introduce a set of three macros:
> >
> >  - is_const(): a one to one replacement of __is_constexpr() in term
> >    of features but written in a less hacky way thanks to _Generic().
> >
> >  - is_const_true(): tells whether or not the argument is a true
> >    integer constant expression.
> >
> >  - is_const_false(): tells whether or not the argument is a false
> >    integer constant expression.
>
> But why make this change? Is something broken? Does it make builds faster?
>
> > 7 files changed, 97 insertions(+), 84 deletions(-)
>
> It makes the code larger too. I don't see what the benefit is, and given how much time has been spent making sure the existing stuff works correctly, I feel like we should have a clear benefit to replacing it all.

It makes the "code" larger because patch 3 ("compiler.h: add
is_const_true() and is_const_false()") adds two new macros with 20
lines of comments to explain the pros and cons. So the added "code" is
only comments. If you ignore the comments, you can see that I am
actually removing a few lines of code.

As for the clear benefit, sorry, but I have nothing more to offer
other than code simplification. The reason why a lot of time was spent
to make __is_constexpr() work correctly is just a testimony of how
complex the thing is. That alone can be a reason to simplify it, now
that new tools (_Generic()) are available.

Of course, modifying __is_constexpr() is not strictly needed to
introduce the new is_const_expr(). My previous series:

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

 did it that way. But I was rightfully pointed out for my macro being
ugly. Maybe I can suggest that you give a look to the above thread and
tell me if you still disagree with David and Linus's comments after
reading it?


Yours sincerely,
Vincent Mailhol