Message-ID: <abdd7862f136aa676b2d2c324369f4a43ff9909c.camel@gwdg.de>
Date: Fri, 6 Dec 2024 07:40:17 +0100
From: Martin Uecker <muecker@...g.de>
To: David Laight <David.Laight@...LAB.COM>, 'Vincent Mailhol'
	<mailhol.vincent@...adoo.fr>
CC: Linus Torvalds <torvalds@...ux-foundation.org>, Luc Van Oostenryck
	<luc.vanoostenryck@...il.com>, Nathan Chancellor <nathan@...nel.org>, "Nick
 Desaulniers" <ndesaulniers@...gle.com>, Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>, Yury Norov <yury.norov@...il.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>, Kees Cook <kees@...nel.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>, Jani Nikula
	<jani.nikula@...ux.intel.com>, Joonas Lahtinen
	<joonas.lahtinen@...ux.intel.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>,
	Tvrtko Ursulin <tursulin@...ulin.net>, David Airlie <airlied@...il.com>,
	Simona Vetter <simona@...ll.ch>, Suzuki K Poulose <suzuki.poulose@....com>,
	Mike Leach <mike.leach@...aro.org>, James Clark <james.clark@...aro.org>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Rikard Falkeborn
	<rikard.falkeborn@...il.com>, "linux-sparse@...r.kernel.org"
	<linux-sparse@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "llvm@...ts.linux.dev"
	<llvm@...ts.linux.dev>, "linux-hardening@...r.kernel.org"
	<linux-hardening@...r.kernel.org>, "intel-gfx@...ts.freedesktop.org"
	<intel-gfx@...ts.freedesktop.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "coresight@...ts.linaro.org"
	<coresight@...ts.linaro.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of
 __is_constexpr()

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

My current one is this: uecker@...raz.at

Martin

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

Powered by blists - more mailing lists