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

_examples/simple/test_complex_stress.py

@@ -0,0 +1,60 @@
+# Copyright 2026 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/stress test for the complex64/complex128 GIL-handling fix.
+#
+# Comp64Add/Comp128Add marshal a raw *C.PyObject on both sides of the call:
+# PyComplex_AsCComplex on the way in, PyComplex_FromDoubles on the way out.
+# Both must run while the GIL is held -- gopy releases the GIL only around
+# the pure-Go call in between. This hammers the round trip from the main
+# thread while a background thread continuously allocates and collects
+# Python objects, so that if the marshalling window ever slipped outside the
+# GIL-held region, refcount/GC corruption would have a chance to surface.
+
+import gc
+import sys
+import threading
+
+import simple as pkg
+
+ITERATIONS = 500
+STOP = threading.Event()
+
+
+def churn():
+    while not STOP.is_set():
+        _ = [object() for _ in range(100)]
+        gc.collect(0)
+
+
+t = threading.Thread(target=churn, daemon=True)
+t.start()
+
+try:
+    for i in range(ITERATIONS):
+        a = complex(i, -i)
+        b = complex(-i, i * 2)
+        want = a + b
+
+        got64 = pkg.Comp64Add(a, b)
+        # complex64 loses precision relative to Python's complex128
+        # arithmetic; compare with a tolerance.
+        if abs(got64 - want) > 1e-3:
+            print("FAIL: Comp64Add(%s, %s) = %s, want ~%s" % (a, b, got64, want),
+                  file=sys.stderr)
+            sys.exit(1)
+
+        got128 = pkg.Comp128Add(a, b)
+        if got128 != want:
+            print("FAIL: Comp128Add(%s, %s) = %s, want %s" % (a, b, got128, want),
+                  file=sys.stderr)
+            sys.exit(1)
+
+        if i % 50 == 0:
+            gc.collect()
+finally:
+    STOP.set()
+    t.join()
+
+print("OK")

bind/gen_func.go

@@ -216,6 +216,49 @@ 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 (isSignature's closure re-acquires the GIL itself),
+// so they always report needsGIL=false, regardless of the symbol's cgoname.
+// Callers must call this once per argument for the premarshal decision and
+// again (with identical inputs) for the call-argument expression -- because
+// both calls run the same deterministic logic, a "_premarshalN" can never
+// be declared under one precedence and then left unreferenced under
+// another, the way it could when the two decisions were made by separately
+// maintained code.
+func 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 sym.py2go, 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
@@ -261,6 +304,25 @@ func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) {
 		}
 	}
 
+	// Convert any argument whose marshalling touches a raw *C.PyObject while
+	// the GIL is still held -- before it is released below for the duration
+	// of the wrapped Go call. argCallExpr is also what decides each
+	// argument's call-site expression below, so a premarshalled variable is
+	// declared here if and only if it is also the expression consumed
+	// there.
+	premarshalled := make(map[int]string)
+	for i, arg := range args {
+		anm := pySafeArg(arg.Name(), i)
+		isVariadicTail := fsym.isVariadic && i == len(args)-1
+		expr, needsGIL := argCallExpr(ifchandle, isVariadicTail, anm, arg.sym)
+		if !needsGIL {
+			continue
+		}
+		varnm := fmt.Sprintf("_premarshal%d", i)
+		g.gofile.Printf("%s := %s\n", varnm, expr)
+		premarshalled[i] = varnm
+	}
+
 	g.gofile.Printf("_saved_thread := C.PyEval_SaveThread()\n")
 	if !rvIsErr && nres != 2 {
 		g.gofile.Printf("defer C.PyEval_RestoreThread(_saved_thread)\n")
@@ -297,19 +359,13 @@ 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 = fmt.Sprintf("%s", arg.sym.py2go)
-		case arg.sym.py2go != "":
-			na = fmt.Sprintf("%s(%s)%s", arg.sym.py2go, anm, arg.sym.py2goParenEx)
-		default:
-			na = anm
+		isVariadicTail := fsym.isVariadic && i == len(args)-1
+		na, needsGIL := argCallExpr(ifchandle, isVariadicTail, anm, arg.sym)
+		if needsGIL {
+			na = premarshalled[i]
 		}
-		if i == len(args)-1 && fsym.isVariadic {
+		if isVariadicTail {
 			na = na + "..."
 		}
 		callArgs = append(callArgs, na)

bind/gen_func_test.go

@@ -0,0 +1,111 @@
+// Copyright 2026 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: both the premarshal pass and the call-argument
+// pass now call this single function with the same inputs, 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: "",
+	}
+
+	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,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotExpr, gotNeedsGIL := 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

@@ -636,6 +636,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++ {
@@ -1115,13 +1128,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 += "}"
 

bind/symbols_test.go

@@ -0,0 +1,75 @@
+// Copyright 2026 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 (
+	"go/token"
+	"go/types"
+	"strings"
+	"testing"
+)
+
+// TestAddSignatureTypeHoldsGILThroughoutClosure guards against the family of
+// GIL bugs already fixed once for complex64/complex128 arguments (see
+// needsGILForArgMarshal in gen_func.go): generated code that touches a raw
+// *C.PyObject must never run outside a PyGILState_Ensure/Release bracket.
+//
+// addSignatureType's py2go closure is the one place this whole family shows
+// up together: it builds a PyTuple from the Go-side arguments (PyTuple_New/
+// SetItem, via buildTuple), invokes the Python callback
+// (PyObject_CallObject), and converts the *C.PyObject result back to a Go
+// value (pyObjectToGo, e.g. PyBytes_AsString) before decref'ing it. All of
+// that touches raw PyObjects and must happen inside the single
+// PyGILState_Ensure/Release the closure takes out -- not just the tuple
+// build (buildTuple itself does not bracket its own output; it relies on
+// its only caller, this closure, to hold the GIL across build, call, and
+// return-conversion together). Getting the release point wrong here is not
+// hypothetical: _examples/funcs' CallBackRval passes a bool-returning
+// callback through exactly this path today.
+func TestAddSignatureTypeHoldsGILThroughoutClosure(t *testing.T) {
+	sig := types.NewSignature(nil,
+		types.NewTuple(types.NewVar(token.NoPos, nil, "x", types.Typ[types.Int])),
+		types.NewTuple(types.NewVar(token.NoPos, nil, "", types.Typ[types.String])),
+		false)
+
+	if err := current.addSignatureType(nil, nil, sig, 0, "sigtest_id", "sigtest"); err != nil {
+		t.Fatalf("addSignatureType returned error: %v", err)
+	}
+
+	sym := current.symtype(sig)
+	if sym == nil {
+		t.Fatalf("addSignatureType did not register a symbol for %s", current.fullTypeString(sig))
+	}
+	got := sym.py2go
+
+	ensureIdx := strings.Index(got, "C.PyGILState_Ensure()")
+	lastTupleIdx := strings.LastIndex(got, "C.PyTuple_")
+	callIdx := strings.Index(got, "C.PyObject_CallObject(")
+	convIdx := strings.Index(got, "C.PyBytes_AsString(_fcret)")
+	decrefIdx := strings.Index(got, "C.gopy_decref(_fcret)")
+	releaseIdx := strings.LastIndex(got, "C.PyGILState_Release(_gstate)")
+
+	for name, idx := range map[string]int{
+		"PyGILState_Ensure":        ensureIdx,
+		"PyTuple_*":                lastTupleIdx,
+		"PyObject_CallObject":      callIdx,
+		"PyBytes_AsString":         convIdx,
+		"gopy_decref(_fcret)":      decrefIdx,
+		"final PyGILState_Release": releaseIdx,
+	} {
+		if idx == -1 {
+			t.Fatalf("could not locate expected %s call in generated closure:\n%s", name, got)
+		}
+	}
+
+	// Everything that touches a PyObject -- building the tuple, invoking
+	// the callback, converting and decref'ing the result -- must occur
+	// strictly between the Ensure and the final Release.
+	if !(ensureIdx < lastTupleIdx && lastTupleIdx < callIdx && callIdx < convIdx &&
+		convIdx < releaseIdx && decrefIdx < releaseIdx) {
+		t.Fatalf("generated closure does not hold the GIL across the entire "+
+			"build-tuple/call/convert/decref sequence:\n%s", got)
+	}
+}