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

[Martin Storsjö]

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.

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