Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
220933002:
Add arm64 support to linux_syscall_support.h (Closed)
|
Created:
6 years, 8 months ago by rmcilroy Modified:
6 years, 8 months ago CC:
Primiano Tucci (use gerrit), chromium-reviews Visibility:
Public. |
DescriptionAdd arm64 support to linux_syscall_support.h.
This CL adds arm64 support for linux syscall support. The arm64
kernel depricates a number of syscalls, such as fork(), poll() and open(), so
we need to replicate the behaviour of these syscalls from newer related
syscalls, such as clone(), ppoll() and openat().
The code added here is based on that in glibc for aarch64, and Android bionic
for arm64, but currently hasn't been tested on either platform. It does
enable breakpad to compile for arm64 though.
BUG=354405, 335641
R=jacob.bramley@arm.com, mseaborn@chromium.org
Committed: 26
Patch Set 1 #Patch Set 2 : #
Total comments: 20
Patch Set 3 : Address Mark's comments #Patch Set 4 : Fix kernel_getdents definition on aarch64 #
Total comments: 36
Patch Set 5 : Address Mark's comments #Patch Set 6 : Add comment. #
Total comments: 1
Patch Set 7 : Address Jacob's comments. #Patch Set 8 : Change adds,bne to mov, cbnz #Patch Set 9 : Fix line lenght #Messages
Total messages: 14 (0 generated)
Mark: This cl adds arm64/aarch64 support to linux_syscall_support.h. Unfortunately aarch64 deprecated a bunch of syscalls, so there are certain syscalls I have had to replicate using newer syscalls (e.g., fork()->clone(), poll()->ppoll() and open()->openat()). Also, this compiles, but it hasn't been tested yet. I am planning on testing with the breakpad unittests, but need to implement some further support in breakbad before I can do so (initial CL is up for review at https://breakpad.appspot.com/1354002/). If you know of any other test for this file please let me know. PTAL.
https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h File linux_syscall_support.h (left): https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2894: #if defined(__x86_64__) || \ Could you possibly commit small cleanup refactorings like this part as separate changes? https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:481: unsigned long st_dev; // Device. Can you make the alignment indentation consistent for this struct, please? https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:482: unsigned long st_ino; // File serial number. I'd leave out the comments for consistency with the other kernel_stat defs. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:945: #define __NR_pread64 67 Could you sort these by number? https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2442: /* Most definitions of _syscallX() neglect to mark "memory" as being I realise this is copied from elsewhere in the file. I'm not sure what it means. Do you know what "Most definitions of _syscallX()" refers to? Most definitions in this file? https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2510: LSS_INLINE pid_t LSS_NAME(getpid)(); Nit: should be "(void)" for when compiling this with -Wstrict-prototypes in C. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2511: LSS_INLINE pid_t LSS_NAME(fork)() { Since this file is huge and unmanageable and difficult to read, I have a suggestion: How about putting all your new polyfill definitions in a separate section at the end of the file, with a clear "/* Polyfills */" comment? I think the file would be easier to read if the polyfills were separate from straightforward syscall definitions. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2516: void * child_stack = NULL; Nit: remove space after "*" https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2539: __asm__ __volatile__(/* if (fn == NULL || child_stack == NULL) How about doing these checks in C? https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:3714: #if defined(__ARM_EABI__) || defined (__aarch64__) || defined(__x86_64__) || \ Line is >80 chars
Thanks for the review Mark. I've addressed the comments, PTAL. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h File linux_syscall_support.h (left): https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2894: #if defined(__x86_64__) || \ On 2014/04/03 21:44:22, Mark Seaborn wrote: > Could you possibly commit small cleanup refactorings like this part as separate > changes? Done. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:481: unsigned long st_dev; // Device. On 2014/04/03 21:44:22, Mark Seaborn wrote: > Can you make the alignment indentation consistent for this struct, please? Done. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:482: unsigned long st_ino; // File serial number. On 2014/04/03 21:44:22, Mark Seaborn wrote: > I'd leave out the comments for consistency with the other kernel_stat defs. Done. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:945: #define __NR_pread64 67 On 2014/04/03 21:44:22, Mark Seaborn wrote: > Could you sort these by number? Done. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2442: /* Most definitions of _syscallX() neglect to mark "memory" as being On 2014/04/03 21:44:22, Mark Seaborn wrote: > I realise this is copied from elsewhere in the file. I'm not sure what it > means. Do you know what "Most definitions of _syscallX()" refers to? Most > definitions in this file? I was assuming it meant the _syscallX() macros which used to be defined in glibc (or similar libraries) - I think they got removed a while ago and replaced with the syscall() function, so I'm not sure if this problem still exists. Shall I remove these comments across the file? https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2510: LSS_INLINE pid_t LSS_NAME(getpid)(); On 2014/04/03 21:44:22, Mark Seaborn wrote: > Nit: should be "(void)" for when compiling this with -Wstrict-prototypes in C. Done. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2511: LSS_INLINE pid_t LSS_NAME(fork)() { On 2014/04/03 21:44:22, Mark Seaborn wrote: > Since this file is huge and unmanageable and difficult to read, I have a > suggestion: How about putting all your new polyfill definitions in a separate > section at the end of the file, with a clear "/* Polyfills */" comment? > > I think the file would be easier to read if the polyfills were separate from > straightforward syscall definitions. Ok, sounds good. Done. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2516: void * child_stack = NULL; On 2014/04/03 21:44:22, Mark Seaborn wrote: > Nit: remove space after "*" Done. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:2539: __asm__ __volatile__(/* if (fn == NULL || child_stack == NULL) On 2014/04/03 21:44:22, Mark Seaborn wrote: > How about doing these checks in C? Done. https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... linux_syscall_support.h:3714: #if defined(__ARM_EABI__) || defined (__aarch64__) || defined(__x86_64__) || \ On 2014/04/03 21:44:22, Mark Seaborn wrote: > Line is >80 chars Done.
On 2014/04/04 13:57:03, rmcilroy wrote: > Thanks for the review Mark. I've addressed the comments, PTAL. > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h > File linux_syscall_support.h (left): > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... > linux_syscall_support.h:2894: #if defined(__x86_64__) || > \ > On 2014/04/03 21:44:22, Mark Seaborn wrote: > > Could you possibly commit small cleanup refactorings like this part as > separate > > changes? > > Done. > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h > File linux_syscall_support.h (right): > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... > linux_syscall_support.h:481: unsigned long st_dev; // Device. > On 2014/04/03 21:44:22, Mark Seaborn wrote: > > Can you make the alignment indentation consistent for this struct, please? > > Done. > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... > linux_syscall_support.h:482: unsigned long st_ino; // File serial number. > On 2014/04/03 21:44:22, Mark Seaborn wrote: > > I'd leave out the comments for consistency with the other kernel_stat defs. > > Done. > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... > linux_syscall_support.h:945: #define __NR_pread64 67 > On 2014/04/03 21:44:22, Mark Seaborn wrote: > > Could you sort these by number? > > Done. > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... > linux_syscall_support.h:2442: /* Most definitions of _syscallX() neglect to mark > "memory" as being > On 2014/04/03 21:44:22, Mark Seaborn wrote: > > I realise this is copied from elsewhere in the file. I'm not sure what it > > means. Do you know what "Most definitions of _syscallX()" refers to? Most > > definitions in this file? > > I was assuming it meant the _syscallX() macros which used to be defined in glibc > (or similar libraries) - I think they got removed a while ago and replaced with > the syscall() function, so I'm not sure if this problem still exists. Shall I > remove these comments across the file? > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... > linux_syscall_support.h:2510: LSS_INLINE pid_t LSS_NAME(getpid)(); > On 2014/04/03 21:44:22, Mark Seaborn wrote: > > Nit: should be "(void)" for when compiling this with -Wstrict-prototypes in C. > > Done. > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... > linux_syscall_support.h:2511: LSS_INLINE pid_t LSS_NAME(fork)() { > On 2014/04/03 21:44:22, Mark Seaborn wrote: > > Since this file is huge and unmanageable and difficult to read, I have a > > suggestion: How about putting all your new polyfill definitions in a separate > > section at the end of the file, with a clear "/* Polyfills */" comment? > > > > I think the file would be easier to read if the polyfills were separate from > > straightforward syscall definitions. > > Ok, sounds good. Done. > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... > linux_syscall_support.h:2516: void * child_stack = NULL; > On 2014/04/03 21:44:22, Mark Seaborn wrote: > > Nit: remove space after "*" > > Done. > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... > linux_syscall_support.h:2539: __asm__ __volatile__(/* if (fn == NULL || > child_stack == NULL) > On 2014/04/03 21:44:22, Mark Seaborn wrote: > > How about doing these checks in C? > > Done. > > https://codereview.chromium.org/220933002/diff/20001/linux_syscall_support.h#... > linux_syscall_support.h:3714: #if defined(__ARM_EABI__) || defined (__aarch64__) > || defined(__x86_64__) || \ > On 2014/04/03 21:44:22, Mark Seaborn wrote: > > Line is >80 chars > > Done. I've just tested this with the breakpad_unittests, and the calls made using this syscall support library seem to work. There was one bug, which was the definition of kernel_dirent on aarch64, which I have fixed by adding a "#define kernel_dirent kernel_dirent64" (since there is no non 64 dirent on aarch64). PTAL, thanks.
My main non-trivial comment is a question about fork()... https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:498: long st_atime; All the other defs of st_[amc]time{,_nsec} have a trailing "_", which I think is necessary to avoid conflicts with glibc's #defines of st_[amc]time. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2520: return -EINVAL; Shouldn't this be setting errno, as LSS_RETURN does? This seems to be a bug in the existing clone() implementations. Rather than returning EINVAL or setting errno, you might just drop the NULL checks, since they don't seem very useful. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3932: #if defined(__aarch64__) All of this added block can be indented by 2 spaces fewer (it's not inside the #if that the earlier code is in). https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3935: int, flags, int, fd, off64_t, offset) You might need to change off64_t to int64_t (see the recent change r25), if glibc defines the same typedefs on aarch64 as on x86-64. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3938: LSS_INLINE _syscall2(int, pipe2, int *, pipefd, int, flags) You've got a mix of "type *" and "type*" spacing styles here. Can you make these consistent in this added code? https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3951: LSS_INLINE int LSS_NAME(dup2) (int s, int d) { Nit: remove space between ") (". Same in some other cases below. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3955: LSS_INLINE int LSS_NAME(open)(const char* pathname, int flags, int mode) { Similar comment about "*" spacing. Most of the existing code seems to use "type *". https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3982: return LSS_NAME(ppoll) (fds, nfds, timeout_ts_p, NULL, 0); Shouldn't the last arg be NULL too, since it's a pointer? https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3986: struct kernel_stat *buf) { Nit: align arg to match "(" https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3992: pid_t pid = LSS_NAME(getpid)(); It's not obvious to me why you'd need this... https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3998: void *child_tidptr = &pid; ...or this, since you don't return pid, you return the result of clone().
Thanks for the comments Mark. All addressed. PTAL. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:498: long st_atime; On 2014/04/08 16:52:45, Mark Seaborn wrote: > All the other defs of st_[amc]time{,_nsec} have a trailing "_", which I think is > necessary to avoid conflicts with glibc's #defines of st_[amc]time. Done. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2520: return -EINVAL; On 2014/04/08 16:52:45, Mark Seaborn wrote: > Shouldn't this be setting errno, as LSS_RETURN does? This seems to be a bug in > the existing clone() implementations. > > Rather than returning EINVAL or setting errno, you might just drop the NULL > checks, since they don't seem very useful. Good point - dropped the NULL checks. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3932: #if defined(__aarch64__) On 2014/04/08 16:52:45, Mark Seaborn wrote: > All of this added block can be indented by 2 spaces fewer (it's not inside the > #if that the earlier code is in). Done. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3935: int, flags, int, fd, off64_t, offset) On 2014/04/08 16:52:45, Mark Seaborn wrote: > You might need to change off64_t to int64_t (see the recent change r25), if > glibc defines the same typedefs on aarch64 as on x86-64. Looks like it does - updated to int64_t. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3938: LSS_INLINE _syscall2(int, pipe2, int *, pipefd, int, flags) On 2014/04/08 16:52:45, Mark Seaborn wrote: > You've got a mix of "type *" and "type*" spacing styles here. Can you make > these consistent in this added code? Done. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3951: LSS_INLINE int LSS_NAME(dup2) (int s, int d) { On 2014/04/08 16:52:45, Mark Seaborn wrote: > Nit: remove space between ") (". Same in some other cases below. Done. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3955: LSS_INLINE int LSS_NAME(open)(const char* pathname, int flags, int mode) { On 2014/04/08 16:52:45, Mark Seaborn wrote: > Similar comment about "*" spacing. Most of the existing code seems to use "type > *". Done. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3982: return LSS_NAME(ppoll) (fds, nfds, timeout_ts_p, NULL, 0); On 2014/04/08 16:52:45, Mark Seaborn wrote: > Shouldn't the last arg be NULL too, since it's a pointer? It's not a pointer, it's "size_t s" (the size of the sigmask). This is an additional argument on the raw syscall compared to the ppoll() library function defined by glibc. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3986: struct kernel_stat *buf) { On 2014/04/08 16:52:45, Mark Seaborn wrote: > Nit: align arg to match "(" Done. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3992: pid_t pid = LSS_NAME(getpid)(); On 2014/04/08 16:52:45, Mark Seaborn wrote: > It's not obvious to me why you'd need this... See below https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3998: void *child_tidptr = &pid; On 2014/04/08 16:52:45, Mark Seaborn wrote: > ...or this, since you don't return pid, you return the result of clone(). Yeah, this was for the CLONE_CHILD_SETTID and CLONE_CHILD_CLEARTID flags, which messes with the tid pointed too by this argument in the child process. GLibC passes a pointer to the tid field in the thread's "struct pthread" data structure, however, we can't really get at that structure out-with glibc, so we wouldn't be able to update the right field anyway. I was originally passing the pid in-case it was important for clone to read the value as well as update it, but it doesn't seem to and you are right - given that it will change a local variable which I am not returning there doesn't seem to be much reason to do this. It seems to work fine without these flags or the pointer, so I've removed them.
On 2014/04/09 12:37:06, rmcilroy wrote: > Thanks for the comments Mark. All addressed. PTAL. > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h > File linux_syscall_support.h (right): > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:498: long st_atime; > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > All the other defs of st_[amc]time{,_nsec} have a trailing "_", which I think > is > > necessary to avoid conflicts with glibc's #defines of st_[amc]time. > > Done. > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:2520: return -EINVAL; > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > Shouldn't this be setting errno, as LSS_RETURN does? This seems to be a bug > in > > the existing clone() implementations. > > > > Rather than returning EINVAL or setting errno, you might just drop the NULL > > checks, since they don't seem very useful. > > Good point - dropped the NULL checks. > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:3932: #if defined(__aarch64__) > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > All of this added block can be indented by 2 spaces fewer (it's not inside the > > #if that the earlier code is in). > > Done. > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:3935: int, flags, int, fd, off64_t, offset) > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > You might need to change off64_t to int64_t (see the recent change r25), if > > glibc defines the same typedefs on aarch64 as on x86-64. > > Looks like it does - updated to int64_t. > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:3938: LSS_INLINE _syscall2(int, pipe2, int *, pipefd, > int, flags) > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > You've got a mix of "type *" and "type*" spacing styles here. Can you make > > these consistent in this added code? > > Done. > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:3951: LSS_INLINE int LSS_NAME(dup2) (int s, int d) { > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > Nit: remove space between ") (". Same in some other cases below. > > Done. > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:3955: LSS_INLINE int LSS_NAME(open)(const char* > pathname, int flags, int mode) { > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > Similar comment about "*" spacing. Most of the existing code seems to use > "type > > *". > > Done. > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:3982: return LSS_NAME(ppoll) (fds, nfds, timeout_ts_p, > NULL, 0); > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > Shouldn't the last arg be NULL too, since it's a pointer? > > It's not a pointer, it's "size_t s" (the size of the sigmask). This is an > additional argument on the raw syscall compared to the ppoll() library function > defined by glibc. > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:3986: struct kernel_stat *buf) { > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > Nit: align arg to match "(" > > Done. > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:3992: pid_t pid = LSS_NAME(getpid)(); > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > It's not obvious to me why you'd need this... > > See below > > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:3998: void *child_tidptr = &pid; > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > ...or this, since you don't return pid, you return the result of clone(). > > Yeah, this was for the CLONE_CHILD_SETTID and CLONE_CHILD_CLEARTID flags, which > messes with the tid pointed too by this argument in the child process. GLibC > passes a pointer to the tid field in the thread's "struct pthread" data > structure, however, we can't really get at that structure out-with glibc, so we > wouldn't be able to update the right field anyway. > > I was originally passing the pid in-case it was important for clone to read the > value as well as update it, but it doesn't seem to and you are right - given > that it will change a local variable which I am not returning there doesn't seem > to be much reason to do this. It seems to work fine without these flags or the > pointer, so I've removed them. Ping?
LGTM. I'm not familiar with AArch64, BTW, so you might want to get someone who is to look at the assembly code. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3998: void *child_tidptr = &pid; On 2014/04/09 12:37:07, rmcilroy wrote: > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > ...or this, since you don't return pid, you return the result of clone(). > > Yeah, this was for the CLONE_CHILD_SETTID and CLONE_CHILD_CLEARTID flags, which > messes with the tid pointed too by this argument in the child process. GLibC > passes a pointer to the tid field in the thread's "struct pthread" data > structure, however, we can't really get at that structure out-with glibc, so we > wouldn't be able to update the right field anyway. Hmm, I see what you mean. I suppose you got this from glibc code which does: #define ARCH_FORK() \ INLINE_SYSCALL (clone, 4, \ CLONE_CHILD_SETTID | CLONE_CHILD_CLEARTID | SIGCHLD, 0, \ NULL, &THREAD_SELF->tid) It might be worth adding a comment like "Note that this does not reset glibc's cached view of the PID/TID, so some glibc interfaces might go wrong in the forked subprocess". Of course, this will be true when calling the fork() syscall directly too, on other architectures.
Thanks for the review Mark. Jacob: Could you have a quick glance at the arm64 assembly and check it's sane please. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:3998: void *child_tidptr = &pid; On 2014/04/14 19:15:21, Mark Seaborn wrote: > On 2014/04/09 12:37:07, rmcilroy wrote: > > On 2014/04/08 16:52:45, Mark Seaborn wrote: > > > ...or this, since you don't return pid, you return the result of clone(). > > > > Yeah, this was for the CLONE_CHILD_SETTID and CLONE_CHILD_CLEARTID flags, > which > > messes with the tid pointed too by this argument in the child process. GLibC > > passes a pointer to the tid field in the thread's "struct pthread" data > > structure, however, we can't really get at that structure out-with glibc, so > we > > wouldn't be able to update the right field anyway. > > Hmm, I see what you mean. I suppose you got this from glibc code which does: > > #define ARCH_FORK() \ > INLINE_SYSCALL (clone, 4, \ > CLONE_CHILD_SETTID | CLONE_CHILD_CLEARTID | SIGCHLD, 0, \ > NULL, &THREAD_SELF->tid) > > It might be worth adding a comment like "Note that this does not reset glibc's > cached view of the PID/TID, so some glibc interfaces might go wrong in the > forked subprocess". Of course, this will be true when calling the fork() > syscall directly too, on other architectures. Yes exactly. I added the comment. https://codereview.chromium.org/220933002/diff/140001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/220933002/diff/140001/linux_syscall_support.h... linux_syscall_support.h:2542: "adds %0, xzr, x0\n" Jacob: Note - this is really meant to be a "movs %0, x0" - but that doesn't seem be an intrinsic in gcc inline asm - this should be the same thing though.
I have a few suggestions, but it looks sane. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2453: #define LSS_REG(r,a) register long __r##r __asm__("x"#r) = (long)a Something like int64_t would be better. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2462: : "x8", "x30", "memory"); \ The kernel preserves everything except x0, so x30 doesn't need to be in this list. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2518: long __res; Something like int64_t would be better. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2523: register int __flags __asm__("x0") = flags; `flags` is W-sized. Does this work? Perhaps make it a uint64_t to be safe. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2531: "stp %1, %4, [%2, #-16]!\n" Named assembly arguments would make this easier to follow. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2545: "adds %0, xzr, x0\n" You can use cbnz for this: cbnz x0, 1f
Thanks for the review Jacob. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h File linux_syscall_support.h (right): https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2453: #define LSS_REG(r,a) register long __r##r __asm__("x"#r) = (long)a On 2014/04/15 10:16:42, jbramley wrote: > Something like int64_t would be better. Done. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2462: : "x8", "x30", "memory"); \ On 2014/04/15 10:16:42, jbramley wrote: > The kernel preserves everything except x0, so x30 doesn't need to be in this > list. Done. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2518: long __res; On 2014/04/15 10:16:42, jbramley wrote: > Something like int64_t would be better. Done. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2523: register int __flags __asm__("x0") = flags; On 2014/04/15 10:16:42, jbramley wrote: > `flags` is W-sized. Does this work? Perhaps make it a uint64_t to be safe. It does work, but I agree making flags uint64_t size is probably safer. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2531: "stp %1, %4, [%2, #-16]!\n" On 2014/04/15 10:16:42, jbramley wrote: > Named assembly arguments would make this easier to follow. If you mean named registers, I'm not sure this would be much easier to follow and I would prefer to keep this consistent with the other arches. https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... linux_syscall_support.h:2545: "adds %0, xzr, x0\n" On 2014/04/15 10:16:42, jbramley wrote: > You can use cbnz for this: > > cbnz x0, 1f That wouldn't move x0 to __res though.
> https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:2531: "stp %1, %4, [%2, #-16]!\n" > On 2014/04/15 10:16:42, jbramley wrote: > > Named assembly arguments would make this easier to follow. > > If you mean named registers, I'm not sure this would be much easier to follow > and I would prefer to keep this consistent with the other arches. Fair enough. > https://codereview.chromium.org/220933002/diff/60001/linux_syscall_support.h#... > linux_syscall_support.h:2545: "adds %0, xzr, x0\n" > On 2014/04/15 10:16:42, jbramley wrote: > > You can use cbnz for this: > > > > cbnz x0, 1f > > That wouldn't move x0 to __res though. Good point! Well in that case, the following is probably still clearer: mov %0, x0 cbnz x0, 1f The performance ought to be the same, so make your own judgement about what is clearest. lgtm
> Good point! Well in that case, the following is probably still clearer: > > mov %0, x0 > cbnz x0, 1f > > The performance ought to be the same, so make your own judgement about what is > clearest. Sounds good, done.
Message was sent while issue was closed.
Committed patchset #9 manually as r26 (presubmit successful). |
