Re: Buiild error in i915/xe

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

On Mon, 20 Jan 2025 12:48:11 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
> > On Sat, 18 Jan 2025 14:58:48 -0800
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >  
> >> On 1/18/25 14:11, David Laight wrote:  
> >> > 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?
> >> >     
> >> 
> >> 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.  
> 
> I want the loud and early warnings for clear bugs instead of
> "gracefully" silencing the errors only to be found through debugging
> user reports.

A user isn't going to notice a WARN() - not until you tell them to look for it.
In any case even if you output a message you really want to return a 'sane'
value, who knows what effect a very out of range value is going to have.

	David