Message ID | 20221009131447.5001-1-Reimar.Doeffinger@gmx.de |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] tx_float_neon: Do not access outside stack. | expand |
Context | Check | Description |
---|---|---|
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | success | Make fate finished |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
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.
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]! 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