[v3,8/9] aarch64: Consolidate NPTL/non versions of clone
diff mbox

Message ID 1401046909-25821-9-git-send-email-rth@twiddle.net
State New
Headers show

Commit Message

Richard Henderson May 25, 2014, 7:41 p.m. UTC
From: Richard Henderson <rth@redhat.com>

At the same time, rely on non-clobbered registers across syscall
so that we eliminate the stack frame that we previously ignored
in the unwind info.
---
 sysdeps/unix/sysv/linux/aarch64/clone.S      | 56 ++++++++++++----------------
 sysdeps/unix/sysv/linux/aarch64/nptl/clone.S | 21 -----------
 2 files changed, 24 insertions(+), 53 deletions(-)
 delete mode 100644 sysdeps/unix/sysv/linux/aarch64/nptl/clone.S

Comments

Marcus Shawcroft May 29, 2014, 5:16 a.m. UTC | #1
On 25 May 2014 20:41, Richard Henderson <rth@twiddle.net> wrote:
> From: Richard Henderson <rth@redhat.com>
>
> At the same time, rely on non-clobbered registers across syscall
> so that we eliminate the stack frame that we previously ignored
> in the unwind info.
> ---
>  sysdeps/unix/sysv/linux/aarch64/clone.S      | 56 ++++++++++++----------------
>  sysdeps/unix/sysv/linux/aarch64/nptl/clone.S | 21 -----------
>  2 files changed, 24 insertions(+), 53 deletions(-)
>  delete mode 100644 sysdeps/unix/sysv/linux/aarch64/nptl/clone.S
>
> diff --git a/sysdeps/unix/sysv/linux/aarch64/clone.S b/sysdeps/unix/sysv/linux/aarch64/clone.S

> +       beq     thread_start


I notice that some of the targets make thread_start local (x86_64,
i386, hppa, tile, mips) while others do not (alpha, m68k, s390).
Which seems odd, any idea why?
/Marcus
Andreas Schwab May 29, 2014, 8:15 a.m. UTC | #2
Marcus Shawcroft <marcus.shawcroft@gmail.com> writes:

> I notice that some of the targets make thread_start local (x86_64,
> i386, hppa, tile, mips) while others do not (alpha, m68k, s390).

An assembler label is always local by default.

Andreas.
Richard Henderson May 29, 2014, 3:34 p.m. UTC | #3
On 05/29/2014 01:15 AM, Andreas Schwab wrote:
> Marcus Shawcroft <marcus.shawcroft@gmail.com> writes:
> 
>> I notice that some of the targets make thread_start local (x86_64,
>> i386, hppa, tile, mips) while others do not (alpha, m68k, s390).
> 
> An assembler label is always local by default.

Indeed, but what Marcus meant was "really" local, i.e. ".L" or "1:"
type symbols, vs "static" symbols that are local in the elf sense but preserved
by the assembler.

My guess is that with the pre-dwarf alpha unwind info one had to use a
different ".ent" directive, which implied that it was impossible to put the
unwind info for thread_start into the same function as clone.  And then others
copied how Alpha had set it up.

Personally, I like having "thread_start" at the top of the backtrace.  But I
can see how others might prefer "clone", as some indication of how that happened.


r~
Marcus Shawcroft June 3, 2014, 9:06 a.m. UTC | #4
On 29 May 2014 16:34, Richard Henderson <rth@twiddle.net> wrote:
> On 05/29/2014 01:15 AM, Andreas Schwab wrote:
>> Marcus Shawcroft <marcus.shawcroft@gmail.com> writes:
>>
>>> I notice that some of the targets make thread_start local (x86_64,
>>> i386, hppa, tile, mips) while others do not (alpha, m68k, s390).
>>
>> An assembler label is always local by default.
>
> Indeed, but what Marcus meant was "really" local, i.e. ".L" or "1:"
> type symbols, vs "static" symbols that are local in the elf sense but preserved
> by the assembler.
>
> My guess is that with the pre-dwarf alpha unwind info one had to use a
> different ".ent" directive, which implied that it was impossible to put the
> unwind info for thread_start into the same function as clone.  And then others
> copied how Alpha had set it up.
>
> Personally, I like having "thread_start" at the top of the backtrace.  But I
> can see how others might prefer "clone", as some indication of how that happened.

I don't have a strong opinion either way, but I am slightly surprised
that we (glibc) don't appear to have a preference for new ports. I'm
happy to go with your proposal.

/Marcus

Patch
diff mbox

diff --git a/sysdeps/unix/sysv/linux/aarch64/clone.S b/sysdeps/unix/sysv/linux/aarch64/clone.S
index f2964f4..a2b5a2b 100644
--- a/sysdeps/unix/sysv/linux/aarch64/clone.S
+++ b/sysdeps/unix/sysv/linux/aarch64/clone.S
@@ -39,47 +39,42 @@ 
  */
         .text
 ENTRY(__clone)
+	/* Save args for the child.  */
+	mov	x10, x0
+	mov	x11, x2
+	mov	x12, x3
+
 	/* Sanity check args.  */
-	cbz	x0, 1f
-	cbz	x1, 1f
-	/* Insert the args onto the new stack.  */
-	stp	x0, x3, [x1, #-16]!	/* Fn, arg.  */
+	mov	x0, #-EINVAL
+	cbz	x10, .Lsyscall_error
+	cbz	x1, .Lsyscall_error
 
 	/* Do the system call.  */
+	/* X0:flags, x1:newsp, x2:parenttidptr, x3:newtls, x4:childtid.  */
 	mov	x0, x2                  /* flags  */
-
 	/* New sp is already in x1.  */
 	mov	x2, x4			/* ptid  */
 	mov	x3, x5			/* tls  */
 	mov	x4, x6			/* ctid  */
-
-#ifdef RESET_PID
-	/* We rely on the kernel preserving the argument regsiters across a
-	   each system call so that we can inspect the flags against after
-	   the clone call.  */
-	mov	x5, x0
-#endif
-
 	mov	x8, #SYS_ify(clone)
-	/* X0:flags, x1:newsp, x2:parenttidptr, x3:newtls, x4:childtid.  */
 	svc	0x0
-	cfi_endproc
+
 	cmp	x0, #0
-	beq	2f
-	blt	3f
+	beq	thread_start
+	blt	.Lsyscall_error
 	RET
-1:	mov	x0, #-EINVAL
-3:
-	b	syscall_error
+PSEUDO_END (__clone)
 
-2:
+	.align 4
+	.type thread_start, %function
+thread_start:
 	cfi_startproc
 	cfi_undefined (x30)
 	mov	x29, 0
-#ifdef RESET_PID
-	tbnz	x5, #CLONE_THREAD_BIT, 3f
+
+	tbnz	x11, #CLONE_THREAD_BIT, 3f
 	mov	x0, #-1
-	tbnz	x5, #CLONE_VM_BIT, 2f
+	tbnz	x11, #CLONE_VM_BIT, 2f
 	mov	x8, #SYS_ify(getpid)
 	svc	0x0
 2:
@@ -87,18 +82,15 @@  ENTRY(__clone)
 	sub	x1, x1, #PTHREAD_SIZEOF
 	str	w0, [x1, #PTHREAD_PID_OFFSET]
 	str	w0, [x1, #PTHREAD_TID_OFFSET]
-
 3:
-#endif
-	/* Pick the function arg and call address from the stack and
-	   execute.  */
-	ldp	x1, x0, [sp], #16
-	blr	x1
+
+	/* Pick the function arg and execute.  */
+	mov	x0, x12
+	blr	x10
 
 	/* We are done, pass the return value through x0.  */
 	b	HIDDEN_JUMPTARGET(_exit)
 	cfi_endproc
-	cfi_startproc
-PSEUDO_END (__clone)
+	.size thread_start, .-thread_start
 
 weak_alias (__clone, clone)
diff --git a/sysdeps/unix/sysv/linux/aarch64/nptl/clone.S b/sysdeps/unix/sysv/linux/aarch64/nptl/clone.S
deleted file mode 100644
index 281be3b..0000000
--- a/sysdeps/unix/sysv/linux/aarch64/nptl/clone.S
+++ /dev/null
@@ -1,21 +0,0 @@ 
-/* Copyright (C) 2009-2014 Free Software Foundation, Inc.
-
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public License as
-   published by the Free Software Foundation; either version 2.1 of the
-   License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#define RESET_PID
-#include <tcb-offsets.h>
-#include "../clone.S"