Re: [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align

["Rémi Denis-Courmont" <remi@remlab.net>]

Le perjantaina 9. helmikuuta 2024, 21.22.17 EET Timo Rothenpieler a écrit :
> On 13.01.2024 16:46, 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.
> > 
> > This patch limits the maximum alignment to the maximum possible simd
> > alignment according to configure.
> > While not perfect, it at the very least gets rid of a lot of UB, by
> > matching up the maximum DECLARE_ALIGNED value with the alignment of heap
> > allocations done by lavu.
> > ---
> > 
> >   libavutil/mem.c          |  8 +++++++-
> >   libavutil/mem_internal.h | 14 ++++++++------
> >   2 files changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/libavutil/mem.c b/libavutil/mem.c
> > index 36b8940a0c..b5bcaab164 100644
> > --- a/libavutil/mem.c
> > +++ b/libavutil/mem.c
> > @@ -62,7 +62,13 @@ void  free(void *ptr);
> > 
> >   #endif /* MALLOC_PREFIX */
> > 
> > -#define ALIGN (HAVE_AVX512 ? 64 : (HAVE_AVX ? 32 : 16))
> > +#if defined(_MSC_VER)
> > +/* MSVC does not support conditionally limiting alignment.
> > +   Set minimum value here to maximum used throughout the codebase. */
> > +#define ALIGN (HAVE_SIMD_ALIGN_64 ? 64 : 32)

Not that I care whatsoever, but are we assuming that MSVC supports only x86? 
Otherwise, this conditional definition does not make much sense and seems very 
sketchy. In fact, I don't see the point in making this distinction at all 
(*unlike* below).

-- 
レミ・デニ-クールモン
http://www.remlab.net/



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