On Mon. 18 Nov. 2024 at 04:45, David Laight <David.Laight@xxxxxxxxxx> 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
