On Sat, 18 Jan 2025 14:58:48 -0800
Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On 1/18/25 14:11, David Laight wrote:
> > On Sat, 18 Jan 2025 13:21:39 -0800
> > Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@xxxxxxxxxxxx> 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?
> >
>
> This helps:
>
> - WARN_ON(source_min > source_max);
> - WARN_ON(target_min > target_max);
> -
> /* defensive */
> source_val = clamp(source_val, source_min, source_max);
>
> + WARN_ON(source_min > source_max);
> + WARN_ON(target_min > target_max);
That is a 'quick fix' ...
Much better would be to replace the WARN() with (say):
if (target_min >= target_max)
return target_min;
if (source_min >= source_max)
return target_min + (target_max - target_min)/2;
So that the return values are actually in range (in as much as one is defined).
Note that the >= cpmparisons also remove a divide by zero.
David
>
> Guenter
>
