This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] i386: Add _startup_sbrk and _startup_fatal [BZ #21913]


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


-- 
Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]