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

[Timo Rothenpieler <timo@rothenpieler.org>]

On 09.12.2023 06:23, Andreas Rheinhardt wrote:
> 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).

ping about this.
I'm still not sure about the correct way forward here.

Aside from the complexity of figuring out the reasonable max align, I'm 
also a not sure on how to modify the macro to make use of it at all.
You can't put any kind of MIN/MAX construct into the body of the 
alignment macro after all.
_______________________________________________
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".