This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

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.

p.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]