diff mbox

Go patch committed: Fix unexpected GC interfering with closure passing

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

Commit Message

Ian Lance Taylor Aug. 13, 2014, 10:32 p.m. UTC 
The Go frontend passes closures through to functions using the functions
__go_set_closure and __go_get_closure.  The expectation is that there
are no function calls between set_closure and get_closure.  However, it
turns out that there can be function calls if some of the function
arguments require type conversion to an interface type.  Converting to
an interface type can allocate memory, and that can in turn trigger a
garbage collection, and that can in turn call pool cleanup functions
that may call __go_set_closure.  So the called function can see the
wrong closure value, which is bad.

This patch fixes the problem in two different ways.  First, we move all
type conversions in function arguments into temporary variables so that
they can not appear before the call to __go_set_closure.  (This required
shifting the flatten phase after the simplify_thunk phase, since the
latter expects to work with unconverted argument types.)  Second, we fix
the memory allocation function to preserve the closure value across any
possible garbage collection.

A test case is the libgo database/sql check run with the environment
variable GOGC set to 1.

Bootstrapped and ran Go testsuite on x86_64-unknown-linux-gnu.
Committed to mainline.  I committed just the patch to the memory
allocation function to the 4.9 branch--the 4.9 branch doesn't have the
framework for the frontend patch.

Ian
diff mbox

Patch

diff -r f66917501eac go/expressions.cc
--- a/go/expressions.cc	Mon Aug 11 12:25:44 2014 -0700
+++ b/go/expressions.cc	Wed Aug 13 14:50:37 2014 -0700
@@ -9013,8 +9013,51 @@ 
 // Flatten a call with multiple results into a temporary.
 
 Expression*
-Call_expression::do_flatten(Gogo*, Named_object*, Statement_inserter* inserter)
-{
+Call_expression::do_flatten(Gogo* gogo, Named_object*,
+			    Statement_inserter* inserter)
+{
+  if (this->classification() == EXPRESSION_ERROR)
+    return this;
+
+  // Add temporary variables for all arguments that require type
+  // conversion.
+  Function_type* fntype = this->get_function_type();
+  go_assert(fntype != NULL);
+  if (this->args_ != NULL && !this->args_->empty()
+      && fntype->parameters() != NULL && !fntype->parameters()->empty())
+    {
+      bool is_interface_method =
+	this->fn_->interface_field_reference_expression() != NULL;
+
+      Expression_list *args = new Expression_list();
+      Typed_identifier_list::const_iterator pp = fntype->parameters()->begin();
+      Expression_list::const_iterator pa = this->args_->begin();
+      if (!is_interface_method && fntype->is_method())
+	{
+	  // The receiver argument.
+	  args->push_back(*pa);
+	  ++pa;
+	}
+      for (; pa != this->args_->end(); ++pa, ++pp)
+	{
+	  go_assert(pp != fntype->parameters()->end());
+	  if (Type::are_identical(pp->type(), (*pa)->type(), true, NULL))
+	    args->push_back(*pa);
+	  else
+	    {
+	      Location loc = (*pa)->location();
+	      Expression* arg =
+		Expression::convert_for_assignment(gogo, pp->type(), *pa, loc);
+	      Temporary_statement* temp =
+		Statement::make_temporary(pp->type(), arg, loc);
+	      inserter->insert(temp);
+	      args->push_back(Expression::make_temporary_reference(temp, loc));
+	    }
+	}
+      delete this->args_;
+      this->args_ = args;
+    }
+
   size_t rc = this->result_count();
   if (rc > 1 && this->call_temp_ == NULL)
     {
diff -r f66917501eac go/go.cc
--- a/go/go.cc	Mon Aug 11 12:25:44 2014 -0700
+++ b/go/go.cc	Wed Aug 13 14:50:37 2014 -0700
@@ -124,15 +124,15 @@ 
   // Convert named types to backend representation.
   ::gogo->convert_named_types();
 
-  // Flatten the parse tree.
-  ::gogo->flatten();
-
   // Build thunks for functions which call recover.
   ::gogo->build_recover_thunks();
 
   // Convert complicated go and defer statements into simpler ones.
   ::gogo->simplify_thunk_statements();
 
+  // Flatten the parse tree.
+  ::gogo->flatten();
+
   // Dump ast, use filename[0] as the base name
   ::gogo->dump_ast(filenames[0]);
 }
diff -r f66917501eac libgo/runtime/malloc.goc
--- a/libgo/runtime/malloc.goc	Mon Aug 11 12:25:44 2014 -0700
+++ b/libgo/runtime/malloc.goc	Wed Aug 13 14:50:37 2014 -0700
@@ -84,6 +84,7 @@ 
 	MLink *v, *next;
 	byte *tiny;
 	bool incallback;
+	void *closure;
 
 	if(size == 0) {
 		// All 0-length allocations use this pointer.
@@ -95,6 +96,10 @@ 
 	m = runtime_m();
 	g = runtime_g();
 
+	// We should not be called in between __go_set_closure and the
+	// actual function call, but cope with it if we are.
+	closure = g->closure;
+
 	incallback = false;
 	if(m->mcache == nil && g->ncgo > 0) {
 		// For gccgo this case can occur when a cgo or SWIG function
@@ -175,6 +180,7 @@ 
 					m->locks--;
 					if(incallback)
 						runtime_entersyscall();
+					g->closure = closure;
 					return v;
 				}
 			}
@@ -264,6 +270,8 @@ 
 	if(incallback)
 		runtime_entersyscall();
 
+	g->closure = closure;
+
 	return v;
 }