On Thu, Oct 15, 2026 at 3:00 AM David Laight <[email protected]> wrote: > On Wed, 14 Oct 2026 13:39:03 -0700 > Bill Wendling <[email protected]> wrote: > > > On Wed, Oct 14, 2026 at 5:38 AM David Laight > > <[email protected]> wrote: > > > On Tue, 13 Oct 2026 20:31:46 +0000 > > > Bill Wendling <[email protected]> wrote: > > > > > > > The two parts of '__builtin_choose_expr()' had different arithmetic > > > > expressions. This patch corrects it so that both parts calculate: > > > > > > > > COUNT * sizeof(*MEMBER) + __must_be_array(MEMBER) > > > > > > > > In practice, '__must_be_array' is meant to assert if 'MEMBER' isn't an > > > > array and return '0' otherwise. However, symmetry is less prone to > > > > subtle errors (and far more satisfying). > > > > > > > > Signed-off-by: Bill Wendling <[email protected]> > > > > --- > > > > v2: Don't use 'size_add', which just adds more complicated macro cruft > > > > to the > > > > expression. Instead, just adjust the parentheses. > > > > --- > > > > include/linux/overflow.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > > > > index a8cb6319b4fb..d0f04436343e 100644 > > > > --- a/include/linux/overflow.h > > > > +++ b/include/linux/overflow.h > > > > @@ -418,7 +418,7 @@ static __always_inline size_t __must_check > > > > size_sub(size_t minuend, size_t subtr > > > > #define flex_array_size(p, member, count) \ > > > > __builtin_choose_expr(__is_constexpr(count), \ > > > > (count) * sizeof(*(p)->member) + > > > > __must_be_array((p)->member), \ > > > > - size_mul(count, sizeof(*(p)->member) + > > > > __must_be_array((p)->member))) > > > > + size_mul(count, sizeof(*(p)->member)) + > > > > __must_be_array((p)->member)) > > > > > > Why does this use __builtin_choose_expr(). > > > The types of the two expressions are the same so it might as well use ?: > > > > > It's to emit only one of the expressions rather than both. If the > > 'count' is a constant expression, then it's possible for the compiler > > to perform the calculation without having to hold onto the "false" > > path of the ternary operator. So much more efficient in the long-run. > > I doubt it. > Even with __builtin_choose_expr() the compile has to do most of the work > for compiling the 'false' path. > If you use ?: then you can also use the much simpler __builtin_constant_p() > and possibly pick up cases where it is only known to be constant later.
In both scenarios the compiler is meant to do the majority of the work, so I don't care about the compiler doing more work, only that the work it does results in better code. First, __builtin_constant_p(n) won't catch as many uses as __is_constexpr(n). With the ternary operator, the compiler will generate code for both sides and the predicate, which adds a needless if-then branch to the CFG. Yes, that will get simplified, but again it's not a benefit over __builtin_choose_expr(). In the simplest cases, the compilers (both GCC and Clang) can decide which branch to take in the front-end. That's possible with the ternary operator, but care needs to be taken due to possible side-effects. Also, Clang performs far fewer of these kinds of code transformations in the front-end. For instance, GCC will perform some inlining in the front-end, making it easier for it to catch __builtin_constant_p() uses that Clang has to delay. All of this to say, there is no benefit to using the ternary operator, even if you want to claim that the compiler is doing more work (which we don't care about), and potentially worse code being generated, due to processing that has to occur later on. > > > Also doesn't this optimisation apply to all uses of size_mul() (aka > > > saturating_mul()). > > > Presumably relying on the compiler to report the compilation time numeric > > > overflow. > > > > I'm not familiar with saturating_mul(). I'm sure others on here can talk to > > it. > > That is me giving is a better name. > The same is true for size_add(). > IMHO saturation size_t calculations is really broken because it allows > through 'bigger than sensible' sizes that still might cause problems > (eg kmalloc() of sizes just below the limit.) > Patches welcome. :-) -bw
