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

[H.J. Lu]

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.

-- 
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-i386-Add-startup.h-BZ-21913.patch
Type: text/x-patch
Size: 7367 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20170807/545dd691/attachment.bin>