[builtins] Delay emutls deallocation for one round

[rprichard]

Summary

With Android/Bionic, delay deallocation to round 2 of 4. It must run after
C++ thread_local destructors have been called, but before the final 2
rounds, because emutls calls free, and jemalloc then needs another 2
rounds to free its thread-specific data.

Fixes https://github.com/android-ndk/ndk/issues/687

[rprichard]

I uploaded this change earlier to https://android-review.googlesource.com/c/toolchain/compiler-rt/+/683602.

[srhines]

Thanks, Ryan, for chasing this down (literally), and restoring some sanity to teardown.

[chh]

lib/builtins/emutls.c

33 ↗

(On Diff #147186)

s/int/uintptr_t/

350 ↗

(On Diff #147186)

s/int/uintptr_t/

[rprichard]

My emutls.c comment:

We can't wait until the final two rounds, because jemalloc needs two rounds
after the final malloc/free call to free its thread-specific data.

It might be helpful to explain on this review why jemalloc needs 2 rounds. Here's my comment from Google's bug tracker, http://b/78022094#comment13. I'll link to it from the code.

jemalloc's TSD (thread-specific data) code has four compile-time modes:

• JEMALLOC_MALLOC_THREAD_CLEANUP: uses __thread variables, and the code using jemalloc must call a _malloc_thread_cleanup function at thread-exit. Apparently FreeBSD uses this.
• JEMALLOC_TLS: uses __thread variables and a pthread key destructor
• _WIN32: self-explanatory
• default: uses pthread_getspecific / pthread_setspecific and a key destructor

Bionic must use the final mode (emutls uses malloc, so malloc can't use emutls).

A jemalloc TSD has four explicit states:

• uninitialized
• nominal
• purgatory
• reincarnated

Summary of jemalloc cleanup states:

• The typical state is nominal
• On nominal cleanup: free everything but the outer TSD struct, move to purgatory state, and schedule another dtor call
• On purgatory cleanup: free the TSD struct
• On reincarnated cleanup: move to purgatory state and schedule another dtor call
• Calling malloc/free moves the state from purgatory to reincarnated.

jemalloc needs 2 pthread destructor rounds to free its TSD (nominal -> purgatory, purgatory -> deallocated).

If emutls cleanup happens on round 2 instead of 3, then jemalloc can be completely deallocated at the start of round 2, then reinitialized when emutls calls free. In round 3, jemalloc would enter purgatory, and in round 4, it would be deallocated again. I *could* try to avoid this with an otherwise pointless realloc call in emutls round 1, but it doesn't seem to make thread exit that much slower. I measured a slowdown on the order of ~10-20us per thread exit.

[rprichard]

Address chh's comments from
https://android-review.googlesource.com/c/toolchain/compiler-rt/+/683602
(change int -> uintptr_t) and add a link to my Phabricator jemalloc comment.

[chh]

Only two minor suggestions. Everything looks fine. Thanks.

lib/builtins/emutls.c

96 ↗

(On Diff #147953)

line 96 could be just emutls_setspecific(array).

373 ↗

(On Diff #147953)

nit, line 373 sets array field; it's better to be nested inside the if-statement of line 371.

[rprichard]

Avoid a NULL dereference on out-of-memory. Reuse emutls_setspecific.

[rprichard]

lib/builtins/emutls.c

373 ↗

(On Diff #147953)

Good catch. I think that line could have dereferenced NULL on out-of-memory.

[srhines]

Everything looks great now. Thanks again, Ryan!

[echristo]

One inline comment, but otherwise looks ok. You could also split out all of the non-bionic/skip_destructor_rounds into cleanup and other patches as well.

lib/builtins/emutls.c

27 ↗

(On Diff #148110)

No links to internal bug reporting infrastructure please.

[rprichard]

Remove links to bug trackers. (I think the links to the internal Google
tracker need to be removed because other members of the LLVM community can't
access them. I think I *could* keep the GitHub link, but I don't think
there's anything especially useful there.) The Phabricator link is useful,
because it explains why jemalloc needs 2 rounds.

Split out part of the change into an earlier cleanup commit.

[rprichard]

I think(?) each git commit should be a separate Phabricator review?

[echristo]

LGTM.

compiler-rt/trunk/lib/builtins/emutls.c

 /* ===---------- emutls.c - Implements __emutls_get_address ---------------===
  *
  *                     The LLVM Compiler Infrastructure
  *
  * This file is dual licensed under the MIT and the University of Illinois Open
  * Source Licenses. See LICENSE.TXT for details.
  *
  * ===----------------------------------------------------------------------===
  */
 #include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 
 #include "int_lib.h"
 #include "int_util.h"
 
+#ifdef __BIONIC__
+/* There are 4 pthread key cleanup rounds on Bionic. Delay emutls deallocation
+   to round 2. We need to delay deallocation because:
+    - Android versions older than M lack __cxa_thread_atexit_impl, so apps
+      use a pthread key destructor to call C++ destructors.
+    - Apps might use __thread/thread_local variables in pthread destructors.
+   We can't wait until the final two rounds, because jemalloc needs two rounds
+   after the final malloc/free call to free its thread-specific data (see
+   https://reviews.llvm.org/D46978#1107507). */
+#define EMUTLS_SKIP_DESTRUCTOR_ROUNDS 1
+#else
+#define EMUTLS_SKIP_DESTRUCTOR_ROUNDS 0
+#endif
+
 typedef struct emutls_address_array {
+    uintptr_t skip_destructor_rounds;
     uintptr_t size;  /* number of elements in the 'data' array */
     void* data[];
 } emutls_address_array;
 
 static void emutls_shutdown(emutls_address_array *array);
 
 #ifndef _WIN32
 

 #if EMUTLS_USE_POSIX_MEMALIGN
     free(base);
 #else
     /* The mallocated address is in ((void**)base)[-1] */
     free(((void**)base)[-1]);
 #endif
 }
 
+static __inline void emutls_setspecific(emutls_address_array *value) {
+    pthread_setspecific(emutls_pthread_key, (void*) value);
+}
+
+static __inline emutls_address_array* emutls_getspecific() {
+    return (emutls_address_array*) pthread_getspecific(emutls_pthread_key);
+}
+
 static void emutls_key_destructor(void* ptr) {
-    emutls_shutdown((emutls_address_array*)ptr);
+    emutls_address_array *array = (emutls_address_array*)ptr;
+    if (array->skip_destructor_rounds > 0) {
+        /* emutls is deallocated using a pthread key destructor. These
+         * destructors are called in several rounds to accommodate destructor
+         * functions that (re)initialize key values with pthread_setspecific.
+         * Delay the emutls deallocation to accommodate other end-of-thread
+         * cleanup tasks like calling thread_local destructors (e.g. the
+         * __cxa_thread_atexit fallback in libc++abi).
+         */
+        array->skip_destructor_rounds--;
+        emutls_setspecific(array);
+    } else {
+        emutls_shutdown(array);
         free(ptr);
     }
+}
 
 static __inline void emutls_init(void) {
     if (pthread_key_create(&emutls_pthread_key, emutls_key_destructor) != 0)
         abort();
 }
 
 static __inline void emutls_init_once(void) {
     static pthread_once_t once = PTHREAD_ONCE_INIT;
     pthread_once(&once, emutls_init);
 }
 
 static __inline void emutls_lock() {
     pthread_mutex_lock(&emutls_mutex);
 }
 
 static __inline void emutls_unlock() {
     pthread_mutex_unlock(&emutls_mutex);
 }
 
-static __inline void emutls_setspecific(emutls_address_array *value) {
-    pthread_setspecific(emutls_pthread_key, (void*) value);
-}
-
-static __inline emutls_address_array* emutls_getspecific() {
-    return (emutls_address_array*) pthread_getspecific(emutls_pthread_key);
-}
-
 #else /* _WIN32 */
 
 #include <windows.h>
 #include <malloc.h>
 #include <stdio.h>
 #include <assert.h>
 
 static LPCRITICAL_SECTION emutls_mutex;

  * Extends its size if necessary to hold address at index.
  */
 static __inline emutls_address_array *
 emutls_get_address_array(uintptr_t index) {
     emutls_address_array* array = emutls_getspecific();
     if (array == NULL) {
         uintptr_t new_size = emutls_new_data_array_size(index);
         array = (emutls_address_array*) malloc(emutls_asize(new_size));
-        if (array)
+        if (array) {
             memset(array->data, 0, new_size * sizeof(void*));
+            array->skip_destructor_rounds = EMUTLS_SKIP_DESTRUCTOR_ROUNDS;
+        }
         emutls_check_array_set_size(array, new_size);
     } else if (index > array->size) {
         uintptr_t orig_size = array->size;
         uintptr_t new_size = emutls_new_data_array_size(index);
         array = (emutls_address_array*) realloc(array, emutls_asize(new_size));
         if (array)
             memset(array->data + orig_size, 0,
                    (new_size - orig_size) * sizeof(void*));