| Message ID | 558DABCC.3020109@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Szabolcs, Thanks for the review, comments below: On 29-06-2015 07:32, Szabolcs Nagy wrote: > On 26/06/15 20:45, Adhemerval Zanella wrote: >> +ENTRY (__syscall_cancel_arch) >> + >> + stp x29, x30, [sp, -16]! >> + cfi_def_cfa_offset (16) >> + cfi_offset (29, -16) >> + cfi_offset (30, -8) >> + add x29, sp, 0 >> + cfi_def_cfa_register (29) >> + > > you save things on the stack here ... > > > ... and tail call into a function that does not restore the stack. > > i think you can omit saving the frame pointer. > (neither syscall, nor tail call needs it). My first though was the FP save-restore was required to correct create unwind information for cancellation handlers, but on a second though they are not necessary indeed. I have removed them and cancellation handlers works as intended, right now the syscall wrappers looks like: ENTRY (__syscall_cancel_arch) .globl __syscall_cancel_arch_start .type __syscall_cancel_arch_start,@function __syscall_cancel_arch_start: /* if (*cancelhandling & CANCELED_BITMASK) __syscall_do_cancel() */ ldr w0, [x0] tbnz w0, 2, 1f /* Issue a 6 argument syscall, the nr [x1] being the syscall number. */ mov x8, x1 mov x0, x2 mov x1, x3 mov x2, x4 mov x3, x5 mov x4, x6 mov x5, x7 svc 0x0 .globl __syscall_cancel_arch_end .type __syscall_cancel_arch_end,@function __syscall_cancel_arch_end: ret 1: b __syscall_do_cancel END (__syscall_cancel_arch) > >> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h >> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h >> @@ -20,42 +20,50 @@ >> #include <tls.h> >> #ifndef __ASSEMBLER__ >> # include <nptl/pthreadP.h> >> +# include <sys/ucontext.h> >> #endif >> >> #if IS_IN (libc) || IS_IN (libpthread) || IS_IN (librt) >> >> +# if IS_IN (libc) >> +# define JMP_SYSCALL_CANCEL HIDDEN_JUMPTARGET(__syscall_cancel) >> +# else >> +# define JMP_SYSCALL_CANCEL __syscall_cancel >> +# endif >> + >> # undef PSEUDO >> # define PSEUDO(name, syscall_name, args) \ >> - .section ".text"; \ >> -ENTRY (__##syscall_name##_nocancel); \ >> -.Lpseudo_nocancel: \ >> - DO_CALL (syscall_name, args); \ >> -.Lpseudo_finish: \ >> - cmn x0, 4095; \ >> - b.cs .Lsyscall_error; \ >> - .subsection 2; \ >> - .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel; \ >> + .section ".text"; \ >> +ENTRY (__##syscall_name##_nocancel); \ >> +L(pseudo_nocancel): \ >> + DO_CALL (syscall_name, args); \ >> +L(pseudo_finish): \ >> + cmn x0, 4095; \ >> + b.cs L(syscall_error); \ >> + .subsection 2; \ >> + .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel; \ > > i think there is a problem here that may need independent > fix (not a regression, but worth a mention): > > a glibc test (debug/tst-backtrace5) checks if 'read' symbol > is found when read is interrupted and the signal handler > calls backtrace_symbols. > > however the interrupt will be in __read_nocancel and that > name is not exported so backtrace does not find it in the > dynamic symbol table so the check fails. (on some other > archs the test pass because the nocancel code is within > the read code, not a separate function). > > it's a silly issue so i haven't got around proposing a fix > for this yet. > Indeed it would be much better to add more cleanup in this macros. I have simplified to: # if IS_IN (libc) # define JMP_SYSCALL_CANCEL HIDDEN_JUMPTARGET(__syscall_cancel) # else # define JMP_SYSCALL_CANCEL __syscall_cancel # endif # undef PSEUDO # define PSEUDO(name, syscall_name, args) \ ENTRY (name); \ SINGLE_THREAD_P(16); \ cbnz w16, L(pseudo_cancel); \ DO_CALL (syscall_name, args); \ b L(pseudo_finish); \ L(pseudo_cancel): \ stp x29, x30, [sp, -16]!; \ cfi_def_cfa_offset (16); \ cfi_offset (29, -16); \ cfi_offset (30, -8); \ add x29, sp, 0; \ cfi_def_cfa_register (29); \ mov x6, x5; \ mov x5, x4; \ mov x4, x3; \ mov x3, x2; \ mov x2, x1; \ mov x1, x0; \ mov x0, SYS_ify (syscall_name); \ bl JMP_SYSCALL_CANCEL; \ ldp x29, x30, [sp], 16; \ cfi_restore (30); \ cfi_restore (29); \ cfi_def_cfa (31, 0); \ L(pseudo_finish): \ cmn x0, 4095; \ b.cs L(syscall_error); # undef PSEUDO_END # define PSEUDO_END(name) \ SYSCALL_ERROR_HANDLER; \ cfi_endproc; \ .size name, .-name; And debug/tst-backtrace{5-6} work as intended as well. What do you think?
On 30-06-2015 05:46, Szabolcs Nagy wrote: > On 29/06/15 23:16, Adhemerval Zanella wrote: >> they are not necessary indeed. I have removed them and cancellation >> handlers works as intended, right now the syscall wrappers looks like: >> >> ENTRY (__syscall_cancel_arch) >> >> .globl __syscall_cancel_arch_start >> .type __syscall_cancel_arch_start,@function >> __syscall_cancel_arch_start: >> >> /* if (*cancelhandling & CANCELED_BITMASK) >> __syscall_do_cancel() */ >> ldr w0, [x0] >> tbnz w0, 2, 1f >> >> /* Issue a 6 argument syscall, the nr [x1] being the syscall >> number. */ >> mov x8, x1 >> mov x0, x2 >> mov x1, x3 >> mov x2, x4 >> mov x3, x5 >> mov x4, x6 >> mov x5, x7 >> svc 0x0 >> >> .globl __syscall_cancel_arch_end >> .type __syscall_cancel_arch_end,@function >> __syscall_cancel_arch_end: >> ret >> >> 1: >> b __syscall_do_cancel >> >> END (__syscall_cancel_arch) >> > > looks good. > >> Indeed it would be much better to add more cleanup in this macros. >> I have simplified to: >> >> # if IS_IN (libc) >> # define JMP_SYSCALL_CANCEL HIDDEN_JUMPTARGET(__syscall_cancel) >> # else >> # define JMP_SYSCALL_CANCEL __syscall_cancel >> # endif >> >> # undef PSEUDO >> # define PSEUDO(name, syscall_name, args) \ >> ENTRY (name); \ >> SINGLE_THREAD_P(16); \ >> cbnz w16, L(pseudo_cancel); \ >> DO_CALL (syscall_name, args); \ >> b L(pseudo_finish); \ >> L(pseudo_cancel): \ >> stp x29, x30, [sp, -16]!; \ >> cfi_def_cfa_offset (16); \ >> cfi_offset (29, -16); \ >> cfi_offset (30, -8); \ >> add x29, sp, 0; \ >> cfi_def_cfa_register (29); \ >> mov x6, x5; \ >> mov x5, x4; \ >> mov x4, x3; \ >> mov x3, x2; \ >> mov x2, x1; \ >> mov x1, x0; \ >> mov x0, SYS_ify (syscall_name); \ >> bl JMP_SYSCALL_CANCEL; \ >> ldp x29, x30, [sp], 16; \ >> cfi_restore (30); \ >> cfi_restore (29); \ >> cfi_def_cfa (31, 0); \ >> L(pseudo_finish): \ >> cmn x0, 4095; \ >> b.cs L(syscall_error); >> >> # undef PSEUDO_END >> # define PSEUDO_END(name) \ >> SYSCALL_ERROR_HANDLER; \ >> cfi_endproc; \ >> .size name, .-name; >> >> And debug/tst-backtrace{5-6} work as intended as well. What do you think? >> > > is it ok to remove the __<syscall>_nocancel internal symbols? > > otherwise it looks good. Yes, the idea of 34caaafd1ae38c9295325a1da491d75a92b205b0 is exactly to remove __<syscall>_nocancel usage. > > thanks. >
diff --git a/sysdeps/unix/sysv/linux/aarch64/syscall_cancel.S b/sysdeps/unix/sysv/linux/aarch64/syscall_cancel.S new file mode 100644 index 0000000..33c51c3 --- /dev/null +++ b/sysdeps/unix/sysv/linux/aarch64/syscall_cancel.S @@ -0,0 +1,75 @@ +/* Cancellable syscall wrapper - aarch64 version. + Copyright (C) 2015 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <sysdep.h> + +/* long int [r0] __syscall_cancel_arch (int *cancelhandling [x0], + long int nr [x1], + long int arg1 [x2], + long int arg2 [x3], + long int arg3 [x4], + long int arg4 [x5], + long int arg5 [x6], + long int arg6 [x7]) */ + +ENTRY (__syscall_cancel_arch) + + stp x29, x30, [sp, -16]! + cfi_def_cfa_offset (16) + cfi_offset (29, -16) + cfi_offset (30, -8) + add x29, sp, 0 + cfi_def_cfa_register (29) + + .globl __syscall_cancel_arch_start + .type __syscall_cancel_arch_start,@function +__syscall_cancel_arch_start: + + /* if (*cancelhandling & CANCELED_BITMASK) + __syscall_do_cancel() */ + ldr w0, [x0] + tbnz w0, 2, 1f + + /* Issue a 6 argument syscall, the nr [x1] being the syscall + number. */ + mov x8, x1 + mov x0, x2 + mov x1, x3 + mov x2, x4 + mov x3, x5 + mov x4, x6 + mov x5, x7 + svc 0x0 + + .globl __syscall_cancel_arch_end + .type __syscall_cancel_arch_end,@function +__syscall_cancel_arch_end: + + ldp x29, x30, [sp], 16 + cfi_remember_state + cfi_restore (30) + cfi_restore (29) + cfi_def_cfa (31, 0) + ret + +1: + cfi_restore_state + b __syscall_do_cancel + +END (__syscall_cancel_arch) +libc_hidden_def (__syscall_cancel_arch) diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h b/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h index 36e8e39..ba91268 100644 --- a/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h @@ -20,42 +20,50 @@ #include <tls.h> #ifndef __ASSEMBLER__ # include <nptl/pthreadP.h> +# include <sys/ucontext.h> #endif #if IS_IN (libc) || IS_IN (libpthread) || IS_IN (librt) +# if IS_IN (libc) +# define JMP_SYSCALL_CANCEL HIDDEN_JUMPTARGET(__syscall_cancel) +# else +# define JMP_SYSCALL_CANCEL __syscall_cancel +# endif + # undef PSEUDO # define PSEUDO(name, syscall_name, args) \ - .section ".text"; \ -ENTRY (__##syscall_name##_nocancel); \ -.Lpseudo_nocancel: \ - DO_CALL (syscall_name, args); \ -.Lpseudo_finish: \ - cmn x0, 4095; \ - b.cs .Lsyscall_error; \ - .subsection 2; \ - .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel; \ + .section ".text"; \ +ENTRY (__##syscall_name##_nocancel); \ +L(pseudo_nocancel): \ + DO_CALL (syscall_name, args); \ +L(pseudo_finish): \ + cmn x0, 4095; \ + b.cs L(syscall_error); \ + .subsection 2; \ + .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel; \ ENTRY (name); \ SINGLE_THREAD_P(16); \ - cbz w16, .Lpseudo_nocancel; \ - /* Setup common stack frame no matter the number of args. \ - Also save the first arg, since it's basically free. */ \ - stp x30, x0, [sp, -64]!; \ - cfi_adjust_cfa_offset (64); \ - cfi_rel_offset (x30, 0); \ - DOCARGS_##args; /* save syscall args around CENABLE. */ \ - CENABLE; \ - mov x16, x0; /* save mask around syscall. */ \ - UNDOCARGS_##args; /* restore syscall args. */ \ - DO_CALL (syscall_name, args); \ - str x0, [sp, 8]; /* save result around CDISABLE. */ \ - mov x0, x16; /* restore mask for CDISABLE. */ \ - CDISABLE; \ - /* Break down the stack frame, restoring result at once. */ \ - ldp x30, x0, [sp], 64; \ - cfi_adjust_cfa_offset (-64); \ - cfi_restore (x30); \ - b .Lpseudo_finish; \ + cbz w16, L(pseudo_nocancel); \ + stp x29, x30, [sp, -16]!; \ + cfi_def_cfa_offset (16); \ + cfi_offset (29, -16); \ + cfi_offset (30, -8); \ + add x29, sp, 0; \ + cfi_def_cfa_register (29); \ + mov x6, x5; \ + mov x5, x4; \ + mov x4, x3; \ + mov x3, x2; \ + mov x2, x1; \ + mov x1, x0; \ + mov x0, SYS_ify (syscall_name); \ + bl JMP_SYSCALL_CANCEL; \ + ldp x29, x30, [sp], 16; \ + cfi_restore (30); \ + cfi_restore (29); \ + cfi_def_cfa (31, 0); \ + b L(pseudo_finish); \ cfi_endproc; \ .size name, .-name; \ .previous @@ -65,35 +73,10 @@ ENTRY (name); \ SYSCALL_ERROR_HANDLER; \ cfi_endproc -# define DOCARGS_0 -# define DOCARGS_1 -# define DOCARGS_2 str x1, [sp, 16] -# define DOCARGS_3 stp x1, x2, [sp, 16] -# define DOCARGS_4 DOCARGS_3; str x3, [sp, 32] -# define DOCARGS_5 DOCARGS_3; stp x3, x4, [sp, 32] -# define DOCARGS_6 DOCARGS_5; str x5, [sp, 48] - -# define UNDOCARGS_0 -# define UNDOCARGS_1 ldr x0, [sp, 8] -# define UNDOCARGS_2 ldp x0, x1, [sp, 8] -# define UNDOCARGS_3 UNDOCARGS_1; ldp x1, x2, [sp, 16] -# define UNDOCARGS_4 UNDOCARGS_2; ldp x2, x3, [sp, 24] -# define UNDOCARGS_5 UNDOCARGS_3; ldp x3, x4, [sp, 32] -# define UNDOCARGS_6 UNDOCARGS_4; ldp x4, x5, [sp, 40] - # if IS_IN (libpthread) -# define CENABLE bl __pthread_enable_asynccancel -# define CDISABLE bl __pthread_disable_asynccancel # define __local_multiple_threads __pthread_multiple_threads # elif IS_IN (libc) -# define CENABLE bl __libc_enable_asynccancel -# define CDISABLE bl __libc_disable_asynccancel # define __local_multiple_threads __libc_multiple_threads -# elif IS_IN (librt) -# define CENABLE bl __librt_enable_asynccancel -# define CDISABLE bl __librt_disable_asynccancel -# else -# error Unsupported library # endif # if IS_IN (libpthread) || IS_IN (libc) @@ -131,4 +114,10 @@ extern int __local_multiple_threads attribute_hidden; # define RTLD_SINGLE_THREAD_P \ __builtin_expect (THREAD_GETMEM (THREAD_SELF, \ header.multiple_threads) == 0, 1) + +static inline +long int __pthread_get_ip (const struct ucontext *uc) +{ + return uc->uc_mcontext.pc; +} #endif diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h index fe94a50..5f73b9e 100644 --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h @@ -199,6 +199,14 @@ # undef INTERNAL_SYSCALL_ERRNO # define INTERNAL_SYSCALL_ERRNO(val, err) (-(val)) +#undef SYSCALL_CANCEL_ERROR +#define SYSCALL_CANCEL_ERROR(__val) \ + ((unsigned long) (__val) >= (unsigned long) -4095) + +#undef SYSCALL_CANCEL_ERRNO +#define SYSCALL_CANCEL_ERRNO(__val) \ + (-(__val)) + # define LOAD_ARGS_0() \ register long _x0 asm ("x0"); # define LOAD_ARGS_1(x0) \