Re: Buiild error in i915/xe

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

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;

	David

> 
> Thanks,
> Guenter
>