> Date: Sun, 7 Aug 2016 20:43:23 +0200
> From: Stefan Kempf <sisnk...@gmail.com>
>
> Philip Guenther wrote:
> > On Mon, Aug 1, 2016 at 11:45 AM, Mark Kettenis <mark.kette...@xs4all.nl>
> > wrote:
> > >> From: j...@wxcvbn.org (Jeremie Courreges-Anglas)
> > >> Date: Mon, 01 Aug 2016 20:30:33 +0200
> > >>
> > >> Stefan Kempf <sisnk...@gmail.com> writes:
> > >>
> > >> > The constructor and destructor tables are declared as arrays with one
> > >> > non-NULL element. Walking those until a NULL element is reached looks
> > >> > like out-of-bound accesses to newer compilers, and they turn the code
> > >> > into infinite loops (e.g. clang 3.8), because it is undefined behavior.
> >
> > So it needs to reference the pointers via an extern incomplete array?
> > Can we move to setting up the leading count via the linker script
> > magic documented in the ld info page, and then just use __CTOR_LIST__
> > as the extern array?
>
> Here's an attempt at this. The startup code itself is unchanged except for
> using extern instead static arrays now.
>
> This needs building and installing binutils first. Afterwards lib/csu
> can be recompiled.
>
> Does that diff look better?
Not to me. I really don't like depending on linker magic for this.
Besides, this conflicts heavily with the relro work going on. Even if
other people think this is the way to go, this will have to be shelved
for now.
If your are emitting inline asm in crtbegin.c anyway, perhaps you can
just do this for __CTOR_LIST__ and __DTOR_LIST__ as well?
> Index: gnu/usr.bin/binutils-2.17/ld/emulparams/elf_obsd.sh
> ===================================================================
> RCS file: /cvs/src/gnu/usr.bin/binutils-2.17/ld/emulparams/elf_obsd.sh,v
> retrieving revision 1.2
> diff -u -p -r1.2 elf_obsd.sh
> --- gnu/usr.bin/binutils-2.17/ld/emulparams/elf_obsd.sh 30 Apr 2015
> 17:56:18 -0000 1.2
> +++ gnu/usr.bin/binutils-2.17/ld/emulparams/elf_obsd.sh 7 Aug 2016
> 17:42:23 -0000
> @@ -7,4 +7,23 @@ PAD_GOT=
> PAD_PLT=
> DATA_START_SYMBOLS='__data_start = . ;'
>
> +if [ "$ELFSIZE" = 64 ]
> +then
> + CTOR_START="PROVIDE_HIDDEN(__CTOR_LIST__ = .); \
> + QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2);"
> + CTOR_END="QUAD(0); PROVIDE_HIDDEN(__CTOR_END__ = .);"
> +
> + DTOR_START="PROVIDE_HIDDEN(__DTOR_LIST__ = .); \
> + QUAD((__DTOR_END__ - __DTOR_LIST__) / 8 - 2);"
> + DTOR_END="QUAD(0); PROVIDE_HIDDEN(__DTOR_END__ = .);"
> +else
> + CTOR_START="PROVIDE_HIDDEN(__CTOR_LIST__ = .); \
> + LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2);"
> + CTOR_END="LONG(0); PROVIDE_HIDDEN(__CTOR_END__ = .);"
> +
> + DTOR_START="PROVIDE_HIDDEN(__DTOR_LIST__ = .); \
> + LONG((__DTOR_END__ - __DTOR_LIST__) / 4 - 2);"
> + DTOR_END="LONG(0); PROVIDE_HIDDEN(__DTOR_END__ = .);"
> +fi
> +
> unset SEPARATE_GOTPLT
> Index: gnu/usr.bin/binutils-2.17/ld/scripttempl/elf.sc
> ===================================================================
> RCS file: /cvs/src/gnu/usr.bin/binutils-2.17/ld/scripttempl/elf.sc,v
> retrieving revision 1.8
> diff -u -p -r1.8 elf.sc
> --- gnu/usr.bin/binutils-2.17/ld/scripttempl/elf.sc 9 Aug 2014 04:49:47
> -0000 1.8
> +++ gnu/usr.bin/binutils-2.17/ld/scripttempl/elf.sc 7 Aug 2016 17:42:23
> -0000
> @@ -203,7 +203,8 @@ fi
> CTOR=".ctors ${CONSTRUCTING-0} :
> {
> ${CONSTRUCTING+${CTOR_START}}
> - /* gcc uses crtbegin.o to find the start of
> + /* on some systems,
> + gcc uses crtbegin.o to find the start of
> the constructors, so we make sure it is
> first. Because this is a wildcard, it
> doesn't matter if the user does not
> @@ -218,7 +219,8 @@ CTOR=".ctors ${CONSTRUCTING-0} :
> /* We don't want to include the .ctor section from
> the crtend.o file until after the sorted ctors.
> The .ctor section from the crtend file contains the
> - end of ctors marker and it must be last */
> + end of ctors marker and it must be last
> + on some systems */
>
> KEEP (*(EXCLUDE_FILE (*crtend*.o $OTHER_EXCLUDE_FILES) .ctors))
> KEEP (*(SORT(.ctors.*)))
> @@ -371,7 +373,11 @@ cat <<EOF
> ${CREATE_SHLIB-${SBSS2}}
> ${OTHER_READONLY_SECTIONS}
> .eh_frame_hdr : { *(.eh_frame_hdr) }
> - .eh_frame ${RELOCATING-0} : ONLY_IF_RO { KEEP (*(.eh_frame)) }
> + .eh_frame ${RELOCATING-0} : ONLY_IF_RO
> + {
> + ${RELOCATING+PROVIDE_HIDDEN(__EH_FRAME_BEGIN__ = .);}
> + KEEP (*(.eh_frame))
> + }
> .gcc_except_table ${RELOCATING-0} : ONLY_IF_RO { *(.gcc_except_table
> .gcc_except_table.*) }
>
> /* Adjust the address for the data segment. We want to adjust up to
> @@ -381,7 +387,11 @@ cat <<EOF
> ${CREATE_PIE+${RELOCATING+. = ${SHLIB_DATA_ADDR-${DATA_SEGMENT_ALIGN}};}}
>
> /* Exception handling */
> - .eh_frame ${RELOCATING-0} : ONLY_IF_RW { KEEP (*(.eh_frame)) }
> + .eh_frame ${RELOCATING-0} : ONLY_IF_RW
> + {
> + ${RELOCATING+PROVIDE_HIDDEN(__EH_FRAME_BEGIN__ = .);}
> + KEEP (*(.eh_frame))
> + }
> .gcc_except_table ${RELOCATING-0} : ONLY_IF_RW { *(.gcc_except_table
> .gcc_except_table.*) }
>
> /* Thread Local Storage sections */
> @@ -414,7 +424,11 @@ cat <<EOF
> }
> ${PAD_CDTOR-${SMALL_DATA_CTOR-${RELOCATING+${CTOR}}}}
> ${PAD_CDTOR-${SMALL_DATA_DTOR-${RELOCATING+${DTOR}}}}
> - .jcr ${RELOCATING-0} : { KEEP (*(.jcr)) }
> + .jcr ${RELOCATING-0} :
> + {
> + ${RELOCATING+PROVIDE_HIDDEN(__JCR_LIST__ = .);}
> + KEEP (*(.jcr))
> + }
>
> ${RELOCATING+${DATARELRO}}
> ${OTHER_RELRO_SECTIONS}
> Index: lib/csu/crtbegin.c
> ===================================================================
> RCS file: /cvs/src/lib/csu/crtbegin.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 crtbegin.c
> --- lib/csu/crtbegin.c 10 Nov 2015 04:14:03 -0000 1.20
> +++ lib/csu/crtbegin.c 7 Aug 2016 17:42:27 -0000
> @@ -56,15 +56,13 @@ void __register_frame_info(const void *b
> {
> }
>
> -static const char __EH_FRAME_BEGIN__[]
> - __attribute__((section(".eh_frame"), aligned(4))) = { };
> +extern __dso_hidden const char __EH_FRAME_BEGIN__[];
>
> /*
> * java class registration hooks
> */
>
> -static void *__JCR_LIST__[]
> - __attribute__((section(".jcr"), aligned(sizeof(void*)))) = { };
> +extern __dso_hidden void *__JCR_LIST__[];
>
> extern void _Jv_RegisterClasses (void *)
> __attribute__((weak));
> @@ -84,10 +82,20 @@ __asm(".hidden __dso_handle");
> long __guard_local __dso_hidden
> __attribute__((section(".openbsd.randomdata")));
>
>
> -static const init_f __CTOR_LIST__[1]
> - __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
> -static const init_f __DTOR_LIST__[1]
> - __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
> +extern __dso_hidden const init_f __CTOR_LIST__[];
> +extern __dso_hidden const init_f __CTOR_END__[];
> +extern __dso_hidden const init_f __DTOR_LIST__[];
> +extern __dso_hidden const init_f __DTOR_END__[];
> +
> +/*
> + * XXX: Even though __CTOR_END__ and __DTOR_END__ are provided as
> + * hidden in the default ld scripts, these symbols are still globally
> + * visible unless they are referenced from the code, or declared as
> + * hidden in the code once more. Enforce hidden attribute here.
> + */
> +/* hppa doesn't permit directives in first column, so space after newline */
> +__asm(".globl __CTOR_END__\n .hidden __CTOR_END__");
> +__asm(".globl __DTOR_END__\n .hidden __DTOR_END__");
>
> static void __dtors(void) __used;
> static void __ctors(void) __used;
> Index: lib/csu/crtbeginS.c
> ===================================================================
> RCS file: /cvs/src/lib/csu/crtbeginS.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 crtbeginS.c
> --- lib/csu/crtbeginS.c 7 Apr 2015 01:27:06 -0000 1.16
> +++ lib/csu/crtbeginS.c 7 Aug 2016 17:42:27 -0000
> @@ -50,8 +50,7 @@
> * java class registration hooks
> */
>
> -static void *__JCR_LIST__[]
> - __attribute__((section(".jcr"), aligned(sizeof(void*)))) = { };
> +extern __dso_hidden void *__JCR_LIST__[];
>
> extern void _Jv_RegisterClasses (void *)
> __attribute__((weak));
> @@ -94,11 +93,20 @@ pthread_atfork(void (*prep)(void), void
> /* hppa doesn't permit directives in first column, so space after newline */
> asm(".hidden pthread_atfork\n .weak pthread_atfork");
>
> +extern __dso_hidden const init_f __CTOR_LIST__[];
> +extern __dso_hidden const init_f __CTOR_END__[];
> +extern __dso_hidden const init_f __DTOR_LIST__[];
> +extern __dso_hidden const init_f __DTOR_END__[];
>
> -static init_f __CTOR_LIST__[1]
> - __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
> -static init_f __DTOR_LIST__[1]
> - __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
> +/*
> + * XXX: Even though __CTOR_END__ and __DTOR_END__ are provided as
> + * hidden in the default ld scripts, these symbols are still globally
> + * visible unless they are referenced from the code, or declared as
> + * hidden in the code once more. Enforce hidden attribute here.
> + */
> +/* hppa doesn't permit directives in first column, so space after newline */
> +__asm(".globl __CTOR_END__\n .hidden __CTOR_END__");
> +__asm(".globl __DTOR_END__\n .hidden __DTOR_END__");
>
> static void __dtors(void) __used;
> static void __ctors(void) __used;
> @@ -107,7 +115,7 @@ void
> __ctors(void)
> {
> unsigned long i = (unsigned long) __CTOR_LIST__[0];
> - init_f *p;
> + const init_f *p;
>
> if (i == -1) {
> for (i = 1; __CTOR_LIST__[i] != NULL; i++)
> @@ -123,7 +131,7 @@ __ctors(void)
> static void
> __dtors(void)
> {
> - init_f *p = __DTOR_LIST__ + 1;
> + const init_f *p = __DTOR_LIST__ + 1;
>
> while (*p) {
> (**p++)();
> Index: lib/csu/crtend.c
> ===================================================================
> RCS file: /cvs/src/lib/csu/crtend.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 crtend.c
> --- lib/csu/crtend.c 4 Apr 2015 18:05:05 -0000 1.11
> +++ lib/csu/crtend.c 7 Aug 2016 17:42:27 -0000
> @@ -5,11 +5,6 @@
> #include "md_init.h"
> #include "extern.h"
>
> -static init_f __CTOR_LIST__[1]
> - __used __attribute__((section(".ctors"))) = { (void *)0 }; /* XXX
> */
> -static init_f __DTOR_LIST__[1]
> - __used __attribute__((section(".dtors"))) = { (void *)0 }; /* XXX
> */
> -
> static const int __EH_FRAME_END__[]
> __used __attribute__((section(".eh_frame"), aligned(4))) = { 0 };
>
> Index: lib/csu/crtendS.c
> ===================================================================
> RCS file: /cvs/src/lib/csu/crtendS.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 crtendS.c
> --- lib/csu/crtendS.c 4 Apr 2015 18:05:05 -0000 1.9
> +++ lib/csu/crtendS.c 7 Aug 2016 17:42:27 -0000
> @@ -5,11 +5,6 @@
> #include "md_init.h"
> #include "extern.h"
>
> -static init_f __CTOR_LIST__[1]
> - __used __attribute__((section(".ctors"))) = { (void *)0 }; /* XXX
> */
> -static init_f __DTOR_LIST__[1]
> - __used __attribute__((section(".dtors"))) = { (void *)0 }; /* XXX
> */
> -
> static void * __JCR_END__[]
> __used __attribute__((section(".jcr"), aligned(sizeof(void*)))) = { 0 };
>
>
> > >> > While there, clean up crtbegin.c and crtbegin.S a little and make them
> > >> > more similar.
> >
> > I'm fine with this being done...once we sure we actually have nailed
> > down what are the actual changes necessary to get the code to compile
> > correctly without changing behavior.
> >
> > ...
> > >> The existing code tries to retrieve the number of valid ctors entries
> > >> from __CTOR_LIST__[0], only when that number is -1 it tries to find
> > >> the actual value by walking the array.
> >
> > Since we only have one version of ld doing linking now, can't we
> > switch from the -1 to the real count version?
> >
> > ...
> > > Also, aren't ctor_list and dtor_list polluting the namespace?
> >
> > Yep.
> >
> >
> > Philip
>
