diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ada2439c..af0017d8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,13 +18,14 @@ env: jobs: build: - name: Build + name: Build (${{ matrix.platform }}, Go ${{ matrix.go-version }}, Python ${{ matrix.python-version }}) strategy: fail-fast: false matrix: # TODO: Consider official support matrix (OS and Go versions) and adjust this matrix accordingly go-version: [1.25.x, 1.24.x, 1.23.x, 1.22.x] platform: [ubuntu-latest, windows-latest, macos-15] + python-version: ['3.11', '3.12'] runs-on: ${{ matrix.platform }} steps: - name: Checkout code @@ -33,7 +34,7 @@ jobs: - name: Set up Python uses: actions/setup-python@v5 with: - python-version: '3.11' + python-version: ${{ matrix.python-version }} - name: Install Go uses: actions/setup-go@v5 diff --git a/_examples/simple/test_complex_stress.py b/_examples/simple/test_complex_stress.py new file mode 100644 index 00000000..80beadf3 --- /dev/null +++ b/_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") diff --git a/bind/gen.go b/bind/gen.go index f6d8604e..fe96bd63 100644 --- a/bind/gen.go +++ b/bind/gen.go @@ -194,7 +194,10 @@ func boolPyToGo(b C.char) bool { } func complex64GoToPy(c complex64) *C.PyObject { - return C.PyComplex_FromDoubles(C.double(real(c)), C.double(imag(c))) + gstate := C.PyGILState_Ensure() + obj := C.PyComplex_FromDoubles(C.double(real(c)), C.double(imag(c))) + C.PyGILState_Release(gstate) + return obj } func complex64PyToGo(o *C.PyObject) complex64 { @@ -203,7 +206,10 @@ func complex64PyToGo(o *C.PyObject) complex64 { } func complex128GoToPy(c complex128) *C.PyObject { - return C.PyComplex_FromDoubles(C.double(real(c)), C.double(imag(c))) + gstate := C.PyGILState_Ensure() + obj := C.PyComplex_FromDoubles(C.double(real(c)), C.double(imag(c))) + C.PyGILState_Release(gstate) + return obj } func complex128PyToGo(o *C.PyObject) complex128 { diff --git a/bind/gen_func.go b/bind/gen_func.go index b2643343..5ec5067f 100644 --- a/bind/gen_func.go +++ b/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) diff --git a/bind/gen_func_test.go b/bind/gen_func_test.go new file mode 100644 index 00000000..e8fda48d --- /dev/null +++ b/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) + } + }) + } +} diff --git a/bind/symbols.go b/bind/symbols.go index 67bb6a5b..3d0fea38 100644 --- a/bind/symbols.go +++ b/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 += "}" diff --git a/bind/symbols_test.go b/bind/symbols_test.go new file mode 100644 index 00000000..4bb8e00a --- /dev/null +++ b/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) + } +} diff --git a/main_test.go b/main_test.go index 1127698c..e150c485 100644 --- a/main_test.go +++ b/main_test.go @@ -851,6 +851,64 @@ func TestGilString(t *testing.T) { } } +// TestComplexGILStress is a regression/stress test for the complex64/128 +// GIL-handling fix. Comp64Add/Comp128Add marshal a raw *C.PyObject on both +// sides of the call (PyComplex_AsCComplex on the argument-read side, +// PyComplex_FromDoubles on the return side); both must run while the GIL is +// held, since gopy releases it only around the pure-Go call in between. This +// repeatedly exercises the round trip under GC/allocation pressure from a +// background thread so a mis-timed marshalling window has a chance to +// surface as corruption rather than passing silently. +func TestComplexGILStress(t *testing.T) { + backends := []string{"py3"} + for _, be := range backends { + vm, ok := testBackends[be] + if !ok || vm == "" { + t.Logf("Skipped testing backend %s for TestComplexGILStress\n", be) + continue + } + t.Run(be, func(t *testing.T) { + cwd, _ := os.Getwd() + + workdir, err := os.MkdirTemp("", "gopy-") + if err != nil { + t.Fatalf("could not create workdir: %v", err) + } + defer os.RemoveAll(workdir) + defer bind.ResetPackages() + + writeGoMod(t, cwd, workdir) + if err := run([]string{"build", "-vm=" + vm, "-output=" + workdir, "-package-prefix", "", "./_examples/simple"}); err != nil { + t.Fatalf("error building simple: %v", err) + } + + tstDst := filepath.Join(workdir, "test_complex_stress.py") + if err := copyCmd(filepath.Join(cwd, "_examples/simple/test_complex_stress.py"), tstDst); err != nil { + t.Fatalf("error copying test_complex_stress.py: %v", err) + } + + env := make([]string, len(testEnvironment)) + copy(env, testEnvironment) + env = append(env, fmt.Sprintf("PYTHONPATH=%s", workdir)) + + cmd := exec.Command(vm, "./test_complex_stress.py") + cmd.Env = env + cmd.Dir = workdir + cmd.Stdin = os.Stdin + buf, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("error running python module: err=%v\n%s", err, string(buf)) + } + + got := strings.Replace(string(buf), "\r\n", "\n", -1) + want := "OK\n" + if got != want { + t.Fatalf("got:\n%s\nwant:\n%s", got, want) + } + }) + } +} + func TestPackagePrefix(t *testing.T) { // t.Parallel() path := "_examples/package/mypkg"