diff mbox

libgo patch committed: Complete defer handling in CgocallBackDone

Message ID CAOyqgcWrPBxrKZdSvQCp98FRL+rsT3PKY_vjmcX6YU4U+yCc0A@mail.gmail.com
State New
Headers show

Commit Message

Ian Lance Taylor June 23, 2017, 8:19 p.m. UTC
This patch to libgo completes the handling of a cgo-generated defer in
CgocallBackDone.

When C code calls a Go function, it actually calls a function
generated by cgo. That function is written in Go, and, among other
things, it calls the real Go function like this:
    CgocallBack()
    defer CgocallBackDone()
    RealGoFunction()
The deferred CgocallBackDone function enters syscall mode as we return
to C. Typically the C function will then eventually return to Go.

However, in the case where the C function is running on a thread
created in C, it will not return to Go. For that case we will have
allocated an m struct, with an associated g struct, for the duration
of the Go code, and when the Go is complete we will return the m and g
to a free list.

That all works, but we are running in a deferred function, which means
that we have been invoked by deferreturn, and deferreturn expects to
do a bit of cleanup to record that the defer has been completed. Doing
that cleanup while using an m and g that have already been returned to
the free list is clearly a bad idea. It was kind of working because
deferreturn was holding the g pointer in a local variable, but there
were races with some other thread picking up and using the newly freed
g.  It was also kind of working because of a special check in
freedefer; that check is no longer necessary.

This patch changes the special case of releasing the m and g to do the
defer cleanup in CgocallBackDone itself.

This patch also checks for the special case of a panic through
CgocallBackDone. In that special case, we don't want to release the m
and g. Since we are returning to C code that was not called by Go
code, we know that the panic is not going to be caught and we are
going to exit the program. So for that special case we keep the m and
g structs so that the rest of the panic code can use them.

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

Ian
diff mbox

Patch

Index: gcc/go/gofrontend/MERGE
===================================================================
--- gcc/go/gofrontend/MERGE	(revision 249609)
+++ gcc/go/gofrontend/MERGE	(working copy)
@@ -1,4 +1,4 @@ 
-fc0cfdff94ca1099421900f43837ca5a70189cd6
+0a20181d00d43a423c55f4e772b759fba0619478
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: libgo/go/runtime/cgo_gccgo.go
===================================================================
--- libgo/go/runtime/cgo_gccgo.go	(revision 249205)
+++ libgo/go/runtime/cgo_gccgo.go	(working copy)
@@ -95,9 +95,34 @@  func CgocallBack() {
 // CgocallBackDone prepares to return to C/C++ code that has called
 // into Go code.
 func CgocallBackDone() {
+	// If we are the top level Go function called from C/C++, then
+	// we need to release the m. But don't release it if we are
+	// panicing; since this is the top level, we are going to
+	// crash the program, and we need the g and m to print the
+	// panic values.
+	//
+	// Dropping the m is going to clear g. This function is being
+	// called as a deferred function, so we will return to
+	// deferreturn which will want to clear the _defer field.
+	// As soon as we call dropm another thread may call needm and
+	// start using g, so we must not tamper with the _defer field
+	// after dropm. So clear _defer now.
+	gp := getg()
+	mp := gp.m
+	drop := false
+	if mp.dropextram && mp.ncgo == 0 && gp._panic == nil {
+		d := gp._defer
+		if d == nil || d.link != nil {
+			throw("unexpected g._defer in CgocallBackDone")
+		}
+		gp._defer = nil
+		freedefer(d)
+		drop = true
+	}
+
 	entersyscall(0)
-	mp := getg().m
-	if mp.dropextram && mp.ncgo == 0 {
+
+	if drop {
 		mp.dropextram = false
 		dropm()
 	}
Index: libgo/go/runtime/panic.go
===================================================================
--- libgo/go/runtime/panic.go	(revision 249590)
+++ libgo/go/runtime/panic.go	(working copy)
@@ -143,14 +143,6 @@  func newdefer() *_defer {
 //
 //go:nosplit
 func freedefer(d *_defer) {
-	// When C code calls a Go function on a non-Go thread, the
-	// deferred call to cgocallBackDone will set g to nil.
-	// Don't crash trying to put d on the free list; just let it
-	// be garbage collected.
-	if getg() == nil {
-		return
-	}
-
 	pp := getg().m.p.ptr()
 	if len(pp.deferpool) == cap(pp.deferpool) {
 		// Transfer half of local cache to the central cache.
@@ -201,6 +193,15 @@  func deferreturn(frame *bool) {
 			fn(d.arg)
 		}
 
+		// If we are returning from a Go function called by a
+		// C function running in a C thread, g may now be nil,
+		// in which case CgocallBackDone will have cleared _defer.
+		// In that case some other goroutine may already be using gp.
+		if getg() == nil {
+			*frame = true
+			return
+		}
+
 		gp._defer = d.link
 
 		freedefer(d)