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-requ...@ffmpeg.org with subject "unsubscribe".
