Re: [PATCH 07/08] nptl: aarch64: Fix Race conditions in pthread cancellation (BZ#12683)

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

Hi Szabolcs,

Thanks for the review, comments below:

On 29-06-2015 07:32, Szabolcs Nagy wrote:
> On 26/06/15 20:45, Adhemerval Zanella wrote:
>> +ENTRY (__syscall_cancel_arch)
>> +
>> +        stp     x29, x30, [sp, -16]!
>> +        cfi_def_cfa_offset (16)
>> +        cfi_offset (29, -16)
>> +        cfi_offset (30, -8)
>> +        add     x29, sp, 0
>> +        cfi_def_cfa_register (29)
>> +
> 
> you save things on the stack here ...
> 
> 
> ... and tail call into a function that does not restore the stack.
> 
> i think you can omit saving the frame pointer.
> (neither syscall, nor tail call needs it).

My first though was the FP save-restore was required to correct create
unwind information for cancellation handlers, but on a second though
they are not necessary indeed. I have removed them and cancellation
handlers works as intended, right now the syscall wrappers looks like:

ENTRY (__syscall_cancel_arch)

        .globl __syscall_cancel_arch_start
        .type  __syscall_cancel_arch_start,@function
__syscall_cancel_arch_start:

        /* if (*cancelhandling & CANCELED_BITMASK)
             __syscall_do_cancel()  */
        ldr     w0, [x0]
        tbnz    w0, 2, 1f

        /* Issue a 6 argument syscall, the nr [x1] being the syscall
           number.  */
        mov     x8, x1
        mov     x0, x2
        mov     x1, x3
        mov     x2, x4
        mov     x3, x5
        mov     x4, x6
        mov     x5, x7
        svc     0x0

        .globl __syscall_cancel_arch_end
        .type  __syscall_cancel_arch_end,@function
__syscall_cancel_arch_end:
        ret

1:
        b       __syscall_do_cancel

END (__syscall_cancel_arch)

> 
>> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h
>> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h
>> @@ -20,42 +20,50 @@
>>  #include <tls.h>
>>  #ifndef __ASSEMBLER__
>>  # include <nptl/pthreadP.h>
>> +# include <sys/ucontext.h>
>>  #endif
>>
>>  #if IS_IN (libc) || IS_IN (libpthread) || IS_IN (librt)
>>
>> +# if IS_IN (libc)
>> +#  define JMP_SYSCALL_CANCEL HIDDEN_JUMPTARGET(__syscall_cancel)
>> +# else
>> +#  define JMP_SYSCALL_CANCEL __syscall_cancel
>> +# endif
>> +
>>  # undef PSEUDO
>>  # define PSEUDO(name, syscall_name, args)                              \
>> -       .section ".text";                                               \
>> -ENTRY (__##syscall_name##_nocancel);                                   \
>> -.Lpseudo_nocancel:                                                     \
>> -       DO_CALL (syscall_name, args);                                   \
>> -.Lpseudo_finish:                                                       \
>> -       cmn     x0, 4095;                                               \
>> -       b.cs    .Lsyscall_error;                                        \
>> -       .subsection 2;                                                  \
>> -       .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel; \
>> +        .section ".text";                                               \
>> +ENTRY (__##syscall_name##_nocancel);                                    \
>> +L(pseudo_nocancel):                                                    \
>> +        DO_CALL (syscall_name, args);                                   \
>> +L(pseudo_finish):                                                      \
>> +        cmn     x0, 4095;                                               \
>> +        b.cs    L(syscall_error);                                      \
>> +        .subsection 2;                                                 \
>> +        .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel; \
> 
> i think there is a problem here that may need independent
> fix (not a regression, but worth a mention):
> 
> a glibc test (debug/tst-backtrace5) checks if 'read' symbol
> is found when read is interrupted and the signal handler
> calls backtrace_symbols.
> 
> however the interrupt will be in __read_nocancel and that
> name is not exported so backtrace does not find it in the
> dynamic symbol table so the check fails. (on some other
> archs the test pass because the nocancel code is within
> the read code, not a separate function).
> 
> it's a silly issue so i haven't got around proposing a fix
> for this yet.
> 

Indeed it would be much better to add more cleanup in this macros.
I have simplified to:

# if IS_IN (libc)
#  define JMP_SYSCALL_CANCEL HIDDEN_JUMPTARGET(__syscall_cancel)
# else
#  define JMP_SYSCALL_CANCEL __syscall_cancel
# endif

# undef PSEUDO
# define PSEUDO(name, syscall_name, args)                               \
ENTRY (name);                                                           \
        SINGLE_THREAD_P(16);                                            \
        cbnz    w16, L(pseudo_cancel);                                  \
        DO_CALL (syscall_name, args);                                   \
        b       L(pseudo_finish);                                       \
L(pseudo_cancel):                                                       \
        stp     x29, x30, [sp, -16]!;                                   \
        cfi_def_cfa_offset (16);                                        \
        cfi_offset (29, -16);                                           \
        cfi_offset (30, -8);                                            \
        add     x29, sp, 0;                                             \
        cfi_def_cfa_register (29);                                      \
        mov     x6, x5;                                                 \
        mov     x5, x4;                                                 \
        mov     x4, x3;                                                 \
        mov     x3, x2;                                                 \
        mov     x2, x1;                                                 \
        mov     x1, x0;                                                 \
        mov     x0, SYS_ify (syscall_name);                             \
        bl      JMP_SYSCALL_CANCEL;                                     \
        ldp     x29, x30, [sp], 16;                                     \
        cfi_restore (30);                                               \
        cfi_restore (29);                                               \
        cfi_def_cfa (31, 0);                                            \
L(pseudo_finish):                                                       \
        cmn     x0, 4095;                                               \
        b.cs    L(syscall_error);

# undef PSEUDO_END
# define PSEUDO_END(name)                                               \
        SYSCALL_ERROR_HANDLER;                                          \
        cfi_endproc;                                                    \
        .size   name, .-name;

And debug/tst-backtrace{5-6} work as intended as well. What do you think?