Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/20/25 03:21, Jani Nikula wrote:
> On Mon, 20 Jan 2025, David Laight <david.laight.linux@xxxxxxxxx> wrote:
> > On Mon, 20 Jan 2025 12:48:11 +0200
> > Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> >
> > > On Sun, 19 Jan 2025, David Laight <david.laight.linux@xxxxxxxxx> wrote:
> > > > 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.
> > >
> > > 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 ?

Thanks,
Guenter