x86: h264: Don't keep data in the redzone across function calls on 64 bit unix

diff mbox

We know that the called function (ff_chroma_inter_body_mmxext)
doesn't touch the redzone, and thus will be kept intact - thus,
this doesn't fix any bug per se.

However, valgrind's memcheck tool intentionally assumes that the
redzone is clobbered on every function call and function return
(see a long comment in valgrind/memcheck/mc_main.c). This avoids
false positives in that tool, at the cost of an extra stack pointer
adjustment.

The other alternative would be a valgrind suppression for this issue,
but that's an extra burden for everybody that wants to run libavcodec
within valgrind.
---
 libavcodec/x86/h264_deblock.asm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

On 08/06/14 00:55, Martin Storsjö wrote:
> However, valgrind's memcheck tool intentionally assumes that the
> redzone is clobbered on every function call and function return
> (see a long comment in valgrind/memcheck/mc_main.c). This avoids
> false positives in that tool, at the cost of an extra stack pointer
> adjustment.

I'm fine with that, people reporting the issue to valgrin by providing a
patch would be welcome but I doubt anybody has time.

lu

On Sun, 8 Jun 2014, Luca Barbato wrote:

> On 08/06/14 00:55, Martin Storsjö wrote:
>> However, valgrind's memcheck tool intentionally assumes that the
>> redzone is clobbered on every function call and function return
>> (see a long comment in valgrind/memcheck/mc_main.c). This avoids
>> false positives in that tool, at the cost of an extra stack pointer
>> adjustment.
>
> I'm fine with that, people reporting the issue to valgrin by providing a
> patch would be welcome but I doubt anybody has time.

Well, given how much this is exactly how they've intended it to behave, I 
doubt they'd be willing to accept a patch that "fixes" it, since it would 
probably silence detection of a whole lot of other issues instead. That's 
why I didn't even try to make such a patch, once I found that explanation.

// Martin

On 2014-06-08 01:55:47 +0300, Martin Storsjö wrote:
> We know that the called function (ff_chroma_inter_body_mmxext)
> doesn't touch the redzone, and thus will be kept intact - thus,
> this doesn't fix any bug per se.
> 
> However, valgrind's memcheck tool intentionally assumes that the
> redzone is clobbered on every function call and function return
> (see a long comment in valgrind/memcheck/mc_main.c). This avoids
> false positives in that tool, at the cost of an extra stack pointer
> adjustment.
> 
> The other alternative would be a valgrind suppression for this issue,
> but that's an extra burden for everybody that wants to run libavcodec
> within valgrind.
> ---
>  libavcodec/x86/h264_deblock.asm | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/x86/h264_deblock.asm b/libavcodec/x86/h264_deblock.asm
> index 8a9fdf6..61642a0 100644
> --- a/libavcodec/x86/h264_deblock.asm
> +++ b/libavcodec/x86/h264_deblock.asm
> @@ -821,10 +821,10 @@ cglobal deblock_v_chroma_8, 5,6
>  ;                          int8_t *tc0)
>  ;-----------------------------------------------------------------------------
>  cglobal deblock_h_chroma_8, 5,7
> -%if UNIX64
> -    %define buf0 [rsp-24]
> -    %define buf1 [rsp-16]
> -%elif WIN64
> +%if ARCH_X86_64
> +    ; This could use the red zone on 64 bit unix to avoid the stack pointer
> +    ; readjustment, but valgrind assumes the red zone is clobbered on
> +    ; function calls and returns.
>      sub   rsp, 16
>      %define buf0 [rsp]
>      %define buf1 [rsp+8]
> @@ -840,7 +840,7 @@ cglobal deblock_h_chroma_8, 5,7
>      movq  m0, buf0
>      movq  m3, buf1
>      TRANSPOSE8x4B_STORE PASS8ROWS(t5, r0, r1, t6)
> -%if WIN64
> +%if ARCH_X86_64
>      add   rsp, 16
>  %endif
>      RET

ok

Janne