| Message ID | 20241113172939.747686-6-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add const_true() to simplify GENMASK_INPUT_CHECK() | expand |
From: Vincent Mailhol > Sent: 13 November 2024 17:19 > > In GENMASK_INPUT_CHECK(), > > __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0) > > is the exact expansion of: > > const_true((l) > (h)) > > Apply const_true() to simplify GENMASK_INPUT_CHECK(). Wouldn't statically_true() give better coverage ? I wouldn't have though that GENMASK() got used anywhere where a constant integer expression was needed. More interesting would be to get it to pass a W=1 build for any place where 'l' is 0u. David > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > This change passes the unit tests from CONFIG_BITS_TEST, including the > extra negative tests provided under #ifdef TEST_GENMASK_FAILURES [1]. > > [1] commit 6d511020e13d ("lib/test_bits.c: add tests of GENMASK") > Link: https://git.kernel.org/torvalds/c/6d511020e13d > --- > include/linux/bits.h | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/include/linux/bits.h b/include/linux/bits.h > index 60044b608817..61a75d3f294b 100644 > --- a/include/linux/bits.h > +++ b/include/linux/bits.h > @@ -20,9 +20,8 @@ > */ > #if !defined(__ASSEMBLY__) > #include <linux/build_bug.h> > -#define GENMASK_INPUT_CHECK(h, l) \ > - (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ > - __is_constexpr((l) > (h)), (l) > (h), 0))) > +#include <linux/compiler.h> > +#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h))) > #else > /* > * BUILD_BUG_ON_ZERO is not available in h files included from asm files, > -- > 2.45.2 > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: David Laight > Sent: 17 November 2024 17:25 > > From: Vincent Mailhol > > Sent: 13 November 2024 17:19 > > > > In GENMASK_INPUT_CHECK(), > > > > __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0) > > > > is the exact expansion of: > > > > const_true((l) > (h)) > > > > Apply const_true() to simplify GENMASK_INPUT_CHECK(). > > Wouldn't statically_true() give better coverage ? > I wouldn't have though that GENMASK() got used anywhere where a constant > integer expression was needed. If it is, maybe add a GENMASK_CONST() that uses BUILD_BUG_ON_ZERO_MSG() (recently proposed) and so validates that the values are constants. And then use statically_true() in the normal case to pick up more errors. (Or just remove the check because coders really aren't that stupid!) The interesting cases are the ones using variables. And they'd need run-time checks of some form. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon. 18 Nov. 2024 at 02:24, David Laight <David.Laight@aculab.com> wrote: > From: Vincent Mailhol > > Sent: 13 November 2024 17:19 > > > > In GENMASK_INPUT_CHECK(), > > > > __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0) > > > > is the exact expansion of: > > > > const_true((l) > (h)) > > > > Apply const_true() to simplify GENMASK_INPUT_CHECK(). > > Wouldn't statically_true() give better coverage ? > I wouldn't have though that GENMASK() got used anywhere where a constant > integer expression was needed. > > More interesting would be to get it to pass a W=1 build for > any place where 'l' is 0u. Are you referring to -Wtype-limits? If yes, this warning was moved to W=2. I suggested in the past to add a cast to silence this warning, but got rejected: https://lore.kernel.org/lkml/CAHk-=whvGWbpsTa538CvQ9e=VF+m8WPQmES2y6-=0=-64uGkgg@mail.gmail.com/ Yours sincerely, Vincent Mailhol
On Mon. 18 Nov. 2024 at 04:45, David Laight <David.Laight@aculab.com> wrote: > From: David Laight > > Sent: 17 November 2024 17:25 > > > > From: Vincent Mailhol > > > Sent: 13 November 2024 17:19 > > > > > > In GENMASK_INPUT_CHECK(), > > > > > > __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0) > > > > > > is the exact expansion of: > > > > > > const_true((l) > (h)) > > > > > > Apply const_true() to simplify GENMASK_INPUT_CHECK(). > > > > Wouldn't statically_true() give better coverage ? Yes, it would. > > I wouldn't have though that GENMASK() got used anywhere where a constant > > integer expression was needed. It is used in many places, including some inline functions such as bitmap_set(): https://elixir.bootlin.com/linux/v6.11/source/include/linux/bitmap.h#L469 where the input is not an integer constant expression (thus preventing the use of statically_true()). > If it is, maybe add a GENMASK_CONST() that uses BUILD_BUG_ON_ZERO_MSG() > (recently proposed) and so validates that the values are constants. > And then use statically_true() in the normal case to pick up more errors. The issue if you introduce GENMASK_CONST() is that there is no gain. The only advantage of const_true(x) is that it works on cases where statically_true(x) would cause a compilation error. But for statically_true(x) to cause a compilation error, x has to be a non constant expression. And if x is non constant, then const_true(x) returns false. Regardless, considering the number of times where GENMASK() is used with integer literals, I do not think it would be worth it to replace all of these with GENMASK_CONST() tree wide. Trying to go in your direction, we already have two genmasks: - GENMASK(): which uses GENMASK_INPUT_CHECK() - __GENMASK(): no check, used in uapi What would make more sense to me would be to: 1. replace const_true() by statically_true() in GENMASK_INPUT_CHECK() 2. replace all the instances where GENMASK() breaks compilation with __GENMASK() But this idea was already proposed in the past and was rejected here: https://lore.kernel.org/lkml/YJm5Dpo+RspbAtye@rikard/ > (Or just remove the check because coders really aren't that stupid!) I think that this check exists in the first place *because* such a mistake was made in the past. > The interesting cases are the ones using variables. > And they'd need run-time checks of some form. Then, instead of introducing GENMASK_CONST(), maybe introduce GENMASK_NON_CONST()? But then, the number of instances in which GENMASK() is used with something other than literal integers is pretty rare. So probably not worth it. Yours sincerely, Vincent Mailhol
diff --git a/include/linux/bits.h b/include/linux/bits.h index 60044b608817..61a75d3f294b 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -20,9 +20,8 @@ */ #if !defined(__ASSEMBLY__) #include <linux/build_bug.h> -#define GENMASK_INPUT_CHECK(h, l) \ - (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ - __is_constexpr((l) > (h)), (l) > (h), 0))) +#include <linux/compiler.h> +#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h))) #else /* * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
In GENMASK_INPUT_CHECK(), __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0) is the exact expansion of: const_true((l) > (h)) Apply const_true() to simplify GENMASK_INPUT_CHECK(). Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- This change passes the unit tests from CONFIG_BITS_TEST, including the extra negative tests provided under #ifdef TEST_GENMASK_FAILURES [1]. [1] commit 6d511020e13d ("lib/test_bits.c: add tests of GENMASK") Link: https://git.kernel.org/torvalds/c/6d511020e13d --- include/linux/bits.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)