[FFmpeg-devel] tx_float_neon: Do not access outside stack.

Use load/store instructions that modify sp to save
registers to stack, like it is done for all other
functions.
At least valgrind complains about the current code.
---
 libavutil/aarch64/tx_float_neon.S | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Le sunnuntaina 9. lokakuuta 2022, 16.14.47 EEST Reimar Döffinger a écrit :
> Use load/store instructions that modify sp to save
> registers to stack, like it is done for all other
> functions.
> At least valgrind complains about the current code.
> ---
>  libavutil/aarch64/tx_float_neon.S | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/libavutil/aarch64/tx_float_neon.S
> b/libavutil/aarch64/tx_float_neon.S index 4126c3b812..4be93cc963 100644
> --- a/libavutil/aarch64/tx_float_neon.S
> +++ b/libavutil/aarch64/tx_float_neon.S
> @@ -866,10 +866,10 @@ FFT16_FN ns_float, 1
> 
>  .macro FFT32_FN name, no_perm
>  function ff_tx_fft32_\name\()_neon, export=1
> -        stp             d8,  d9,  [sp, #-16]
> -        stp             d10, d11, [sp, #-32]
> -        stp             d12, d13, [sp, #-48]
> -        stp             d14, d15, [sp, #-64]
> +        stp             d8,  d9,  [sp, #-16]!
> +        stp             d10, d11, [sp, #-16]!
> +        stp             d12, d13, [sp, #-16]!
> +        stp             d14, d15, [sp, #-16]!

While this fixes the ABI violation, it introduces multiple data dependencies on 
stack pointer due to write-back.

The idiomatic way to do this is to allocate the entire needed stack space in 
the first store / last load, and use positive offsets elsewhence.

> 
>          LOAD_SUBADD
>          SETUP_SR_RECOMB 32, x7, x8, x9
> @@ -911,10 +911,10 @@ function ff_tx_fft32_\name\()_neon, export=1
>          zip2            v31.2d, v11.2d, v15.2d
>          st1             { v28.4s, v29.4s, v30.4s, v31.4s }, [x1]
> 
> -        ldp             d14, d15, [sp, #-64]
> -        ldp             d12, d13, [sp, #-48]
> -        ldp             d10, d11, [sp, #-32]
> -        ldp             d8,  d9,  [sp, #-16]
> +        ldp             d14, d15, [sp], #16
> +        ldp             d12, d13, [sp], #16
> +        ldp             d10, d11, [sp], #16
> +        ldp             d8,  d9,  [sp], #16
> 
>          ret
>  endfunc

> On 9 Oct 2022, at 16:11, Rémi Denis-Courmont <remi@remlab.net> wrote:
> 
> Le sunnuntaina 9. lokakuuta 2022, 16.14.47 EEST Reimar Döffinger a écrit :
>> Use load/store instructions that modify sp to save
>> registers to stack, like it is done for all other
>> functions.
>> At least valgrind complains about the current code.
>> ---
>> libavutil/aarch64/tx_float_neon.S | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/libavutil/aarch64/tx_float_neon.S
>> b/libavutil/aarch64/tx_float_neon.S index 4126c3b812..4be93cc963 100644
>> --- a/libavutil/aarch64/tx_float_neon.S
>> +++ b/libavutil/aarch64/tx_float_neon.S
>> @@ -866,10 +866,10 @@ FFT16_FN ns_float, 1
>> 
>> .macro FFT32_FN name, no_perm
>> function ff_tx_fft32_\name\()_neon, export=1
>> -        stp             d8,  d9,  [sp, #-16]
>> -        stp             d10, d11, [sp, #-32]
>> -        stp             d12, d13, [sp, #-48]
>> -        stp             d14, d15, [sp, #-64]
>> +        stp             d8,  d9,  [sp, #-16]!
>> +        stp             d10, d11, [sp, #-16]!
>> +        stp             d12, d13, [sp, #-16]!
>> +        stp             d14, d15, [sp, #-16]!
> 
> While this fixes the ABI violation, it introduces multiple data dependencies on 
> stack pointer due to write-back.

That is true in principle, this is not done consistently at all.

> The idiomatic way to do this is to allocate the entire needed stack space in 
> the first store / last load, and use positive offsets elsewhence.

Are you sure this is really relevant at all, considering it's so rarely done in the code?
I'm fine looking into doing that consistently if that's what it takes, but I don't think it's good to make this the only function in the file (if not the whole lib) that does it this way.
If we want the idiomatic way to be used, we should
update all the code so people get the right idea.
(I'll note that some places might be tricky, there seems to be code that updates sp back and forth in a loop which looks like it might be for no reason, or there is a weird reason - check SR_TRANSFORM_DEF in the same file).

Just to clarify the details, then end result should look like this?
-        stp             d8,  d9,  [sp, #-16]
-        stp             d10, d11, [sp, #-32]
-        stp             d12, d13, [sp, #-48]
-        stp             d14, d15, [sp, #-64]
+        stp             d14, d15, [sp, #-16*4]!
+        stp             d8,  d9,  [sp, #16*3]
+        stp             d10, d11, [sp, #16*2]
+        stp             d12, d13, [sp, #16]

Or something slightly different?

Best regards,
Reimar

Le sunnuntaina 9. lokakuuta 2022, 19.36.24 EEST Reimar Döffinger a écrit :
> > While this fixes the ABI violation, it introduces multiple data
> > dependencies on stack pointer due to write-back.
> 
> That is true in principle, this is not done consistently at all.

I have not checked the FFmpeg code base, but this *is* done consistently by 
compilers and in a number of other assembler-heavy project, notably the Linux 
kernel (see especially arch/arm64/crypto/*.S).

> > The idiomatic way to do this is to allocate the entire needed stack space
> > in the first store / last load, and use positive offsets elsewhence.
> 
> Are you sure this is really relevant at all, considering it's so rarely done
> in the code?

I don't know what you base that statement on. I have rarely seen it not done 
that way. And even then, it mostly occurs inside loops, where it cannot fully 
be avoided and where there will be a couple of other instructions that don't 
suffer the data dependency.

> Just to clarify the details, then end result should look like this?
> -        stp             d8,  d9,  [sp, #-16]
> -        stp             d10, d11, [sp, #-32]
> -        stp             d12, d13, [sp, #-48]
> -        stp             d14, d15, [sp, #-64]
> +        stp             d14, d15, [sp, #-16*4]!
> +        stp             d8,  d9,  [sp, #16*3]
> +        stp             d10, d11, [sp, #16*2]
> +        stp             d12, d13, [sp, #16]

Yes.