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

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

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

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

[Timo Rothenpieler]

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

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

[James Almer]

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

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

[Ronald S. Bultje]

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

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

[James Almer]

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

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

[Timo Rothenpieler]

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

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

[Martin Storsjö]

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

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

[Timo Rothenpieler]

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

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

[Martin Storsjö]

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

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

[Timo Rothenpieler]

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

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

[Martin Storsjö]

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

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

[Andreas Rheinhardt]

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

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

[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?

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

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

[Nicolas George]

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

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

[Andreas Rheinhardt]

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

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

[Timo Rothenpieler]

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

[FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simg align

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

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

[Timo Rothenpieler]

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

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

[Timo Rothenpieler]

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

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

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

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

[Timo Rothenpieler]

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

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

[Andreas Rheinhardt]

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

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

[Sam James]

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

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

[Rémi Denis-Courmont]

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

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

[Timo Rothenpieler]

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

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

[Timo Rothenpieler]

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

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

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

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

[Timo Rothenpieler]

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

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

[Timo Rothenpieler]

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