[PATCH] D12001: Implement __emutls_get_address

[Chih-Hung Hsieh via llvm-commits]

chh marked 10 inline comments as done.

================
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.
+ */
----------------
danalbert wrote:
> 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.
If posix_memalign is used, current Android linker won't link because it wants to redefine malloc and free, but not posix_memalign. We can change Android linker, but I think making this module not depending on only basic malloc/free will be more portable. It's not going to be fast accessing emutls variables anyway.


================
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)); \
----------------
danalbert wrote:
> Should probably find (or create) a common header for this.
Still looking for a place to put this.


================
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;
----------------
danalbert wrote:
> emutls_address_array* instead of void*? This is C, so there's no mangling concern.
It should be void* for pthread_key_create's argument type.


================
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 *));
----------------
danalbert wrote:
> Probably better to be aligning this multiples of 16 or something instead of just adding 16 since index can be any integer here.
This calloc or realloc does not need 16-byte or 16-slot aligned address.
It's just some extra slots to avoid too many calloc or realloc.
libgcc uses increment of 32 slots, which seems too big.
Most apps have only a few TLS variables.



================
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 *));
----------------
danalbert wrote:
> 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.
Other threads can use more than index + 16 variables, but the current thread only need to allocate space for index number of slots, for this call. When other TLS variables are used by this thread later, this thread can allocate more slots if necessary.


================
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; \
----------------
danalbert wrote:
> include file and line number?
I would usually do that, but all other tests use this simple style without file/line number.
Guess it's the style preferred here.


================
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));
----------------
danalbert wrote:
> If this isn't true we have a much bigger problem.
Yes, I hope the compile time check is useful to catch configuration errors in cross compilers.


================
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));
----------------
danalbert wrote:
> The above three don't offer anything that the static asserts don't.
Yes, removing them.




http://reviews.llvm.org/D12001