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

Commit Message

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

From: Vincent Mailhol <[e-mail]>

__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 <[e-mail]>
---
 include/linux/compiler.h | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

Comments

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

From: Vincent Mailhol
> Sent: 02 December 2024 17:33
> 
> From: Vincent Mailhol <[e-mail]>
> 
> __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 <[e-mail]>
> ---
>  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 <[e-mail]>

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)

Yury Norov Dec. 4, 2024, 9:20 p.m. UTC | #2

> > 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 <[e-mail]>
> 
> 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)

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

-CC: Martin Uecker <[e-mail]>
+CC: Martin Uecker <[e-mail]>
(seems that Martin changed his address)

On Thu. 5 Dec. 2024 at 03:39, David Laight <[e-mail]> wrote:
> > Sent: 02 December 2024 17:33
> >
> > From: Vincent Mailhol <[e-mail]>
> >
> > __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 <[e-mail]>
> > ---
> >  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-=[e-mail]/

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 <[e-mail]>
>
> 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

David Laight Dec. 6, 2024, 2:25 a.m. UTC | #4

From: Vincent Mailhol
> Sent: 05 December 2024 15:31
> 
> -CC: Martin Uecker <[e-mail]>
> +CC: Martin Uecker <[e-mail]>
> (seems that Martin changed his address)
> 
> On Thu. 5 Dec. 2024 at 03:39, David Laight <[e-mail]> wrote:
> > > Sent: 02 December 2024 17:33
> > >
> > > From: Vincent Mailhol <[e-mail]>
> > >
> > > __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 <[e-mail]>
> > > ---
> > >  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-=[e-mail]/
> 
> that this deserves a detailed comment.

And he wrote one in https://lore.kernel.org/all/CAHk-=wiq=[e-mail]/

   /*
    * 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 <[e-mail]>
> >
> > 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)

Linus Torvalds Dec. 6, 2024, 6:14 a.m. UTC | #5

On Thu, 5 Dec 2024 at 18:26, David Laight <[e-mail]> 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

Martin Uecker Dec. 6, 2024, 6:40 a.m. UTC | #6

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 <[e-mail]>
> > +CC: Martin Uecker <[e-mail]>
> > (seems that Martin changed his address)

My current one is this: [e-mail]

Martin

> > 
> > On Thu. 5 Dec. 2024 at 03:39, David Laight <[e-mail]> wrote:
> > > > Sent: 02 December 2024 17:33
> > > > 
> > > > From: Vincent Mailhol <[e-mail]>
> > > > 
> > > > __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 <[e-mail]>
> > > > ---
> > > >  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-=[e-mail]/
> > 
> > that this deserves a detailed comment.
> 
> And he wrote one in https://lore.kernel.org/all/CAHk-=wiq=[e-mail]/
> 
>    /*
>     * 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 <[e-mail]>
> > > 
> > > 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)

Vincent Mailhol Dec. 6, 2024, 7:19 a.m. UTC | #7

On Fri. 6 Dec. 2024 at 15:14, Linus Torvalds
<[e-mail]> wrote:
> On Thu, 5 Dec 2024 at 18:26, David Laight <[e-mail]> 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/[e-mail]/

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

Vincent Mailhol Dec. 6, 2024, 7:26 a.m. UTC | #8

On Fri. 6 Dec. 2024 at 15:40, Martin Uecker <[e-mail]> 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 <[e-mail]>
> > > +CC: Martin Uecker <[e-mail]>
> > > (seems that Martin changed his address)
>
> My current one is this: [e-mail]

Ack

(...)

> > > > > + *
> > > > > + * Glory to Martin Uecker <[e-mail]>
> > > >
> > > > 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

Martin Uecker Dec. 6, 2024, 8:29 a.m. UTC | #9

Am Donnerstag, dem 05.12.2024 um 22:14 -0800 schrieb Linus Torvalds:
> On Thu, 5 Dec 2024 at 18:26, David Laight <[e-mail]> 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?

Vincent Mailhol Dec. 6, 2024, 8:49 a.m. UTC | #10

On Fri. 6 Dec. 2024 at 16:19, Vincent Mailhol
<[e-mail]> wrote:
> On Fri. 6 Dec. 2024 at 15:14, Linus Torvalds
> <[e-mail]> wrote:
> > On Thu, 5 Dec 2024 at 18:26, David Laight <[e-mail]> 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/[e-mail]/
>
> 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

David Laight Dec. 6, 2024, 9:34 a.m. UTC | #11

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=[e-mail]/
> 
>    /*
>     * 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)

Vincent Mailhol Dec. 6, 2024, 6:30 p.m. UTC | #12

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
<[e-mail]> wrote:
> On Thu, 5 Dec 2024 at 18:26, David Laight <[e-mail]> 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/[e-mail]/

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

Linus Torvalds Dec. 6, 2024, 6:52 p.m. UTC | #13

On Fri, 6 Dec 2024 at 10:31, Vincent Mailhol <[e-mail]> 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

Linus Torvalds Dec. 6, 2024, 7:02 p.m. UTC | #14

On Fri, 6 Dec 2024 at 10:52, Linus Torvalds
<[e-mail]> 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

David Laight Dec. 6, 2024, 7:06 p.m. UTC | #15

From: Linus Torvalds
> Sent: 06 December 2024 18:53
> 
> On Fri, 6 Dec 2024 at 10:31, Vincent Mailhol <[e-mail]> 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)

Linus Torvalds Dec. 6, 2024, 7:15 p.m. UTC | #16

On Fri, 6 Dec 2024 at 11:07, David Laight <[e-mail]> 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

Willy Tarreau Dec. 6, 2024, 7:38 p.m. UTC | #17

On Fri, Dec 06, 2024 at 11:15:20AM -0800, Linus Torvalds wrote:
> On Fri, 6 Dec 2024 at 11:07, David Laight <[e-mail]> 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

David Laight Dec. 6, 2024, 7:38 p.m. UTC | #18

From: Linus Torvalds
> Sent: 06 December 2024 19:15
> On Fri, 6 Dec 2024 at 11:07, David Laight <[e-mail]> 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)

David Laight Dec. 6, 2024, 7:43 p.m. UTC | #19

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 <[e-mail]> 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)

David Laight Dec. 6, 2024, 8:23 p.m. UTC | #20

...
> > > #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)

Vincent Mailhol Dec. 7, 2024, 7:42 a.m. UTC | #21

On Sat. 7 Dec. 2024 at 05:24, David Laight <[e-mail]> 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

Martin Uecker Dec. 7, 2024, 8:39 a.m. UTC | #22

Am Freitag, dem 06.12.2024 um 16:26 +0900 schrieb Vincent Mailhol:
> > On Fri. 6 Dec. 2024 at 15:40, Martin Uecker <[e-mail]> 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 <[e-mail]>
> > > > > > > > +CC: Martin Uecker <[e-mail]>
> > > > > > > > (seems that Martin changed his address)
> > > > 
> > > > My current one is this: [e-mail]
> > 
> > Ack
> > 
> > (...)
> > 
> > > > > > > > > > > > + *
> > > > > > > > > > > > + * Glory to Martin Uecker <[e-mail]>
> > > > > > > > > > 
> > > > > > > > > > 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

David Laight Dec. 7, 2024, 10:33 a.m. UTC | #23

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)

David Laight Dec. 7, 2024, 11:19 a.m. UTC | #24

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)

Vincent Mailhol Dec. 7, 2024, 12:24 p.m. UTC | #25

On Sat. 7 Dec. 2024 at 20:19, David Laight <[e-mail]> 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

Vincent Mailhol Dec. 7, 2024, 12:45 p.m. UTC | #26

On Sat. 7 Dec. 2024 at 17:39, Martin Uecker <[e-mail]> wrote:
> Am Freitag, dem 06.12.2024 um 16:26 +0900 schrieb Vincent Mailhol:
> > > On Fri. 6 Dec. 2024 at 15:40, Martin Uecker <[e-mail]> 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

Martin Uecker Dec. 7, 2024, 1:07 p.m. UTC | #27

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

Martin Uecker Dec. 7, 2024, 1:18 p.m. UTC | #28

Am Samstag, dem 07.12.2024 um 21:45 +0900 schrieb Vincent Mailhol:
> On Sat. 7 Dec. 2024 at 17:39, Martin Uecker <[e-mail]> 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

Vincent Mailhol Dec. 7, 2024, 1:50 p.m. UTC | #29

On Sat. 7 Dec. 2024 à 22:19, Martin Uecker <[e-mail]> wrote:
> Am Samstag, dem 07.12.2024 um 21:45 +0900 schrieb Vincent Mailhol:
> > On Sat. 7 Dec. 2024 at 17:39, Martin Uecker <[e-mail]> 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-=[e-mail]/T/#m1ba33a804b4041154b72a1d0333f90ec7204c461

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


Yours sincerely,
Vincent Mailhol

Martin Uecker Dec. 7, 2024, 2:59 p.m. UTC | #30

Am Samstag, dem 07.12.2024 um 22:50 +0900 schrieb Vincent Mailhol:
> On Sat. 7 Dec. 2024 à 22:19, Martin Uecker <[e-mail]> wrote:
> > Am Samstag, dem 07.12.2024 um 21:45 +0900 schrieb Vincent Mailhol:
> > > On Sat. 7 Dec. 2024 at 17:39, Martin Uecker <[e-mail]> 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-=[e-mail]/T/#m1ba33a804b4041154b72a1d0333f90ec7204c461
> 
> And I will definitely follow the progress of constexpr functions in C2Y.
> 
> 
> Yours sincerely,
> Vincent Mailhol

Martin Uecker Dec. 7, 2024, 3:10 p.m. UTC | #31

Am Samstag, dem 07.12.2024 um 22:50 +0900 schrieb Vincent Mailhol:
> On Sat. 7 Dec. 2024 à 22:19, Martin Uecker <[e-mail]> 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-=[e-mail]/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

Vincent Mailhol Dec. 7, 2024, 3:23 p.m. UTC | #32

On Sun. 8 Dec. 2024 at 00:10, Martin Uecker <[e-mail]> wrote:
> Am Samstag, dem 07.12.2024 um 22:50 +0900 schrieb Vincent Mailhol:
> > On Sat. 7 Dec. 2024 à 22:19, Martin Uecker <[e-mail]> 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-=[e-mail]/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

David Laight Dec. 7, 2024, 6:07 p.m. UTC | #33

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)

Linus Torvalds Dec. 7, 2024, 6:19 p.m. UTC | #34

On Sat, 7 Dec 2024 at 04:24, Vincent Mailhol <[e-mail]> 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

Linus Torvalds Dec. 7, 2024, 6:26 p.m. UTC | #35

On Sat, 7 Dec 2024 at 05:07, Martin Uecker <[e-mail]> 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

Martin Uecker Dec. 7, 2024, 7:19 p.m. UTC | #36

Am Samstag, dem 07.12.2024 um 10:26 -0800 schrieb Linus Torvalds:
> On Sat, 7 Dec 2024 at 05:07, Martin Uecker <[e-mail]> 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

Martin Uecker Dec. 7, 2024, 7:51 p.m. UTC | #37

Am Samstag, dem 07.12.2024 um 10:19 -0800 schrieb Linus Torvalds:
> On Sat, 7 Dec 2024 at 04:24, Vincent Mailhol <[e-mail]> 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

Linus Torvalds Dec. 7, 2024, 8:28 p.m. UTC | #38

On Sat, 7 Dec 2024 at 11:19, Martin Uecker <[e-mail]> 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

Linus Torvalds Dec. 7, 2024, 8:31 p.m. UTC | #39

On Sat, 7 Dec 2024 at 11:51, Martin Uecker <[e-mail]> 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

David Laight Dec. 7, 2024, 8:54 p.m. UTC | #40

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)

David Laight Dec. 7, 2024, 9 p.m. UTC | #41

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)

Martin Uecker Dec. 7, 2024, 9:06 p.m. UTC | #42

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

David Laight Dec. 7, 2024, 9:45 p.m. UTC | #43

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)

Martin Uecker Dec. 7, 2024, 11:52 p.m. UTC | #44

Am Samstag, dem 07.12.2024 um 12:28 -0800 schrieb Linus Torvalds:
> On Sat, 7 Dec 2024 at 11:19, Martin Uecker <[e-mail]> 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

Linus Torvalds Dec. 8, 2024, 1:58 a.m. UTC | #45

On Sat, 7 Dec 2024 at 15:52, Martin Uecker <[e-mail]> 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

Martin Uecker Dec. 8, 2024, 9:18 a.m. UTC | #46

Am Samstag, dem 07.12.2024 um 17:58 -0800 schrieb Linus Torvalds:
> On Sat, 7 Dec 2024 at 15:52, Martin Uecker <[e-mail]> 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

David Laight Dec. 8, 2024, 11:26 a.m. UTC | #47

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)

Martin Uecker Dec. 8, 2024, 12:38 p.m. UTC | #48

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

David Laight Dec. 8, 2024, 4:48 p.m. UTC | #49

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)

Martin Uecker Dec. 8, 2024, 6:10 p.m. UTC | #50

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

Linus Torvalds Dec. 8, 2024, 7:05 p.m. UTC | #51

On Sun, 8 Dec 2024 at 10:11, Martin Uecker <[e-mail]> 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

Rasmus Villemoes Dec. 9, 2024, 9:59 a.m. UTC | #52

On Sat, Dec 07 2024, Linus Torvalds <[e-mail]> wrote:

> On Sat, 7 Dec 2024 at 04:24, Vincent Mailhol <[e-mail]> 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.

Patch

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 <[e-mail]>
+ */
+#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.