i386: Add _startup_sbrk and _startup_fatal [BZ #21913]
Commit Message
On Mon, Aug 7, 2017 at 1:52 PM, Zack Weinberg <zackw@panix.com> wrote:
> On Mon, Aug 7, 2017 at 4:21 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Mon, Aug 7, 2017 at 10:39 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Mon, Aug 7, 2017 at 10:37 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>>
>>>> How about making sbrk always use int $0x80?
>>>
>>> We can do that in sbrk.o and will make change less complex.
>>> I will give it a try.
>>
>> Here is a patch to add <startup.h>. Tested on i686. OK for master?
>
> This is looking a lot better. I withdraw my earlier objections. (I
> would still like to see further investigation of not needing to call
> either sbrk or __libc_fatal before TLS is initialized, at least in
> static binaries, but it shouldn't block you fixing the immediate
> problem.) However:
>
>> This patch adds <startup.h> which defines _startup_fatal and defaults
>> it to __libc_fatal. It replaces __libc_fatal with _startup_fatal in
>> static executables where it is called before __libc_setup_tls is called.
>> This header file is included in all files containing functions which are
>> called before __libc_setup_tls is called. On Linux/i386, when PIE is
>> enabled by default, _startup_fatal is turned into ABORT_INSTRUCTION and
>> I386_USE_SYSENTER is defined to 0 so that "int $0x80" is used for system
>> calls before __libc_setup_tls is called.
>
> This would be a good point in the commit message to explain how this
> is tested. Something like "Without this patch, all statically-linked
> tests will fail on i386 when the compiler defaults to -fPIE."
Done.
>> +/* When PIC is defined and SHARED isn't defined, we are building PIE
>> + by default. */
>> +#if defined PIC && !defined SHARED
>> +# define BUILD_PIE_DEFAULT
>> +#endif
>
> Please don't add new conditionals to config.h.in. Put them in
> libc-symbols.h instead (after the inclusion of config.h). If this
> won't work for some reason, please explain.
Done.
>> --- /dev/null
>> +++ b/sysdeps/generic/startup.h
>> @@ -0,0 +1,20 @@
>> +/* Generic definitions of functions used by static libc main startup.
>> + Copyright (C) 2017 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/>. */
>> +
>
> Document at this point in this file when it is necessary to override
> this file. Something like "Ports should override this file when the
> default definitions below will not work correctly very early in
> startup (e.g. before thread-local storage is initialized)."
Done.
>> --- a/sysdeps/unix/sysv/linux/i386/brk.c
>> +++ b/sysdeps/unix/sysv/linux/i386/brk.c
>> @@ -16,6 +16,7 @@
>> License along with the GNU C Library; if not, see
>> <http://www.gnu.org/licenses/>. */
>>
>> +#include <startup.h>
>> #include <errno.h>
>> #include <unistd.h>
>> #include <sysdep.h>
>
> Having linux/i386/startup.h override I386_USE_SYSENTER, and therefore
> change how sysdep.h defines INTERNAL_SYSCALL, for just this file, is
> too much magic. Someone five years from now is going to come along and
> remove that #include on the grounds that it doesn't appear to be necessary.
>
> Since this is already an i386-specific implementation of __brk, it would
> be better to have
>
> #ifdef BUILD_PIE_DEFAULT
> /* Can't use "call *%gs:SYSINFO_OFFSET" during statup in static PIE. */
> # define I386_USE_SYSENTER 0
> #endif
>
> directly in this file.
>
Done.
Here is the updated patch. OK for master?
Thanks.
Comments
On Mon, Aug 7, 2017 at 5:17 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
> Here is the updated patch. OK for master?
Thanks for the quick turnaround!
Please find a more logical place in libc-symbols.h for the definition
of BUILD_PIE_DEFAULT; I didn't mean to suggest you should put it
_immediately_ after the #include <config.h>. Otherwise, looks good to
me, but please wait at least 48 hours for more feedback; this is not
code I understand deeply, so I don't feel like I can be the sole
reviewer.
zw
On Mon, Aug 7, 2017 at 2:24 PM, Zack Weinberg <zackw@panix.com> wrote:
> On Mon, Aug 7, 2017 at 5:17 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> Here is the updated patch. OK for master?
>
> Thanks for the quick turnaround!
>
> Please find a more logical place in libc-symbols.h for the definition
> of BUILD_PIE_DEFAULT; I didn't mean to suggest you should put it
> _immediately_ after the #include <config.h>. Otherwise, looks good to
I think that place isn't bad. I have a follwup patch to reduce the
size of libc.a when PIE is the default. It is better to define
BUILD_PIE_DEFAULT early.
> me, but please wait at least 48 hours for more feedback; this is not
> code I understand deeply, so I don't feel like I can be the sole
> reviewer.
Will do.
Thanks.
On 08/07/2017 05:17 PM, H.J. Lu wrote:
> From 58d4201ae107eab72478a366a24135246236f060 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Wed, 19 Jul 2017 14:32:42 -0700
> Subject: [PATCH] i386: Add <startup.h> [BZ #21913]
>
> On Linux/i386, there are 3 ways to make a system call:
>
> 1. call *%gs:SYSINFO_OFFSET. This requires TLS initialization.
> 2. call *_dl_sysinfo. This requires relocation of _dl_sysinfo.
> 3. int $0x80. This is slower than #2 and #3, but works everywhere.
>
> When an object file is compiled with PIC, #1 is prefered since it is
> faster than #3 and doesn't require relocation of _dl_sysinfo. For
> dynamic executables, ld.so initializes TLS. However, for static
> executables, before TLS is initialized by __libc_setup_tls, #3 should
> be used for system calls.
>
> This patch adds <startup.h> which defines _startup_fatal and defaults
> it to __libc_fatal. It replaces __libc_fatal with _startup_fatal in
> static executables where it is called before __libc_setup_tls is called.
> This header file is included in all files containing functions which are
> called before __libc_setup_tls is called. On Linux/i386, when PIE is
> enabled by default, _startup_fatal is turned into ABORT_INSTRUCTION and
> I386_USE_SYSENTER is defined to 0 so that "int $0x80" is used for system
> calls before __libc_setup_tls is called.
>
> Tested on i686 and x86-64. Without this patch, all statically-linked
> tests will fail on i686 when the compiler defaults to -fPIE.
Overall I don't have any objections except that you have a typo-prone
macro API that needs fixing.
> [BZ #21913]
> * csu/libc-tls.c: Include <startup.h> first.
> (__libc_setup_tls): Call _startup_fatal instead of __libc_fatal.
> * elf/dl-tunables.c: Include <startup.h> first.
> * include/libc-symbols.h (BUILD_PIE_DEFAULT): New.
> * sysdeps/generic/startup.h: New file.
> * sysdeps/unix/sysv/linux/i386/startup.h: Likewise.
> * sysdeps/unix/sysv/linux/i386/brk.c [BUILD_PIE_DEFAULT]
> (I386_USE_SYSENTER): New. Defined to 0.
> ---
> csu/libc-tls.c | 3 ++-
> elf/dl-tunables.c | 1 +
> include/libc-symbols.h | 6 ++++++
> sysdeps/generic/startup.h | 23 ++++++++++++++++++++++
> sysdeps/unix/sysv/linux/i386/brk.c | 5 +++++
> sysdeps/unix/sysv/linux/i386/startup.h | 36 ++++++++++++++++++++++++++++++++++
> 6 files changed, 73 insertions(+), 1 deletion(-)
> create mode 100644 sysdeps/generic/startup.h
> create mode 100644 sysdeps/unix/sysv/linux/i386/startup.h
>
> diff --git a/csu/libc-tls.c b/csu/libc-tls.c
> index 3c897bf28b..00138eb43a 100644
> --- a/csu/libc-tls.c
> +++ b/csu/libc-tls.c
> @@ -16,6 +16,7 @@
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
>
> +#include <startup.h>
> #include <errno.h>
> #include <ldsodefs.h>
> #include <tls.h>
> @@ -193,7 +194,7 @@ __libc_setup_tls (void)
> # error "Either TLS_TCB_AT_TP or TLS_DTV_AT_TP must be defined"
> #endif
> if (__builtin_expect (lossage != NULL, 0))
> - __libc_fatal (lossage);
> + _startup_fatal (lossage);
>
> /* Update the executable's link map with enough information to make
> the TLS routines happy. */
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 231fb8ca93..b964a09413 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -18,6 +18,7 @@
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
>
> +#include <startup.h>
> #include <stdint.h>
> #include <stdbool.h>
> #include <unistd.h>
> diff --git a/include/libc-symbols.h b/include/libc-symbols.h
> index 3310e3a678..9a94676c43 100644
> --- a/include/libc-symbols.h
> +++ b/include/libc-symbols.h
> @@ -84,6 +84,12 @@
>
> #include <config.h>
>
> +/* When PIC is defined and SHARED isn't defined, we are building PIE
> + by default. */
> +#if defined PIC && !defined SHARED
> +# define BUILD_PIE_DEFAULT
> +#endif
This is typo-prone.
We should define BUILD_PIE_DEFAULT to 1 or 0, and always defined.
> +
> /* Define this for the benefit of portable GNU code that wants to check it.
> Code that checks with #if will not #include <config.h> again, since we've
> already done it (and this file is implicitly included in every compile,
> diff --git a/sysdeps/generic/startup.h b/sysdeps/generic/startup.h
> new file mode 100644
> index 0000000000..a961e277bf
> --- /dev/null
> +++ b/sysdeps/generic/startup.h
> @@ -0,0 +1,23 @@
> +/* Generic definitions of functions used by static libc main startup.
> + Copyright (C) 2017 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/>. */
> +
> +/* Targets should override this file if the default definitions below
> + will not work correctly very early before TLS is initialized. */
> +
> +/* Use macro instead of inline function to avoid including <stdio.h>. */
> +#define _startup_fatal(message) __libc_fatal ((message))
> diff --git a/sysdeps/unix/sysv/linux/i386/brk.c b/sysdeps/unix/sysv/linux/i386/brk.c
> index 25ab1015d3..b55f0236d1 100644
> --- a/sysdeps/unix/sysv/linux/i386/brk.c
> +++ b/sysdeps/unix/sysv/linux/i386/brk.c
> @@ -16,6 +16,11 @@
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
>
> +#ifdef BUILD_PIE_DEFAULT
This is a typo-prone macro-api and should use #if to allow -Wunused
checking.
> +/* Can't use "call *%gs:SYSINFO_OFFSET" during statup in static PIE. */
> +# define I386_USE_SYSENTER 0
> +#endif
> +
> #include <errno.h>
> #include <unistd.h>
> #include <sysdep.h>
> diff --git a/sysdeps/unix/sysv/linux/i386/startup.h b/sysdeps/unix/sysv/linux/i386/startup.h
> new file mode 100644
> index 0000000000..a8fd3134b7
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/i386/startup.h
> @@ -0,0 +1,36 @@
> +/* Linux/i386 definitions of functions used by static libc main startup.
> + Copyright (C) 2017 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/>. */
> +
> +#ifdef BUILD_PIE_DEFAULT
Likewise.
> +# include <abort-instr.h>
> +
> +/* Can't use "call *%gs:SYSINFO_OFFSET" during statup in static PIE. */
> +# define I386_USE_SYSENTER 0
> +
> +__attribute__ ((__noreturn__))
> +static inline void
> +_startup_fatal (const char *message __attribute__ ((unused)))
> +{
> + /* This is only called very early during startup in static PIE.
> + FIXME: How can it be improved? */
> + ABORT_INSTRUCTION;
> + __builtin_unreachable ();
> +}
> +#else
> +# include_next <startup.h>
> +#endif
> -- 2.13.3
From 58d4201ae107eab72478a366a24135246236f060 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 19 Jul 2017 14:32:42 -0700
Subject: [PATCH] i386: Add <startup.h> [BZ #21913]
On Linux/i386, there are 3 ways to make a system call:
1. call *%gs:SYSINFO_OFFSET. This requires TLS initialization.
2. call *_dl_sysinfo. This requires relocation of _dl_sysinfo.
3. int $0x80. This is slower than #2 and #3, but works everywhere.
When an object file is compiled with PIC, #1 is prefered since it is
faster than #3 and doesn't require relocation of _dl_sysinfo. For
dynamic executables, ld.so initializes TLS. However, for static
executables, before TLS is initialized by __libc_setup_tls, #3 should
be used for system calls.
This patch adds <startup.h> which defines _startup_fatal and defaults
it to __libc_fatal. It replaces __libc_fatal with _startup_fatal in
static executables where it is called before __libc_setup_tls is called.
This header file is included in all files containing functions which are
called before __libc_setup_tls is called. On Linux/i386, when PIE is
enabled by default, _startup_fatal is turned into ABORT_INSTRUCTION and
I386_USE_SYSENTER is defined to 0 so that "int $0x80" is used for system
calls before __libc_setup_tls is called.
Tested on i686 and x86-64. Without this patch, all statically-linked
tests will fail on i686 when the compiler defaults to -fPIE.
[BZ #21913]
* csu/libc-tls.c: Include <startup.h> first.
(__libc_setup_tls): Call _startup_fatal instead of __libc_fatal.
* elf/dl-tunables.c: Include <startup.h> first.
* include/libc-symbols.h (BUILD_PIE_DEFAULT): New.
* sysdeps/generic/startup.h: New file.
* sysdeps/unix/sysv/linux/i386/startup.h: Likewise.
* sysdeps/unix/sysv/linux/i386/brk.c [BUILD_PIE_DEFAULT]
(I386_USE_SYSENTER): New. Defined to 0.
---
csu/libc-tls.c | 3 ++-
elf/dl-tunables.c | 1 +
include/libc-symbols.h | 6 ++++++
sysdeps/generic/startup.h | 23 ++++++++++++++++++++++
sysdeps/unix/sysv/linux/i386/brk.c | 5 +++++
sysdeps/unix/sysv/linux/i386/startup.h | 36 ++++++++++++++++++++++++++++++++++
6 files changed, 73 insertions(+), 1 deletion(-)
create mode 100644 sysdeps/generic/startup.h
create mode 100644 sysdeps/unix/sysv/linux/i386/startup.h
@@ -16,6 +16,7 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
+#include <startup.h>
#include <errno.h>
#include <ldsodefs.h>
#include <tls.h>
@@ -193,7 +194,7 @@ __libc_setup_tls (void)
# error "Either TLS_TCB_AT_TP or TLS_DTV_AT_TP must be defined"
#endif
if (__builtin_expect (lossage != NULL, 0))
- __libc_fatal (lossage);
+ _startup_fatal (lossage);
/* Update the executable's link map with enough information to make
the TLS routines happy. */
@@ -18,6 +18,7 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
+#include <startup.h>
#include <stdint.h>
#include <stdbool.h>
#include <unistd.h>
@@ -84,6 +84,12 @@
#include <config.h>
+/* When PIC is defined and SHARED isn't defined, we are building PIE
+ by default. */
+#if defined PIC && !defined SHARED
+# define BUILD_PIE_DEFAULT
+#endif
+
/* Define this for the benefit of portable GNU code that wants to check it.
Code that checks with #if will not #include <config.h> again, since we've
already done it (and this file is implicitly included in every compile,
new file mode 100644
@@ -0,0 +1,23 @@
+/* Generic definitions of functions used by static libc main startup.
+ Copyright (C) 2017 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/>. */
+
+/* Targets should override this file if the default definitions below
+ will not work correctly very early before TLS is initialized. */
+
+/* Use macro instead of inline function to avoid including <stdio.h>. */
+#define _startup_fatal(message) __libc_fatal ((message))
@@ -16,6 +16,11 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
+#ifdef BUILD_PIE_DEFAULT
+/* Can't use "call *%gs:SYSINFO_OFFSET" during statup in static PIE. */
+# define I386_USE_SYSENTER 0
+#endif
+
#include <errno.h>
#include <unistd.h>
#include <sysdep.h>
new file mode 100644
@@ -0,0 +1,36 @@
+/* Linux/i386 definitions of functions used by static libc main startup.
+ Copyright (C) 2017 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/>. */
+
+#ifdef BUILD_PIE_DEFAULT
+# include <abort-instr.h>
+
+/* Can't use "call *%gs:SYSINFO_OFFSET" during statup in static PIE. */
+# define I386_USE_SYSENTER 0
+
+__attribute__ ((__noreturn__))
+static inline void
+_startup_fatal (const char *message __attribute__ ((unused)))
+{
+ /* This is only called very early during startup in static PIE.
+ FIXME: How can it be improved? */
+ ABORT_INSTRUCTION;
+ __builtin_unreachable ();
+}
+#else
+# include_next <startup.h>
+#endif
--
2.13.3