Message ID | 20230411133004.2268170-3-josimmon@redhat.com |
---|---|
State | New |
Headers | show |
Series | x86_64: aarch64: Set call number just before syscall | expand |
* Joe Simmons-Talbott via Libc-alpha: > ({ long _sys_result; \ > { \ > LOAD_ARGS_##nr (args) \ > register long _x8 asm ("x8") = (name); \ > + if (__builtin_constant_p(name)) \ > + asm volatile ("mov x8, " MSTR(name) ";" \ > + : /* no output */ : "i" (name) : "x8"); \ > asm volatile ("svc 0 // syscall " # name \ > : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \ > _sys_result = _x0; \ I think you should do this in a single assembler statement, load the constant only once. Thanks, Florian
On 11/04/23 10:50, Florian Weimer via Libc-alpha wrote: > * Joe Simmons-Talbott via Libc-alpha: > >> ({ long _sys_result; \ >> { \ >> LOAD_ARGS_##nr (args) \ >> register long _x8 asm ("x8") = (name); \ >> + if (__builtin_constant_p(name)) \ >> + asm volatile ("mov x8, " MSTR(name) ";" \ >> + : /* no output */ : "i" (name) : "x8"); \ >> asm volatile ("svc 0 // syscall " # name \ >> : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \ >> _sys_result = _x0; \ > > I think you should do this in a single assembler statement, load the > constant only once. Is this required because compiler is free to reorganize the argument list? I think it should me it clear on the commit message. Using a single assembler would require two inline asm, something like: diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h index e94d1703ad..2a128bb72d 100644 --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h @@ -172,9 +172,19 @@ ({ long _sys_result; \ { \ LOAD_ARGS_##nr (args) \ - register long _x8 asm ("x8") = (name); \ - asm volatile ("svc 0 // syscall " # name \ - : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \ + if (__builtin_constant_p (name)) \ + asm volatile ("mov x8, %1\n" \ + "svc 0 // syscall " # name \ + : "=r" (_x0) \ + : "i" (name) ASM_ARGS_##nr \ + : "x8", "memory"); \ + else \ + { \ + register long _x8 asm ("x8") = (name); \ + asm volatile ("svc 0 // syscall " # name \ + : "=r" (_x0) \ + : "r"(_x8) ASM_ARGS_##nr : "memory"); \ + } \ _sys_result = _x0; \ } \ _sys_result; }) Which really makes me doubt if this extra complexity is really necessary...
The 04/11/2023 11:15, Adhemerval Zanella Netto via Libc-alpha wrote: > On 11/04/23 10:50, Florian Weimer via Libc-alpha wrote: > > * Joe Simmons-Talbott via Libc-alpha: > > > >> ({ long _sys_result; \ > >> { \ > >> LOAD_ARGS_##nr (args) \ > >> register long _x8 asm ("x8") = (name); \ > >> + if (__builtin_constant_p(name)) \ > >> + asm volatile ("mov x8, " MSTR(name) ";" \ > >> + : /* no output */ : "i" (name) : "x8"); \ > >> asm volatile ("svc 0 // syscall " # name \ > >> : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \ > >> _sys_result = _x0; \ > > > > I think you should do this in a single assembler statement, load the > > constant only once. > > Is this required because compiler is free to reorganize the argument > list? I think it should me it clear on the commit message. > > Using a single assembler would require two inline asm, something like: i don't like the use of __builtin_constant_name_p here. if we want different behaviour for INTERNAL_SYSCALL_NCS and other inlined syscall things then we should do the dispatch there. and yes it has to be one asm block since the compiler is free to add random nops (or equivalent) anywhere. i'm not entirely sure why this change is needed, a bit more background would be useful.
* Adhemerval Zanella Netto: > On 11/04/23 10:50, Florian Weimer via Libc-alpha wrote: >> * Joe Simmons-Talbott via Libc-alpha: >> >>> ({ long _sys_result; \ >>> { \ >>> LOAD_ARGS_##nr (args) \ >>> register long _x8 asm ("x8") = (name); \ >>> + if (__builtin_constant_p(name)) \ >>> + asm volatile ("mov x8, " MSTR(name) ";" \ >>> + : /* no output */ : "i" (name) : "x8"); \ >>> asm volatile ("svc 0 // syscall " # name \ >>> : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \ >>> _sys_result = _x0; \ >> >> I think you should do this in a single assembler statement, load the >> constant only once. > > Is this required because compiler is free to reorganize the argument > list? I think it should me it clear on the commit message. Yes, that's the reason. It's a bit tricky to recover the system call number using static analysis otherwise. I suggested to Joe that we should put something into glibc, rather than improving that static analysis tool so that it's fully reliable. Thanks, Florian
On 11/04/23 13:03, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 11/04/23 10:50, Florian Weimer via Libc-alpha wrote: >>> * Joe Simmons-Talbott via Libc-alpha: >>> >>>> ({ long _sys_result; \ >>>> { \ >>>> LOAD_ARGS_##nr (args) \ >>>> register long _x8 asm ("x8") = (name); \ >>>> + if (__builtin_constant_p(name)) \ >>>> + asm volatile ("mov x8, " MSTR(name) ";" \ >>>> + : /* no output */ : "i" (name) : "x8"); \ >>>> asm volatile ("svc 0 // syscall " # name \ >>>> : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \ >>>> _sys_result = _x0; \ >>> >>> I think you should do this in a single assembler statement, load the >>> constant only once. >> >> Is this required because compiler is free to reorganize the argument >> list? I think it should me it clear on the commit message. > > Yes, that's the reason. It's a bit tricky to recover the system call > number using static analysis otherwise. I suggested to Joe that we > should put something into glibc, rather than improving that static > analysis tool so that it's fully reliable. Direct syscalls are done by different projects, like sanitizer, libgomp, etc; so imho improving the static analysis tool could potentially catch a wide range of usages than trying to fix only on glibc.
On Tue, Apr 11, 2023 at 01:39:31PM -0300, Adhemerval Zanella Netto wrote: > > > On 11/04/23 13:03, Florian Weimer wrote: > > * Adhemerval Zanella Netto: > > > >> On 11/04/23 10:50, Florian Weimer via Libc-alpha wrote: > >>> * Joe Simmons-Talbott via Libc-alpha: > >>> > >>>> ({ long _sys_result; \ > >>>> { \ > >>>> LOAD_ARGS_##nr (args) \ > >>>> register long _x8 asm ("x8") = (name); \ > >>>> + if (__builtin_constant_p(name)) \ > >>>> + asm volatile ("mov x8, " MSTR(name) ";" \ > >>>> + : /* no output */ : "i" (name) : "x8"); \ > >>>> asm volatile ("svc 0 // syscall " # name \ > >>>> : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \ > >>>> _sys_result = _x0; \ > >>> > >>> I think you should do this in a single assembler statement, load the > >>> constant only once. > >> > >> Is this required because compiler is free to reorganize the argument > >> list? I think it should me it clear on the commit message. > > > > Yes, that's the reason. It's a bit tricky to recover the system call > > number using static analysis otherwise. I suggested to Joe that we > > should put something into glibc, rather than improving that static > > analysis tool so that it's fully reliable. > > Direct syscalls are done by different projects, like sanitizer, libgomp, > etc; so imho improving the static analysis tool could potentially catch > a wide range of usages than trying to fix only on glibc. > I agree that improving the static analysis tool would be helpful. One thing to keep in mind is that this patchset will also aid people doing static analysis manually. It also seems likely that other static analysis tools would benefit from this change. Thanks, Joe
diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h index e94d1703ad..42a70feac2 100644 --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h @@ -167,12 +167,18 @@ # define HAVE_CLONE3_WRAPPER 1 +# define MSTR_HELPER(x) # x +# define MSTR(x) MSTR_HELPER(x) + # undef INTERNAL_SYSCALL_RAW # define INTERNAL_SYSCALL_RAW(name, nr, args...) \ ({ long _sys_result; \ { \ LOAD_ARGS_##nr (args) \ register long _x8 asm ("x8") = (name); \ + if (__builtin_constant_p(name)) \ + asm volatile ("mov x8, " MSTR(name) ";" \ + : /* no output */ : "i" (name) : "x8"); \ asm volatile ("svc 0 // syscall " # name \ : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory"); \ _sys_result = _x0; \