[libav-devel] [PATCH 1/1] x86: checkasm: check for or handle missing cleanup after MMX instructions

Henrik Gramner henrik at gramner.com
Tue Dec 22 18:21:44 CET 2015


On Fri, Dec 11, 2015 at 6:40 PM, Janne Grunau <janne-libav at jannau.net> wrote:
> +#define declare_new_emms(cpu_flags, ret, ...) \
> +    ret (*checked_call)(void *, int, int, int, int, int, __VA_ARGS__) = \
> +        ((cpu_flags) & av_get_cpu_flags()) ? (void *)checkasm_checked_call_emms : \
> +                                             (void *)checkasm_checked_call;

> +#define declare_new_emms(cpu_flags, ret, ...) ret (*checked_call)(void *, __VA_ARGS__) = \
> +        ((cpu_flags) & av_get_cpu_flags()) ? (void *)checkasm_checked_call_emms :        \
> +

Do we need to have cpu_flags as a parameter here? Couldn't we just use
the checked_call_emms codepath unconditionally whenever
declare_new_emms is used on x86 or am I missing something?

> +%macro check_call 0-1
> +cglobal checked_call%1, 2,15,16,max_args*8+8

s/check_call/CHECKED_CALL/

Also I'm not sure if using %1 when it can be undefined is a good idea.
It might just happen to accidentally work right now.

> +%ifnid %1, _emms
> +    fstenv [rsp]
> +    mov  r9h, [rsp + 8]
> +    add  r9h, 1
> +    jz   .emms_ok
> +    report_fail error_message_emms
> +    emms
> +.emms_ok:
> +%else
> +    emms
> +%endif

You're not checking if registers 4-7 are empty here because the FPU
tag word is 16 bits and rNh is an 8-bit register corresponding to bits
8-15 of a full register.

mov/add should be replaced with cmp word [rsp + 8], 0xffff (and jz
with je IMO even though they assemble to the same opcode because
"equal" makes more sense than "zero" in that case).

> +%ifnid %1, _emms
> +    fstenv [rsp]
> +    mov  r3h, [rsp + 8]
> +    add  r3h, 1
> +    jz   .emms_ok
> +    report_fail error_message_emms
> +    emms
> +.emms_ok:
> +%else
> +    emms
> +%endif

Ditto, also s/rsp/esp/ for consistency with the rest of the 32-bit code.


More information about the libav-devel mailing list