diff mbox

Go patch committed: Multiplex goroutines onto OS threads

Message ID mcrhaz1vqms.fsf@dhcp-172-18-216-180.mtv.corp.google.com
State New
Headers show

Commit Message

Ian Lance Taylor Feb. 8, 2012, 5:30 a.m. UTC
Ian Lance Taylor <iant@google.com> writes:

> Right now it looks like there is a bug, or at least an incompatibility,
> in the 64-bit versions of getcontext and setcontext.  It looks like
> calling setcontext on the 32-bit version does not change the value of
> TLS variables, which is also the case on GNU/Linux.  In the 64-bit
> version, calling setcontext does change the value of TLS variables.
> That is, on the 64-bit version, getcontext preserves the value of TLS
> variables and setcontext restores the old value.  (Of course it's really
> the pointer, not the TLS variables themselves).  This incompatibility
> has to be a bug, and of course I would prefer that the incompatibility
> be resolved in favor of the implementation used on GNU/Linux.

Well, I thought I could fix this in a sensible way, but I really
couldn't.  It's a fairly serious bug.  Calling setcontext in thread T2
when getcontext was called in thread T1 causes T2 to start using T1's
TLS variables, T1's pthread_getspecific values, and even T1's
pthread_self.  I couldn't figure out any way that T2 could do any thread
specific.

So I cheated and wrote a system-specific hack.  With this patch applied,
I see only 4 failures running the testsuite on x86/x86_64 Solaris.
Bootstrapped and ran Go testsuite on x86_64-unknown-linux-gnu, where the
code is not used.  Committed to mainline.

Ian
diff mbox

Patch

diff -r d9bce1ef2e17 libgo/configure.ac
--- a/libgo/configure.ac	Tue Feb 07 11:19:23 2012 -0800
+++ b/libgo/configure.ac	Tue Feb 07 21:19:06 2012 -0800
@@ -549,6 +549,87 @@ 
 STRUCT_EPOLL_EVENT_FD_OFFSET=${libgo_cv_c_epoll_event_fd_offset}
 AC_SUBST(STRUCT_EPOLL_EVENT_FD_OFFSET)
 
+dnl See whether setcontext changes the value of TLS variables.
+AC_CACHE_CHECK([whether setcontext clobbers TLS variables],
+[libgo_cv_lib_setcontext_clobbers_tls],
+[LIBS_hold="$LIBS"
+LIBS="$LIBS $PTHREAD_LIBS"
+AC_RUN_IFELSE(
+  [AC_LANG_SOURCE([
+#include <pthread.h>
+#include <stdlib.h>
+#include <ucontext.h>
+#include <unistd.h>
+
+__thread int tls;
+
+static char stack[[10 * 1024 * 1024]];
+static ucontext_t c;
+
+/* Called via makecontext/setcontext.  */
+
+static void
+cfn (void)
+{
+  exit (tls);
+}
+
+/* Called via pthread_create.  */
+
+static void *
+tfn (void *dummy)
+{
+  /* The thread should still see this value after calling
+     setcontext.  */
+  tls = 0;
+
+  setcontext (&c);
+
+  /* The call to setcontext should not return.  */
+  abort ();
+}
+
+int
+main ()
+{
+  pthread_t tid;
+
+  /* The thread should not see this value.  */
+  tls = 1;
+
+  if (getcontext (&c) < 0)
+    abort ();
+
+  c.uc_stack.ss_sp = stack;
+  c.uc_stack.ss_flags = 0;
+  c.uc_stack.ss_size = sizeof stack;
+  c.uc_link = NULL;
+  makecontext (&c, cfn, 0);
+
+  if (pthread_create (&tid, NULL, tfn, NULL) != 0)
+    abort ();
+
+  if (pthread_join (tid, NULL) != 0)
+    abort ();
+
+  /* The thread should have called exit.  */
+  abort ();
+}
+])],
+[libgo_cv_lib_setcontext_clobbers_tls=no],
+[libgo_cv_lib_setcontext_clobbers_tls=yes],
+[case "$target" in
+  x86_64*-*-solaris2.10) libgo_cv_lib_setcontext_clobbers_tls=yes ;;
+  *) libgo_cv_lib_setcontext_clobbers_tls=no ;;
+ esac
+])
+LIBS="$LIBS_hold"
+])
+if test "$libgo_cv_lib_setcontext_clobbers_tls" = "yes"; then
+  AC_DEFINE(SETCONTEXT_CLOBBERS_TLS, 1,
+	    [Define if setcontext clobbers TLS variables])
+fi
+
 AC_CACHE_SAVE
 
 if test ${multilib} = yes; then
diff -r d9bce1ef2e17 libgo/runtime/proc.c
--- a/libgo/runtime/proc.c	Tue Feb 07 11:19:23 2012 -0800
+++ b/libgo/runtime/proc.c	Tue Feb 07 21:19:06 2012 -0800
@@ -60,6 +60,54 @@ 
 static __thread G *g;
 static __thread M *m;
 
+#ifndef SETCONTEXT_CLOBBERS_TLS
+
+static inline void
+initcontext(void)
+{
+}
+
+static inline void
+fixcontext(ucontext_t *c __attribute__ ((unused)))
+{
+}
+
+# else
+
+# if defined(__x86_64__) && defined(__sun__)
+
+// x86_64 Solaris 10 and 11 have a bug: setcontext switches the %fs
+// register to that of the thread which called getcontext.  The effect
+// is that the address of all __thread variables changes.  This bug
+// also affects pthread_self() and pthread_getspecific.  We work
+// around it by clobbering the context field directly to keep %fs the
+// same.
+
+static __thread greg_t fs;
+
+static inline void
+initcontext(void)
+{
+	ucontext_t c;
+
+	getcontext(&c);
+	fs = c.uc_mcontext.gregs[REG_FSBASE];
+}
+
+static inline void
+fixcontext(ucontext_t* c)
+{
+	c->uc_mcontext.gregs[REG_FSBASE] = fs;
+}
+
+# else
+
+#  error unknown case for SETCONTEXT_CLOBBERS_TLS
+
+# endif
+
+#endif
+
 // We can not always refer to the TLS variables directly.  The
 // compiler will call tls_get_addr to get the address of the variable,
 // and it may hold it in a register across a call to schedule.  When
@@ -248,7 +296,9 @@ 
 #endif
 	g = newg;
 	newg->fromgogo = true;
+	fixcontext(&newg->context);
 	setcontext(&newg->context);
+	runtime_throw("gogo setcontext returned");
 }
 
 // Save context and call fn passing g as a parameter.  This is like
@@ -287,6 +337,7 @@ 
 		m->g0->entry = (byte*)pfn;
 		m->g0->param = g;
 		g = m->g0;
+		fixcontext(&m->g0->context);
 		setcontext(&m->g0->context);
 		runtime_throw("runtime: mcall function returned");
 	}
@@ -312,6 +363,8 @@ 
 	m->curg = g;
 	g->m = m;
 
+	initcontext();
+
 	m->nomemprof++;
 	runtime_mallocinit();
 	mcommoninit(m);
@@ -844,6 +897,8 @@ 
 	m = (M*)mp;
 	g = m->g0;
 
+	initcontext();
+
 	g->entry = nil;
 	g->param = nil;