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 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."

> +/* 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.

> --- /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)."

> --- 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.

zw


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