Re: Buiild error in i915/xe

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Jan 20, 2025 at 06:41:43PM +0000, David Laight wrote:
> On Mon, 20 Jan 2025 06:15:30 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> > On 1/20/25 03:21, Jani Nikula wrote:
> > > On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:  
> > >> 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.  
> > > 
> > > The point is, we'll catch the WARN in CI before it goes out to users.
> > 
> > It isn't going to catch the divide by 0 error, and it obviously doesn't
> > catch the build problem on parisc with gcc 13.x because the CI isn't
> > testing it.
> > 
> > How about disabling DRM_XE on architectures where it isn't supported,
> > matching DRM_I915 ?
> 
> That'll just bite back later.
> As Linus spotted the compiler is just 'optimising' some code paths.
> It could happen for any architecture including x64.
> The repeated tests are basically slightly odd, although you might only
> expect them to be present in debug builds.
> 
> An alternative would be to replace the clamp() with:
> 	if (source_val <= source_min)
> 		return target_min;
> 	if (source_val >= source_max)
> 		return target_max;

Excuse me if I am missing something, but clamp() has a warning inside it, correct?
Why do wee need an additional warning on top of that?

P.S. However, I agree that ideally clamp() should work independently on the
caller to use WARN*() or other similar stuff.

-- 
With Best Regards,
Andy Shevchenko