On Fri, 8 Dec 2023, Timo Rothenpieler wrote:
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.
I still think it'd be good to have a comment here, too, to explain this
code to whoever is looking at it (why 32 or 64, why not 16/32/64 etc). I
agree that it can be good to mention it near DECLARE_ALIGNED as well, but
these comments serve different readers.
// 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-requ...@ffmpeg.org with subject "unsubscribe".
