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

Message

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

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

The goal is to introduce a set of three macros:

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

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

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

Once defined, apply them tree-wide.

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

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

Patch 2 adds __is_const_zero() and is_const().

Patch 3 adds is_const_true() and is_const_false().

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

Patch 10 finally remove __is_constexpr(). RIP!

[1] add const_true() to simplify GENMASK_INPUT_CHECK()
Link: https://lore.kernel.org/all/[e-mail]/

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

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

Best regards,

Comments

Vincent Mailhol Dec. 3, 2024, 11:32 a.m. UTC | #1

Hi checkpatch maintainers,

On Tue, 03 Dec 2024 at 10:19:34, Patchwork <[e-mail]> wrote:
> == Series Details ==
>
> Series: compiler.h: refactor __is_constexpr() into is_const{,_true,_false}()
> URL   : https://patchwork.freedesktop.org/series/142040/
> State : warning
>
> == Summary ==
>
> Error: dim checkpatch failed
> 65c5a73cbdd2 compiler.h: add statically_false()
> 25d8e6973c58 compiler.h: add is_const() as a replacement of __is_constexpr()
> -:75: CHECK:SPACING: spaces preferred around that '*' (ctx:WxO)
> #75: FILE: include/linux/compiler.h:349:
> +	_Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)
>  	                                                 ^
>
> -:75: ERROR:SPACING: spaces required around that ':' (ctx:OxW)
> #75: FILE: include/linux/compiler.h:349:
> +	_Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)
>  	                                                  ^
>
> -:75: CHECK:SPACING: spaces preferred around that '*' (ctx:WxO)
> #75: FILE: include/linux/compiler.h:349:
> +	_Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)
>  	                                                            ^
>
> -:75: ERROR:SPACING: spaces required around that ':' (ctx:OxW)
> #75: FILE: include/linux/compiler.h:349:
> +	_Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)
>  	                                                             ^
>
> -:75: CHECK:CAMELCASE: Avoid CamelCase: <_Generic>
> #75: FILE: include/linux/compiler.h:349:
> +	_Generic(0 ? (void *)(long)(x) : (char *)0, char *: 1, void *: 0)

I submitted a patch which contained a _Generic() selection [1] and got
above checkpatch warnings and errors.

Before blindly fixing those, I wanted to discuss this with you because
it looks like a false positive to me.

I think that the _Generic() selection should follow the switch case or
the goto label style, that is to say, no space before the colon.

For example:

  _Generic(foo,
	   char: 0,
	   short: 1,
	   char *: 2,
	   short *: 3,
	   default: 4);

instead of:

  _Generic(foo,
	   char : 0,
	   short : 1,
	   char * : 2,
	   short * : 3,
	   default : 4);

The fact is that the introduction of _Generic in the kernel is fairly
recent (can be used since July 2020 following the bump to GCC 4.9 in [2]).
So I guess that it is just that the _Generic()s where never considered
in checkpatch.pl.

I would have liked to send a fix, but I am not fluent in perl, so the
best I have to offer is this report.

Let me know if it is acceptable to ignore those checkpatch errors :)

[1] compiler.h: add is_const() as a replacement of __is_constexpr()
Link: https://lore.kernel.org/intel-gfx/[e-mail]/

[2] commit 6ec4476ac825 ("Raise gcc version requirement to 4.9")
Link: https://git.kernel.org/torvalds/c/6ec4476ac825


Yours sincerely,
Vincent Mailhol

Kees Cook Dec. 4, 2024, 11:58 p.m. UTC | #2

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

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

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

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

-Kees

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

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

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

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

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

  https://lore.kernel.org/all/[e-mail]/

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


Yours sincerely,
Vincent Mailhol