code review 106380043: cmd/go, cmd/ld, runtime, os/user: TLS emultion for android

src/pkg/runtime/cgo/gcc_android_arm.c

+// Copyright 2014 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+#include <android/log.h>
+#include <pthread.h>
+#include <signal.h>
+#include <stdio.h>
+#include "libcgo.h"
+
+#define magic1 (0x23581321U)
+
+// inittls finds the first available pthread slot and uses it as

[minux, 2014/07/02 04:21:15]

s/pthread slot/thread-local storage slot/

see pkg/runtime/cgo/gcc_darwin_386.c for an example of
this comment.

[crawshaw, 2014/07/02 14:57:22]

Done.

+// the offset value for runtime.tlsg.
+static void
+inittls(void **tlsg, void **tlsbase)
+{
+        pthread_key_t k;
+        int i, err;
+
+        err = pthread_key_create(&k, nil);
+        if(err != 0) {
+                fprintf(stderr, "runtime/cgo: pthread_key_create failed: %d\n", err);
+                __android_log_print(ANDROID_LOG_FATAL, "runtime/cgo", "pthread_key_create failed: %d", err);
+                abort();
+        }
+        pthread_setspecific(k, (void*)magic1);
+        for (i=0; i<50; i++) {

[minux, 2014/07/02 04:21:15]

probably 50 is too small? darwin/386 tries 128 slots.

however, too big a number and we might get SIGSEGV/SIGBUS.

i wonder if we should just use pthread_getspecific in
load_g and save_g?

Either way we need to make a few assumptions.
1. (your current way) we can read at least N words of TLS without problem and
tls slots used by other part of the application is less than N.
2. (use pthread_getspecific always) it doesn't use much stack space so it can be
called from goroutine stack.

i think 2 is better than 1. what do you think?
of course, in load_g and save_g we need to save all the caller-save
registers, but it's not that bad. (we need to save r0-r3, r9, r12, r14)

[crawshaw, 2014/07/02 14:57:22]

I'm not comfortable assuming that pthread_getspecific uses little stack space.
There is no promise of that in the bionic API, nor POSIX.

However, good news, bionic tip adds #define PTHREAD_KEYS_MAX, bringing their
pthread implementation a little closer to spec. Sadly it's not in the current
NDK release, so I have added a define and a TODO. Does that seem reasonable?

[minux, 2014/07/02 17:16:53]

ok.

+                if (*(tlsbase+i) == (void*)magic1) {
+                        *tlsg = (void*)(i*sizeof(void *));
+                        pthread_setspecific(k, 0);
+                        return;
+                }
+        }
+        fprintf(stderr, "runtime/cgo: could not find pthread key\n");
+        __android_log_print(ANDROID_LOG_FATAL, "runtime/cgo", "could not find pthread key");
+        abort();
+}
+
+extern void (*x_cgo_inittls)(void **tlsg, void **tlsbase);
+void (*x_cgo_inittls)(void **tlsg, void **tlsbase) = inittls;

[minux, 2014/07/02 04:21:15]

i think you can remove the extern declaration here.

[crawshaw, 2014/07/02 14:57:22]

Done.