* [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes @ 2023-12-03 20:10 Timo Rothenpieler 2023-12-06 12:27 ` Timo Rothenpieler ` (4 more replies) 0 siblings, 5 replies; 29+ messages in thread From: Timo Rothenpieler @ 2023-12-03 20:10 UTC (permalink / raw) To: ffmpeg-devel; +Cc: 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 -- 2.34.1 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-03 20:10 [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes Timo Rothenpieler @ 2023-12-06 12:27 ` Timo Rothenpieler 2023-12-06 12:31 ` James Almer ` (3 subsequent siblings) 4 siblings, 0 replies; 29+ messages in thread From: Timo Rothenpieler @ 2023-12-06 12:27 UTC (permalink / raw) To: ffmpeg-devel On 03/12/2023 21:10, 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) > > /* NOTE: if you want to override these functions with your own > * implementations (not recommended) you have to link libav* as ping _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-03 20:10 [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes Timo Rothenpieler 2023-12-06 12:27 ` Timo Rothenpieler @ 2023-12-06 12:31 ` James Almer 2023-12-06 12:56 ` Timo Rothenpieler 2023-12-06 12:50 ` Ronald S. Bultje ` (2 subsequent siblings) 4 siblings, 1 reply; 29+ messages in thread From: James Almer @ 2023-12-06 12:31 UTC (permalink / raw) To: ffmpeg-devel On 12/3/2023 5:10 PM, 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. Wont we run into similar issues with avx512 eventually? > 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 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-06 12:31 ` James Almer @ 2023-12-06 12:56 ` Timo Rothenpieler 0 siblings, 0 replies; 29+ messages in thread From: Timo Rothenpieler @ 2023-12-06 12:56 UTC (permalink / raw) To: ffmpeg-devel On 06/12/2023 13:31, James Almer wrote: > On 12/3/2023 5:10 PM, 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. > > Wont we run into similar issues with avx512 eventually? It's only indirectly related to AVX. The core issue is that we have structs with elements that declare an alignment of 32 bytes all over the codebase. I checked all instances, and we do not have any struct members that declare a higher alignment requirement than 32. (There's two instances of 64 byte alignment, but those are not on struct members, but on stack variables.) This promises the compiler that the memory of the whole struct is aligned accordingly. So no matter if it breaks because of AVX or something else, the compiler could generate broken code if we heap allocate those structs with too small of an alignment. It could generate other, non-AVX code, that depends on that alignment. So we will only run into this again if someone decides to add a struct member with bigger alignment to a heap allocated struct somewhere. >> 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 > _______________________________________________ > 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". _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-03 20:10 [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes Timo Rothenpieler 2023-12-06 12:27 ` Timo Rothenpieler 2023-12-06 12:31 ` James Almer @ 2023-12-06 12:50 ` Ronald S. Bultje 2023-12-06 12:54 ` James Almer 2023-12-06 13:25 ` Martin Storsjö 2023-12-08 10:01 ` Andreas Rheinhardt 4 siblings, 1 reply; 29+ messages in thread From: Ronald S. Bultje @ 2023-12-06 12:50 UTC (permalink / raw) To: FFmpeg development discussions and patches; +Cc: Timo Rothenpieler Hi, On Sun, Dec 3, 2023 at 3:10 PM Timo Rothenpieler <timo@rothenpieler.org> wrote: > So if the compiler does a copy > in decode_cpe() with avx instructions, but ffmpeg is built with > --disable-avx > Ehm... What? That seems like the core bug then? Ronald _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-06 12:50 ` Ronald S. Bultje @ 2023-12-06 12:54 ` James Almer 0 siblings, 0 replies; 29+ messages in thread From: James Almer @ 2023-12-06 12:54 UTC (permalink / raw) To: ffmpeg-devel On 12/6/2023 9:50 AM, Ronald S. Bultje wrote: > Hi, > > On Sun, Dec 3, 2023 at 3:10 PM Timo Rothenpieler <timo@rothenpieler.org> > wrote: > >> So if the compiler does a copy >> in decode_cpe() with avx instructions, but ffmpeg is built with >> --disable-avx >> > > Ehm... What? That seems like the core bug then? --disable-avx will disable any ffmpeg specific AVX dsp function that depends on HAVE_AVX_EXTERNAL and similar, but will not do anything to the -march argument you can pass the compiler with the --cpu configure option. configure --cpu=haswell --disable-avx is completely valid. > > Ronald > _______________________________________________ > 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". _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-03 20:10 [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes Timo Rothenpieler ` (2 preceding siblings ...) 2023-12-06 12:50 ` Ronald S. Bultje @ 2023-12-06 13:25 ` Martin Storsjö 2023-12-06 13:27 ` Timo Rothenpieler 2023-12-08 0:15 ` Timo Rothenpieler 2023-12-08 10:01 ` Andreas Rheinhardt 4 siblings, 2 replies; 29+ messages in thread From: Martin Storsjö @ 2023-12-06 13:25 UTC (permalink / raw) To: FFmpeg development discussions and patches; +Cc: Timo Rothenpieler 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. For others who commented in this thread, it all boils down to something like this: struct MyData { uint8_t __attribute__((aligned(32))) aligned_data[1024]; }; void func(void) { struct MyData *obj = malloc(sizeof(*obj)); // Uusally only aligned to 8 bytes // operate on obj->aligned_data[] } Due to how aligned_data is declared, we promise to the compiler that it is aligned to 32 bytes, and that the compiler can assume this wherever. Depending on -march or whatever, this can be to access it with instructions that assume 32 byte alignment. // 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-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-06 13:25 ` Martin Storsjö @ 2023-12-06 13:27 ` Timo Rothenpieler 2023-12-06 13:29 ` Martin Storsjö 2023-12-08 0:15 ` Timo Rothenpieler 1 sibling, 1 reply; 29+ messages in thread From: Timo Rothenpieler @ 2023-12-06 13:27 UTC (permalink / raw) To: ffmpeg-devel 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. > > For others who commented in this thread, it all boils down to something > like this: > > struct MyData { > uint8_t __attribute__((aligned(32))) aligned_data[1024]; > }; It's even a bit more complex than that. The case that's crashing right now is a member that has no alignment declared on itself at all. But another member of the same struct does, and so the compiler assumes the whole struct to be aligned. > void func(void) { > struct MyData *obj = malloc(sizeof(*obj)); // Uusally only aligned > to 8 bytes > // operate on obj->aligned_data[] > } > > Due to how aligned_data is declared, we promise to the compiler that it > is aligned to 32 bytes, and that the compiler can assume this wherever. > Depending on -march or whatever, this can be to access it with > instructions that assume 32 byte alignment. > > // 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-request@ffmpeg.org with subject "unsubscribe". _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-06 13:27 ` Timo Rothenpieler @ 2023-12-06 13:29 ` Martin Storsjö 0 siblings, 0 replies; 29+ messages in thread From: Martin Storsjö @ 2023-12-06 13:29 UTC (permalink / raw) To: FFmpeg development discussions and patches On Wed, 6 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. >> >> For others who commented in this thread, it all boils down to something >> like this: >> >> struct MyData { >> uint8_t __attribute__((aligned(32))) aligned_data[1024]; >> }; > > It's even a bit more complex than that. > The case that's crashing right now is a member that has no alignment > declared on itself at all. > But another member of the same struct does, and so the compiler assumes > the whole struct to be aligned. Ah, tricky! Yeah, that's also a valid assumption for the compiler, but also a rather non-obvious one. // 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-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-06 13:25 ` Martin Storsjö 2023-12-06 13:27 ` Timo Rothenpieler @ 2023-12-08 0:15 ` Timo Rothenpieler 2023-12-08 5:57 ` Martin Storsjö 1 sibling, 1 reply; 29+ messages in thread From: Timo Rothenpieler @ 2023-12-08 0:15 UTC (permalink / raw) To: ffmpeg-devel 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. Will add that and push soon. I'll also check how far this will need backported. Likely to almost all versions ever. > For others who commented in this thread, it all boils down to something > like this: > > struct MyData { > uint8_t __attribute__((aligned(32))) aligned_data[1024]; > }; > > void func(void) { > struct MyData *obj = malloc(sizeof(*obj)); // Uusally only aligned > to 8 bytes > // operate on obj->aligned_data[] > } > > Due to how aligned_data is declared, we promise to the compiler that it > is aligned to 32 bytes, and that the compiler can assume this wherever. > Depending on -march or whatever, this can be to access it with > instructions that assume 32 byte alignment. > > // 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-request@ffmpeg.org with subject "unsubscribe". _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-08 0:15 ` Timo Rothenpieler @ 2023-12-08 5:57 ` Martin Storsjö 0 siblings, 0 replies; 29+ messages in thread From: Martin Storsjö @ 2023-12-08 5:57 UTC (permalink / raw) To: FFmpeg development discussions and patches 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-request@ffmpeg.org with subject "unsubscribe". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-03 20:10 [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes Timo Rothenpieler ` (3 preceding siblings ...) 2023-12-06 13:25 ` Martin Storsjö @ 2023-12-08 10:01 ` Andreas Rheinhardt 2023-12-08 17:56 ` Timo Rothenpieler 4 siblings, 1 reply; 29+ messages in thread From: Andreas Rheinhardt @ 2023-12-08 10:01 UTC (permalink / raw) To: ffmpeg-devel 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. 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. - 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-08 10:01 ` Andreas Rheinhardt @ 2023-12-08 17:56 ` Timo Rothenpieler 2023-12-08 18:11 ` Nicolas George 2023-12-09 5:23 ` Andreas Rheinhardt 0 siblings, 2 replies; 29+ messages in thread From: Timo Rothenpieler @ 2023-12-08 17:56 UTC (permalink / raw) To: ffmpeg-devel 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? 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. > 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. 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. > - 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". _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-08 17:56 ` Timo Rothenpieler @ 2023-12-08 18:11 ` Nicolas George 2023-12-09 5:23 ` Andreas Rheinhardt 1 sibling, 0 replies; 29+ messages in thread From: Nicolas George @ 2023-12-08 18:11 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 1481 bytes --] Timo Rothenpieler (12023-12-08): > Is mixing the libraries really a scenario we need to care about/support? No. We should merge all the libraries into a single libffmpeg.so. Having separate libraries brings us no end of hassle and drawbacks, starting with all the avpriv symbols and backward compatibility layers, and the benefits it brings could be reached in simpler and more efficient ways. But anytime I brought it up, the same naysayers would object, but when I ask what precise benefit they think the current situation brings, with the intent of explaining how it can be done better differently (I do not bring that half-backed, I have thought about it beforehand) or in some case explaining that no, this is not a benefit because linking does not work like that. And then the naysayers would whine that I am making too much a fuss. Barring merging all libraries into a single libffmpeg.so, have configure compute AV_LIBRARY_SIGNATURE as a 64 bits hash of the version and configuration, then have in version.h of each library: #define avsmth_version_check_signature() \ avsmth_version_check_signature_ ## AV_LIBRARY_SIGNATURE() then have avsmth_version_check_signature() in each library call the ones in the library it depends on, and core functions like *register_all() call it. Then if the libraries are mixed at run time it will produce an error message by the linker that can be searched on the web. Regards, -- Nicolas George [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-08 17:56 ` Timo Rothenpieler 2023-12-08 18:11 ` Nicolas George @ 2023-12-09 5:23 ` Andreas Rheinhardt 2024-01-12 23:10 ` Timo Rothenpieler 1 sibling, 1 reply; 29+ messages in thread From: Andreas Rheinhardt @ 2023-12-09 5:23 UTC (permalink / raw) To: ffmpeg-devel 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes 2023-12-09 5:23 ` Andreas Rheinhardt @ 2024-01-12 23:10 ` Timo Rothenpieler 2024-01-13 0:57 ` [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simg align Timo Rothenpieler 0 siblings, 1 reply; 29+ messages in thread From: Timo Rothenpieler @ 2024-01-12 23:10 UTC (permalink / raw) To: ffmpeg-devel 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simg align 2024-01-12 23:10 ` Timo Rothenpieler @ 2024-01-13 0:57 ` Timo Rothenpieler 2024-01-13 1:00 ` Timo Rothenpieler ` (2 more replies) 0 siblings, 3 replies; 29+ messages in thread From: Timo Rothenpieler @ 2024-01-13 0:57 UTC (permalink / raw) To: ffmpeg-devel; +Cc: 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. 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 | 2 +- libavutil/mem_internal.h | 20 +++++++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/libavutil/mem.c b/libavutil/mem.c index 36b8940a0c..62163b4cb3 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_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) /* NOTE: if you want to override these functions with your own * implementations (not recommended) you have to link libav* as diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h index 2448c606f1..ddd3c24806 100644 --- a/libavutil/mem_internal.h +++ b/libavutil/mem_internal.h @@ -75,22 +75,24 @@ * @param v Name of the variable */ +#define MAX_ALIGNMENT (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) + #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C) - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v - #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v - #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (n))) v + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v + #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v + #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v #elif defined(__DJGPP__) #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v #elif defined(__GNUC__) || defined(__clang__) - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v - #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (n))) v - #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (n))) v + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v + #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v + #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v #elif defined(_MSC_VER) - #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v - #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(n)) t v - #define DECLARE_ASM_CONST(n,t,v) __declspec(align(n)) static const t v + #define DECLARE_ALIGNED(n,t,v) __declspec(align(FFMIN(n, MAX_ALIGNMENT))) t v + #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(FFMIN(n, MAX_ALIGNMENT))) t v + #define DECLARE_ASM_CONST(n,t,v) __declspec(align(FFMIN(n, MAX_ALIGNMENT))) static const t v #else #define DECLARE_ALIGNED(n,t,v) t v #define DECLARE_ASM_ALIGNED(n,t,v) t v -- 2.34.1 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simg align 2024-01-13 0:57 ` [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simg align Timo Rothenpieler @ 2024-01-13 1:00 ` Timo Rothenpieler 2024-01-13 15:24 ` Timo Rothenpieler 2024-01-13 15:46 ` [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align Timo Rothenpieler 2 siblings, 0 replies; 29+ messages in thread From: Timo Rothenpieler @ 2024-01-13 1:00 UTC (permalink / raw) To: ffmpeg-devel On 13.01.2024 01:57, 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 | 2 +- > libavutil/mem_internal.h | 20 +++++++++++--------- > 2 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/libavutil/mem.c b/libavutil/mem.c > index 36b8940a0c..62163b4cb3 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_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) > > /* NOTE: if you want to override these functions with your own > * implementations (not recommended) you have to link libav* as > diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h > index 2448c606f1..ddd3c24806 100644 > --- a/libavutil/mem_internal.h > +++ b/libavutil/mem_internal.h > @@ -75,22 +75,24 @@ > * @param v Name of the variable > */ > > +#define MAX_ALIGNMENT (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) The issue with this approach is that code that previously allowed the compiler to optimize it heavily will now only be optimized if ffmpeg was built without disabling avx. Probably a fair tradeoff though, since that's a very niche case. I also did not test this with MSVC or ICC, so I have no idea if they allow FFMIN in the middle of an alignment attribute. _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simg align 2024-01-13 0:57 ` [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simg align Timo Rothenpieler 2024-01-13 1:00 ` Timo Rothenpieler @ 2024-01-13 15:24 ` Timo Rothenpieler 2024-01-13 15:46 ` [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align Timo Rothenpieler 2 siblings, 0 replies; 29+ messages in thread From: Timo Rothenpieler @ 2024-01-13 15:24 UTC (permalink / raw) To: ffmpeg-devel On 13.01.2024 01:57, 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 | 2 +- > libavutil/mem_internal.h | 20 +++++++++++--------- > 2 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/libavutil/mem.c b/libavutil/mem.c > index 36b8940a0c..62163b4cb3 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_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) > > /* NOTE: if you want to override these functions with your own > * implementations (not recommended) you have to link libav* as > diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h > index 2448c606f1..ddd3c24806 100644 > --- a/libavutil/mem_internal.h > +++ b/libavutil/mem_internal.h > @@ -75,22 +75,24 @@ > * @param v Name of the variable > */ > > +#define MAX_ALIGNMENT (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) > + > #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C) > - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (n))) v > + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > #elif defined(__DJGPP__) > #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v > #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v > #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v > #elif defined(__GNUC__) || defined(__clang__) > - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (n))) v > + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > #elif defined(_MSC_VER) > - #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v > - #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(n)) t v > - #define DECLARE_ASM_CONST(n,t,v) __declspec(align(n)) static const t v > + #define DECLARE_ALIGNED(n,t,v) __declspec(align(FFMIN(n, MAX_ALIGNMENT))) t v > + #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(FFMIN(n, MAX_ALIGNMENT))) t v > + #define DECLARE_ASM_CONST(n,t,v) __declspec(align(FFMIN(n, MAX_ALIGNMENT))) static const t v Just checked, this does in fact not work with msvc: libavfilter/af_arnndn.c(122): error C2059: Syntaxfehler: "(" So I guess for MSVC, the alignment will always have to be the full 32 or 64. > #else > #define DECLARE_ALIGNED(n,t,v) t v > #define DECLARE_ASM_ALIGNED(n,t,v) t v _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align 2024-01-13 0:57 ` [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simg align Timo Rothenpieler 2024-01-13 1:00 ` Timo Rothenpieler 2024-01-13 15:24 ` Timo Rothenpieler @ 2024-01-13 15:46 ` Timo Rothenpieler 2024-02-09 19:22 ` Timo Rothenpieler 2024-02-11 14:00 ` Andreas Rheinhardt 2 siblings, 2 replies; 29+ messages in thread From: Timo Rothenpieler @ 2024-01-13 15:46 UTC (permalink / raw) To: ffmpeg-devel; +Cc: 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. 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) +#else +#define ALIGN (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) +#endif /* NOTE: if you want to override these functions with your own * implementations (not recommended) you have to link libav* as diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h index 2448c606f1..e2911b5610 100644 --- a/libavutil/mem_internal.h +++ b/libavutil/mem_internal.h @@ -75,18 +75,20 @@ * @param v Name of the variable */ +#define MAX_ALIGNMENT (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) + #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C) - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v - #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v - #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (n))) v + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v + #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v + #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v #elif defined(__DJGPP__) #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v #elif defined(__GNUC__) || defined(__clang__) - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v - #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (n))) v - #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (n))) v + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v + #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v + #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v #elif defined(_MSC_VER) #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(n)) t v -- 2.34.1 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align 2024-01-13 15:46 ` [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align Timo Rothenpieler @ 2024-02-09 19:22 ` Timo Rothenpieler 2024-02-11 14:05 ` Sam James 2024-02-11 14:22 ` Rémi Denis-Courmont 2024-02-11 14:00 ` Andreas Rheinhardt 1 sibling, 2 replies; 29+ messages in thread From: Timo Rothenpieler @ 2024-02-09 19:22 UTC (permalink / raw) To: ffmpeg-devel 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) > +#else > +#define ALIGN (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) > +#endif > > /* NOTE: if you want to override these functions with your own > * implementations (not recommended) you have to link libav* as > diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h > index 2448c606f1..e2911b5610 100644 > --- a/libavutil/mem_internal.h > +++ b/libavutil/mem_internal.h > @@ -75,18 +75,20 @@ > * @param v Name of the variable > */ > > +#define MAX_ALIGNMENT (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) > + > #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C) > - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (n))) v > + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > #elif defined(__DJGPP__) > #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v > #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v > #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v > #elif defined(__GNUC__) || defined(__clang__) > - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (n))) v > + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > #elif defined(_MSC_VER) > #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v > #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(n)) t v ping We really should fix this before 7.0 (and probably also backport it, since UB is UB). I'm fine with whatever approach, as long as the UB is gone. _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align 2024-02-09 19:22 ` Timo Rothenpieler @ 2024-02-11 14:05 ` Sam James 2024-02-11 14:22 ` Rémi Denis-Courmont 1 sibling, 0 replies; 29+ messages in thread From: Sam James @ 2024-02-11 14:05 UTC (permalink / raw) To: FFmpeg development discussions and patches Timo Rothenpieler <timo@rothenpieler.org> writes: > 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) >> +#else >> +#define ALIGN (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) >> +#endif >> /* NOTE: if you want to override these functions with your own >> * implementations (not recommended) you have to link libav* as >> diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h >> index 2448c606f1..e2911b5610 100644 >> --- a/libavutil/mem_internal.h >> +++ b/libavutil/mem_internal.h >> @@ -75,18 +75,20 @@ >> * @param v Name of the variable >> */ >> +#define MAX_ALIGNMENT (HAVE_SIMD_ALIGN_64 ? 64 : >> (HAVE_SIMD_ALIGN_32 ? 32 : 16)) >> + >> #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C) >> - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v >> - #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v >> - #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (n))) v >> + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> + #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> + #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> #elif defined(__DJGPP__) >> #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v >> #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v >> #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v >> #elif defined(__GNUC__) || defined(__clang__) >> - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v >> - #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (n))) v >> - #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (n))) v >> + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> + #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> + #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> #elif defined(_MSC_VER) >> #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v >> #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(n)) t v > > ping > > We really should fix this before 7.0 (and probably also backport it, > since UB is UB). > > I'm fine with whatever approach, as long as the UB is gone. Yes please, we keep getting users hitting this. (There's a packaging improvement we can make which Timo has suggested and I need to implement, but the issue is there nonetheless.) _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align 2024-02-09 19:22 ` Timo Rothenpieler 2024-02-11 14:05 ` Sam James @ 2024-02-11 14:22 ` Rémi Denis-Courmont 2024-02-11 15:47 ` Timo Rothenpieler 1 sibling, 1 reply; 29+ messages in thread From: Rémi Denis-Courmont @ 2024-02-11 14:22 UTC (permalink / raw) To: ffmpeg-devel 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align 2024-02-11 14:22 ` Rémi Denis-Courmont @ 2024-02-11 15:47 ` Timo Rothenpieler 0 siblings, 0 replies; 29+ messages in thread From: Timo Rothenpieler @ 2024-02-11 15:47 UTC (permalink / raw) To: ffmpeg-devel On 11.02.2024 15:22, Rémi Denis-Courmont wrote: > 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). > MSVC straight up _does not support_ putting conditionals into its alignment macros. It initially had the same treatment, but failed with compile errors. _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align 2024-01-13 15:46 ` [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align Timo Rothenpieler 2024-02-09 19:22 ` Timo Rothenpieler @ 2024-02-11 14:00 ` Andreas Rheinhardt 2024-02-11 16:06 ` Timo Rothenpieler 2024-02-11 17:40 ` [FFmpeg-devel] [PATCH] " Timo Rothenpieler 1 sibling, 2 replies; 29+ messages in thread From: Andreas Rheinhardt @ 2024-02-11 14:00 UTC (permalink / raw) To: ffmpeg-devel 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. > > 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) > +#else > +#define ALIGN (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) > +#endif > > /* NOTE: if you want to override these functions with your own > * implementations (not recommended) you have to link libav* as > diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h > index 2448c606f1..e2911b5610 100644 > --- a/libavutil/mem_internal.h > +++ b/libavutil/mem_internal.h > @@ -75,18 +75,20 @@ > * @param v Name of the variable > */ > > +#define MAX_ALIGNMENT (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) > + > #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C) > - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (n))) v > + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > #elif defined(__DJGPP__) > #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v > #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v > #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v > #elif defined(__GNUC__) || defined(__clang__) > - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (n))) v > - #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (n))) v > + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > + #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v > #elif defined(_MSC_VER) > #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v > #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(n)) t v We use alignment for three different usecases: a) Variables on the stack; b) variables in structs and c) static data. If we limit alignment, we should only limit it for b). But unfortunately they use the same macro as c), so someone would need to untangle this by adding new macros. In the meantime, your original patch seems like the way to go. - Andreas One can probably make MSVC happy by avoiding FFMIN like this: #if HAVE_SIMD_ALIGN_32 #define ALIGN_32 32 #else #define ALIGN_32 16 #endif #define DECLARE_VAR_ALIGNED_32(t, v) DECLARE_ALIGNED(ALIGN_32, t, v) _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align 2024-02-11 14:00 ` Andreas Rheinhardt @ 2024-02-11 16:06 ` Timo Rothenpieler 2024-02-11 17:40 ` [FFmpeg-devel] [PATCH] " Timo Rothenpieler 1 sibling, 0 replies; 29+ messages in thread From: Timo Rothenpieler @ 2024-02-11 16:06 UTC (permalink / raw) To: ffmpeg-devel On 11.02.2024 15:00, 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. >> >> 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) >> +#else >> +#define ALIGN (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) >> +#endif >> >> /* NOTE: if you want to override these functions with your own >> * implementations (not recommended) you have to link libav* as >> diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h >> index 2448c606f1..e2911b5610 100644 >> --- a/libavutil/mem_internal.h >> +++ b/libavutil/mem_internal.h >> @@ -75,18 +75,20 @@ >> * @param v Name of the variable >> */ >> >> +#define MAX_ALIGNMENT (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) >> + >> #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C) >> - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v >> - #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v >> - #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (n))) v >> + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> + #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> + #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> #elif defined(__DJGPP__) >> #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v >> #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v >> #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v >> #elif defined(__GNUC__) || defined(__clang__) >> - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v >> - #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (n))) v >> - #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (n))) v >> + #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> + #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> + #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v >> #elif defined(_MSC_VER) >> #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v >> #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(n)) t v > > We use alignment for three different usecases: a) Variables on the > stack; b) variables in structs and c) static data. If we limit > alignment, we should only limit it for b). But unfortunately they use > the same macro as c), so someone would need to untangle this by adding > new macros. In the meantime, your original patch seems like the way to go. Is it really such an issue to limit the alignment to less than some of those request, if there are no SIMD instructions would would ever need a higher alignment on that platform? I can't think of many situations where you'd need alignment other than SIMD, outside of crazy page alignment stuff, for which 32/64 bytes are far from enough anyway. If there's no further objections, I'll push a simple bump to 32 bytes, as per the original patch now. And then we can figure out how to make it a bit nicer. Cause as it is now, it does unneccesarily force double the alignment size to a whole bunch of arches. > - Andreas > > One can probably make MSVC happy by avoiding FFMIN like this: > #if HAVE_SIMD_ALIGN_32 > #define ALIGN_32 32 > #else > #define ALIGN_32 16 > #endif > #define DECLARE_VAR_ALIGNED_32(t, v) DECLARE_ALIGNED(ALIGN_32, t, v) _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simd align 2024-02-11 14:00 ` Andreas Rheinhardt 2024-02-11 16:06 ` Timo Rothenpieler @ 2024-02-11 17:40 ` Timo Rothenpieler 2024-02-26 16:58 ` Timo Rothenpieler 1 sibling, 1 reply; 29+ messages in thread From: Timo Rothenpieler @ 2024-02-11 17:40 UTC (permalink / raw) To: ffmpeg-devel; +Cc: 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. 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 | 2 +- libavutil/mem_internal.h | 33 ++++++++++++++++++++++++++++----- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/libavutil/mem.c b/libavutil/mem.c index 36b8940a0c..62163b4cb3 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_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) /* NOTE: if you want to override these functions with your own * implementations (not recommended) you have to link libav* as diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h index 2448c606f1..b1d89a0605 100644 --- a/libavutil/mem_internal.h +++ b/libavutil/mem_internal.h @@ -76,27 +76,50 @@ */ #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C) - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v + #define DECLARE_ALIGNED_T(n,t,v) t __attribute__ ((aligned (n))) v #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (n))) v #elif defined(__DJGPP__) - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v + #define DECLARE_ALIGNED_T(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v #elif defined(__GNUC__) || defined(__clang__) - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v + #define DECLARE_ALIGNED_T(n,t,v) t __attribute__ ((aligned (n))) v #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (n))) v #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (n))) v #elif defined(_MSC_VER) - #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v + #define DECLARE_ALIGNED_T(n,t,v) __declspec(align(n)) t v #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(n)) t v #define DECLARE_ASM_CONST(n,t,v) __declspec(align(n)) static const t v #else - #define DECLARE_ALIGNED(n,t,v) t v + #define DECLARE_ALIGNED_T(n,t,v) t v #define DECLARE_ASM_ALIGNED(n,t,v) t v #define DECLARE_ASM_CONST(n,t,v) static const t v #endif +#if HAVE_SIMD_ALIGN_64 + #define ALIGN_64 64 + #define ALIGN_32 32 +#elif HAVE_SIMD_ALIGN_32 + #define ALIGN_64 32 + #define ALIGN_32 32 +#else + #define ALIGN_64 16 + #define ALIGN_32 16 +#endif + +#define DECLARE_ALIGNED(n,t,v) DECLARE_ALIGNED_V(n,t,v) + +// Macro needs to be double-wrapped in order to expand +// possible other macros being passed for n. +#define DECLARE_ALIGNED_V(n,t,v) DECLARE_ALIGNED_##n(t,v) + +#define DECLARE_ALIGNED_4(t,v) DECLARE_ALIGNED_T( 4, t, v) +#define DECLARE_ALIGNED_8(t,v) DECLARE_ALIGNED_T( 8, t, v) +#define DECLARE_ALIGNED_16(t,v) DECLARE_ALIGNED_T( 16, t, v) +#define DECLARE_ALIGNED_32(t,v) DECLARE_ALIGNED_T(ALIGN_32, t, v) +#define DECLARE_ALIGNED_64(t,v) DECLARE_ALIGNED_T(ALIGN_64, t, v) + // Some broken preprocessors need a second expansion // to be forced to tokenize __VA_ARGS__ #define E1(x) x -- 2.34.1 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simd align 2024-02-11 17:40 ` [FFmpeg-devel] [PATCH] " Timo Rothenpieler @ 2024-02-26 16:58 ` Timo Rothenpieler 2024-02-27 18:45 ` Timo Rothenpieler 0 siblings, 1 reply; 29+ messages in thread From: Timo Rothenpieler @ 2024-02-26 16:58 UTC (permalink / raw) To: ffmpeg-devel On 11/02/2024 18:40, 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 | 2 +- > libavutil/mem_internal.h | 33 ++++++++++++++++++++++++++++----- > 2 files changed, 29 insertions(+), 6 deletions(-) > > diff --git a/libavutil/mem.c b/libavutil/mem.c > index 36b8940a0c..62163b4cb3 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_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) > > /* NOTE: if you want to override these functions with your own > * implementations (not recommended) you have to link libav* as > diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h > index 2448c606f1..b1d89a0605 100644 > --- a/libavutil/mem_internal.h > +++ b/libavutil/mem_internal.h > @@ -76,27 +76,50 @@ > */ > > #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C) > - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > + #define DECLARE_ALIGNED_T(n,t,v) t __attribute__ ((aligned (n))) v > #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ ((aligned (n))) v > #elif defined(__DJGPP__) > - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v > + #define DECLARE_ALIGNED_T(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v > #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v > #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v > #elif defined(__GNUC__) || defined(__clang__) > - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v > + #define DECLARE_ALIGNED_T(n,t,v) t __attribute__ ((aligned (n))) v > #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (n))) v > #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (n))) v > #elif defined(_MSC_VER) > - #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v > + #define DECLARE_ALIGNED_T(n,t,v) __declspec(align(n)) t v > #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(n)) t v > #define DECLARE_ASM_CONST(n,t,v) __declspec(align(n)) static const t v > #else > - #define DECLARE_ALIGNED(n,t,v) t v > + #define DECLARE_ALIGNED_T(n,t,v) t v > #define DECLARE_ASM_ALIGNED(n,t,v) t v > #define DECLARE_ASM_CONST(n,t,v) static const t v > #endif > > +#if HAVE_SIMD_ALIGN_64 > + #define ALIGN_64 64 > + #define ALIGN_32 32 > +#elif HAVE_SIMD_ALIGN_32 > + #define ALIGN_64 32 > + #define ALIGN_32 32 > +#else > + #define ALIGN_64 16 > + #define ALIGN_32 16 > +#endif > + > +#define DECLARE_ALIGNED(n,t,v) DECLARE_ALIGNED_V(n,t,v) > + > +// Macro needs to be double-wrapped in order to expand > +// possible other macros being passed for n. > +#define DECLARE_ALIGNED_V(n,t,v) DECLARE_ALIGNED_##n(t,v) > + > +#define DECLARE_ALIGNED_4(t,v) DECLARE_ALIGNED_T( 4, t, v) > +#define DECLARE_ALIGNED_8(t,v) DECLARE_ALIGNED_T( 8, t, v) > +#define DECLARE_ALIGNED_16(t,v) DECLARE_ALIGNED_T( 16, t, v) > +#define DECLARE_ALIGNED_32(t,v) DECLARE_ALIGNED_T(ALIGN_32, t, v) > +#define DECLARE_ALIGNED_64(t,v) DECLARE_ALIGNED_T(ALIGN_64, t, v) > + > // Some broken preprocessors need a second expansion > // to be forced to tokenize __VA_ARGS__ > #define E1(x) x I intend to push this patch soon. _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simd align 2024-02-26 16:58 ` Timo Rothenpieler @ 2024-02-27 18:45 ` Timo Rothenpieler 0 siblings, 0 replies; 29+ messages in thread From: Timo Rothenpieler @ 2024-02-27 18:45 UTC (permalink / raw) To: ffmpeg-devel On 26.02.2024 17:58, Timo Rothenpieler wrote: > On 11/02/2024 18:40, 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 | 2 +- >> libavutil/mem_internal.h | 33 ++++++++++++++++++++++++++++----- >> 2 files changed, 29 insertions(+), 6 deletions(-) >> >> diff --git a/libavutil/mem.c b/libavutil/mem.c >> index 36b8940a0c..62163b4cb3 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_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16)) >> /* NOTE: if you want to override these functions with your own >> * implementations (not recommended) you have to link libav* as >> diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h >> index 2448c606f1..b1d89a0605 100644 >> --- a/libavutil/mem_internal.h >> +++ b/libavutil/mem_internal.h >> @@ -76,27 +76,50 @@ >> */ >> #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || >> defined(__SUNPRO_C) >> - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned >> (n))) v >> + #define DECLARE_ALIGNED_T(n,t,v) t __attribute__ ((aligned >> (n))) v >> #define DECLARE_ASM_ALIGNED(n,t,v) t __attribute__ ((aligned >> (n))) v >> #define DECLARE_ASM_CONST(n,t,v) const t __attribute__ >> ((aligned (n))) v >> #elif defined(__DJGPP__) >> - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned >> (FFMIN(n, 16)))) v >> + #define DECLARE_ALIGNED_T(n,t,v) t __attribute__ ((aligned >> (FFMIN(n, 16)))) v >> #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ >> ((aligned (FFMIN(n, 16)))) v >> #define DECLARE_ASM_CONST(n,t,v) static const t av_used >> __attribute__ ((aligned (FFMIN(n, 16)))) v >> #elif defined(__GNUC__) || defined(__clang__) >> - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned >> (n))) v >> + #define DECLARE_ALIGNED_T(n,t,v) t __attribute__ ((aligned >> (n))) v >> #define DECLARE_ASM_ALIGNED(n,t,v) t av_used __attribute__ >> ((aligned (n))) v >> #define DECLARE_ASM_CONST(n,t,v) static const t av_used >> __attribute__ ((aligned (n))) v >> #elif defined(_MSC_VER) >> - #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v >> + #define DECLARE_ALIGNED_T(n,t,v) __declspec(align(n)) t v >> #define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(n)) t v >> #define DECLARE_ASM_CONST(n,t,v) __declspec(align(n)) static >> const t v >> #else >> - #define DECLARE_ALIGNED(n,t,v) t v >> + #define DECLARE_ALIGNED_T(n,t,v) t v >> #define DECLARE_ASM_ALIGNED(n,t,v) t v >> #define DECLARE_ASM_CONST(n,t,v) static const t v >> #endif >> +#if HAVE_SIMD_ALIGN_64 >> + #define ALIGN_64 64 >> + #define ALIGN_32 32 >> +#elif HAVE_SIMD_ALIGN_32 >> + #define ALIGN_64 32 >> + #define ALIGN_32 32 >> +#else >> + #define ALIGN_64 16 >> + #define ALIGN_32 16 >> +#endif >> + >> +#define DECLARE_ALIGNED(n,t,v) DECLARE_ALIGNED_V(n,t,v) >> + >> +// Macro needs to be double-wrapped in order to expand >> +// possible other macros being passed for n. >> +#define DECLARE_ALIGNED_V(n,t,v) DECLARE_ALIGNED_##n(t,v) >> + >> +#define DECLARE_ALIGNED_4(t,v) DECLARE_ALIGNED_T( 4, t, v) >> +#define DECLARE_ALIGNED_8(t,v) DECLARE_ALIGNED_T( 8, t, v) >> +#define DECLARE_ALIGNED_16(t,v) DECLARE_ALIGNED_T( 16, t, v) >> +#define DECLARE_ALIGNED_32(t,v) DECLARE_ALIGNED_T(ALIGN_32, t, v) >> +#define DECLARE_ALIGNED_64(t,v) DECLARE_ALIGNED_T(ALIGN_64, t, v) >> + >> // Some broken preprocessors need a second expansion >> // to be forced to tokenize __VA_ARGS__ >> #define E1(x) x > > I intend to push this patch soon. applied _______________________________________________ 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". ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2024-02-27 18:45 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-12-03 20:10 [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes Timo Rothenpieler 2023-12-06 12:27 ` Timo Rothenpieler 2023-12-06 12:31 ` James Almer 2023-12-06 12:56 ` Timo Rothenpieler 2023-12-06 12:50 ` Ronald S. Bultje 2023-12-06 12:54 ` James Almer 2023-12-06 13:25 ` Martin Storsjö 2023-12-06 13:27 ` Timo Rothenpieler 2023-12-06 13:29 ` Martin Storsjö 2023-12-08 0:15 ` Timo Rothenpieler 2023-12-08 5:57 ` Martin Storsjö 2023-12-08 10:01 ` Andreas Rheinhardt 2023-12-08 17:56 ` Timo Rothenpieler 2023-12-08 18:11 ` Nicolas George 2023-12-09 5:23 ` Andreas Rheinhardt 2024-01-12 23:10 ` Timo Rothenpieler 2024-01-13 0:57 ` [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simg align Timo Rothenpieler 2024-01-13 1:00 ` Timo Rothenpieler 2024-01-13 15:24 ` Timo Rothenpieler 2024-01-13 15:46 ` [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align Timo Rothenpieler 2024-02-09 19:22 ` Timo Rothenpieler 2024-02-11 14:05 ` Sam James 2024-02-11 14:22 ` Rémi Denis-Courmont 2024-02-11 15:47 ` Timo Rothenpieler 2024-02-11 14:00 ` Andreas Rheinhardt 2024-02-11 16:06 ` Timo Rothenpieler 2024-02-11 17:40 ` [FFmpeg-devel] [PATCH] " Timo Rothenpieler 2024-02-26 16:58 ` Timo Rothenpieler 2024-02-27 18:45 ` Timo Rothenpieler
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel This inbox may be cloned and mirrored by anyone: git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \ ffmpegdev@gitmailbox.com public-inbox-index ffmpegdev Example config snippet for mirrors. AGPL code for this site: git clone https://public-inbox.org/public-inbox.git