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

[Linus Torvalds <torvalds@linuxfoundation.org>]

On Tue, 30 Jul 2024 at 09:35, Linus Torvalds
<torvalds@linuxfoundation.org> wrote:
>
> 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);
>         }

Confirmed with your .config - removing the WARN_ON() removes the
clamping range error, because then there is no "move code into shared
conditional section" case any more.

That's slightly annoying. The new clamp() logic is not only a much
cleaner macro expansion, it's also *much* smarter and would find real
problems when the limits have been passed as arguments to inline
functions etc.

But obviously this "it's statically wrong in one path when the code
has been duplicated by the compiler" means that it's just too smart
for its own good in this case.

If the WARN_ON() had a "return error", it would all work out
beautifully. But now we literally have code that says "I just tested
for this error condition, and then I went ahead and did the error case
anyway", and that just makes my nice new sanity check unhappy.

Darn.

                Linus