This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 08/08] nptl: arm: Fix Race conditions in pthread cancellation (BZ#12683)
- From: Phil Blundell <pb at pbcl dot net>
- To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Thu, 03 Sep 2015 11:26:41 +0100
- Subject: Re: [PATCH 08/08] nptl: arm: Fix Race conditions in pthread cancellation (BZ#12683)
- Authentication-results: sourceware.org; auth=none
- References: <55E4C300 dot 9080800 at linaro dot org> <1441148926 dot 1680 dot 51 dot camel at pbcl dot net> <55E768E8 dot 8020904 at linaro dot org>
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.