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

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

On 03-09-2015 07:26, Phil Blundell wrote:
> On Wed, 2015-09-02 at 18:23 -0300, Adhemerval Zanella wrote:
>> Rechecking and for 'r7' it is not really required.  However afaik to use
>> the glibc cancellation mechanism based on libgcc unwind for arm it
>> requires to at least save the 'lr'. I have not debug in details, but
>> omitting lr save I see failures with this mechanism for
>> tst-cancel{4,5,17,18}x.
> 
> Ah right, fair enough.  As a general rule we want to stack an even
> number of registers so that the stack pointer remains 64-bit aligned.
> If you just drop the pushing of r7 then you'd have four here.
> 
> Also. some more questions:
> 
>> +       bl      __syscall_do_cancel
> 
> It might be nice to make it more obvious that this isn't expected to
> return.  Right now it looks as though it will just fall off the end of
> __syscall_cancel_arch() which is a bit disconcerting.  Either change
> "bl" to "b", or at a minimum add a comment about it.

I can change to:

        mov     lr, pc
        b       __syscall_do_cancel

Which explicit state it is a tail cail that do not return.

> 
> Is __syscall_cancel_arch() ever called from anywhere other than
> PSEUDO_CANCEL_AFTER()?  If not, is there a reason that its contents
> couldn't just be inlined into PSEUDO?  The amount of argument shuffling
> that you're having to do in order to make an ABI-conformant call to
> __syscall_cancel_arch is substantial and it seems like a lot of this
> could be eliminated.  In fact, it looks to me as though almost all of
> PSEUDO_CANCEL_BEFORE could be eliminated even if __syscall_cancel_arch
> stays, and replaced with simply:
> 
> push {r3}
> mov r3, r2
> mov r2, r1
> mov r1, r0
> 
> This would also allow the stacking of r4/r5 in PSEUDO() to be
> eliminated.  I can't see any obvious reason PSEUDO_CANCEL_BEFORE needs
> to reserve 20 bytes of stack space but perhaps I'm overlooking something
> there.
> 
> Similarly, the PSEUDO_CANCEL_BEFORE/PSEUDO_CANCEL_AFTER thing seems a
> bit of an artificial distinction since all that happens between them is
> that you load a constant into a register.  If these macros aren't used
> anywhere else then it seems like the code could be somewhat simplified.

If you check my first message (00/08: nptl: Fix Race conditions in pthread 
cancellation (BZ#12683)) the idea of this modification it exactly to route
*all* cancellable syscall to the __syscall_cancel_arch.  It is required
because the __syscall_cancel_arch have the global marks the signal cancel
handler (sigcancel_handle in nptl/nptl-init.c) will use to check if the 
instruction pointer falls within the cancellable syscall code.

> 
> p.
> 
>