Re: V2 [PATCH 1/5] i386: Don't unnecessarily save and restore EAX, ECX and EDX [BZ# 25262]

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 08/01/2020 13:15, H.J. Lu wrote:
> On i386, since EAX, ECX and EDX are caller-saved, there are no need
> to save and restore EAX, ECX and EDX in getcontext, setcontext and
> swapcontext.  They just need to clear EAX on success.  The extra
> scratch registers are needed to enable CET.
> 
> Tested on i386.

LGTM thanks. You might also remove the oE{A,C,D}X definitions from
Linux i386 ucontext_i.sym.

As a side note, Hurd might do the same on its implementations.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  sysdeps/unix/sysv/linux/i386/getcontext.S  |  8 +-------
>  sysdeps/unix/sysv/linux/i386/setcontext.S  | 11 ++++-------
>  sysdeps/unix/sysv/linux/i386/swapcontext.S | 17 +++++------------
>  3 files changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/i386/getcontext.S b/sysdeps/unix/sysv/linux/i386/getcontext.S
> index f86df4d555..9c1df9a2aa 100644
> --- a/sysdeps/unix/sysv/linux/i386/getcontext.S
> +++ b/sysdeps/unix/sysv/linux/i386/getcontext.S
> @@ -26,13 +26,7 @@ ENTRY(__getcontext)
>  	/* Load address of the context data structure.  */
>  	movl	4(%esp), %eax
>  
> -	/* Return value of getcontext.  EAX is the only register whose
> -	   value is not preserved.  */
> -	movl	$0, oEAX(%eax)
> -
> -	/* Save the 32-bit register values and the return address.  */
> -	movl	%ecx, oECX(%eax)
> -	movl	%edx, oEDX(%eax)
> +	/* Save the preserved register values and the return address.  */
>  	movl	%edi, oEDI(%eax)
>  	movl	%esi, oESI(%eax)
>  	movl	%ebp, oEBP(%eax)

Ok.

> diff --git a/sysdeps/unix/sysv/linux/i386/setcontext.S b/sysdeps/unix/sysv/linux/i386/setcontext.S
> index b4b5c0298c..f042d80bf4 100644
> --- a/sysdeps/unix/sysv/linux/i386/setcontext.S
> +++ b/sysdeps/unix/sysv/linux/i386/setcontext.S
> @@ -65,22 +65,19 @@ ENTRY(__setcontext)
>  	cfi_offset (esi, oESI)
>  	cfi_offset (ebp, oEBP)
>  	cfi_offset (ebx, oEBX)
> -	cfi_offset (edx, oEDX)
> -	cfi_offset (ecx, oECX)
>  	movl	oESP(%eax), %esp
>  
>  	/* Push the return address on the new stack so we can return there.  */
>  	pushl	%ecx
>  
> -	/* Load the values of all the 32-bit registers (except ESP).
> -	   Since we are loading from EAX, it must be last.  */
> +	/* Load the values of all the preserved registers (except ESP).  */
>  	movl	oEDI(%eax), %edi
>  	movl	oESI(%eax), %esi
>  	movl	oEBP(%eax), %ebp
>  	movl	oEBX(%eax), %ebx
> -	movl	oEDX(%eax), %edx
> -	movl	oECX(%eax), %ecx
> -	movl	oEAX(%eax), %eax
> +
> +	/* All done, return 0 for success.  */
> +	xorl	%eax, %eax
>  
>  	/* End FDE here, we fall into another context.  */
>  	cfi_endproc

Ok.

> diff --git a/sysdeps/unix/sysv/linux/i386/swapcontext.S b/sysdeps/unix/sysv/linux/i386/swapcontext.S
> index 792bfdf7e6..090c2d8c3e 100644
> --- a/sysdeps/unix/sysv/linux/i386/swapcontext.S
> +++ b/sysdeps/unix/sysv/linux/i386/swapcontext.S
> @@ -26,13 +26,7 @@ ENTRY(__swapcontext)
>  	/* Load address of the context data structure we save in.  */
>  	movl	4(%esp), %eax
>  
> -	/* Return value of swapcontext.  EAX is the only register whose
> -	   value is not preserved.  */
> -	movl	$0, oEAX(%eax)
> -
> -	/* Save the 32-bit register values and the return address.  */
> -	movl	%ecx, oECX(%eax)
> -	movl	%edx, oEDX(%eax)
> +	/* Save the preserved register values and the return address.  */
>  	movl	%edi, oEDI(%eax)
>  	movl	%esi, oESI(%eax)
>  	movl	%ebp, oEBP(%eax)
> @@ -91,15 +85,14 @@ ENTRY(__swapcontext)
>  	/* Push the return address on the new stack so we can return there.  */
>  	pushl	%ecx
>  
> -	/* Load the values of all the 32-bit registers (except ESP).
> -	   Since we are loading from EAX, it must be last.  */
> +	/* Load the values of all the preserved registers (except ESP).  */
>  	movl	oEDI(%eax), %edi
>  	movl	oESI(%eax), %esi
>  	movl	oEBP(%eax), %ebp
>  	movl	oEBX(%eax), %ebx
> -	movl	oEDX(%eax), %edx
> -	movl	oECX(%eax), %ecx
> -	movl	oEAX(%eax), %eax
> +
> +	/* All done, return 0 for success.  */
> +	xorl	%eax, %eax
>  
>  	/* The following 'ret' will pop the address of the code and jump
>  	   to it.  */
>