| Message ID | 20241111164743.339117-2-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] linux/bits: simplify GENMASK_INPUT_CHECK() | expand |
On Mon, 11 Nov 2024 at 08:48, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > > - introduce _statically_true(), taking inspiration from > statically_true() as introduced in commit 22f546873149 ("minmax: > improve macro expansion and type checking") So I really think this needs an explanation of what the difference is when using __builtin_constant_p() vs using __is_constexpr(), and why the existing statically_true() didn't work for you. In my experience, __is_constexpr() is too limited, because it literally requires a syntactically constant expression. In contrast, __builtin_constant_p() often works for things that aren't constant expressions, but that evaluate to constants at build time. For example, I had a test patch that used statically_true() to do things like "if the size of a user copy is a multiple of the size of 'long', call a simplified version without the byte copy part". And sure, __is_constexpr() gets it right for completely constant arguments. But __builtin_constant_p() will actually trigger not only those, but also when the argument is something like if (copy_to_user(buf, values, n * sizeof(u64))) because it sees that even if "n * sizeof(u64)" is not a constant, the "is this a multiple of 'long' size" _is_ constant. IOW, I think __builtin_constant_p() is preferable, because it not only doesn't expand to the horror that is __is_constexpr(), it also generally does better when you have the flexibility to use it. Of course, I do think that the use in BUILD_BUG_ON_ZERO() requires something that is more statically reliable, and so __is_constexpr() that is purely syntactic is probably the right thing to have. So I'm not objecting to your _statically_true() per se. I just think this needs a big comment about why we have both versions, and when to use one over the other. Linus
diff --git a/include/linux/bits.h b/include/linux/bits.h index 60044b608817..01713e1eda56 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(_statically_true((l) > (h))) #else /* * BUILD_BUG_ON_ZERO is not available in h files included from asm files, diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 4d4e23b6e3e7..fee66166eca2 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -307,6 +307,7 @@ static inline void *offset_to_ptr(const int *off) * values to determine that the condition is statically true. */ #define statically_true(x) (__builtin_constant_p(x) && (x)) +#define _statically_true(x) (__is_constexpr(x) && (x)) /* * This is needed in functions which generate the stack canary, see
Because of the shortcut logic of the && operator, below expression: __builtin_choose_expr(condition, boolean_expression, false) can be simplified as: condition && boolean_expression Applied to GENMASK_INPUT_CHECK(), __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0) can be replaced by: __is_constexpr((l) > (h)) && (l) > (h) Finally, above expression is nearly the same as the expansion of statically_true((l) > (h)), except from the use of __is_constexpr() instead of __builtin_constant_p(). Introduce _statically_true() which is similar to statically_true() but with __is_constexpr(). Apply _statically_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 ** Changelog ** v1 -> v2: - introduce _statically_true(), taking inspiration from statically_true() as introduced in commit 22f546873149 ("minmax: improve macro expansion and type checking") Link: https://lore.kernel.org/all/20240609073513.256179-1-mailhol.vincent@wanadoo.fr/ --- include/linux/bits.h | 5 ++--- include/linux/compiler.h | 1 + 2 files changed, 3 insertions(+), 3 deletions(-)