C startup code: make crtbegin code safe for clang

[Stefan Kempf]

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.

Use constructor/destructor calling code that should be legal in both the
gcc and clang C dialect, to hopefully be immune from undefined behavior
optimizations in the future.

While there, clean up crtbegin.c and crtbegin.S a little and make them
more similar.

ok?

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 1 Aug 2016 16:56:31 -0000
@@ -85,36 +85,37 @@ long __guard_local __dso_hidden __attrib


static const init_f __CTOR_LIST__[1]
- __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
static const init_f __DTOR_LIST__[1]
- __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+
+extern const init_f ctor_list[] asm(".ctors");
+extern const init_f dtor_list[] asm(".dtors");

static void __dtors(void) __used;
static void __ctors(void) __used;

static void
-__ctors()
+__ctors(void)
{
- unsigned long i = (unsigned long) __CTOR_LIST__[0];
- const init_f *p;
+ int i;
+
+ for (i = 0; ctor_list[i + 1] != NULL; i++)
+ continue;

- if (i == -1) {
- for (i = 1; __CTOR_LIST__[i] != NULL; i++)
- ;
+ while (i > 0) {
+ ctor_list[i]();
i--;
}
- p = __CTOR_LIST__ + i;
- while (i--)
- (**p--)();
}

static void
-__dtors()
+__dtors(void)
{
- const init_f *p = __DTOR_LIST__ + 1;
+ int i;

- while (*p)
- (**p++)();
+ for (i = 1; dtor_list[i] != NULL; i++)
+ dtor_list[i]();
}

void __init(void);
@@ -130,8 +131,8 @@ MD_SECT_CALL_FUNC(".init", __do_init);
MD_SECT_CALL_FUNC(".fini", __do_fini);


-void
-__do_init()
+static void
+__do_init(void)
{
static int initialized = 0;
static struct dwarf2_eh_object object;
@@ -146,14 +147,14 @@ __do_init()
__register_frame_info(__EH_FRAME_BEGIN__, &object);
if (__JCR_LIST__[0] && _Jv_RegisterClasses)
_Jv_RegisterClasses(__JCR_LIST__);
- (__ctors)();
+ __ctors();

atexit(__fini);
}
}

-void
-__do_fini()
+static void
+__do_fini(void)
{
static int finalized = 0;

@@ -162,7 +163,7 @@ __do_fini()
/*
* Call global destructors.
*/
- (__dtors)();
+ __dtors();
}
}

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 1 Aug 2016 16:56:31 -0000
@@ -38,7 +38,6 @@
* constructors and destructors. The first element contains the
* number of pointers in each.
* The tables are also null-terminated.
-
*/
#include <stdlib.h>

@@ -96,39 +95,39 @@ asm(".hidden pthread_atfork\n .weak pthr


static init_f __CTOR_LIST__[1]
- __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
static init_f __DTOR_LIST__[1]
- __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+
+extern const init_f ctor_list[] asm(".ctors");
+extern const init_f dtor_list[] asm(".dtors");

static void __dtors(void) __used;
static void __ctors(void) __used;

-void
+static void
__ctors(void)
{
- unsigned long i = (unsigned long) __CTOR_LIST__[0];
- init_f *p;
+ int i;

- if (i == -1) {
- for (i = 1; __CTOR_LIST__[i] != NULL; i++)
- ;
+ for (i = 0; ctor_list[i + 1] != NULL; i++)
+ continue;
+
+ while (i > 0) {
+ ctor_list[i]();
i--;
}
- p = __CTOR_LIST__ + i;
- while (i--) {
- (**p--)();
- }
}

static void
__dtors(void)
{
- init_f *p = __DTOR_LIST__ + 1;
+ int i;

- while (*p) {
- (**p++)();
- }
+ for (i = 1; dtor_list[i] != NULL; i++)
+ dtor_list[i]();
}
+
void _init(void);
void _fini(void);
static void _do_init(void) __used;
@@ -141,7 +140,7 @@ MD_SECTION_PROLOGUE(".fini", _fini);
MD_SECT_CALL_FUNC(".init", _do_init);
MD_SECT_CALL_FUNC(".fini", _do_fini);

-void
+static void
_do_init(void)
{
static int initialized = 0;
@@ -159,7 +158,7 @@ _do_init(void)
}
}

-void
+static void
_do_fini(void)
{
static int finalized = 0;

[Jeremie Courreges-Anglas]

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.

The ld(1) info page states:

The symbol `__CTOR_LIST__' marks the start of the global
constructors, and the symbol `__CTOR_END__' marks the end.
Similarly, `__DTOR_LIST__' and `__DTOR_END__' mark the start and
end of the global destructors. The first word in the list is the
number of entries, followed by the address of each constructor or
destructor, followed by a zero word. The compiler must arrange to
actually run the code. For these object file formats GNU C++
normally calls constructors from a subroutine `__main'; a call to
`__main' is automatically inserted into the startup code for
`main'. GNU C++ normally runs destructors either by using
`atexit', or directly from the function `exit'.

If that is correct your code should behave the same. But what if...?

--
jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE

[Mark Kettenis]

> From: j...@wxcvbn.org (Jeremie Courreges-Anglas)
> Date: Mon, 01 Aug 2016 20:30:33 +0200

Right, the code is not equivalent. We'd have to look carefully at gcc
and ld to see if that matters.

Also, aren't ctor_list and dtor_list polluting the namespace?

[Philip Guenther]

On Mon, Aug 1, 2016 at 11:45 AM, Mark Kettenis <mark.k...@xs4all.nl> wrote:
>> From: j...@wxcvbn.org (Jeremie Courreges-Anglas)
>> Date: Mon, 01 Aug 2016 20:30:33 +0200
>>
>> Stefan Kempf <sisn...@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?

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

[Stefan Kempf]

Philip Guenther wrote:
> On Mon, Aug 1, 2016 at 11:45 AM, Mark Kettenis <mark.k...@xs4all.nl> wrote:
> >> From: j...@wxcvbn.org (Jeremie Courreges-Anglas)
> >> Date: Mon, 01 Aug 2016 20:30:33 +0200
> >>
> >> Stefan Kempf <sisn...@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?

Right, we need the extern trick.
I'll take a look into doing it with counts and __CTOR_LIST__ and
__CTOR_END__. We want __CTOR_LIST__ and __CTOR_END__ to be .hidden
symbols, right?

[Joerg Sonnenberger]

On Mon, Aug 01, 2016 at 07:28:34PM +0200, Stefan Kempf wrote:
> 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.

I don't think your code is safe for the more aggressive behavior esp. of
newer GCC. Given how much fun I had recently on this, consider just
using the NetBSD version of this code.

Joerg