Message ID | 571E5F5B.6080903@redhat.com |
---|---|
State | New |
Headers | show |
On 25/04/2016 15:18, Carlos O'Donell wrote: > On 04/22/2016 02:54 PM, Adhemerval Zanella wrote: >> Changes from previous version: >> >> * 'Fixed' the remaning ports: alpha, microblaze, sh, ia64, tile, >> hppa, m68k, and nio2. I did actually tested the fix for these >> architecture in specific, so any review would be appreciated. > > Looks good to me. > > Thanks for the new test! > > Two required changes: > * hppa changes are wrong, fixed version provided below. > * ppc32 comment needs fixup. > > Suggestions: > * Mention bug number in test. > * Suggest changes to test comments. Thanks for the review. As we discussed in IRC I think I will just commit this change and let the machine maintainers check if tst-clone2 is really passing on the cited architectures. I will prob commit by the end of the week. > >> diff --git a/sysdeps/unix/sysv/linux/hppa/clone.S b/sysdeps/unix/sysv/linux/hppa/clone.S >> index 0a18137..3716acd 100644 >> --- a/sysdeps/unix/sysv/linux/hppa/clone.S >> +++ b/sysdeps/unix/sysv/linux/hppa/clone.S > > Should be: > > diff --git a/sysdeps/unix/sysv/linux/hppa/clone.S b/sysdeps/unix/sysv/linux/hppa/clone.S > index 0a18137..43f5a78 100644 > --- a/sysdeps/unix/sysv/linux/hppa/clone.S > +++ b/sysdeps/unix/sysv/linux/hppa/clone.S > @@ -135,17 +135,12 @@ ENTRY(__clone) > # define CLONE_VM_BIT 23 /* 0x00000100 */ > # define CLONE_THREAD_BIT 15 /* 0x00010000 */ > /* Load original clone flags. > - If CLONE_THREAD was passed, don't reset the PID/TID. > - If CLONE_VM was passed, we need to store -1 to PID/TID. > - If CLONE_VM and CLONE_THREAD were not set store the result > - of getpid to PID/TID. */ > + If CLONE_VM was passed, don't modify PID/TID. > + Otherwise store the result of getpid to PID/TID. */ > ldw -56(%sp), %r26 > - bb,<,n %r26, CLONE_THREAD_BIT, 1f > - bb,< %r26, CLONE_VM_BIT, 2f > - ldi -1, %ret0 > + bb,<,n %r26, CLONE_VM_BIT, 1f > ble 0x100(%sr2, %r0) > ldi __NR_getpid, %r20 > -2: > mfctl %cr27, %r26 > stw %ret0, PID_THREAD_OFFSET(%r26) > stw %ret0, TID_THREAD_OFFSET(%r26) > --- > > Your original v3 patch removes the 'ble' required for the syscall. Ack, I will change it. > >> diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S b/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S >> index eb973db..3761ded 100644 >> --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S >> +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S >> @@ -76,13 +76,11 @@ ENTRY (__clone) >> crandc cr1*4+eq,cr1*4+eq,cr0*4+so >> bne- cr1,L(parent) /* The '-' is to minimise the race. */ >> >> - andis. r0,r28,CLONE_THREAD>>16 >> - bne+ r0,L(oldpid) >> - andi. r0,r28,CLONE_VM >> - li r3,-1 >> - bne- r0,L(nomoregetpid) >> + /* If CLONE_VM is set does not update the pid/tid field. */ > > s/does not/do not/g Likewise. >> diff --git a/sysdeps/unix/sysv/linux/tst-clone2.c b/sysdeps/unix/sysv/linux/tst-clone2.c >> new file mode 100644 >> index 0000000..7452a47 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/tst-clone2.c >> @@ -0,0 +1,178 @@ >> +/* Test if CLONE_VM does not change pthread pid/tid field. > > Mention bug #. Likewise. >> +int >> +do_test (void) >> +{ >> + /* It first checks the clone implementation without any flag, which will >> + make the child new pid to be cached on its pthread structure. */ > > Suggest: > > /* First, check that the clone implementation, without any flag, updates > the struct pthread to contain the new PID and TID. */ Likewise. > >> + int ret = clone_test (0); >> + /* Then check if clone with CLONE_VM avoid caching it since memory is >> + shared between parent and it might overwrite parent's pthread >> + structure. */ > > Suggest: > > /* Second, check that with CLONE_VM the struct pthread PID and TID fields > remain unmodified after the clone. Any modifications would cause problem > for the parent as described in bug 19957. */ Likewise.
diff --git a/sysdeps/unix/sysv/linux/hppa/clone.S b/sysdeps/unix/sysv/linux/hppa/clone.S index 0a18137..43f5a78 100644 --- a/sysdeps/unix/sysv/linux/hppa/clone.S +++ b/sysdeps/unix/sysv/linux/hppa/clone.S @@ -135,17 +135,12 @@ ENTRY(__clone) # define CLONE_VM_BIT 23 /* 0x00000100 */ # define CLONE_THREAD_BIT 15 /* 0x00010000 */ /* Load original clone flags. - If CLONE_THREAD was passed, don't reset the PID/TID. - If CLONE_VM was passed, we need to store -1 to PID/TID. - If CLONE_VM and CLONE_THREAD were not set store the result - of getpid to PID/TID. */ + If CLONE_VM was passed, don't modify PID/TID. + Otherwise store the result of getpid to PID/TID. */ ldw -56(%sp), %r26 - bb,<,n %r26, CLONE_THREAD_BIT, 1f - bb,< %r26, CLONE_VM_BIT, 2f - ldi -1, %ret0 + bb,<,n %r26, CLONE_VM_BIT, 1f ble 0x100(%sr2, %r0) ldi __NR_getpid, %r20 -2: mfctl %cr27, %r26 stw %ret0, PID_THREAD_OFFSET(%r26) stw %ret0, TID_THREAD_OFFSET(%r26)