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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 07/08/2017 13:48, H.J. Lu wrote:
> On Mon, Aug 7, 2017 at 6:34 AM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>> On 06/08/2017 19:26, H.J. Lu wrote:
>> [..]
>>> diff --git a/sysdeps/generic/startup.h b/sysdeps/generic/startup.h
>>> new file mode 100644
>>> index 0000000000..aa63b31181
>>> --- /dev/null
>>> +++ b/sysdeps/generic/startup.h
>>> @@ -0,0 +1,30 @@
>>> +/* 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/>.  */
>>> +
>>> +static inline void *
>>> +_startup_sbrk (intptr_t __delta)
>>> +{
>>> +  return __sbrk (__delta);
>>> +}
>>> +
>>> +__attribute__ ((__noreturn__))
>>> +static inline void
>>> +_startup_fatal (const char *__message)
>>> +{
>>> +  __libc_fatal (__message);
>>> +}
>>
>> I think there is no need of underlying prefixes for inline functions.
> 
> But _startup_sbrk may not be inlined for i386.

I meant for arguments name.

> 
>>> diff --git a/sysdeps/unix/sysv/linux/i386/Makefile b/sysdeps/unix/sysv/linux/i386/Makefile
>>> index 4080b8c966..cefa1511f6 100644
>>> --- a/sysdeps/unix/sysv/linux/i386/Makefile
>>> +++ b/sysdeps/unix/sysv/linux/i386/Makefile
>>> @@ -31,6 +31,10 @@ sysdep_routines += divdi3
>>>  shared-only-routines += divdi3
>>>  CPPFLAGS-divdi3.c = -Din_divdi3_c
>>>  endif
>>> +ifneq (,$(pic-default))
>>> +sysdep_routines += startup_sbrk
>>> +static-only-routines += startup_sbrk
>>> +endif
>>>  endif
>>>
>>>  ifeq ($(subdir),nptl)
>>> diff --git a/sysdeps/unix/sysv/linux/i386/startup.h b/sysdeps/unix/sysv/linux/i386/startup.h
>>> new file mode 100644
>>> index 0000000000..ccfba45153
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/i386/startup.h
>>> @@ -0,0 +1,38 @@
>>> +/* 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/>.  */
>>> +
>>> +#if defined PIC && !defined SHARED
>>
>> We already check and set build-pie-default on configure/make.in, wouldn't
>> be useful if we also define a BUILD_PIE as well?
> 
> We can have
> 
> #if define PIC && !defined SHARED
> # define BUILD_PIE
> #endif
> 
> in include/libc-symbols.h or config.h.in

I would prefer it to make it more readable.

> 
>>> +# include <abort-instr.h>
>>> +
>>> +/* Can't use "call *%gs:SYSINFO_OFFSET" during statup in static PIE.  */
>>> +# define I386_USE_SYSENTER 0
>>> +
>>> +extern void * _startup_sbrk (intptr_t) attribute_hidden;
>>> +
>>> +__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 ();
>>> +}
>>
>> Maybe also provide a __writev using 'int $0x80' so it can use _dl_debug_printf?
> 
> _dl_debug_printf calls _dl_debug_vdprintf which makes quite
> a few syscalls.   _startup_fatal is called very early during start
> up when something goes wrong.  When it is called, something
> very very bad must have happened.  I don't think ABORT_INSTRUCTION
> is a terrible choice.
> 

I do agree the ABORT_INSTRUCTION is not a bad choice, it is the lack
of information of what might the cause it that bothers me.  Even with
strace it might be the case where syscalls does not fail, but there
is an issue with pointer value check.  I do not have a simple good
solution though and I also think this kind of issues would indicate
an underlying issue with either the kernel or toolchain, so I think we
can live with it for now.