Re: Buiild error in i915/xe

[Jani Nikula <jani.nikula@linux.intel.com>]

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.

BR,
Jani.



-- 
Jani Nikula, Intel