Re: [PATCH v4] Fix clone (CLONE_VM) pid/tid reset (BZ#19957)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

Thanks for the review.

On 28/04/2016 17:07, Chris Metcalf wrote:
> 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).  */

Ack, I will change it.
 
> 
>>   #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.
>

I must admit this assembly syntax eluded me and I did not really dig into
to check it in details.  Just to certify I understood correctly the changes
required, the final code should be similar to:

[...]
.Lthread_start:
        cfi_def_cfa_offset (FRAME_SIZE)
        cfi_undefined (lr)
        /* 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).  */
        moveli r0, CLONE_VM
        and r0, r30, r0
        BNEZ r0, .Lno_reset_pid   /* CLONE_VM is set */
        moveli TREG_SYSCALL_NR_NAME, __NR_getpid
        swint1
        ADDLI_PTR r2, tp, PID_OFFSET
        {
         ST4 r2, r0
         ADDLI_PTR r2, tp, TID_OFFSET
        }
        ST4 r2, r0
.Lno_reset_pid:
[...]

Correct?

 
>> +    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.
>