diff mbox

[v2,1/3] posix: Remove dynamic memory allocation from execl{e,p}

Message ID 56B9CF1A.6020807@twiddle.net
State New
Headers show

Commit Message

Richard Henderson Feb. 9, 2016, 11:35 a.m. UTC
On 02/08/2016 08:28 AM, Rasmus Villemoes wrote:
> On Tue, Feb 02 2016, Rich Felker <dalias@libc.org> wrote:
>
>> On Mon, Feb 01, 2016 at 04:52:15PM +0000, Joseph Myers wrote:
>>> On Mon, 1 Feb 2016, Adhemerval Zanella wrote:
>>>
>>>> +  char *argv[argc+1];
>>>> +  va_start (ap, arg);
>>>> +  argv[0] = (char*) arg;
>>>> +  for (i = 1; i < argc; i++)
>>>> +     argv[i] = va_arg (ap, char *);
>>>> +  argv[i] = NULL;
>>>
>>> I don't see how you're ensuring this stack allocation is safe (i.e. if
>>> it's too big, it doesn't corrupt memory that's in use by other threads).
>>
>> There's no obligation to. If you pass something like a million
>> arguments to a variadic function, the compiler will generate code in
>> the caller that overflows the stack before the callee is even reached.
>> The size of the vla used in execl is exactly the same size as the
>> argument block on the stack used for passing arguments to execl from
>> its caller, and it's nobody's fault but the programmer's if this is
>> way too big. It's not a runtime variable.
>
> This is true, and maybe it's not worth the extra complication, but if
> we're willing to make arch-specific versions of execl and execle we can
> avoid the double stack use and the time spent copying the argv
> array. That won't remove the possible stack overflow, of course, but
> then it'll in all likelihood happen in the user code and not glibc.

I like the idea.  It's a quality of implementation issue, wherein by re-using 
the data that's (mostly) on the stack already we don't need twice again the 
amount of stack space for any given call.

I think that Adhemerval's patch should go in as a default implementation, and 
various targets can implement the assembly as desired.

I've tested the following on x86_64 and i686.  I've compile-tested for x32 (but 
need a more complete x32 installation for testing), and alpha (testing is just 
slow).

Thoughts?


r~

Comments

Szabolcs Nagy Feb. 9, 2016, 1:30 p.m. UTC | #1
On 09/02/16 11:35, Richard Henderson wrote:
> On 02/08/2016 08:28 AM, Rasmus Villemoes wrote:
>> On Tue, Feb 02 2016, Rich Felker <dalias@libc.org> wrote:
>>
>>> On Mon, Feb 01, 2016 at 04:52:15PM +0000, Joseph Myers wrote:
>>>> On Mon, 1 Feb 2016, Adhemerval Zanella wrote:
>>>>
>>>>> +  char *argv[argc+1];
>>>>> +  va_start (ap, arg);
>>>>> +  argv[0] = (char*) arg;
>>>>> +  for (i = 1; i < argc; i++)
>>>>> +     argv[i] = va_arg (ap, char *);
>>>>> +  argv[i] = NULL;
>>>>
>>>> I don't see how you're ensuring this stack allocation is safe (i.e. if
>>>> it's too big, it doesn't corrupt memory that's in use by other threads).
>>>
>>> There's no obligation to. If you pass something like a million
>>> arguments to a variadic function, the compiler will generate code in
>>> the caller that overflows the stack before the callee is even reached.
>>> The size of the vla used in execl is exactly the same size as the
>>> argument block on the stack used for passing arguments to execl from
>>> its caller, and it's nobody's fault but the programmer's if this is
>>> way too big. It's not a runtime variable.
>>
>> This is true, and maybe it's not worth the extra complication, but if
>> we're willing to make arch-specific versions of execl and execle we can
>> avoid the double stack use and the time spent copying the argv
>> array. That won't remove the possible stack overflow, of course, but
>> then it'll in all likelihood happen in the user code and not glibc.
> 
> I like the idea.  It's a quality of implementation issue, wherein by re-using the data that's (mostly) on the
> stack already we don't need twice again the amount of stack space for any given call.
> 
> I think that Adhemerval's patch should go in as a default implementation, and various targets can implement the
> assembly as desired.
> 
> I've tested the following on x86_64 and i686.  I've compile-tested for x32 (but need a more complete x32
> installation for testing), and alpha (testing is just slow).
> 
> Thoughts?
> 

i think it is a hard to maintain, nasty hack

is execl stack usage the bottleneck somewhere?

to improve the stack usage in glibc, the extreme
wasteful cases should be fixed first.
(crypt uses >100k, strtod uses ???, etc)

e.g. musl guarantees hard limits on libc stack usage
and execl is one of the (two) exceptions where it just
seems useless to do so (you cannot do it portably anyway).
Adhemerval Zanella Netto Feb. 10, 2016, 4:36 p.m. UTC | #2
On 09-02-2016 11:30, Szabolcs Nagy wrote:
> On 09/02/16 11:35, Richard Henderson wrote:
>> On 02/08/2016 08:28 AM, Rasmus Villemoes wrote:
>>> On Tue, Feb 02 2016, Rich Felker <dalias@libc.org> wrote:
>>>
>>>> On Mon, Feb 01, 2016 at 04:52:15PM +0000, Joseph Myers wrote:
>>>>> On Mon, 1 Feb 2016, Adhemerval Zanella wrote:
>>>>>
>>>>>> +  char *argv[argc+1];
>>>>>> +  va_start (ap, arg);
>>>>>> +  argv[0] = (char*) arg;
>>>>>> +  for (i = 1; i < argc; i++)
>>>>>> +     argv[i] = va_arg (ap, char *);
>>>>>> +  argv[i] = NULL;
>>>>>
>>>>> I don't see how you're ensuring this stack allocation is safe (i.e. if
>>>>> it's too big, it doesn't corrupt memory that's in use by other threads).
>>>>
>>>> There's no obligation to. If you pass something like a million
>>>> arguments to a variadic function, the compiler will generate code in
>>>> the caller that overflows the stack before the callee is even reached.
>>>> The size of the vla used in execl is exactly the same size as the
>>>> argument block on the stack used for passing arguments to execl from
>>>> its caller, and it's nobody's fault but the programmer's if this is
>>>> way too big. It's not a runtime variable.
>>>
>>> This is true, and maybe it's not worth the extra complication, but if
>>> we're willing to make arch-specific versions of execl and execle we can
>>> avoid the double stack use and the time spent copying the argv
>>> array. That won't remove the possible stack overflow, of course, but
>>> then it'll in all likelihood happen in the user code and not glibc.
>>
>> I like the idea.  It's a quality of implementation issue, wherein by re-using the data that's (mostly) on the
>> stack already we don't need twice again the amount of stack space for any given call.
>>
>> I think that Adhemerval's patch should go in as a default implementation, and various targets can implement the
>> assembly as desired.
>>
>> I've tested the following on x86_64 and i686.  I've compile-tested for x32 (but need a more complete x32
>> installation for testing), and alpha (testing is just slow).
>>
>> Thoughts?
>>
> 
> i think it is a hard to maintain, nasty hack
> 
> is execl stack usage the bottleneck somewhere?
> 
> to improve the stack usage in glibc, the extreme
> wasteful cases should be fixed first.
> (crypt uses >100k, strtod uses ???, etc)
> 
> e.g. musl guarantees hard limits on libc stack usage
> and execl is one of the (two) exceptions where it just
> seems useless to do so (you cannot do it portably anyway).
> 


I agree with Szabolcs and I also see these kind of asm specific
implementation also adds platform different behaviour which I also
think we should avoid to add within GLIBC.

The only doubt with my patch I have now is if it is worth to add
the -fstack-check or not. Florian has raised some questioning about
its efficacy and I am not sure how well it is supported on all 
architectures.
diff mbox

Patch

>From 5b78856069a21550d4b67b4c0a269915f37fce0f Mon Sep 17 00:00:00 2001
From: Richard Henderson <rth@twiddle.net>
Date: Tue, 9 Feb 2016 13:43:08 +1100
Subject: [PATCH 5/5] alpha: Implement execl{,e,p} without double stack
 allocation

---
 sysdeps/unix/sysv/linux/alpha/execl.S  | 52 ++++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/alpha/execle.S | 58 ++++++++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/alpha/execlp.S | 53 +++++++++++++++++++++++++++++++
 3 files changed, 163 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/alpha/execl.S
 create mode 100644 sysdeps/unix/sysv/linux/alpha/execle.S
 create mode 100644 sysdeps/unix/sysv/linux/alpha/execlp.S

diff --git a/sysdeps/unix/sysv/linux/alpha/execl.S b/sysdeps/unix/sysv/linux/alpha/execl.S
new file mode 100644
index 0000000..11f4307
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/alpha/execl.S
@@ -0,0 +1,52 @@ 
+/* Copyright (C) 2016 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/>.  */
+
+#include <sysdep.h>
+
+ENTRY(execl)
+	cfi_startproc
+	ldgp	gp, 0(pv)
+	lda	sp, -48(sp)
+	cfi_adjust_cfa_offset(48)
+	.frame	sp, 48, ra
+	.prologue 1
+
+	/* Save the portions of the argv argument list in registers.  */
+	stq	a5, 40(sp)
+	stq	a4, 32(sp)
+	stq	a3, 24(sp)
+	stq	a2, 16(sp)
+	stq	a1,  8(sp)
+
+	/* Load the argv and envp arguments; path is already in place.  */
+	lda	a1, 8(sp)
+	ldq	a2, __environ
+
+	lda	v0, SYS_ify(execve)
+	call_pal PAL_callsys
+
+	/* Discard the stack frame now.  */
+	lda	sp, 48(sp)
+	cfi_adjust_cfa_offset(-48)
+
+	/* All returns are errors.  */
+	br	SYSCALL_ERROR_LABEL
+
+PSEUDO_END (execle)
+	cfi_endproc
+
+libc_hidden_def (execl)
diff --git a/sysdeps/unix/sysv/linux/alpha/execle.S b/sysdeps/unix/sysv/linux/alpha/execle.S
new file mode 100644
index 0000000..ce75ce3
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/alpha/execle.S
@@ -0,0 +1,58 @@ 
+/* Copyright (C) 2016 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/>.  */
+
+#include <sysdep.h>
+
+ENTRY(execle)
+	cfi_startproc
+	lda	sp, -48(sp)
+	cfi_adjust_cfa_offset(48)
+	.frame  sp, 48, ra
+	.prologue 0
+
+	/* Save the portions of the argv argument list in registers.  */
+	stq	a5, 40(sp)
+	stq	a4, 32(sp)
+	stq	a3, 24(sp)
+	stq	a2, 16(sp)
+	stq	a1,  8(sp)
+
+	/* Find the env argument.  It is the array element after the argv
+	   NULL terminator, which cannot be located before argv[1].  */
+	lda	t0, 16(sp)
+1:	ldq	t1, 0(t0)
+	addq	t0, 8, t0
+	bne	t1, 1b
+
+	/* Load the argv and envp arguments; path is already in place.  */
+	lda	a1, 8(sp)
+	ldq	a2, 0(t0)
+
+	lda	v0, SYS_ify(execve)
+	call_pal PAL_callsys
+
+	/* Discard the stack frame now.  */
+	lda	sp, 48(sp)
+	cfi_adjust_cfa_offset(-48)
+
+	/* All returns are errors.  */
+	br	SYSCALL_ERROR_LABEL
+
+PSEUDO_END (execle)
+	cfi_endproc
+
+libc_hidden_def (execle)
diff --git a/sysdeps/unix/sysv/linux/alpha/execlp.S b/sysdeps/unix/sysv/linux/alpha/execlp.S
new file mode 100644
index 0000000..b0ef76d
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/alpha/execlp.S
@@ -0,0 +1,53 @@ 
+/* Copyright (C) 2016 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/>.  */
+
+#include <sysdep.h>
+
+ENTRY(execlp)
+	cfi_startproc
+	ldgp	gp, 0(pv)
+	lda	sp, -48(sp)
+	cfi_adjust_cfa_offset(48)
+	stq	ra, 0(sp)
+	cfi_rel_offset(ra, 0)
+	.prologue 1
+
+	/* Save the portions of the argv argument list in registers.  */
+	stq	a5, 40(sp)
+	stq	a4, 32(sp)
+	stq	a3, 24(sp)
+	stq	a2, 16(sp)
+	stq	a1,  8(sp)
+
+	/* Load the argv and envp arguments; path is already in place.  */
+	lda	a1, 8(sp)
+	ldq	a2, __environ
+#ifdef PIC
+	bsr	ra, __execvpe		!samegp
+#else
+	jsr	ra, __execvpe
+	ldgp	gp, 0(ra)
+#endif
+
+	lda	sp, 48(sp)
+	cfi_adjust_cfa_offset(-48)
+	ret
+
+END (execlp)
+	cfi_endproc
+
+libc_hidden_def (execlp)
-- 
2.5.0