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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Timo Rothenpieler:
> On 08.12.2023 11:01, Andreas Rheinhardt wrote:
>> Timo Rothenpieler:
>>> 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)
>>>     /* NOTE: if you want to override these functions with your own
>>>    * implementations (not recommended) you have to link libav* as
>>
>> 1. There is another way in which this can be triggered: Namely if one
>> uses a build with AVX, but combines it with a lavu built without it; it
>> is also triggerable on non-x86 (having an insufficiently aligned pointer
>> is always UB even if the CPU does not have instructions that would
>> benefit from the additional alignment). You should mention this in the
>> commit message.
> 
> Is mixing the libraries really a scenario we need to care about/support?
> 

IMO, no, but Anton cares about it a lot.

> And yeah, this is only marginally related to AVX, in that it's what
> triggers a crash in this scenario.
> But as stated in the commit message, it's not a valid thing to do in any
> case, on any arch. It just happens not to crash.
> 

I know, but this patch also happens to fix this (at least to some
degree), so this should be mentioned in the commit message.

>> 2. This topic gave me headaches when creating RefStruct. I "solved" it
>> by (ab)using STRIDE_ALIGN which mimicks the alignment of av_malloc(),
>> thereby ensuring that RefStruct does not break lavc builds built with
>> the avx dsp functions enabled (but it does not guard against using a
>> lavu whose av_malloc() only provides less alignment).
>>
>> 3. There is a downside to your patch: It bumps alignment for non-x86
>> arches which wastes memory (and may make allocators slower). We could
>> fix this by modifying the 32-byte-alignment macros to only provide 16
>> byte alignment if the ARCH_ (and potentially the HAVE_) defines indicate
>> that no alignment bigger than 16 is needed.
> 
> But it's invalid on any other arch as well, just hasn't bitten us yet.

It is not invalid if we modify DECLARE_ALIGNED to never use more
alignment than 16 on non-x86 arches. Then all the other arches can
continue to use 16.

> I'm not sure if I'd want to start maintaining a growingly complex list
> of CPU extensions and make the DECLARE_ALIGNED macro lie if we think it
> doesn't need 32 byte alignment.
> We don't really know why someone wants a variable aligned after all.

I am fine with that point. Although I don't think it would be that
complicated if it is done at one point (namely in configure) and if all
the other places would just use a macro for max alignment (that would be
placed in config.h).

- Andreas

_______________________________________________
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".