DragonFlyBSD

I forgot: To test this, one must have world built at least to commit b28bf640312db2b299faff75052fbb01d67fd821
Sat, 11 Feb 2012 20:04:12 +0000 (21:04 +0100)
rtld: Add support for preinit, init, and fini arrays

dmalloc is unhappy because the preinit code is calling malloc() before the malloc_init constructor has had a chance to run.

malloc_init() for dmalloc is called by a reserved-priority constructor; that call is set to happen after the preinit vector is executed, however. nmalloc is okay because malloc_init does nothing of urgent need there; dmalloc uses it to call its own (misnamed) _nmalloc_thr_init() for the first thread. [nmalloc calls that from the thread library and doesn't need it in the first thread].

• Have dmalloc check if it has run malloc_init, and if not, run it. {remember to interlock correctly!}
• Force malloc's initialization to happen before preinit
• something else?

Which fix do you recommend?
Or to put it another way: Who do you want to answer this question? It seems to me that you are more qualified than most...

--- /usr/src/lib/libc/stdlib/dmalloc.c 2012-05-10 17:31:04.234525000 -0700
+++ dmalloc.c 2012-05-18 22:50:07.624198000 -0700
@ -303,6 +303,7 @
static int opt_utrace = 0;
static int g_malloc_flags = 0;
static int malloc_panic;
+static int malloc_started = 0;

static const int32_t weirdary[16] = {
     WEIRD_ADDR, WEIRD_ADDR, WEIRD_ADDR, WEIRD_ADDR,
@ -320,11 +321,7 @
 static void *_vmem_alloc(int ri, size_t slab_size);
 static void _vmem_free(void *ptr, size_t slab_size);
 static void _mpanic(const char *ctl, ...) __printflike(1, 2);
#ifndef STANDALONE_DEBUG
-static void malloc_init(void) __constructor(0);
#else
static void malloc_init(void) __constructor(101);
#endif
+static void malloc_init(void);

struct nmalloc_utrace {
     void *p;
@ -353,6 +350,18 @
 malloc_init(void)
{
     const char *p = NULL;
+    static spinlock_t malloc_init_lock;

    if (malloc_started)
+        return;

    if (__isthreaded) {
+        _SPINLOCK(&malloc_init_lock);
+        if (malloc_started) {
+            _SPINUNLOCK(&malloc_init_lock);
+            return;
+        }
+    }

Regions[0].mask = -1; /* disallow activity in lowest region */

@ -399,6 +408,10 @

UTRACE((void *) -1, 0, NULL);
     nmalloc_thr_init();
+    malloc_started = 1;

    if (_isthreaded)
+        _SPINUNLOCK(&malloc_init_lock);
 }

/*
@ -712,6 +725,9 @
     size_t off;
     char *obj;

+ if (!malloc_started)
+ malloc_init();
+
/* * If 0 bytes is requested we have to return a unique pointer, allocate * at least one byte.

Is a diff that does solve this problem (and is tested). However, it adds a (hopefully) minor cost to malloc() calls, a test of the malloc_started variable. I'd like to apply it after some review, but other opinions welcome.

The patch was premature; it had malloc_init happen before libpthread_init in threaded programs, which is not acceptable...

The straightforward fix is to apply the original commit,
(http://gitweb.dragonflybsd.org/dragonfly.git/blobdiff_plain/7a7e10d6267ef12bf6e84d69a9e77fec14d6dccb..e12d3396c777165504d60d2a1408dcd7cb63660d:/lib/libc/stdlib/dmalloc.c), however do not remove the __constructor(0) attribute on malloc_init.

This will force malloc_init to always run before pthread init; initialization works correctly when malloc is set up before pthreads, as malloc will not force pthreads to be set up before it is safe. This is tested in full-system builds, with both threaded and unthreaded programs, with and without preinit arrays.

There is one concern which has been raised -- the addition of a conditional (if (malloc_started == 0) malloc_init(); adds a cost to allocations and frees. I don't think the cost is significant, as this branch is going to predict as 'not taken' roughly all the time (except for once), but it has been brought up. An alternate approach would be to add the notion of 'preconstructors' to RTLD and to execute malloc initialization and pthread initialization from there.