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

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

On 11/12/2017 21:57, Joseph Myers wrote:
> On Mon, 11 Dec 2017, Adhemerval Zanella wrote:
> 
>> +ENTRY (__syscall_cancel_arch)
>> +	.fnstart
>> +	mov	ip,sp
>> +	stmfd	sp!, {r4,r5,r6,r7,lr}
>> +	.save	{r4,r5,r6,r7,lr}
>> +
>> +	cfi_adjust_cfa_offset (20)
>> +	cfi_rel_offset (lr, 16)
> 
> I'd expect the saves of other registers to be described in the CFI as well 
> (that is, the .debug_frame CFI which is generated by cfi_* on ARM).
> 
>> +	.globl __syscall_cancel_arch_end
>> +__syscall_cancel_arch_end:
>> +	ldmfd	sp!, {r4,r5,r6,r7,lr}
>> +	cfi_adjust_cfa_offset (-20);
> 
> Then, I'd expect cfi_restore calls here.
> 
>> +1:
>> +	mov	lr, pc
>> +	b	__syscall_do_cancel
> 
> And this code is jumped to from a position where the stack has been 
> adjusted, but the CFI at this point reflects a state where it has been 
> restored.  So you need a fresh set of CFI directives to make the CFI 
> accurate in this part of the function.  (I haven't checked whether any 
> other architecture versions of this function might have similar CFI 
> issues.)
> 
> Also, it looks like you jump to the C function __syscall_do_cancel with a 
> stack adjustment that is not a multiple of 8.  I think the AAPCS 
> requirement for the stack pointer to be a multiple of 8 at public 
> interfaces applies to such a jump to a C function, so you need to save and 
> restore an additional register to preserve alignment.
> 

I used the generic version as base for ARM one with the expected flags
required for correctly unwind to work, -fexcept and 
-fasynchronous-unwind-tables (which pointed that I need to add 
-fasynchronous-unwind-tables on generic syscall_cancel CFLAGS as well).

Adjusting ARM to build the generic version I see the following assembly
being generated (I had to add -marm) with GCC 5:

---
        .type   __GI___syscall_cancel_arch, %function
__GI___syscall_cancel_arch:
        .fnstart
.LFB41: 
        push    {r4, r5, r7, lr}
        .save {r4, r5, r7, lr}
        .syntax divided
        .global __syscall_cancel_arch_start
.type __syscall_cancel_arch_start,%function
__syscall_cancel_arch_start:

        .arm
        .syntax unified
        ldr     ip, [r0]
        tst     ip, #4
        bne     .L5
        mov     r0, r2
        add     r2, sp, #16
        mov     r7, r1
        mov     r1, r3
        ldm     r2, {r2, r3, r4, r5}
        .syntax divided
        swi     0x0     @ syscall nr

        .global __syscall_cancel_arch_end
.type __syscall_cancel_arch_end,%function
__syscall_cancel_arch_end:

        .arm
        .syntax unified
        pop     {r4, r5, r7, pc}
.L5:
        bl      __syscall_do_cancel(PLT)
        .fnend
---

So I am not sure we need all the CFI directives to enable cancellation work
on ARM (at least current syscall wrappers which use C version of 
{INLINE,INTERNAL}_SYSCALL are not generating them).

The generate code seems correct, does not shown any regression on nptl 
testcases and it is slight better than my version so I think we can set ARM
to use -marm on syscall_cancel.{o,os} built and remove the arch specific
assembly.