[PATCH] D12001: Implement __emutls_get_address

[Dan Albert via llvm-commits]

danalbert added inline comments.

================
Comment at: compiler-rt/lib/builtins/emutls.c:17
@@ +16,3 @@
+/* Default is not to use posix_memalign, so systems like Android
+ * can use thread local data without heavier POSIX memory allocators.
+ */
----------------
Just checked with cferris. I don't think this is a default we even want on Android. Sounds like this would maybe be a win for dlmalloc with us, but for jemalloc `posix_memalign` should be fine. Older devices + N9 are dlmalloc, but he still thinks it's unlikely this is necessary.

================
Comment at: compiler-rt/lib/builtins/emutls.c:33
@@ +32,3 @@
+        uintptr_t index;  /* data[index-1] is the object address */
+        void*   address;  /* object address, when in single thread env */
+    } object;
----------------
Only one space between `void*` and `address` (I believe, I'm not very familiar with style practices here)

================
Comment at: compiler-rt/lib/builtins/emutls.c:35
@@ +34,3 @@
+    } object;
+    void* value;  /* non-zero initial value for the object */
+} __emutls_control;
----------------
Or null

================
Comment at: compiler-rt/lib/builtins/emutls.c:40
@@ +39,3 @@
+static inline void* emutls_allocate_object(__emutls_control* control) {
+    #define COMPILE_TIME_ASSERT(pred) { \
+        char dummy[(pred) ? 0 : -1] __attribute__((unused)); \
----------------
Should probably find (or create) a common header for this.

================
Comment at: compiler-rt/lib/builtins/emutls.c:67
@@ +66,3 @@
+        abort();
+    base = (void *) (((uintptr_t) (object + sizeof(void*) + align - 1))
+                     & ~(uintptr_t)(align - 1));
----------------
No space between the cast and the value

================
Comment at: compiler-rt/lib/builtins/emutls.c:85
@@ +84,3 @@
+    uintptr_t size;
+    void*     data[];
+} emutls_address_array;
----------------
text alignment again

================
Comment at: compiler-rt/lib/builtins/emutls.c:90
@@ +89,3 @@
+
+static void emutls_key_destructor(void *ptr) {
+    emutls_address_array* array = (emutls_address_array*) ptr;
----------------
emutls_address_array* instead of void*? This is C, so there's no mangling concern.

================
Comment at: compiler-rt/lib/builtins/emutls.c:93
@@ +92,3 @@
+    uintptr_t i;
+    for (i = 0; i < array->size; ++i)
+        if (array->data[i]) {
----------------
A non-trivial loop body should have braces.

================
Comment at: compiler-rt/lib/builtins/emutls.c:107
@@ +106,3 @@
+    if (pthread_key_create(&emutls_pthread_key, emutls_key_destructor) != 0)
+        abort ();
+}
----------------
`abort()` (no space)

================
Comment at: compiler-rt/lib/builtins/emutls.c:128
@@ +127,3 @@
+/* Updates newly allocated thread local emutls_address_array. */
+static inline void emutls_check_array_set_size(emutls_address_array *array,
+                                               uintptr_t size) {
----------------
Inconsistent pointer positioning. Most of the file has `type* var`, here is `type *var`. Not sure what the style is here, but stick to it (or stick to one for this particular file if there isn't consistent style across the whole tree).

================
Comment at: compiler-rt/lib/builtins/emutls.c:133
@@ +132,3 @@
+    array->size = size;
+    pthread_setspecific(emutls_pthread_key, (void *) array);
+}
----------------
No space after the cast

================
Comment at: compiler-rt/lib/builtins/emutls.c:142
@@ +141,3 @@
+    if (array == NULL) {
+        uintptr_t size = index + 16;  /* allocate 16 extra slots */
+        array = calloc(size + 1, sizeof(void *));
----------------
Probably better to be aligning this multiples of 16 or something instead of just adding 16 since index can be any integer here.

================
Comment at: compiler-rt/lib/builtins/emutls.c:149
@@ +148,3 @@
+        if (index > size)
+            size = index + 16;  /* allocate 16 extra slots */
+        array = realloc(array, (size + 1) * sizeof(void *));
----------------
This doesn't actually guarantee that we've made enough space, right? If I'm reading this correctly, we need the check on the line above because some other thread may have used many TLS variables that our thread hasn't encountered yet, so our array needs to expand to accommodate them, but if the index is significantly larger than our current array then we won't have made enough space.

================
Comment at: compiler-rt/test/builtins/Unit/emutls_test.c:14
@@ +13,3 @@
+
+#include <stdlib.h>
+#include <stdint.h>
----------------
sort

================
Comment at: compiler-rt/test/builtins/Unit/emutls_test.c:50
@@ +49,3 @@
+    if (!(v)) { \
+        printf("error in emutls expect true: (%s)\n", #v); \
+        return 1; \
----------------
include file and line number?

================
Comment at: compiler-rt/test/builtins/Unit/emutls_test.c:74
@@ +73,3 @@
+    COMPILE_TIME_ASSERT(sizeof(uintptr_t) == sizeof(gcc_pointer));
+    COMPILE_TIME_ASSERT(sizeof(uintptr_t) == sizeof(void*));
+    ASSERT_EQ(sizeof(size_t), sizeof(gcc_word));
----------------
If this isn't true we have a much bigger problem.

================
Comment at: compiler-rt/test/builtins/Unit/emutls_test.c:77
@@ +76,3 @@
+    ASSERT_EQ(sizeof(uintptr_t), sizeof(gcc_pointer));
+    ASSERT_EQ(sizeof(uintptr_t), sizeof(void*));
+    ASSERT_EQ(sizeof(__emutls_control), sizeof(struct gcc_emutls_object));
----------------
The above three don't offer anything that the static asserts don't.

================
Comment at: compiler-rt/test/builtins/Unit/emutls_test.c:78
@@ +77,3 @@
+    ASSERT_EQ(sizeof(uintptr_t), sizeof(void*));
+    ASSERT_EQ(sizeof(__emutls_control), sizeof(struct gcc_emutls_object));
+    return 0;
----------------
Make it a static assert.


http://reviews.llvm.org/D12001