[FFmpeg-devel] [PATCH] avcodec/x86/mathops: use constrained immediate operands

[James Almer]

On 7/16/2023 6:23 AM, Rémi Denis-Courmont wrote:
> Le sunnuntaina 16. heinäkuuta 2023, 2.58.32 EEST James Almer a écrit :
>> Should fix assembling with binutil as >= 2.41
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> This is IMO a big breakage. binutil's as has until now clipped these values
>> on its own, and never required the compiler to do it.
> 
> TBH, silently clipping immediate constants sounds like a nasty bug that could
> cause really nasty suprises if somebody every passes an out-of-range constant.

We're passing it out or range constants alright. I tried adding an 
av_assert0((uint8_t)(-s) <= 31) and most fate tests started failing.

> This has happened to me many times, typically with incidentally out-of-range
> immediate offsets in loads/stores.
> 
> (...)
> 
>>       __asm__ ("shrl %1, %0\n\t"
>>            : "+r" (a)
>> -         : "ic" ((uint8_t)(-s))
>> +         : "Ic" ((uint8_t)(-s))
> 
> Note that this is not equivalent. Now, if `s` is constant but out of range,
> the compiler will be required to fit it. And it does that by moving it into
> ECX. This is probably not what you want.
> 
> AFAICT, you should keep the constraint as it is, and fix the operand value
> instead by masking it, e.g.:
> 
>          if (__builtin_constant_p(s))
>                  __asm__ ("shrl %1, %0\n\t"
>                          : "+r" (a)
>                          : "i" ((-s) & 0x1f)
>                  );
>          else
>                  __asm__ ("shrl %1, %0\n\t"
>                          : "+r" (a)
>                          : "c" (-s)
>                  );
> 
> (Not sure if the the 0x1f mask is correct, but you get the idea.)

It is, just tested.