Re: [PATCH] arm: add RESET_PID in the clone impl
Hi, I don't think clone on arm is missing RESET_PID if you use the correct implementation from libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S. Cheers Carmelo Inviato con AquaMail per Android http://www.aqua-mail.com Il 02 settembre 2014 14:12:24 Wangyufen ha scritto: From: Wang Yufen Called getpid() When creating a new process with clone(), getpid() returns the father_process's value. It should be child_process's value. The reason is missing a RESET_PID in the arm clone impl. Signed-off-by: Wang Yufen --- libc/sysdeps/linux/arm/clone.S | 61 ++- libc/sysdeps/linux/arm/sysdep.h | 45 2 files changed, 92 insertions(+), 14 deletions(-) diff --git a/libc/sysdeps/linux/arm/clone.S b/libc/sysdeps/linux/arm/clone.S index 03cd10e..29045ef 100644 --- a/libc/sysdeps/linux/arm/clone.S +++ b/libc/sysdeps/linux/arm/clone.S @@ -19,12 +19,17 @@ /* clone() is even more special than fork() as it mucks with stacks and invokes a function in the right context after its all over. */ +#include #define _ERRNO_H #include #include #include #include #include +#include + +#define CLONE_VM 0x0100 +#define CLONE_THREAD 0x0001 #if defined(__NR_clone) /* int clone(int (*fn)(void *arg), void *child_stack, int flags, void *arg); */ @@ -87,6 +92,8 @@ __error: .pool #else __clone: +.fnstart +.cantunwind @ sanity check args cmp r0, #0 IT(te, ne) @@ -95,32 +102,58 @@ __clone: beq __error @ insert the args onto the new stack - sub r1, r1, #8 - str r3, [r1, #4] - @ save the function pointer as the 0th element - str r0, [r1] + str r3, [r1, #-4]! + str r0, [r1, #-4]! @ do the system call @ get flags mov r0, r2 +#ifdef RESET_PID + mov ip, r2 +#endif @ new sp is already in r1 - @ load remaining arguments off the stack - stmfd sp!, {r4} - ldr r2, [sp, #4] - ldr r3, [sp, #8] - ldr r4, [sp, #12] - DO_CALL (clone) - movsa1, a1 - IT(t, ne) - ldmnefd sp!, {r4} + push{r4, r7} + cfi_adjust_cfa_offset (8) + cfi_rel_offset (r4, 0) + cfi_rel_offset (r7, 4) + ldr r2, [sp, #8] + ldr r3, [sp, #12] + ldr r4, [sp, #16] + ldr r7, =SYS_ify(clone) + swi 0x0 + cfi_endproc + cmp r0, #0 + beq 1f + pop {r4, r7} blt __error - IT(t, ne) #if defined(__USE_BX__) bxnelr #else movne pc, lr #endif + cfi_startproc +.fnend +PSEUDO_END (__clone) + +1: + .fnstart + .cantunwind +#ifdef RESET_PID + tst ip, #CLONE_THREAD + bne 3f + GET_TLS (lr) + mov r1, r0 + tst ip, #CLONE_VM + ldr r7, =SYS_ify(getpid) + ite ne + movne r0, #-1 + swieq 0x0 + NEGOFF_ADJ_BASE (r1, TID_OFFSET) + str r0, NEGOFF_OFF1 (r1, TID_OFFSET) + str r0, NEGOFF_OFF2 (r1, PID_OFFSET, TID_OFFSET) +3: +#endif @ pick the function arg and call address off the stack and execute ldr r0, [sp, #4] mov lr, pc diff --git a/libc/sysdeps/linux/arm/sysdep.h b/libc/sysdeps/linux/arm/sysdep.h index 64f4040..38a131d 100644 --- a/libc/sysdeps/linux/arm/sysdep.h +++ b/libc/sysdeps/linux/arm/sysdep.h @@ -213,6 +213,51 @@ __local_syscall_error: \ sees the right arguments. */ +#if __ARM_ARCH > 4 || defined (__ARM_ARCH_4T__) +# define ARCH_HAS_BX +#endif +#if __ARM_ARCH > 4 +# define ARCH_HAS_BLX +#endif +#if __ARM_ARCH > 6 || defined (__ARM_ARCH_6K__) || defined (__ARM_ARCH_6ZK__) +# define ARCH_HAS_HARD_TP +#endif +#if __ARM_ARCH > 6 || defined (__ARM_ARCH_6T2__) +# define ARCH_HAS_T2 +#endif + +# ifdef __thumb2__ +# define NEGOFF_ADJ_BASE(R, OFF) add R, R, $OFF +# define NEGOFF_ADJ_BASE2(D, S, OFF) add D, S, $OFF +# define NEGOFF_OFF1(R, OFF) [R] +# define NEGOFF_OFF2(R, OFFA, OFFB) [R, $((OFFA) - (OFFB))] +# else +# define NEGOFF_ADJ_BASE(R, OFF) +# define NEGOFF_ADJ_BASE2(D, S, OFF) mov D, S +# define NEGOFF_OFF1(R, OFF) [R, $OFF] +# define NEGOFF_OFF2(R, OFFA, OFFB) [R, $OFFA] +# endif + +# ifdef ARCH_HAS_HARD_TP +/* If the cpu has cp15 available, use it. */ +# define GET_TLS(TMP) mrc p15, 0, r0, c13, c0, 3 +# else +/* At this generic level we have no tricks to pull. Call the ABI routine. */ +# define GET_TLS(TMP) \ + push{ r1, r2, r3, lr }; \ + cfi_remember_state; \ + cfi_adjust_cfa_offset (16); \ + cfi_rel_offset (r1, 0); \ + cfi_rel_offset (r2, 4); \ + cfi_rel_offset (r3, 8);
Re: [PATCH] arm: add RESET_PID in the clone impl
Hi Carmelo, Carmelo Amoroso wrote, > Hi, > I don't think clone on arm is missing RESET_PID if you use the > correct implementation from > libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S. Can you explain this a little bit more? cat ./libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S #define RESET_PID #include #include "../../../../../../../libc/sysdeps/linux/arm/clone.S" Do you think the test-suite failures on arm come from a buildsystem problem? best regards Waldemar > Cheers > Carmelo > > Inviato con AquaMail per Android > http://www.aqua-mail.com > > > Il 02 settembre 2014 14:12:24 Wangyufen ha scritto: > > >From: Wang Yufen > > > >Called getpid() When creating a new process with clone(), getpid() returns > >the father_process's value. It should be child_process's value. > >The reason is missing a RESET_PID in the arm clone impl. > > > >Signed-off-by: Wang Yufen > >--- > > libc/sysdeps/linux/arm/clone.S | 61 > > ++- > > libc/sysdeps/linux/arm/sysdep.h | 45 > > 2 files changed, 92 insertions(+), 14 deletions(-) > > > >diff --git a/libc/sysdeps/linux/arm/clone.S b/libc/sysdeps/linux/arm/clone.S > >index 03cd10e..29045ef 100644 > >--- a/libc/sysdeps/linux/arm/clone.S > >+++ b/libc/sysdeps/linux/arm/clone.S > >@@ -19,12 +19,17 @@ > > /* clone() is even more special than fork() as it mucks with stacks > >and invokes a function in the right context after its all over. */ > > > >+#include > > #define _ERRNO_H > > #include > > #include > > #include > > #include > > #include > >+#include > >+ > >+#define CLONE_VM 0x0100 > >+#define CLONE_THREAD 0x0001 > > > > #if defined(__NR_clone) > > /* int clone(int (*fn)(void *arg), void *child_stack, int flags, void > > *arg); */ > >@@ -87,6 +92,8 @@ __error: > > .pool > > #else > > __clone: > >+.fnstart > >+.cantunwind > > @ sanity check args > > cmp r0, #0 > > IT(te, ne) > >@@ -95,32 +102,58 @@ __clone: > > beq __error > > > > @ insert the args onto the new stack > >-sub r1, r1, #8 > >-str r3, [r1, #4] > >-@ save the function pointer as the 0th element > >-str r0, [r1] > >+str r3, [r1, #-4]! > >+str r0, [r1, #-4]! > > > > @ do the system call > > @ get flags > > mov r0, r2 > >+#ifdef RESET_PID > >+mov ip, r2 > >+#endif > > @ new sp is already in r1 > >-@ load remaining arguments off the stack > >-stmfd sp!, {r4} > >-ldr r2, [sp, #4] > >-ldr r3, [sp, #8] > >-ldr r4, [sp, #12] > >-DO_CALL (clone) > >-movsa1, a1 > >-IT(t, ne) > >-ldmnefd sp!, {r4} > >+push{r4, r7} > >+cfi_adjust_cfa_offset (8) > >+cfi_rel_offset (r4, 0) > >+cfi_rel_offset (r7, 4) > >+ldr r2, [sp, #8] > >+ldr r3, [sp, #12] > >+ldr r4, [sp, #16] > >+ldr r7, =SYS_ify(clone) > >+swi 0x0 > >+cfi_endproc > >+cmp r0, #0 > >+beq 1f > >+pop {r4, r7} > > blt __error > >-IT(t, ne) > > #if defined(__USE_BX__) > > bxnelr > > #else > > movne pc, lr > > #endif > > > >+cfi_startproc > >+.fnend > >+PSEUDO_END (__clone) > >+ > >+1: > >+.fnstart > >+.cantunwind > >+#ifdef RESET_PID > >+tst ip, #CLONE_THREAD > >+bne 3f > >+GET_TLS (lr) > >+mov r1, r0 > >+tst ip, #CLONE_VM > >+ldr r7, =SYS_ify(getpid) > >+ite ne > >+movne r0, #-1 > >+swieq 0x0 > >+NEGOFF_ADJ_BASE (r1, TID_OFFSET) > >+str r0, NEGOFF_OFF1 (r1, TID_OFFSET) > >+str r0, NEGOFF_OFF2 (r1, PID_OFFSET, TID_OFFSET) > >+3: > >+#endif > > @ pick the function arg and call address off the stack and execute > > ldr r0, [sp, #4] > > mov lr, pc > >diff --git a/libc/sysdeps/linux/arm/sysdep.h > >b/libc/sysdeps/linux/arm/sysdep.h > >index 64f4040..38a131d 100644 > >--- a/libc/sysdeps/linux/arm/sysdep.h > >+++ b/libc/sysdeps/linux/arm/sysdep.h > >@@ -213,6 +213,51 @@ __local_syscall_error: > >\ > >sees the right arguments. > > > > */ > >+#if __ARM_ARCH > 4 || defined (__ARM_ARCH_4T__) > >+# define ARCH_HAS_BX > >+#endif > >+#if __ARM_ARCH > 4 > >+# define ARCH_HAS_BLX > >+#endif > >+#if __ARM_ARCH > 6 || defined (__ARM_ARCH_6K__) || defined > >(__ARM_ARCH_6ZK__) > >+# define ARCH_HAS_HARD_TP > >+#endif > >+#if __ARM_ARCH > 6 || defined (__ARM_ARCH_6T2__) > >+# define ARCH_HAS_T2 > >+#endif > >+ > >+# ifdef __thumb2__ > >+# define NEGOFF_ADJ_BASE(R, OFF) add R, R, $OFF > >+# define NEGOFF_ADJ_BASE2(D, S, OFF) add D, S, $OFF > >+# define NEGOFF_OFF1(R, OFF) [R] > >+# define NEGOFF_OFF2(R, OFFA, OFFB)[R, $((OFFA) - (OFFB))] > >+# else > >+# define NEGOFF_ADJ_BASE(R, OFF) > >+# define NEGOFF_ADJ_BASE2(D, S, OFF) mov D, S > >+# define NEGOFF_OFF1(R, OFF) [R, $OFF] > >+# define NEGOFF_OFF2
Re: [PATCH] arm: add RESET_PID in the clone impl
On 4 September 2014 00:01, Waldemar Brodkorb wrote: > Hi Carmelo, > Carmelo Amoroso wrote, > >> Hi, >> I don't think clone on arm is missing RESET_PID if you use the >> correct implementation from >> libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S. > > Can you explain this a little bit more? > > cat ./libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S > #define RESET_PID > #include > #include "../../../../../../../libc/sysdeps/linux/arm/clone.S" > > Do you think the test-suite failures on arm come from a buildsystem > problem? No, the patch is fine and needed. ARCH_HAS_BX and ARCH_HAS_BLX should be deleted though, we handle that via bits/arm_bx.h Can somebody run this through a test-harness for !threads and NPTL on the affected ISA revisions just to be sure? That'd help. TIA, ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] arm: add RESET_PID in the clone impl
Il 04 settembre 2014 00:01:38 Waldemar Brodkorb ha scritto: Hi Carmelo, Hello, Carmelo Amoroso wrote, > Hi, > I don't think clone on arm is missing RESET_PID if you use the > correct implementation from > libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S. Can you explain this a little bit more? What I mean to say is that if the root cause is the lack of RESET_PIN definition it's not the case. Then, I did not enter into the details of the patch, but at first glance I could see several other unrelated changes that could eventually have been split in more patches, but I could be wrong. On my platform armv7 I don't remember to have such failures. cat ./libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S #define RESET_PID #include #include "../../../../../../../libc/sysdeps/linux/arm/clone.S" Do you think the test-suite failures on arm come from a buildsystem problem? best regards Waldemar Carmelo > Cheers > Carmelo > > Inviato con AquaMail per Android > http://www.aqua-mail.com > > > Il 02 settembre 2014 14:12:24 Wangyufen ha scritto: > > >From: Wang Yufen > > > >Called getpid() When creating a new process with clone(), getpid() returns > >the father_process's value. It should be child_process's value. > >The reason is missing a RESET_PID in the arm clone impl. > > > >Signed-off-by: Wang Yufen > >--- > > libc/sysdeps/linux/arm/clone.S | 61 ++- > > libc/sysdeps/linux/arm/sysdep.h | 45 > > 2 files changed, 92 insertions(+), 14 deletions(-) > > > >diff --git a/libc/sysdeps/linux/arm/clone.S b/libc/sysdeps/linux/arm/clone.S > >index 03cd10e..29045ef 100644 > >--- a/libc/sysdeps/linux/arm/clone.S > >+++ b/libc/sysdeps/linux/arm/clone.S > >@@ -19,12 +19,17 @@ > > /* clone() is even more special than fork() as it mucks with stacks > >and invokes a function in the right context after its all over. */ > > > >+#include > > #define _ERRNO_H > > #include > > #include > > #include > > #include > > #include > >+#include > >+ > >+#define CLONE_VM 0x0100 > >+#define CLONE_THREAD 0x0001 > > > > #if defined(__NR_clone) > > /* int clone(int (*fn)(void *arg), void *child_stack, int flags, void *arg); */ > >@@ -87,6 +92,8 @@ __error: > > .pool > > #else > > __clone: > >+.fnstart > >+.cantunwind > > @ sanity check args > > cmp r0, #0 > > IT(te, ne) > >@@ -95,32 +102,58 @@ __clone: > > beq __error > > > > @ insert the args onto the new stack > >- sub r1, r1, #8 > >- str r3, [r1, #4] > >- @ save the function pointer as the 0th element > >- str r0, [r1] > >+ str r3, [r1, #-4]! > >+ str r0, [r1, #-4]! > > > > @ do the system call > > @ get flags > > mov r0, r2 > >+#ifdef RESET_PID > >+ mov ip, r2 > >+#endif > > @ new sp is already in r1 > >- @ load remaining arguments off the stack > >- stmfd sp!, {r4} > >- ldr r2, [sp, #4] > >- ldr r3, [sp, #8] > >- ldr r4, [sp, #12] > >- DO_CALL (clone) > >- movsa1, a1 > >- IT(t, ne) > >- ldmnefd sp!, {r4} > >+ push{r4, r7} > >+ cfi_adjust_cfa_offset (8) > >+ cfi_rel_offset (r4, 0) > >+ cfi_rel_offset (r7, 4) > >+ ldr r2, [sp, #8] > >+ ldr r3, [sp, #12] > >+ ldr r4, [sp, #16] > >+ ldr r7, =SYS_ify(clone) > >+ swi 0x0 > >+ cfi_endproc > >+ cmp r0, #0 > >+ beq 1f > >+ pop {r4, r7} > > blt __error > >- IT(t, ne) > > #if defined(__USE_BX__) > > bxnelr > > #else > > movne pc, lr > > #endif > > > >+ cfi_startproc > >+.fnend > >+PSEUDO_END (__clone) > >+ > >+1: > >+ .fnstart > >+ .cantunwind > >+#ifdef RESET_PID > >+ tst ip, #CLONE_THREAD > >+ bne 3f > >+ GET_TLS (lr) > >+ mov r1, r0 > >+ tst ip, #CLONE_VM > >+ ldr r7, =SYS_ify(getpid) > >+ ite ne > >+ movne r0, #-1 > >+ swieq 0x0 > >+ NEGOFF_ADJ_BASE (r1, TID_OFFSET) > >+ str r0, NEGOFF_OFF1 (r1, TID_OFFSET) > >+ str r0, NEGOFF_OFF2 (r1, PID_OFFSET, TID_OFFSET) > >+3: > >+#endif > > @ pick the function arg and call address off the stack and execute > > ldr r0, [sp, #4] > > mov lr, pc > >diff --git a/libc/sysdeps/linux/arm/sysdep.h b/libc/sysdeps/linux/arm/sysdep.h > >index 64f4040..38a131d 100644 > >--- a/libc/sysdeps/linux/arm/sysdep.h > >+++ b/libc/sysdeps/linux/arm/sysdep.h > >@@ -213,6 +213,51 @@ __local_syscall_error: \ > >sees the right arguments. > > > > */ > >+#if __ARM_ARCH > 4 || defined (__ARM_ARCH_4T__) > >+# define ARCH_HAS_BX > >+#endif > >+#if __ARM_ARCH > 4 > >+# define ARCH_HAS_BLX > >+#endif > >+#if __ARM_ARCH > 6 || defined (__ARM_ARCH_6K__) || defined (__ARM_ARCH_6ZK__) > >+# define ARCH_HAS_HARD_TP >
Re: [PATCH] arm: add RESET_PID in the clone impl
>On 4 September 2014 19:32, Carmelo Amoroso wrote: > > > Il 04 settembre 2014 00:01:38 Waldemar Brodkorb ha > scritto: > >> Hi Carmelo, > > > Hello, > > >> Carmelo Amoroso wrote, >> >> > Hi, >> > I don't think clone on arm is missing RESET_PID if you use the >> > correct implementation from >> > libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S. >> >> Can you explain this a little bit more? >> > > What I mean to say is that if the root cause is the lack of RESET_PIN > definition it's not the case. No, it's not the define of RESET_PID but nothing in the arm clone.S actually does anything with RESET_PID, i.e. the actual implementation is missing, AFAICS. > > Then, I did not enter into the details of the patch, but at first glance I > could see several other unrelated changes that could eventually have been > split in more patches, but I could be wrong. > On my platform armv7 I don't remember to have such failures. Curious how you get a correct getpid() on arm then? This whole caching stuff is a pain and should be removed instead. ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] arm: add RESET_PID in the clone impl
Il 04 settembre 2014 22:32:01 Bernhard Reutner-Fischer ha scritto: >On 4 September 2014 19:32, Carmelo Amoroso wrote: > > > Il 04 settembre 2014 00:01:38 Waldemar Brodkorb ha > scritto: > >> Hi Carmelo, > > > Hello, > > >> Carmelo Amoroso wrote, >> >> > Hi, >> > I don't think clone on arm is missing RESET_PID if you use the >> > correct implementation from >> > libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S. >> >> Can you explain this a little bit more? >> > > What I mean to say is that if the root cause is the lack of RESET_PIN > definition it's not the case. No, it's not the define of RESET_PID but nothing in the arm clone.S actually does anything with RESET_PID, i.e. the actual implementation is missing, AFAICS. Indeed by looking at the patch better I've noticed the actual fix. > > Then, I did not enter into the details of the patch, but at first glance I > could see several other unrelated changes that could eventually have been > split in more patches, but I could be wrong. > On my platform armv7 I don't remember to have such failures. Curious how you get a correct getpid() on arm then? This whole caching stuff is a pain and should be removed instead. I will test and check it again as soon as back from my bz trip... I could not remember correctly. For sure I did not debug it. Carmelo Inviato con AquaMail per Android http://www.aqua-mail.com ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc
Re: [PATCH] arm: add RESET_PID in the clone impl
On 2014/9/4 19:40, Bernhard Reutner-Fischer wrote: > On 4 September 2014 00:01, Waldemar Brodkorb wrote: >> Hi Carmelo, >> Carmelo Amoroso wrote, >> >>> Hi, >>> I don't think clone on arm is missing RESET_PID if you use the >>> correct implementation from >>> libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S. >> >> Can you explain this a little bit more? >> >> cat ./libpthread/nptl/sysdeps/unix/sysv/linux/arm/clone.S >> #define RESET_PID >> #include >> #include "../../../../../../../libc/sysdeps/linux/arm/clone.S" >> >> Do you think the test-suite failures on arm come from a buildsystem >> problem? > > No, the patch is fine and needed. > ARCH_HAS_BX and ARCH_HAS_BLX should be deleted though, we handle that > via bits/arm_bx.h Actually,to fix missing RESET_PID, I just need ARCH_HAS_HARD_TP. I'll resend the patch. Regards, Wang > Can somebody run this through a test-harness for !threads and NPTL on > the affected ISA revisions just to be sure? > > That'd help. > TIA, > ___ > uClibc mailing list > uClibc@uclibc.org > http://lists.busybox.net/mailman/listinfo/uclibc > > ___ uClibc mailing list uClibc@uclibc.org http://lists.busybox.net/mailman/listinfo/uclibc