Go patch committed: Fix defer

[Ian Lance Taylor <iant at google dot com>]

The Go frontend had a bug when a function with named result parameter
called defer on a function which did not call recover.  The caller would
execute a return statement to return the updated values of the named
result parameters.  This was wrong when the deferred function did not
call recover and when some callee called panic, because in that case the
panic should continue propagating up the stack, but executing the return
caused it to return instead.

This patch fixes the problem by only executing the return in the case
where it is appropriate, and otherwise falling out of the
TRY_FINALLY_EXPR in order to continue throwing the exception.  It would
be a bit nicer if we could ask the try/finally handler whether we are
going to return since, after all, it does know.  However, there is
currently no way to do that.

Bootstrapped and ran Go testsuite on x86_64-unknown-linux-gnu.
Committed to mainline.

Ian

diff -r a10bd634db30 go/gogo-tree.cc
--- a/go/gogo-tree.cc	Wed Sep 14 15:25:34 2011 -0700
+++ b/go/gogo-tree.cc	Thu Sep 15 22:36:43 2011 -0700
@@ -1592,15 +1592,25 @@
       && !this->type_->results()->empty()
       && !this->type_->results()->front().name().empty())
     {
-      // If the result variables are named, we need to return them
-      // again, because they might have been changed by a defer
-      // function.
+      // If the result variables are named, and we are returning from
+      // this function rather than panicing through it, we need to
+      // return them again, because they might have been changed by a
+      // defer function.  The runtime routines set the defer_stack
+      // variable to true if we are returning from this function.
       retval = this->return_value(gogo, named_function, end_loc,
 				  &stmt_list);
       set = fold_build2_loc(end_loc, MODIFY_EXPR, void_type_node,
 			    DECL_RESULT(this->fndecl_), retval);
       ret_stmt = fold_build1_loc(end_loc, RETURN_EXPR, void_type_node, set);
-      append_to_statement_list(ret_stmt, &stmt_list);
+
+      Expression* ref =
+	Expression::make_temporary_reference(this->defer_stack_, end_loc);
+      tree tref = ref->get_tree(&context);
+      tree s = build3_loc(end_loc, COND_EXPR, void_type_node, tref,
+			  ret_stmt, NULL_TREE);
+
+      append_to_statement_list(s, &stmt_list);
+
     }
   
   go_assert(*fini == NULL_TREE);
diff -r a10bd634db30 go/gogo.cc
--- a/go/gogo.cc	Wed Sep 14 15:25:34 2011 -0700
+++ b/go/gogo.cc	Thu Sep 15 22:36:43 2011 -0700
@@ -2976,27 +2976,27 @@
     this->block_->determine_types();
 }
 
-// Get a pointer to the variable holding the defer stack for this
-// function, making it if necessary.  At least at present, the value
-// of this variable is not used.  However, a pointer to this variable
-// is used as a marker for the functions on the defer stack associated
-// with this function.  Doing things this way permits inlining a
+// Get a pointer to the variable representing the defer stack for this
+// function, making it if necessary.  The value of the variable is set
+// by the runtime routines to true if the function is returning,
+// rather than panicing through.  A pointer to this variable is used
+// as a marker for the functions on the defer stack associated with
+// this function.  A function-specific variable permits inlining a
 // function which uses defer.
 
 Expression*
 Function::defer_stack(source_location location)
 {
-  Type* t = Type::make_pointer_type(Type::make_void_type());
   if (this->defer_stack_ == NULL)
     {
-      Expression* n = Expression::make_nil(location);
+      Type* t = Type::lookup_bool_type();
+      Expression* n = Expression::make_boolean(false, location);
       this->defer_stack_ = Statement::make_temporary(t, n, location);
       this->defer_stack_->set_is_address_taken();
     }
   Expression* ref = Expression::make_temporary_reference(this->defer_stack_,
 							 location);
-  Expression* addr = Expression::make_unary(OPERATOR_AND, ref, location);
-  return Expression::make_unsafe_cast(t, addr, location);
+  return Expression::make_unary(OPERATOR_AND, ref, location);
 }
 
 // Export the function.
diff -r a10bd634db30 go/runtime.def
--- a/go/runtime.def	Wed Sep 14 15:25:34 2011 -0700
+++ b/go/runtime.def	Thu Sep 15 22:36:43 2011 -0700
@@ -165,10 +165,10 @@
 	       R1(BOOL))
 
 // Check for a deferred function in an exception handler.
-DEF_GO_RUNTIME(CHECK_DEFER, "__go_check_defer", P1(POINTER), R0())
+DEF_GO_RUNTIME(CHECK_DEFER, "__go_check_defer", P1(BOOLPTR), R0())
 
 // Run deferred functions.
-DEF_GO_RUNTIME(UNDEFER, "__go_undefer", P1(POINTER), R0())
+DEF_GO_RUNTIME(UNDEFER, "__go_undefer", P1(BOOLPTR), R0())
 
 // Panic with a runtime error.
 DEF_GO_RUNTIME(RUNTIME_ERROR, "__go_runtime_error", P1(INT), R0())
@@ -207,7 +207,7 @@
 
 
 // Defer a function.
-DEF_GO_RUNTIME(DEFER, "__go_defer", P3(POINTER, FUNC_PTR, POINTER), R0())
+DEF_GO_RUNTIME(DEFER, "__go_defer", P3(BOOLPTR, FUNC_PTR, POINTER), R0())
 
 
 // Run a select statement.
diff -r a10bd634db30 go/statements.cc
--- a/go/statements.cc	Wed Sep 14 15:25:34 2011 -0700
+++ b/go/statements.cc	Thu Sep 15 22:36:43 2011 -0700
@@ -2539,11 +2539,10 @@
 
 // Lower a return statement.  If we are returning a function call
 // which returns multiple values which match the current function,
-// split up the call's results.  If the function has named result
-// variables, and the return statement lists explicit values, then
-// implement it by assigning the values to the result variables and
-// changing the statement to not list any values.  This lets
-// panic/recover work correctly.
+// split up the call's results.  If the return statement lists
+// explicit values, implement this statement by assigning the values
+// to the result variables and change this statement to a naked
+// return.  This lets panic/recover work correctly.
 
 Statement*
 Return_statement::do_lower(Gogo*, Named_object* function, Block* enclosing,
diff -r a10bd634db30 libgo/runtime/go-defer.c
--- a/libgo/runtime/go-defer.c	Wed Sep 14 15:25:34 2011 -0700
+++ b/libgo/runtime/go-defer.c	Thu Sep 15 22:36:43 2011 -0700
@@ -13,7 +13,7 @@
 /* This function is called each time we need to defer a call.  */
 
 void
-__go_defer (void *frame, void (*pfn) (void *), void *arg)
+__go_defer (_Bool *frame, void (*pfn) (void *), void *arg)
 {
   struct __go_defer_stack *n;
 
@@ -34,7 +34,7 @@
 /* This function is called when we want to undefer the stack.  */
 
 void
-__go_undefer (void *frame)
+__go_undefer (_Bool *frame)
 {
   if (__go_panic_defer == NULL)
     return;
@@ -53,6 +53,12 @@
 
       __go_panic_defer->__defer = d->__next;
       __go_free (d);
+
+      /* Since we are executing a defer function here, we know we are
+	 returning from the calling function.  If the calling
+	 function, or one of its callees, paniced, then the defer
+	 functions would be executed by __go_panic.  */
+      *frame = 1;
     }
 }
 
diff -r a10bd634db30 libgo/runtime/go-defer.h
--- a/libgo/runtime/go-defer.h	Wed Sep 14 15:25:34 2011 -0700
+++ b/libgo/runtime/go-defer.h	Thu Sep 15 22:36:43 2011 -0700
@@ -13,9 +13,10 @@
   /* The next entry in the stack.  */
   struct __go_defer_stack *__next;
 
-  /* The frame pointer for the function which called this defer
-     statement.  */
-  void *__frame;
+  /* The stack variable for the function which called this defer
+     statement.  This is set to 1 if we are returning from that
+     function, 0 if we are panicing through it.  */
+  _Bool *__frame;
 
   /* The value of the panic stack when this function is deferred.
      This function can not recover this value from the panic stack.
diff -r a10bd634db30 libgo/runtime/go-panic.c
--- a/libgo/runtime/go-panic.c	Wed Sep 14 15:25:34 2011 -0700
+++ b/libgo/runtime/go-panic.c	Thu Sep 15 22:36:43 2011 -0700
@@ -87,6 +87,12 @@
 	      /* __go_unwind_stack should not return.  */
 	      abort ();
 	    }
+
+	  /* Because we executed that defer function by a panic, and
+	     it did not call recover, we know that we are not
+	     returning from the calling function--we are panicing
+	     through it.  */
+	  *d->__frame = 0;
 	}
 
       __go_panic_defer->__defer = d->__next;
diff -r a10bd634db30 libgo/runtime/go-unwind.c
--- a/libgo/runtime/go-unwind.c	Wed Sep 14 15:25:34 2011 -0700
+++ b/libgo/runtime/go-unwind.c	Thu Sep 15 22:36:43 2011 -0700
@@ -44,7 +44,7 @@
    continue unwinding.  */
 
 void
-__go_check_defer (void *frame)
+__go_check_defer (_Bool *frame)
 {
   struct _Unwind_Exception *hdr;
 
@@ -103,8 +103,12 @@
       if (was_recovered)
 	{
 	  /* Just return and continue executing Go code.  */
+	  *frame = 1;
 	  return;
 	}
+
+      /* We are panicing through this function.  */
+      *frame = 0;
     }
   else if (__go_panic_defer->__defer != NULL
 	   && __go_panic_defer->__defer->__pfn == NULL
@@ -118,6 +122,7 @@
       d = __go_panic_defer->__defer;
       __go_panic_defer->__defer = d->__next;
       __go_free (d);
+      *frame = 1;
       return;
     }