Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes

[Timo Rothenpieler <timo@rothenpieler.org>]

On 06/12/2023 13:31, James Almer wrote:
> On 12/3/2023 5:10 PM, Timo Rothenpieler wrote:
>> FFmpeg has instances of DECLARE_ALIGNED(32, ...) in a lot of structs,
>> which then end up heap-allocated.
>> By declaring any variable in a struct, or tree of structs, to be 32 byte
>> aligned, it allows the compiler to safely assume the entire struct
>> itself is also 32 byte aligned.
>>
>> This might make the compiler emit code which straight up crashes or
>> misbehaves in other ways, and at least in one instances is now
>> documented to actually do (see ticket 10549 on trac).
>> The issue there is that an unrelated variable in SingleChannelElement is
>> declared to have an alignment of 32 bytes. So if the compiler does a copy
>> in decode_cpe() with avx instructions, but ffmpeg is built with
>> --disable-avx, this results in a crash, since the memory is only 16 byte
>> aligned.
> 
> Wont we run into similar issues with avx512 eventually?

It's only indirectly related to AVX.
The core issue is that we have structs with elements that declare an 
alignment of 32 bytes all over the codebase.
I checked all instances, and we do not have any struct members that 
declare a higher alignment requirement than 32.
(There's two instances of 64 byte alignment, but those are not on struct 
members, but on stack variables.)

This promises the compiler that the memory of the whole struct is 
aligned accordingly. So no matter if it breaks because of AVX or 
something else, the compiler could generate broken code if we heap 
allocate those structs with too small of an alignment.
It could generate other, non-AVX code, that depends on that alignment.

So we will only run into this again if someone decides to add a struct 
member with bigger alignment to a heap allocated struct somewhere.

>> Mind you, even if the compiler does not emit avx instructions, the code
>> is still invalid and could misbehave. It just happens not to. Declaring
>> any variable in a struct with a 32 byte alignment promises 32 byte
>> alignment of the whole struct to the compiler.
>>
>> Instead of now going through all instances of variables in structs
>> being declared as 32 byte aligned, this patch bumps the minimum alignment
>> to 32 bytes.
>> ---
>>   libavutil/mem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavutil/mem.c b/libavutil/mem.c
>> index 36b8940a0c..26a9b9753b 100644
>> --- a/libavutil/mem.c
>> +++ b/libavutil/mem.c
>> @@ -62,7 +62,7 @@ void  free(void *ptr);
>>   #endif /* MALLOC_PREFIX */
>> -#define ALIGN (HAVE_AVX512 ? 64 : (HAVE_AVX ? 32 : 16))
>> +#define ALIGN (HAVE_AVX512 ? 64 : 32)
>>   /* NOTE: if you want to override these functions with your own
>>    * implementations (not recommended) you have to link libav* as
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".