diff mbox

[3/6] aarch64: Tidy syscall error check

Message ID 1400619378-7262-4-git-send-email-rth@twiddle.net
State New
Headers show

Commit Message

Richard Henderson May 20, 2014, 8:56 p.m. UTC
From: Richard Henderson <rth@redhat.com>

Move the error branch from the PSEUDO_RET macro to the PSEUDO macro.
This is in line with other architectures, and will enable further improvments.

	* sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h (PSEUDO):
	Branch to .Lsyscall_error on error.  Do not call DOARGS/UNDOARGS.
	* sysdeps/unix/sysv/linux/aarch64/sysdep.h (PSEUDO): Branch to
	.Lsyscall_error on error.
	(PSEUDO_RET): Do not check for error.
	(SYSCALL_ERROR) [NOT_IN_libc]: Use .Lsyscall_error.
	(SYSCALL_ERROR_HANDLER) [!NOT_IN_libc]: Branch to __syscall_error.
---
 sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h |  7 ++++---
 sysdeps/unix/sysv/linux/aarch64/sysdep.h             | 18 +++++++++---------
 2 files changed, 13 insertions(+), 12 deletions(-)

Comments

Will Newton May 21, 2014, 8:28 a.m. UTC | #1
On 20 May 2014 21:56, Richard Henderson <rth@twiddle.net> wrote:
> From: Richard Henderson <rth@redhat.com>
>
> Move the error branch from the PSEUDO_RET macro to the PSEUDO macro.
> This is in line with other architectures, and will enable further improvments.
>
>         * sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h (PSEUDO):
>         Branch to .Lsyscall_error on error.  Do not call DOARGS/UNDOARGS.

It's not clear to me why the DOARGS/UNDOARGS change is part of this
patch. Could you elaborate?

>         * sysdeps/unix/sysv/linux/aarch64/sysdep.h (PSEUDO): Branch to
>         .Lsyscall_error on error.
>         (PSEUDO_RET): Do not check for error.
>         (SYSCALL_ERROR) [NOT_IN_libc]: Use .Lsyscall_error.
>         (SYSCALL_ERROR_HANDLER) [!NOT_IN_libc]: Branch to __syscall_error.
> ---
>  sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h |  7 ++++---
>  sysdeps/unix/sysv/linux/aarch64/sysdep.h             | 18 +++++++++---------
>  2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h b/sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h
> index acaed5d..e3b4b56 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h
> @@ -33,16 +33,16 @@
>      cfi_startproc;                                                     \
>      DO_CALL (syscall_name, args);                                      \
>      cmn x0, 4095;                                                      \
> +    b.cs .Lsyscall_error;                                              \
>      PSEUDO_RET;                                                                \
>      cfi_endproc;                                                       \
>      .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel;   \
>    ENTRY (name);                                                                \
>      SINGLE_THREAD_P;                                                   \
> -    DOARGS_##args;                                                     \
>      bne .Lpseudo_cancel;                                               \
>      DO_CALL (syscall_name, 0);                                         \
> -    UNDOARGS_##args;                                                   \
>      cmn x0, 4095;                                                      \
> +    b.cs .Lsyscall_error;                                              \
>      PSEUDO_RET;                                                                \
>    .Lpseudo_cancel:                                                     \
>      DOCARGS_##args;    /* save syscall args etc. around CENABLE.  */   \
> @@ -61,7 +61,8 @@
>      cfi_adjust_cfa_offset (-16);                                       \
>      cfi_restore (x30);                                                 \
>      UNDOARGS_##args;                                                   \
> -    cmn x0, 4095;
> +    cmn x0, 4095;                                                      \
> +    b.cs .Lsyscall_error;
>
>  # define DOCARGS_0                                                     \
>         str x30, [sp, -16]!;                                            \
> diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> index 8397ad3..b32a565 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> @@ -58,17 +58,15 @@
>    .text;                                                                     \
>    ENTRY (name);                                                                      \
>      DO_CALL (syscall_name, args);                                            \
> -    cmn x0, #4095;
> +    cmn x0, #4095;                                                           \
> +    b.cs .Lsyscall_error;
>
>  /* Notice the use of 'RET' instead of 'ret' the assembler is case
>     insensitive and eglibc already uses the preprocessor symbol 'ret'
>     so we use the upper case 'RET' to force through a ret instruction
>     to the assembler */
>  # define PSEUDO_RET                                                          \
> -    b.cs 1f;                                                                 \
> -    RET;                                                                     \
> -    1:                                                                        \
> -    b SYSCALL_ERROR
> +    RET;
>  # undef ret
>  # define ret PSEUDO_RET
>
> @@ -112,10 +110,10 @@
>  # define ret_ERRVAL PSEUDO_RET_NOERRNO
>
>  # if NOT_IN_libc
> -#  define SYSCALL_ERROR __local_syscall_error
> +#  define SYSCALL_ERROR  .Lsyscall_error
>  #  if RTLD_PRIVATE_ERRNO
>  #   define SYSCALL_ERROR_HANDLER                               \
> -__local_syscall_error:                                         \
> +.Lsyscall_error:                                               \
>         adrp    x1, C_SYMBOL_NAME(rtld_errno);                  \
>         neg     w0, w0;                                         \
>         str     w0, [x1, :lo12:C_SYMBOL_NAME(rtld_errno)];      \
> @@ -124,7 +122,7 @@ __local_syscall_error:                                              \
>  #  else
>
>  #   define SYSCALL_ERROR_HANDLER                               \
> -__local_syscall_error:                                         \
> +.Lsyscall_error:                                               \
>         stp     x29, x30, [sp, -32]!;                           \
>         cfi_adjust_cfa_offset (32);                             \
>         cfi_rel_offset (x29, 0);                                \
> @@ -143,8 +141,10 @@ __local_syscall_error:                                             \
>         RET;
>  #  endif
>  # else
> -#  define SYSCALL_ERROR_HANDLER        /* Nothing here; code in sysdep.S is used.  */
>  #  define SYSCALL_ERROR __syscall_error
> +#  define SYSCALL_ERROR_HANDLER                                 \
> +.Lsyscall_error:                                                \
> +       b       __syscall_error;
>  # endif
>
>  /* Linux takes system call args in registers:
> --
> 1.9.0
>
Richard Henderson May 21, 2014, 3:04 p.m. UTC | #2
On 05/21/2014 01:28 AM, Will Newton wrote:
> On 20 May 2014 21:56, Richard Henderson <rth@twiddle.net> wrote:
>> From: Richard Henderson <rth@redhat.com>
>>
>> Move the error branch from the PSEUDO_RET macro to the PSEUDO macro.
>> This is in line with other architectures, and will enable further improvments.
>>
>>         * sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h (PSEUDO):
>>         Branch to .Lsyscall_error on error.  Do not call DOARGS/UNDOARGS.
> 
> It's not clear to me why the DOARGS/UNDOARGS change is part of this
> patch. Could you elaborate?

Oops, incomplete splitting of the patches.

(1) If they did something, their placement is buggy, as they're not properly
nested around the DO_CALL.  But...

(2) They do nothing on aarch64, since all parameters are already in registers.
 So why should we pretend to invoke them?


r~
Marcus Shawcroft May 22, 2014, 11:02 a.m. UTC | #3
On 21 May 2014 16:04, Richard Henderson <rth@twiddle.net> wrote:

> (2) They do nothing on aarch64, since all parameters are already in registers.
>  So why should we pretend to invoke them?

We should just remove all of the DOARGS/UNDOARGS definitions and uses
from  sysdep.h and sysdep-cancel.h.  Do you want to  write that patch?

/Marcus
Marcus Shawcroft May 22, 2014, 11:50 a.m. UTC | #4
On 20 May 2014 21:56, Richard Henderson <rth@twiddle.net> wrote:
> From: Richard Henderson <rth@redhat.com>
>
> Move the error branch from the PSEUDO_RET macro to the PSEUDO macro.
> This is in line with other architectures, and will enable further improvments.
>
>         * sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h (PSEUDO):
>         Branch to .Lsyscall_error on error.  Do not call DOARGS/UNDOARGS.
>         * sysdeps/unix/sysv/linux/aarch64/sysdep.h (PSEUDO): Branch to
>         .Lsyscall_error on error.
>         (PSEUDO_RET): Do not check for error.
>         (SYSCALL_ERROR) [NOT_IN_libc]: Use .Lsyscall_error.
>         (SYSCALL_ERROR_HANDLER) [!NOT_IN_libc]: Branch to __syscall_error.

>      SINGLE_THREAD_P;                                                   \
> -    DOARGS_##args;                                                     \
>      bne .Lpseudo_cancel;                                               \
>      DO_CALL (syscall_name, 0);                                         \
> -    UNDOARGS_##args;                                                   \
>      cmn x0, 4095;                                                      \

If we drop the removal of DOARGS and UNDOARGS and take that cleanup
separately then the remainder of this patch looks OK to me.

/Marcus
Richard Henderson May 22, 2014, 2:58 p.m. UTC | #5
On 05/22/2014 04:02 AM, Marcus Shawcroft wrote:
> On 21 May 2014 16:04, Richard Henderson <rth@twiddle.net> wrote:
> 
>> (2) They do nothing on aarch64, since all parameters are already in registers.
>>  So why should we pretend to invoke them?
> 
> We should just remove all of the DOARGS/UNDOARGS definitions and uses
> from  sysdep.h and sysdep-cancel.h.  Do you want to  write that patch?

Yes, I can do that.


r~
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h b/sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h
index acaed5d..e3b4b56 100644
--- a/sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h
+++ b/sysdeps/unix/sysv/linux/aarch64/nptl/sysdep-cancel.h
@@ -33,16 +33,16 @@ 
     cfi_startproc;							\
     DO_CALL (syscall_name, args);					\
     cmn x0, 4095;							\
+    b.cs .Lsyscall_error;						\
     PSEUDO_RET;								\
     cfi_endproc;							\
     .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel;	\
   ENTRY (name);								\
     SINGLE_THREAD_P;							\
-    DOARGS_##args;							\
     bne .Lpseudo_cancel;						\
     DO_CALL (syscall_name, 0);						\
-    UNDOARGS_##args;							\
     cmn x0, 4095;							\
+    b.cs .Lsyscall_error;						\
     PSEUDO_RET;								\
   .Lpseudo_cancel:							\
     DOCARGS_##args;	/* save syscall args etc. around CENABLE.  */	\
@@ -61,7 +61,8 @@ 
     cfi_adjust_cfa_offset (-16);					\
     cfi_restore (x30);							\
     UNDOARGS_##args;							\
-    cmn x0, 4095;
+    cmn x0, 4095;							\
+    b.cs .Lsyscall_error;
 
 # define DOCARGS_0							\
 	str x30, [sp, -16]!;						\
diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
index 8397ad3..b32a565 100644
--- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
@@ -58,17 +58,15 @@ 
   .text;								      \
   ENTRY (name);								      \
     DO_CALL (syscall_name, args);					      \
-    cmn x0, #4095;
+    cmn x0, #4095;							      \
+    b.cs .Lsyscall_error;
 
 /* Notice the use of 'RET' instead of 'ret' the assembler is case
    insensitive and eglibc already uses the preprocessor symbol 'ret'
    so we use the upper case 'RET' to force through a ret instruction
    to the assembler */
 # define PSEUDO_RET							      \
-    b.cs 1f;								      \
-    RET;								      \
-    1:                                                                        \
-    b SYSCALL_ERROR
+    RET;
 # undef ret
 # define ret PSEUDO_RET
 
@@ -112,10 +110,10 @@ 
 # define ret_ERRVAL PSEUDO_RET_NOERRNO
 
 # if NOT_IN_libc
-#  define SYSCALL_ERROR __local_syscall_error
+#  define SYSCALL_ERROR  .Lsyscall_error
 #  if RTLD_PRIVATE_ERRNO
 #   define SYSCALL_ERROR_HANDLER				\
-__local_syscall_error:						\
+.Lsyscall_error:						\
 	adrp	x1, C_SYMBOL_NAME(rtld_errno);			\
 	neg     w0, w0;						\
 	str     w0, [x1, :lo12:C_SYMBOL_NAME(rtld_errno)];	\
@@ -124,7 +122,7 @@  __local_syscall_error:						\
 #  else
 
 #   define SYSCALL_ERROR_HANDLER				\
-__local_syscall_error:						\
+.Lsyscall_error:						\
 	stp     x29, x30, [sp, -32]!;				\
 	cfi_adjust_cfa_offset (32);				\
 	cfi_rel_offset (x29, 0);				\
@@ -143,8 +141,10 @@  __local_syscall_error:						\
 	RET;
 #  endif
 # else
-#  define SYSCALL_ERROR_HANDLER	/* Nothing here; code in sysdep.S is used.  */
 #  define SYSCALL_ERROR __syscall_error
+#  define SYSCALL_ERROR_HANDLER                                 \
+.Lsyscall_error:                                                \
+	b	__syscall_error;
 # endif
 
 /* Linux takes system call args in registers: