Re: [PATCH v2 1/8] minmax: Put all the clamp() definitions together

[Linus Torvalds]

On Tue, 30 Jul 2024 at 03:11, Arnd Bergmann <arnd@xxxxxxxxxx> wrote:
>
> I'm giving this a spin on the randconfig test setup now to see
> if there are some other cases like the bcachefs one. So far I've
> seen one failure, but I can't make sense of it yet:

So the new checks are actually a lot smarter, since unlike the old
ones they don't require a C constant expression, and will find cases
where the compiler can see expressions that turn out statically
optimizable.

This is a great example of that, although "great" in this case is
sadly not what we want:

> drivers/gpu/drm/i915/display/intel_backlight.c: In function 'scale':
> include/linux/compiler_types.h:510:45: error: call to '__compiletime_assert_905' declared with attribute error: clamp() low limit source_min greater than high limit source_max
> include/linux/minmax.h:107:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
>   107 |         BUILD_BUG_ON_MSG(statically_true(ulo > uhi),                            \
> drivers/gpu/drm/i915/display/intel_backlight.c:47:22: note: in expansion of macro 'clamp'
>    47 |         source_val = clamp(source_val, source_min, source_max);

So here *locally*, source_min and source_max can't be ordered, but
what I think has happened is that we had that earlier

        WARN_ON(source_min > source_max);

and then gcc sees the "statically_true(ulo > uhi)" test, and will do
CSE on the variables and on the test condition and the conditional,
and basically have turned all of this into

        if (source_min > source_max) {
                WARN(..)
                source_val = clamp(source_val, source_min, source_max);
        } else {
                source_val = clamp(source_val, source_min, source_max);
        }

and now the case with the WARN() will statically obviously be bad.

I don't see the failure, so it clearly depends on some config default,
and I suspect with my allmodconfig build, for example, there is so
much else going on that gcc won't have done the above trivial
conversion.

The old code never saw any of this, because the old code was using the
terminally stupid _static_assert(), and within the much more limited
scope of a "C constant expression", that "source_min < source_max"
could never be true, even if there are code paths where it *is* true.

But here I think we were bitten by excessive cleverness.

> That's still a typo in the 32-bit case, right?
> I've changed
>
>  __builtin_choose_expr(sizeof(ux)>32,1LL,1L))
>
> to check for sizeof(ux)>4 for my testing.

Bah yes. I had that fix locally, and sent the old patch.

            Linus