diff mbox

libgo patch committed: Fix recover.go test for large args, FFI

Message ID mcrfveypskc.fsf@iant-glaptop.roam.corp.google.com
State New
Headers show

Commit Message

Ian Lance Taylor Oct. 8, 2014, 2:03 p.m. UTC
This patch to libgo fixes PR 60406, in which a problem arises when
passing a large argument to a deferred function.  The current code
generates a thunk that looks like this:

    if (__go_set_defer_retaddr (&&L))
      goto L;
    deferred_function(args);
  L:

Then the deferred_function calls

    __go_can_recover (__builtin_return_address())

and the code in libgo compares the address passed to __go_can_recover to
the address passed to __go_set_defer_retaddr.  This rather baroque
approach is so that a defer of a variable of function type will work
correctly.

This code normally works fine.  However, if args is very large, then it
will cause GCC to generate a block copy.  And on systems where that
block copy introduces a loop into the code, the exact location of L may
be split to a different block so that it no longer immediately follows
the call to deferred_function.  In that case the comparison in
__go_can_recover will fail, and the call to recover will fail when it
should succeed.

This is an unusual case.  Most calls to defer pass no arguments to the
deferred function.  It's quite unusual to pass a large argument.
However, we should of course handle it correctly.

This patch does this by adding new code when the return address fails to
match.  We fetch the callers to see whether we are being directly
invoked as a deferred function.  We detect this case by the simple
approach of seeing whether the caller starts with "__go_".  The patch
also adjusts one case that calls a deferred function to start with __go_
to match this.  This is a heuristic that is safe at present and be made
more safe in the future.

This patch also tightens up the checks for deferring a function created
by reflect.MakeFunc, to make that case more reliable.

This patch also introduces uses of __builtin_extract_return_addr for
systems that need it, which permits us to remove a #ifdef __sparc__.

Bootstrapped and ran Go testsuite on x86_64-unknown-linux-gnu and
powerpc64le-unnown-linux-gnu.  Tested by others on Alpha and S/390.

Thanks to Dominik Vogt for forcing the issue and examining the
underlying assumptions.

Committed to mainline.  Although this fixes a bug this seems to me to be
too risky for the 4.9 branch, especially given that the bug has only
ever been seen in the testsuite, and is extremely unlikely to arise in
ordinary user code.

Ian
diff mbox

Patch

diff -r 50bdfe92960f libgo/go/reflect/makefunc_ffi_c.c
--- a/libgo/go/reflect/makefunc_ffi_c.c	Fri Oct 03 11:27:25 2014 -0700
+++ b/libgo/go/reflect/makefunc_ffi_c.c	Wed Oct 08 06:45:51 2014 -0700
@@ -36,21 +36,23 @@ 
    Go callback function (passed in user_data) with the pointer to the
    arguments and the results area.  */
 
+static void ffi_callback (ffi_cif *, void *, void **, void *)
+  __asm__ ("reflect.ffi_callback");
+
 static void
 ffi_callback (ffi_cif* cif __attribute__ ((unused)), void *results,
 	      void **args, void *user_data)
 {
-  Location locs[6];
+  Location locs[8];
   int n;
   int i;
-  const void *pc;
   FuncVal *fv;
   void (*f) (void *, void *);
 
   /* This function is called from some series of FFI closure functions
-     called by a Go function.  We want to pass the PC of the Go
-     function to makefunc_can_recover.  Look up the stack for a
-     function that is definitely not an FFI function.  */
+     called by a Go function.  We want to see whether the caller of
+     the closure functions can recover.  Look up the stack and skip
+     the FFI functions.  */
   n = runtime_callers (1, &locs[0], sizeof locs / sizeof locs[0], true);
   for (i = 0; i < n; i++)
     {
@@ -61,28 +63,19 @@ 
       if (locs[i].function.len < 4)
 	break;
       name = locs[i].function.str;
-      if (*name == '_')
-	{
-	  if (locs[i].function.len < 5)
-	    break;
-	  ++name;
-	}
       if (name[0] != 'f' || name[1] != 'f' || name[2] != 'i' || name[3] != '_')
 	break;
     }
   if (i < n)
-    pc = (const void *) locs[i].pc;
-  else
-    pc = __builtin_return_address (0);
-
-  __go_makefunc_can_recover (pc);
+    __go_makefunc_ffi_can_recover (locs + i, n - i);
 
   fv = (FuncVal *) user_data;
   __go_set_closure (fv);
   f = (void *) fv->fn;
   f (args, results);
 
-  __go_makefunc_returning ();
+  if (i < n)
+    __go_makefunc_returning ();
 }
 
 /* Allocate an FFI closure and arrange to call ffi_callback.  */
diff -r 50bdfe92960f libgo/runtime/go-defer.c
--- a/libgo/runtime/go-defer.c	Fri Oct 03 11:27:25 2014 -0700
+++ b/libgo/runtime/go-defer.c	Wed Oct 08 06:45:51 2014 -0700
@@ -80,6 +80,6 @@ 
 
   g = runtime_g ();
   if (g->defer != NULL)
-    g->defer->__retaddr = retaddr;
+    g->defer->__retaddr = __builtin_extract_return_addr (retaddr);
   return 0;
 }
diff -r 50bdfe92960f libgo/runtime/go-panic.h
--- a/libgo/runtime/go-panic.h	Fri Oct 03 11:27:25 2014 -0700
+++ b/libgo/runtime/go-panic.h	Wed Oct 08 06:45:51 2014 -0700
@@ -38,9 +38,12 @@ 
 
 extern struct __go_empty_interface __go_recover (void);
 
-extern _Bool __go_can_recover (const void *);
+extern _Bool __go_can_recover (void *);
 
-extern void __go_makefunc_can_recover (const void *retaddr);
+extern void __go_makefunc_can_recover (void *retaddr);
+
+struct Location;
+extern void __go_makefunc_ffi_can_recover (struct Location *, int);
 
 extern void __go_makefunc_returning (void);
 
diff -r 50bdfe92960f libgo/runtime/go-recover.c
--- a/libgo/runtime/go-recover.c	Fri Oct 03 11:27:25 2014 -0700
+++ b/libgo/runtime/go-recover.c	Wed Oct 08 06:45:51 2014 -0700
@@ -9,6 +9,36 @@ 
 #include "go-panic.h"
 #include "go-defer.h"
 
+/* If the top of the defer stack can be recovered, then return it.
+   Otherwise return NULL.  */
+
+static struct __go_defer_stack *
+current_defer ()
+{
+  G *g;
+  struct __go_defer_stack *d;
+
+  g = runtime_g ();
+
+  d = g->defer;
+  if (d == NULL)
+    return NULL;
+
+  /* The panic which would be recovered is the one on the top of the
+     panic stack.  We do not want to recover it if that panic was on
+     the top of the panic stack when this function was deferred.  */
+  if (d->__panic == g->panic)
+    return NULL;
+
+  /* The deferred thunk will call _go_set_defer_retaddr.  If this has
+     not happened, then we have not been called via defer, and we can
+     not recover.  */
+  if (d->__retaddr == NULL)
+    return NULL;
+
+  return d;
+}
+
 /* This is called by a thunk to see if the real function should be
    permitted to recover a panic value.  Recovering a value is
    permitted if the thunk was called directly by defer.  RETADDR is
@@ -16,79 +46,126 @@ 
    __go_can_recover--this is, the thunk.  */
 
 _Bool
-__go_can_recover (const void *retaddr)
+__go_can_recover (void *retaddr)
 {
-  G *g;
   struct __go_defer_stack *d;
   const char* ret;
   const char* dret;
-  Location loc;
+  Location locs[16];
   const byte *name;
+  intgo len;
+  int n;
+  int i;
+  _Bool found_ffi_callback;
 
-  g = runtime_g ();
-
-  d = g->defer;
+  d = current_defer ();
   if (d == NULL)
     return 0;
 
-  /* The panic which this function would recover is the one on the top
-     of the panic stack.  We do not want to recover it if that panic
-     was on the top of the panic stack when this function was
-     deferred.  */
-  if (d->__panic == g->panic)
-    return 0;
-
-  /* D->__RETADDR is the address of a label immediately following the
-     call to the thunk.  We can recover a panic if that is the same as
-     the return address of the thunk.  We permit a bit of slack in
-     case there is any code between the function return and the label,
-     such as an instruction to adjust the stack pointer.  */
-
-  ret = (const char *) retaddr;
-
-#ifdef __sparc__
-  /* On SPARC the address we get, from __builtin_return_address, is
-     the address of the call instruction.  Adjust forward, also
-     skipping the delayed instruction following the call.  */
-  ret += 8;
-#endif
+  ret = (const char *) __builtin_extract_return_addr (retaddr);
 
   dret = (const char *) d->__retaddr;
   if (ret <= dret && ret + 16 >= dret)
     return 1;
 
+  /* On some systems, in some cases, the return address does not work
+     reliably.  See http://gcc.gnu.org/PR60406.  If we are permitted
+     to call recover, the call stack will look like this:
+       __go_panic, __go_undefer, etc.
+       thunk to call deferred function (calls __go_set_defer_retaddr)
+       function that calls __go_can_recover (passing return address)
+       __go_can_recover
+     Calling runtime_callers will skip the thunks.  So if our caller's
+     caller starts with __go, then we are permitted to call
+     recover.  */
+
+  if (runtime_callers (1, &locs[0], 2, false) < 2)
+    return 0;
+
+  name = locs[1].function.str;
+  len = locs[1].function.len;
+
+  /* Although locs[1].function is a Go string, we know it is
+     NUL-terminated.  */
+  if (len > 4
+      && __builtin_strchr ((const char *) name, '.') == NULL
+      && __builtin_strncmp ((const char *) name, "__go_", 4) == 0)
+    return 1;
+
+  /* If we are called from __go_makefunc_can_recover, then we need to
+     look one level higher.  */
+  if (locs[0].function.len > 0
+      && __builtin_strcmp ((const char *) locs[0].function.str,
+			   "__go_makefunc_can_recover") == 0)
+    {
+      if (runtime_callers (3, &locs[0], 1, false) < 1)
+	return 0;
+      name = locs[0].function.str;
+      len = locs[0].function.len;
+      if (len > 4
+	  && __builtin_strchr ((const char *) name, '.') == NULL
+	  && __builtin_strncmp ((const char *) name, "__go_", 4) == 0)
+	return 1;
+    }
+
   /* If the function calling recover was created by reflect.MakeFunc,
-     then RETADDR will be somewhere in libffi.  Our caller is
-     permitted to recover if it was called from libffi.  */
+     then __go_makefunc_can_recover or __go_makefunc_ffi_can_recover
+     will have set the __makefunc_can_recover field.  */
   if (!d->__makefunc_can_recover)
     return 0;
 
-  if (runtime_callers (2, &loc, 1, false) < 1)
-    return 0;
+  /* We look up the stack, ignoring libffi functions and functions in
+     the reflect package, until we find reflect.makeFuncStub or
+     reflect.ffi_callback called by FFI functions.  Then we check the
+     caller of that function.  */
 
-  /* If we have no function name, then we weren't called by Go code.
-     Guess that we were called by libffi.  */
-  if (loc.function.len == 0)
-    return 1;
+  n = runtime_callers (2, &locs[0], sizeof locs / sizeof locs[0], false);
+  found_ffi_callback = 0;
+  for (i = 0; i < n; i++)
+    {
+      const byte *name;
 
-  if (loc.function.len < 4)
-    return 0;
-  name = loc.function.str;
-  if (*name == '_')
-    {
-      if (loc.function.len < 5)
-	return 0;
-      ++name;
+      if (locs[i].function.len == 0)
+	{
+	  /* No function name means this caller isn't Go code.  Assume
+	     that this is libffi.  */
+	  continue;
+	}
+
+      /* Ignore functions in libffi.  */
+      name = locs[i].function.str;
+      if (__builtin_strncmp ((const char *) name, "ffi_", 4) == 0)
+	continue;
+
+      if (found_ffi_callback)
+	break;
+
+      if (__builtin_strcmp ((const char *) name, "reflect.ffi_callback") == 0)
+	{
+	  found_ffi_callback = 1;
+	  continue;
+	}
+
+      if (__builtin_strcmp ((const char *) name, "reflect.makeFuncStub") == 0)
+	{
+	  i++;
+	  break;
+	}
+
+      /* Ignore other functions in the reflect package.  */
+      if (__builtin_strncmp ((const char *) name, "reflect.", 8) == 0)
+	continue;
+
+      /* We should now be looking at the real caller.  */
+      break;
     }
 
-  if (name[0] == 'f' && name[1] == 'f' && name[2] == 'i' && name[3] == '_')
-    return 1;
-
-  /* We may also be called by reflect.makeFuncImpl.call or
-     reflect.ffiCall, for a function created by reflect.MakeFunc.  */
-  if (__builtin_strstr ((const char *) name, "makeFuncImpl") != NULL
-      || __builtin_strcmp ((const char *) name, "reflect.ffiCall") == 0)
-    return 1;
+  if (i < n && locs[i].function.len > 0)
+    {
+      name = locs[i].function.str;
+      if (__builtin_strncmp ((const char *) name, "__go_", 4) == 0)
+	return 1;
+    }
 
   return 0;
 }
@@ -99,14 +176,58 @@ 
    real MakeFunc function is permitted to call recover.  */
 
 void
-__go_makefunc_can_recover (const void *retaddr)
+__go_makefunc_can_recover (void *retaddr)
 {
   struct __go_defer_stack *d;
 
-  d = runtime_g ()->defer;
-  if (d != NULL
-      && !d->__makefunc_can_recover
-      && __go_can_recover (retaddr))
+  d = current_defer ();
+  if (d == NULL)
+    return;
+
+  /* If we are already in a call stack of MakeFunc functions, there is
+     nothing we can usefully check here.  */
+  if (d->__makefunc_can_recover)
+    return;
+
+  if (__go_can_recover (retaddr))
+    d->__makefunc_can_recover = 1;
+}
+
+/* This function is called when code is about to enter a function
+   created by the libffi version of reflect.MakeFunc.  This function
+   is passed the names of the callers of the libffi code that called
+   the stub.  It uses to decide whether it is permitted to call
+   recover, and sets d->__makefunc_can_recover so that __go_recover
+   can make the same decision.  */
+
+void
+__go_makefunc_ffi_can_recover (struct Location *loc, int n)
+{
+  struct __go_defer_stack *d;
+  const byte *name;
+  intgo len;
+
+  d = current_defer ();
+  if (d == NULL)
+    return;
+
+  /* If we are already in a call stack of MakeFunc functions, there is
+     nothing we can usefully check here.  */
+  if (d->__makefunc_can_recover)
+    return;
+
+  /* LOC points to the caller of our caller.  That will be a thunk.
+     If its caller was a runtime function, then it was called directly
+     by defer.  */
+
+  if (n < 2)
+    return;
+
+  name = (loc + 1)->function.str;
+  len = (loc + 1)->function.len;
+  if (len > 4
+      && __builtin_strchr ((const char *) name, '.') == NULL
+      && __builtin_strncmp ((const char *) name, "__go_", 4) == 0)
     d->__makefunc_can_recover = 1;
 }
 
diff -r 50bdfe92960f libgo/runtime/panic.c
--- a/libgo/runtime/panic.c	Fri Oct 03 11:27:25 2014 -0700
+++ b/libgo/runtime/panic.c	Wed Oct 08 06:45:51 2014 -0700
@@ -49,8 +49,10 @@ 
 }
 
 // Run all deferred functions for the current goroutine.
+// This is noinline for go_can_recover.
+static void __go_rundefer (void) __attribute__ ((noinline));
 static void
-rundefer(void)
+__go_rundefer(void)
 {
 	G *g;
 	Defer *d;
@@ -219,7 +221,7 @@ 
 void
 runtime_Goexit(void)
 {
-	rundefer();
+	__go_rundefer();
 	runtime_goexit();
 }