[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

32

s/int/uintptr_t/

349–350

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

102

line 96 could be just emutls_setspecific(array).

371

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

371

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

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.

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
+#else /* _WIN32 */
 
 #include <windows.h>
 #include <malloc.h>
 #include <stdio.h>
 #include <assert.h>
 
 static LPCRITICAL_SECTION emutls_mutex;
 static DWORD emutls_tls_index = TLS_OUT_OF_INDEXES;

 #endif
 }
 
 static __inline void __atomic_store_n(void *ptr, uintptr_t val, unsigned type) {
     assert(type == __ATOMIC_RELEASE);
     InterlockedExchangePointer((void *volatile *)ptr, (void *)val);
 }
 
-#endif
+#endif /* __ATOMIC_RELEASE */
 
 #pragma warning (pop)
 
-#endif
+#endif /* _WIN32 */
 
 static size_t emutls_num_object = 0;  /* number of allocated TLS objects */
 
 /* Free the allocated TLS data
  */
 static void emutls_shutdown(emutls_address_array *array) {
     if (array) {
         uintptr_t i;
static __inline void emutls_check_array_set_size(emutls_address_array *array,
     array->size = size;
     emutls_setspecific(array);
 }
 
 /* Returns the new 'data' array size, number of elements,
  * which must be no smaller than the given index.
  */
 static __inline uintptr_t emutls_new_data_array_size(uintptr_t index) {
-   /* Need to allocate emutls_address_array with one extra slot
-    * to store the data array size.
+   /* Need to allocate emutls_address_array with extra slots
+    * to store the header.
     * Round up the emutls_address_array size to multiple of 16.
     */
-    return ((index + 1 + 15) & ~((uintptr_t)15)) - 1;
+    uintptr_t header_words = sizeof(emutls_address_array) / sizeof(void *);
+    return ((index + header_words + 15) & ~((uintptr_t)15)) - header_words;
 }
 
 /* Returns the size in bytes required for an emutls_address_array with
  * N number of elements for data field.
  */
 static __inline uintptr_t emutls_asize(uintptr_t N) {
     return N * sizeof(void *) + sizeof(emutls_address_array);
 }
 
 /* Returns the thread local emutls_address_array.
  * 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*));