fix: proper gil handling, increase testing for symbols & complex numbers

_examples/callbacks/test_callback_rval.py

@@ -0,0 +1,33 @@
+# Copyright 2015-present, The go-python Authors.  All rights reserved.
+# Use of this source code is governed by a BSD-style
+# license that can be found in the LICENSE file.
+
+# Regression test for the GIL handling of a Python callback's return value.
+#
+# When Go calls a Python callback, the generated wrapper takes the GIL, calls
+# the callback, and converts the returned object to a Go value. That
+# conversion, and the decref of the returned object, must both happen before
+# the wrapper releases the GIL again.
+
+import sys
+
+import callbacks
+
+# A callback that keeps returning the same float must not leak a reference to
+# it on each call.
+X = float("0.25")
+before = sys.getrefcount(X)
+for _ in range(100):
+    callbacks.Apply(1.5, lambda x: X)
+delta = sys.getrefcount(X) - before
+if delta != 0:
+    raise AssertionError("refcount of the returned float grew by %d over 100 calls" % delta)
+
+# A float64 callback that returns an int makes the conversion
+# (PyFloat_AsDouble) run int.__float__, which allocates a Python float. That
+# segfaults if the GIL has already been released.
+got = callbacks.Apply(1.5, lambda x: 3)
+if got != 3.0:
+    raise AssertionError("Apply(1.5, lambda x: 3) = %r, want 3.0" % got)
+
+print("OK")

_examples/simple/test_complex_args.py

@@ -0,0 +1,29 @@
+# Copyright 2015-present, The go-python Authors.  All rights reserved.
+# Use of this source code is governed by a BSD-style
+# license that can be found in the LICENSE file.
+
+# Regression test for the complex64/complex128 GIL-handling fix.
+#
+# A complex argument is converted with PyComplex_AsCComplex. For an exact
+# `complex` that is a plain read of two doubles and works even without the
+# GIL, so passing complex values cannot catch the bug. For a float or an int
+# it runs Python code (the __complex__ lookup, then __float__/__index__),
+# which segfaults if the GIL has already been released for the wrapped Go
+# call. Each call below crashed before the fix and returns the right value
+# after it.
+
+import simple as pkg
+
+cases = [
+    ("float", 1.5, 2.5, 4 + 0j),
+    ("int", 1, 2, 3 + 0j),
+]
+
+for name, a, b, want in cases:
+    for fn in (pkg.Comp64Add, pkg.Comp128Add):
+        got = fn(a, b)
+        # every expected value is exactly representable as a complex64
+        if got != want:
+            raise AssertionError("%s(%s args) = %s, want %s" % (fn.__name__, name, got, want))
+
+print("OK")

bind/gen_func.go

@@ -274,6 +274,47 @@ func isIfaceHandle(gdoc string) (bool, string) {
 	return false, gdoc
 }
 
+// needsGILForArgMarshal reports whether converting a Python-side argument of
+// this symbol's type touches a raw *C.PyObject on the Go side (e.g.
+// complex64/complex128 via PyComplex_AsCComplex). Such conversions must run
+// while the GIL is still held, not inline in the wrapped call after
+// C.PyEval_SaveThread has released it -- touching a PyObject without the GIL
+// is undefined behavior. The py2go signature closure (isSignature) is
+// excluded: it is invoked later, from inside the wrapped Go call, and already
+// reacquires the GIL itself via PyGILState_Ensure/Release around its body.
+func needsGILForArgMarshal(sym *symbol) bool {
+	return sym.cgoname == "*C.PyObject" && !sym.isSignature() && sym.py2go != ""
+}
+
+// argCallExpr returns the Go-side expression used to pass a Python-wrapped
+// argument to the wrapped Go call, and whether that expression must be
+// evaluated before the GIL is released (see needsGILForArgMarshal). anm is
+// the argument's Python-side (pySafeArg) name; isVariadicTail marks the
+// trailing "arg" that stands in for a whole variadic slice rather than a
+// single marshalled scalar.
+//
+// This is the single place that decides both questions, in one precedence
+// order: ifchandle/interface{} and isSignature never touch a raw PyObject
+// via this expression (a callback's wrapper re-acquires the GIL itself, see
+// shimABI.callbackArg), so they always report needsGIL=false, regardless of
+// the symbol's cgoname. genFuncBody calls it exactly once per argument and
+// uses the returned expression (or the premarshalled variable holding it)
+// at the call site, so a "_premarshalN" can never be declared under one
+// precedence and then left unreferenced under another.
+func (g *pyGen) argCallExpr(ifchandle, isVariadicTail bool, anm string, sym *symbol) (expr string, needsGIL bool) {
+	switch {
+	case ifchandle && sym.goname == "interface{}":
+		return fmt.Sprintf(`gopyh.VarFromHandle((gopyh.CGoHandle)(%s), "interface{}")`, anm), false
+	case sym.isSignature():
+		return g.shim.callbackArg(g, anm, sym), false
+	case sym.py2go != "":
+		expr := fmt.Sprintf("%s(%s)%s", sym.py2go, anm, sym.py2goParenEx)
+		return expr, needsGILForArgMarshal(sym) && !isVariadicTail
+	default:
+		return anm, false
+	}
+}
+
 func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) {
 	isMethod := (sym != nil)
 	isIface := false
@@ -319,6 +360,25 @@ func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) {
 		}
 	}
 
+	// Decide each argument's call-site expression once, up front, and convert
+	// any argument whose marshalling touches a raw *C.PyObject now, while the
+	// GIL is still held -- before saveThread releases it below for the
+	// duration of the wrapped Go call. The call-argument loop further down
+	// consumes argExprs as-is, so a premarshalled variable is declared here if
+	// and only if it is also the expression used at the call site.
+	argExprs := make([]string, len(args))
+	for i, arg := range args {
+		anm := pySafeArg(arg.Name(), i)
+		isVariadicTail := fsym.isVariadic && i == len(args)-1
+		expr, needsGIL := g.argCallExpr(ifchandle, isVariadicTail, anm, arg.sym)
+		if needsGIL {
+			varnm := fmt.Sprintf("_premarshal%d", i)
+			g.gofile.Printf("%s := %s\n", varnm, expr)
+			expr = varnm
+		}
+		argExprs[i] = expr
+	}
+
 	g.gofile.Print(g.shim.saveThread(!rvIsErr && nres != 2))
 
 	if isMethod {
@@ -352,19 +412,10 @@ if __err != nil {
 		wrapArgs = append(wrapArgs, "self.handle")
 	}
 	for i, arg := range args {
-		na := ""
 		anm := pySafeArg(arg.Name(), i)
-		switch {
-		case ifchandle && arg.sym.goname == "interface{}":
-			na = fmt.Sprintf(`gopyh.VarFromHandle((gopyh.CGoHandle)(%s), "interface{}")`, anm)
-		case arg.sym.isSignature():
-			na = g.shim.callbackArg(g, anm, arg.sym)
-		case arg.sym.py2go != "":
-			na = fmt.Sprintf("%s(%s)%s", arg.sym.py2go, anm, arg.sym.py2goParenEx)
-		default:
-			na = anm
-		}
-		if i == len(args)-1 && fsym.isVariadic {
+		isVariadicTail := fsym.isVariadic && i == len(args)-1
+		na := argExprs[i]
+		if isVariadicTail {
 			na = na + "..."
 		}
 		callArgs = append(callArgs, na)

bind/gen_func_test.go

@@ -0,0 +1,125 @@
+// Copyright 2015-present, The go-python Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package bind
+
+import "testing"
+
+// TestArgCallExprPrecedenceMatchesPremarshalDecision guards against the
+// failure mode flagged in the PR #401 review of needsGILForArgMarshal: the
+// call-argument expression for a Python-wrapped argument used to be decided
+// by a switch with a fixed case order (ifchandle&&interface{} > isSignature
+// > premarshalled > py2go > default), while a *separate* loop decided,
+// independently, whether to emit a "_premarshalN := ..." declaration before
+// the GIL is released. Nothing today makes an argument match both an
+// earlier switch case and needsGILForArgMarshal, but if it ever did, the
+// premarshal declaration would be emitted and never referenced --
+// "_premarshalN declared and not used" -- a compile error surfacing in the
+// user's generated package, not in gopy.
+//
+// argCallExpr is the fix: genFuncBody calls this single function once per
+// argument and uses its result both for the premarshal decision and at the
+// call site, so a premarshalled variable is declared if and only if it is
+// also the expression that gets used. This test exercises that precedence directly,
+// including a contrived interface{}/*C.PyObject collision that cannot
+// happen with today's type table (interface{}'s cgoname is "*C.char") but
+// is exactly the shape the review warned about.
+func TestArgCallExprPrecedenceMatchesPremarshalDecision(t *testing.T) {
+	complex128Sym := &symbol{
+		goname:       "complex128",
+		cgoname:      "*C.PyObject",
+		py2go:        "complex128FromPyObject",
+		py2goParenEx: "",
+	}
+
+	ifaceHandleSym := &symbol{
+		goname:       "interface{}",
+		cgoname:      "*C.PyObject", // contrived: not true of any symbol today
+		py2go:        "wouldBeMarshalFunc",
+		py2goParenEx: "",
+	}
+
+	sigSym := &symbol{
+		kind:         skSignature,
+		goname:       "func(int) string",
+		cgoname:      "*C.PyObject",
+		py2go:        "func (x int) string { ... }",
+		py2goParenEx: "",
+	}
+
+	plainStringSym := &symbol{
+		goname:       "string",
+		cgoname:      "*C.char",
+		py2go:        "C.GoString",
+		py2goParenEx: "",
+	}
+
+	// contrived: a *C.PyObject cgoname with no py2go converter must still
+	// fall through to the default case, never be premarshalled.
+	noConvSym := &symbol{
+		goname:  "int64",
+		cgoname: "*C.PyObject",
+	}
+
+	tests := []struct {
+		name         string
+		ifchandle    bool
+		isVariadic   bool
+		sym          *symbol
+		wantExpr     string
+		wantNeedsGIL bool
+	}{
+		{
+			name:         "complex128 arg is premarshalled",
+			sym:          complex128Sym,
+			wantExpr:     "complex128FromPyObject(x)",
+			wantNeedsGIL: true,
+		},
+		{
+			name:         "complex128 arg in variadic tail is not premarshalled",
+			isVariadic:   true,
+			sym:          complex128Sym,
+			wantExpr:     "complex128FromPyObject(x)",
+			wantNeedsGIL: false,
+		},
+		{
+			name:         "ifchandle interface{} wins over premarshalling even if cgoname collides",
+			ifchandle:    true,
+			sym:          ifaceHandleSym,
+			wantExpr:     `gopyh.VarFromHandle((gopyh.CGoHandle)(x), "interface{}")`,
+			wantNeedsGIL: false,
+		},
+		{
+			name:         "isSignature wins over premarshalling",
+			sym:          sigSym,
+			wantExpr:     "func (x int) string { ... }",
+			wantNeedsGIL: false,
+		},
+		{
+			name:         "plain string arg is not premarshalled",
+			sym:          plainStringSym,
+			wantExpr:     "C.GoString(x)",
+			wantNeedsGIL: false,
+		},
+		{
+			name:         "arg without a py2go conversion is passed through as-is",
+			sym:          noConvSym,
+			wantExpr:     "x",
+			wantNeedsGIL: false,
+		},
+	}
+
+	g := &pyGen{shim: cpythonShim{}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotExpr, gotNeedsGIL := g.argCallExpr(tt.ifchandle, tt.isVariadic, "x", tt.sym)
+			if gotExpr != tt.wantExpr {
+				t.Errorf("argCallExpr() expr = %q, want %q", gotExpr, tt.wantExpr)
+			}
+			if gotNeedsGIL != tt.wantNeedsGIL {
+				t.Errorf("argCallExpr() needsGIL = %v, want %v", gotNeedsGIL, tt.wantNeedsGIL)
+			}
+		})
+	}
+}

bind/symbols.go

@@ -575,6 +575,19 @@ func (sym *symtab) buildTuple(tuple *types.Tuple, varnm string, methvar string)
 	// 	bstr += fmt.Sprintf("}\n")
 	// }
 
+	// PyTuple_New/PyTuple_SetItem below touch raw *C.PyObject values and
+	// must run with the GIL held. This code is spliced verbatim into a
+	// caller-generated function body, so the caller is responsible for
+	// holding the GIL across it -- addSignatureType (its only caller) does
+	// so already, bracketing this output together with the
+	// PyObject_CallObject/decref/return-conversion that follows it in the
+	// same generated closure, under one PyGILState_Ensure/Release. Do not
+	// re-bracket it here: a self-contained Ensure/Release around just this
+	// fragment would be redundant with the caller's (nesting is safe, so it
+	// wouldn't break anything), but it also wouldn't buy anything, since
+	// the built tuple is only ever used later, by the same caller, which
+	// must keep holding the GIL regardless.
+	//
 	// TODO: more efficient to use strings.Builder here..
 	bstr := fmt.Sprintf("%s := C.PyTuple_New(%d)\n", varnm, sz)
 	for i := 0; i < sz; i++ {
@@ -1054,13 +1067,23 @@ func (sym *symtab) addSignatureType(pkg *types.Package, obj types.Object, t type
 		py2g += retstr + "C.PyObject_CallObject(_fun_arg, nil)\n"
 	}
 	py2g += "C.gopy_err_handle()\n"
-	py2g += "C.PyGILState_Release(_gstate)\n"
+	// _fcret (if any) is a *C.PyObject -- a new reference returned by
+	// PyObject_CallObject. Converting it to a Go value (pyObjectToGo, e.g.
+	// PyLong_AsLongLong/PyBytes_AsString) and decref'ing it both touch a
+	// raw PyObject and so must happen before PyGILState_Release, not after.
+	// The converted value is stashed in a local so it can still be returned
+	// once the GIL is no longer held.
 	if rets.Len() == 1 {
 		cvt, err := sym.pyObjectToGo(ret.Type(), rsym, "_fcret")
 		if err != nil {
 			return err
 		}
-		py2g += fmt.Sprintf("return %s", cvt)
+		py2g += fmt.Sprintf("_fcretgo := %s\n", cvt)
+		py2g += "C.gopy_decref(_fcret)\n"
+	}
+	py2g += "C.PyGILState_Release(_gstate)\n"
+	if rets.Len() == 1 {
+		py2g += "return _fcretgo"
 	}
 	py2g += "}"