This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v4] Fix clone (CLONE_VM) pid/tid reset (BZ#19957)
- From: Chris Metcalf <cmetcalf at mellanox dot com>
- To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>, <libc-alpha at sourceware dot org>
- Date: Thu, 28 Apr 2016 16:07:27 -0400
- Subject: Re: [PATCH v4] Fix clone (CLONE_VM) pid/tid reset (BZ#19957)
- Authentication-results: sourceware.org; auth=none
- Authentication-results: sourceware.org; dkim=none (message not signed) header.d=none;sourceware.org; dmarc=none action=none header.from=mellanox.com;
- References: <1461685391-5669-1-git-send-email-adhemerval dot zanella at linaro dot org>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:23
On 4/26/2016 11:43 AM, Adhemerval Zanella wrote:
--- a/sysdeps/unix/sysv/linux/tile/clone.S
+++ b/sysdeps/unix/sysv/linux/tile/clone.S
@@ -164,43 +164,31 @@ ENTRY (__clone)
cfi_def_cfa_offset (FRAME_SIZE)
cfi_undefined (lr)
/* Check and see if we need to reset the PID, which we do if
- CLONE_THREAD isn't set, i.e. we're not staying in the thread group.
- If CLONE_VM is set, we're doing some kind of thread-like clone,
- so we set the tid/pid to -1 to disable using the cached values
- in getpid(). Otherwise (if CLONE_VM isn't set), it's a
+ CLONE_VM isn't set, i.e. we're do not share the same pthread_t
+ with parent. Otherwise (if CLONE_VM isn't set), it's a
fork-like clone, and we go ahead and write the cached values
from the true system pid (retrieved via __NR_getpid syscall). */
I'd change this to:
/* Check and see if we need to reset the PID, which we do if
CLONE_VM isn't set, i.e. it's a fork-like clone with a new
address space. In that case we update the cached values
from the true system pid (retrieved via __NR_getpid syscall). */
#ifdef __tilegx__
{
moveli r0, hw1_last(CLONE_VM)
- moveli r1, hw1_last(CLONE_THREAD)
}
{
shl16insli r0, r0, hw0(CLONE_VM)
- shl16insli r1, r1, hw0(CLONE_THREAD)
}
#else
{
moveli r0, lo16(CLONE_VM)
- moveli r1, lo16(CLONE_THREAD)
}
{
auli r0, r0, ha16(CLONE_VM)
- auli r1, r1, ha16(CLONE_THREAD)
}
#endif
{
and r0, r30, r0
- and r1, r30, r1
- }
- BNEZ r1, .Lno_reset_pid /* CLONE_THREAD is set */
- {
- movei r0, -1
- BNEZ r0, .Lgotpid /* CLONE_VM is set */
}
Here you have left the "{ }" around some single instructions. Stylistically,
we typically don't do that. Also, we only used the two-instruction format
for loading up r0/r1 because CLONE_THREAD needed it; CLONE_VM doesn't,
so we can collapse all the quoted code above down to just:
moveli r0, CLONE_VM
and r0, r30, r0
We could do this in one instruction if we had the bit position (i.e. log-2) of
CLONE_VM, but the obfuscation is not worth it on this not very hot path.
+ BNEZ r0, .Lno_reset_pid /* CLONE_VM is set */
moveli TREG_SYSCALL_NR_NAME, __NR_getpid
swint1
-.Lgotpid:
ADDLI_PTR r2, tp, PID_OFFSET
{
ST4 r2, r0
With those changes, looks good to me.
--
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com