Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[David Laight <david.laight.linux@gmail.com>]

On Sat, 18 Jan 2025 13:21:39 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > No idea why the compiler would know that the values are invalid.  
> 
> It's not that the compiler knows tat they are invalid, but I bet what
> happens is in scale() (and possibly other places that do similar
> checks), which does this:
> 
>         WARN_ON(source_min > source_max);
>         ...
>         source_val = clamp(source_val, source_min, source_max);
> 
> and the compiler notices that the ordering comparison in the first
> WARN_ON() is the same as the one in clamp(), so it basically converts
> the logic to
> 
>         if (source_min > source_max) {
>                 WARN(..);
>                 /* Do the clamp() knowing that source_min > source_max */
>                 source_val = clamp(source_val, source_min, source_max);
>         } else {
>                 /* Do the clamp knowing that source_min <= source_max */
>                 source_val = clamp(source_val, source_min, source_max);
>         }
> 
> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> relevant for this case).
> 
> And now that first clamp() case is done with source_min > source_max,
> and it triggers that build error because that's invalid.
> 
> So the condition is not statically true in the *source* code, but in
> the "I have moved code around to combine tests" case it now *is*
> statically true as far as the compiler is concerned.

Well spotted :-)

One option would be to move the WARN_ON() below the clamp() and
add an OPTIMISER_HIDE_VAR(source_max) between them.

Or do something more sensible than the WARN().
Perhaps return target_min on any such errors?

	David

> 
>               Linus
>