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

[Timo Rothenpieler <timo@rothenpieler.org>]

On 06.12.2023 14:25, Martin Storsjö wrote:
> On Sun, 3 Dec 2023, 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.
>> 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)
> 
> LGTM
> 
> It could be good to add a comment here, to indicate how this value 
> relates to the alignemnts used in structs.

Thinking about this, I don't think a comment _here_ is a good idea.
I'd be more inclined to add it to the DECLARE_ALIGNED macro.
People defining a new aligned member variable are at least a little more 
likely to read that, compared to something next to this random define 
that's not even in a header.

Will add that and push soon.
I'll also check how far this will need backported. Likely to almost all 
versions ever.

> For others who commented in this thread, it all boils down to something 
> like this:
> 
> struct MyData {
>      uint8_t __attribute__((aligned(32))) aligned_data[1024];
> };
> 
> void func(void) {
>      struct MyData *obj = malloc(sizeof(*obj)); // Uusally only aligned 
> to 8 bytes
>      // operate on obj->aligned_data[]
> }
> 
> Due to how aligned_data is declared, we promise to the compiler that it 
> is aligned to 32 bytes, and that the compiler can assume this wherever. 
> Depending on -march or whatever, this can be to access it with 
> instructions that assume 32 byte alignment.
> 
> // Martin
> 
> _______________________________________________
> 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".