From 68f5fadf92c4af3975e708bfe10025c95a202383 Mon Sep 17 00:00:00 2001 From: b-long Date: Thu, 23 Jul 2026 21:13:48 -0400 Subject: [PATCH 1/8] feat: matrix build `python-version: ['3.11', '3.12']` --- .github/workflows/ci.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ada2439..af0017d 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 From 1273f4657f4c02c2d87ce1fe06ac1c5d0563e776 Mon Sep 17 00:00:00 2001 From: Vadim Dyadkin Date: Thu, 23 Jul 2026 11:10:36 +0200 Subject: [PATCH 2/8] bind: make _gopy_clear_go_tls opt-in, off by default The unconditional _gopy_clear_go_tls() call (issue #370) performs a hardcoded TLS store (movq $0, %fs:-8 on linux/amd64). On glibc + CPython 3.12+ that offset overlaps CPython's current-thread-state TLS slot, so the store nulls it and the interpreter segfaults on the first CGo entry in the common single-extension case (issue #395). Add a -clear-go-tls build flag (default false) and gate the single call site on it. The C helper and its pybindgen registration are left in place so opt-in restores the previous behavior exactly. RTLD_GLOBAL-local loading, which is what actually isolates each runtime's goroutine-pointer TLS, is untouched and remains unconditional. Fixes #395 --- bind/gen.go | 24 ++++++++++++++++++------ bind/gen_func.go | 9 +++++++-- cmd_build.go | 2 ++ cmd_exe.go | 2 ++ cmd_gen.go | 2 ++ cmd_pkg.go | 2 ++ 6 files changed, 33 insertions(+), 8 deletions(-) diff --git a/bind/gen.go b/bind/gen.go index 97c1ade..f6d8604 100644 --- a/bind/gen.go +++ b/bind/gen.go @@ -329,12 +329,13 @@ cwd = os.getcwd() currentdir = os.path.dirname(os.path.abspath(inspect.getfile(inspect.currentframe()))) os.chdir(currentdir) # When multiple gopy extensions coexist in one Python process each carries its own -# independent Go runtime. The Go extension is loaded without RTLD_GLOBAL below, and -# _gopy_clear_go_tls() is called before each CGo entry to force needm() to run, which -# establishes the correct per-extension M/P/mcache context (issue #370). -# Also load the extension without RTLD_GLOBAL so that Go runtime symbols stay -# local to each .so — belt-and-suspenders on platforms where RTLD_GLOBAL is the -# Python default (e.g. some Linux builds). +# independent Go runtime. Loading each extension without RTLD_GLOBAL below keeps its +# Go runtime symbols (including the per-runtime goroutine-pointer TLS slot) local to +# its own .so, which is what prevents the runtimes from colliding (issue #370). +# A _gopy_clear_go_tls() call before each CGo entry is available as an extra safety +# net but is opt-in (gopy build -clear-go-tls), off by default: on glibc + CPython +# 3.12+ its hardcoded TLS store clobbers the interpreter thread state and crashes on +# the first call in the common single-extension case (issue #395). if hasattr(sys, 'getdlopenflags'): try: import ctypes as _gopy_ctypes @@ -518,6 +519,17 @@ var NoWarn = false // NoMake turns off generation of Makefiles var NoMake = false +// ClearGoTLS controls whether generated wrappers emit a _gopy_clear_go_tls() +// call before every CGo entry point (issue #370). It is opt-in and off by +// default. The clear performs a hardcoded TLS store (movq $0, %gs:0x30 on +// darwin/amd64, movq $0, %fs:-8 on linux/amd64); with a single gopy extension +// in the process it is unnecessary, and on glibc + CPython 3.12+ that offset +// overlaps the TLS slot CPython uses for the current thread state, so the store +// nulls it and the interpreter segfaults on the first call (issue #395). +// Loading each extension without RTLD_GLOBAL, done unconditionally, already +// keeps each runtime's goroutine-pointer TLS local to its own .so. +var ClearGoTLS = false + // GenPyBind generates a .go file, build.py file to enable pybindgen to create python bindings, // and wrapper .py file(s) that are loaded as the interface to the package with shadow // python-side classes diff --git a/bind/gen_func.go b/bind/gen_func.go index fc2ee15..b264334 100644 --- a/bind/gen_func.go +++ b/bind/gen_func.go @@ -339,8 +339,13 @@ if __err != nil { // Clear the Go TLS goroutine slot before the CGo entry point so that // Go's needm() runs and establishes the correct per-extension context. // Without this, two extensions sharing the same process can corrupt - // each other's heap via TLS collision (issue #370). - g.pywrap.Printf("_%s._gopy_clear_go_tls()\n", pkgname) + // each other's heap via TLS collision (issue #370). Opt-in and off by + // default: the hardcoded TLS store crashes CPython 3.12+ in the common + // single-extension case (issue #395), and RTLD_GLOBAL-local loading + // already isolates each runtime's goroutine-pointer TLS. + if ClearGoTLS { + g.pywrap.Printf("_%s._gopy_clear_go_tls()\n", pkgname) + } // pywrap output mnm := fsym.ID() diff --git a/cmd_build.go b/cmd_build.go index 3def118..7e38997 100644 --- a/cmd_build.go +++ b/cmd_build.go @@ -45,6 +45,7 @@ ex: cmd.Flag.Bool("symbols", true, "include symbols in output") cmd.Flag.Bool("no-warn", false, "suppress warning messages, which may be expected") cmd.Flag.Bool("no-make", false, "do not generate a Makefile, e.g., when called from Makefile") + cmd.Flag.Bool("clear-go-tls", false, "emit a _gopy_clear_go_tls() call before every CGo entry (issue #370); off by default, needed only when several gopy extensions share one process and known to crash CPython 3.12+ (issue #395)") cmd.Flag.Bool("dynamic-link", false, "whether to link output shared library dynamically to Python") cmd.Flag.String("build-tags", "", "build tags to be passed to `go build`") return cmd @@ -72,6 +73,7 @@ func gopyRunCmdBuild(cmdr *commander.Command, args []string) error { bind.NoWarn = cfg.NoWarn bind.NoMake = cfg.NoMake + bind.ClearGoTLS = cmdr.Flag.Lookup("clear-go-tls").Value.Get().(bool) for _, path := range args { bpkg, err := loadPackage(path, true, cfg.BuildTags) // build first diff --git a/cmd_exe.go b/cmd_exe.go index 60ee3ce..acbd51e 100644 --- a/cmd_exe.go +++ b/cmd_exe.go @@ -58,6 +58,7 @@ ex: cmd.Flag.String("url", "https://github.com/go-python/gopy", "home page for project") cmd.Flag.Bool("no-warn", false, "suppress warning messages, which may be expected") cmd.Flag.Bool("no-make", false, "do not generate a Makefile, e.g., when called from Makefile") + cmd.Flag.Bool("clear-go-tls", false, "emit a _gopy_clear_go_tls() call before every CGo entry (issue #370); off by default, needed only when several gopy extensions share one process and known to crash CPython 3.12+ (issue #395)") cmd.Flag.Bool("dynamic-link", false, "whether to link output shared library dynamically to Python") cmd.Flag.String("build-tags", "", "build tags to be passed to `go build`") @@ -97,6 +98,7 @@ func gopyRunCmdExe(cmdr *commander.Command, args []string) error { bind.NoWarn = cfg.NoWarn bind.NoMake = cfg.NoMake + bind.ClearGoTLS = cmdr.Flag.Lookup("clear-go-tls").Value.Get().(bool) if cfg.Name == "" { path := args[0] diff --git a/cmd_gen.go b/cmd_gen.go index becd0f1..8b76735 100644 --- a/cmd_gen.go +++ b/cmd_gen.go @@ -37,6 +37,7 @@ ex: cmd.Flag.Bool("rename", false, "rename Go symbols to python PEP snake_case") cmd.Flag.Bool("no-warn", false, "suppress warning messages, which may be expected") cmd.Flag.Bool("no-make", false, "do not generate a Makefile, e.g., when called from Makefile") + cmd.Flag.Bool("clear-go-tls", false, "emit a _gopy_clear_go_tls() call before every CGo entry (issue #370); off by default, needed only when several gopy extensions share one process and known to crash CPython 3.12+ (issue #395)") cmd.Flag.Bool("dynamic-link", false, "whether to link output shared library dynamically to Python") cmd.Flag.String("build-tags", "", "build tags to be passed to `go build`") return cmd @@ -69,6 +70,7 @@ func gopyRunCmdGen(cmdr *commander.Command, args []string) error { bind.NoWarn = cfg.NoWarn bind.NoMake = cfg.NoMake + bind.ClearGoTLS = cmdr.Flag.Lookup("clear-go-tls").Value.Get().(bool) for _, path := range args { bpkg, err := loadPackage(path, true, cfg.BuildTags) // build first diff --git a/cmd_pkg.go b/cmd_pkg.go index 0f58480..9891907 100644 --- a/cmd_pkg.go +++ b/cmd_pkg.go @@ -55,6 +55,7 @@ ex: cmd.Flag.String("url", "https://github.com/go-python/gopy", "home page for project") cmd.Flag.Bool("no-warn", false, "suppress warning messages, which may be expected") cmd.Flag.Bool("no-make", false, "do not generate a Makefile, e.g., when called from Makefile") + cmd.Flag.Bool("clear-go-tls", false, "emit a _gopy_clear_go_tls() call before every CGo entry (issue #370); off by default, needed only when several gopy extensions share one process and known to crash CPython 3.12+ (issue #395)") cmd.Flag.Bool("dynamic-link", false, "whether to link output shared library dynamically to Python") cmd.Flag.String("build-tags", "", "build tags to be passed to `go build`") @@ -93,6 +94,7 @@ func gopyRunCmdPkg(cmdr *commander.Command, args []string) error { bind.NoWarn = cfg.NoWarn bind.NoMake = cfg.NoMake + bind.ClearGoTLS = cmdr.Flag.Lookup("clear-go-tls").Value.Get().(bool) if cfg.Name == "" { path := args[0] From d41db33699c93239969e74e9afe1effe006c802b Mon Sep 17 00:00:00 2001 From: b-long Date: Thu, 23 Jul 2026 22:39:01 -0400 Subject: [PATCH 3/8] fix complex conversion crash on CPython 3.12 Ensure GIL state before allocating; fixes stale thread-state segfault. --- bind/gen.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bind/gen.go b/bind/gen.go index f6d8604..fe96bd6 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 { From b46aa9e9c8cd5328fe3d5f32338e7cb0e32dad6e Mon Sep 17 00:00:00 2001 From: b-long Date: Thu, 23 Jul 2026 22:53:49 -0400 Subject: [PATCH 4/8] Revert "bind: make _gopy_clear_go_tls opt-in, off by default" This reverts commit 1273f4657f4c02c2d87ce1fe06ac1c5d0563e776. --- bind/gen.go | 24 ++++++------------------ bind/gen_func.go | 9 ++------- cmd_build.go | 2 -- cmd_exe.go | 2 -- cmd_gen.go | 2 -- cmd_pkg.go | 2 -- 6 files changed, 8 insertions(+), 33 deletions(-) diff --git a/bind/gen.go b/bind/gen.go index fe96bd6..a4623f2 100644 --- a/bind/gen.go +++ b/bind/gen.go @@ -335,13 +335,12 @@ cwd = os.getcwd() currentdir = os.path.dirname(os.path.abspath(inspect.getfile(inspect.currentframe()))) os.chdir(currentdir) # When multiple gopy extensions coexist in one Python process each carries its own -# independent Go runtime. Loading each extension without RTLD_GLOBAL below keeps its -# Go runtime symbols (including the per-runtime goroutine-pointer TLS slot) local to -# its own .so, which is what prevents the runtimes from colliding (issue #370). -# A _gopy_clear_go_tls() call before each CGo entry is available as an extra safety -# net but is opt-in (gopy build -clear-go-tls), off by default: on glibc + CPython -# 3.12+ its hardcoded TLS store clobbers the interpreter thread state and crashes on -# the first call in the common single-extension case (issue #395). +# independent Go runtime. The Go extension is loaded without RTLD_GLOBAL below, and +# _gopy_clear_go_tls() is called before each CGo entry to force needm() to run, which +# establishes the correct per-extension M/P/mcache context (issue #370). +# Also load the extension without RTLD_GLOBAL so that Go runtime symbols stay +# local to each .so — belt-and-suspenders on platforms where RTLD_GLOBAL is the +# Python default (e.g. some Linux builds). if hasattr(sys, 'getdlopenflags'): try: import ctypes as _gopy_ctypes @@ -525,17 +524,6 @@ var NoWarn = false // NoMake turns off generation of Makefiles var NoMake = false -// ClearGoTLS controls whether generated wrappers emit a _gopy_clear_go_tls() -// call before every CGo entry point (issue #370). It is opt-in and off by -// default. The clear performs a hardcoded TLS store (movq $0, %gs:0x30 on -// darwin/amd64, movq $0, %fs:-8 on linux/amd64); with a single gopy extension -// in the process it is unnecessary, and on glibc + CPython 3.12+ that offset -// overlaps the TLS slot CPython uses for the current thread state, so the store -// nulls it and the interpreter segfaults on the first call (issue #395). -// Loading each extension without RTLD_GLOBAL, done unconditionally, already -// keeps each runtime's goroutine-pointer TLS local to its own .so. -var ClearGoTLS = false - // GenPyBind generates a .go file, build.py file to enable pybindgen to create python bindings, // and wrapper .py file(s) that are loaded as the interface to the package with shadow // python-side classes diff --git a/bind/gen_func.go b/bind/gen_func.go index b264334..fc2ee15 100644 --- a/bind/gen_func.go +++ b/bind/gen_func.go @@ -339,13 +339,8 @@ if __err != nil { // Clear the Go TLS goroutine slot before the CGo entry point so that // Go's needm() runs and establishes the correct per-extension context. // Without this, two extensions sharing the same process can corrupt - // each other's heap via TLS collision (issue #370). Opt-in and off by - // default: the hardcoded TLS store crashes CPython 3.12+ in the common - // single-extension case (issue #395), and RTLD_GLOBAL-local loading - // already isolates each runtime's goroutine-pointer TLS. - if ClearGoTLS { - g.pywrap.Printf("_%s._gopy_clear_go_tls()\n", pkgname) - } + // each other's heap via TLS collision (issue #370). + g.pywrap.Printf("_%s._gopy_clear_go_tls()\n", pkgname) // pywrap output mnm := fsym.ID() diff --git a/cmd_build.go b/cmd_build.go index 7e38997..3def118 100644 --- a/cmd_build.go +++ b/cmd_build.go @@ -45,7 +45,6 @@ ex: cmd.Flag.Bool("symbols", true, "include symbols in output") cmd.Flag.Bool("no-warn", false, "suppress warning messages, which may be expected") cmd.Flag.Bool("no-make", false, "do not generate a Makefile, e.g., when called from Makefile") - cmd.Flag.Bool("clear-go-tls", false, "emit a _gopy_clear_go_tls() call before every CGo entry (issue #370); off by default, needed only when several gopy extensions share one process and known to crash CPython 3.12+ (issue #395)") cmd.Flag.Bool("dynamic-link", false, "whether to link output shared library dynamically to Python") cmd.Flag.String("build-tags", "", "build tags to be passed to `go build`") return cmd @@ -73,7 +72,6 @@ func gopyRunCmdBuild(cmdr *commander.Command, args []string) error { bind.NoWarn = cfg.NoWarn bind.NoMake = cfg.NoMake - bind.ClearGoTLS = cmdr.Flag.Lookup("clear-go-tls").Value.Get().(bool) for _, path := range args { bpkg, err := loadPackage(path, true, cfg.BuildTags) // build first diff --git a/cmd_exe.go b/cmd_exe.go index acbd51e..60ee3ce 100644 --- a/cmd_exe.go +++ b/cmd_exe.go @@ -58,7 +58,6 @@ ex: cmd.Flag.String("url", "https://github.com/go-python/gopy", "home page for project") cmd.Flag.Bool("no-warn", false, "suppress warning messages, which may be expected") cmd.Flag.Bool("no-make", false, "do not generate a Makefile, e.g., when called from Makefile") - cmd.Flag.Bool("clear-go-tls", false, "emit a _gopy_clear_go_tls() call before every CGo entry (issue #370); off by default, needed only when several gopy extensions share one process and known to crash CPython 3.12+ (issue #395)") cmd.Flag.Bool("dynamic-link", false, "whether to link output shared library dynamically to Python") cmd.Flag.String("build-tags", "", "build tags to be passed to `go build`") @@ -98,7 +97,6 @@ func gopyRunCmdExe(cmdr *commander.Command, args []string) error { bind.NoWarn = cfg.NoWarn bind.NoMake = cfg.NoMake - bind.ClearGoTLS = cmdr.Flag.Lookup("clear-go-tls").Value.Get().(bool) if cfg.Name == "" { path := args[0] diff --git a/cmd_gen.go b/cmd_gen.go index 8b76735..becd0f1 100644 --- a/cmd_gen.go +++ b/cmd_gen.go @@ -37,7 +37,6 @@ ex: cmd.Flag.Bool("rename", false, "rename Go symbols to python PEP snake_case") cmd.Flag.Bool("no-warn", false, "suppress warning messages, which may be expected") cmd.Flag.Bool("no-make", false, "do not generate a Makefile, e.g., when called from Makefile") - cmd.Flag.Bool("clear-go-tls", false, "emit a _gopy_clear_go_tls() call before every CGo entry (issue #370); off by default, needed only when several gopy extensions share one process and known to crash CPython 3.12+ (issue #395)") cmd.Flag.Bool("dynamic-link", false, "whether to link output shared library dynamically to Python") cmd.Flag.String("build-tags", "", "build tags to be passed to `go build`") return cmd @@ -70,7 +69,6 @@ func gopyRunCmdGen(cmdr *commander.Command, args []string) error { bind.NoWarn = cfg.NoWarn bind.NoMake = cfg.NoMake - bind.ClearGoTLS = cmdr.Flag.Lookup("clear-go-tls").Value.Get().(bool) for _, path := range args { bpkg, err := loadPackage(path, true, cfg.BuildTags) // build first diff --git a/cmd_pkg.go b/cmd_pkg.go index 9891907..0f58480 100644 --- a/cmd_pkg.go +++ b/cmd_pkg.go @@ -55,7 +55,6 @@ ex: cmd.Flag.String("url", "https://github.com/go-python/gopy", "home page for project") cmd.Flag.Bool("no-warn", false, "suppress warning messages, which may be expected") cmd.Flag.Bool("no-make", false, "do not generate a Makefile, e.g., when called from Makefile") - cmd.Flag.Bool("clear-go-tls", false, "emit a _gopy_clear_go_tls() call before every CGo entry (issue #370); off by default, needed only when several gopy extensions share one process and known to crash CPython 3.12+ (issue #395)") cmd.Flag.Bool("dynamic-link", false, "whether to link output shared library dynamically to Python") cmd.Flag.String("build-tags", "", "build tags to be passed to `go build`") @@ -94,7 +93,6 @@ func gopyRunCmdPkg(cmdr *commander.Command, args []string) error { bind.NoWarn = cfg.NoWarn bind.NoMake = cfg.NoMake - bind.ClearGoTLS = cmdr.Flag.Lookup("clear-go-tls").Value.Get().(bool) if cfg.Name == "" { path := args[0] From 950015237ee5284c36f47f84b38cf2b4222bd553 Mon Sep 17 00:00:00 2001 From: b-long Date: Sun, 26 Jul 2026 18:49:02 -0400 Subject: [PATCH 5/8] Hold GIL while marshalling complex arguments Convert PyObject-touching args before releasing GIL; add stress test. --- _examples/simple/test_complex_stress.py | 60 +++++++++++++++++++++++++ bind/gen_func.go | 32 +++++++++++++ main_test.go | 58 ++++++++++++++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 _examples/simple/test_complex_stress.py diff --git a/_examples/simple/test_complex_stress.py b/_examples/simple/test_complex_stress.py new file mode 100644 index 0000000..5d9f7cd --- /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 = 20000 +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 % 500 == 0: + gc.collect() +finally: + STOP.set() + t.join() + +print("OK") diff --git a/bind/gen_func.go b/bind/gen_func.go index b264334..fcc131a 100644 --- a/bind/gen_func.go +++ b/bind/gen_func.go @@ -216,6 +216,18 @@ 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 != "" +} + func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) { isMethod := (sym != nil) isIface := false @@ -261,6 +273,24 @@ 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. The variadic tail is skipped: its "arg" stands + // in for the whole trailing slice, not a single marshalled scalar. + premarshalled := make(map[int]string) + for i, arg := range args { + if !needsGILForArgMarshal(arg.sym) { + continue + } + if fsym.isVariadic && i == len(args)-1 { + continue + } + anm := pySafeArg(arg.Name(), i) + varnm := fmt.Sprintf("_premarshal%d", i) + g.gofile.Printf("%s := %s(%s)%s\n", varnm, arg.sym.py2go, anm, arg.sym.py2goParenEx) + 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") @@ -304,6 +334,8 @@ if __err != nil { na = fmt.Sprintf(`gopyh.VarFromHandle((gopyh.CGoHandle)(%s), "interface{}")`, anm) case arg.sym.isSignature(): na = fmt.Sprintf("%s", arg.sym.py2go) + case premarshalled[i] != "": + na = premarshalled[i] case arg.sym.py2go != "": na = fmt.Sprintf("%s(%s)%s", arg.sym.py2go, anm, arg.sym.py2goParenEx) default: diff --git a/main_test.go b/main_test.go index 1127698..e150c48 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" From ff0e140c79362f5b08f6bd2c4cf3ded3e24234c5 Mon Sep 17 00:00:00 2001 From: b-long Date: Sun, 26 Jul 2026 18:49:19 -0400 Subject: [PATCH 6/8] Make tuple builder self-bracket the GIL buildTuple no longer relies on callers to hold the GIL. --- bind/symbols.go | 15 ++++++++++- bind/symbols_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 bind/symbols_test.go diff --git a/bind/symbols.go b/bind/symbols.go index 67bb6a5..d560ac8 100644 --- a/bind/symbols.go +++ b/bind/symbols.go @@ -636,8 +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 it cannot assume the GIL is + // already held there -- bracket it here instead of relying on every + // caller to do so correctly (PyGILState_Ensure/Release nest safely, so + // this is sound even when the caller already holds the GIL itself). + // varnm is declared outside the block so it stays visible to + // caller-emitted code that follows (e.g. PyObject_CallObject). + // // TODO: more efficient to use strings.Builder here.. - bstr := fmt.Sprintf("%s := C.PyTuple_New(%d)\n", varnm, sz) + bstr := fmt.Sprintf("var %s *C.PyObject\n{\n", varnm) + bstr += "_tuple_gstate := C.PyGILState_Ensure()\n" + bstr += fmt.Sprintf("%s = C.PyTuple_New(%d)\n", varnm, sz) for i := 0; i < sz; i++ { v := tuple.At(i) typ := v.Type() @@ -679,6 +690,8 @@ func (sym *symtab) buildTuple(tuple *types.Tuple, varnm string, methvar string) return "", fmt.Errorf("buildTuple: type not handled: %s", typ.String()) } } + bstr += "C.PyGILState_Release(_tuple_gstate)\n" + bstr += "}\n" return bstr, nil } diff --git a/bind/symbols_test.go b/bind/symbols_test.go new file mode 100644 index 0000000..68449c4 --- /dev/null +++ b/bind/symbols_test.go @@ -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. + +package bind + +import ( + "go/token" + "go/types" + "strings" + "testing" +) + +// TestBuildTupleHoldsGIL guards against a regression class already fixed +// once for complex64/complex128 (see needsGILForArgMarshal in gen_func.go): +// generated code that touches a raw *C.PyObject must never run outside a +// PyGILState_Ensure/Release (or equivalent) bracket. +// +// buildTuple emits PyTuple_New/PyTuple_SetItem -- pure PyObject manipulation +// -- as a bare code fragment with no way to know whether its caller already +// holds the GIL. Its one current caller (addSignatureType's py2go closure) +// happens to bracket it correctly today, but that's a convention external +// to buildTuple, not something buildTuple enforces. This test requires +// buildTuple's own output to be self-bracketing, so a future caller can't +// reintroduce the complex64/128 bug by forgetting to hold the GIL. +func TestBuildTupleHoldsGIL(t *testing.T) { + tuple := types.NewTuple( + types.NewVar(token.NoPos, nil, "i", types.Typ[types.Int]), + types.NewVar(token.NoPos, nil, "s", types.Typ[types.String]), + ) + + got, err := current.buildTuple(tuple, "_fcargs", "_fun_arg") + if err != nil { + t.Fatalf("buildTuple returned error: %v", err) + } + + ensureCount := strings.Count(got, "C.PyGILState_Ensure()") + releaseCount := strings.Count(got, "C.PyGILState_Release(") + if ensureCount == 0 || releaseCount == 0 { + t.Fatalf("buildTuple output does not self-bracket its PyTuple_* calls "+ + "with PyGILState_Ensure/Release; got:\n%s", got) + } + if ensureCount != releaseCount { + t.Fatalf("mismatched PyGILState_Ensure/Release counts (%d vs %d) in "+ + "buildTuple output:\n%s", ensureCount, releaseCount, got) + } + + ensureIdx := strings.Index(got, "C.PyGILState_Ensure()") + firstTupleIdx := strings.Index(got, "C.PyTuple_") + lastTupleIdx := strings.LastIndex(got, "C.PyTuple_") + releaseIdx := strings.LastIndex(got, "C.PyGILState_Release(") + + if ensureIdx == -1 || firstTupleIdx == -1 || releaseIdx == -1 { + t.Fatalf("could not locate expected markers in buildTuple output:\n%s", got) + } + if !(ensureIdx < firstTupleIdx && lastTupleIdx < releaseIdx) { + t.Fatalf("PyTuple_* calls are not nested inside the "+ + "PyGILState_Ensure/Release bracket in buildTuple output:\n%s", got) + } +} From 77da2d6286c954b120cdb07dbcc6aeaa6e337041 Mon Sep 17 00:00:00 2001 From: b-long Date: Mon, 27 Jul 2026 04:27:33 -0400 Subject: [PATCH 7/8] Fix GIL release ordering in signature callbacks Converts and decrefs return value before releasing GIL, per review. Fixes undefined behavior and leak; drops buildTuple's redundant self-bracket. --- bind/symbols.go | 36 +++++++++++------- bind/symbols_test.go | 87 ++++++++++++++++++++++++++------------------ 2 files changed, 74 insertions(+), 49 deletions(-) diff --git a/bind/symbols.go b/bind/symbols.go index d560ac8..3d0fea3 100644 --- a/bind/symbols.go +++ b/bind/symbols.go @@ -638,17 +638,19 @@ func (sym *symtab) buildTuple(tuple *types.Tuple, varnm string, methvar string) // 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 it cannot assume the GIL is - // already held there -- bracket it here instead of relying on every - // caller to do so correctly (PyGILState_Ensure/Release nest safely, so - // this is sound even when the caller already holds the GIL itself). - // varnm is declared outside the block so it stays visible to - // caller-emitted code that follows (e.g. PyObject_CallObject). + // 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("var %s *C.PyObject\n{\n", varnm) - bstr += "_tuple_gstate := C.PyGILState_Ensure()\n" - bstr += fmt.Sprintf("%s = C.PyTuple_New(%d)\n", varnm, sz) + bstr := fmt.Sprintf("%s := C.PyTuple_New(%d)\n", varnm, sz) for i := 0; i < sz; i++ { v := tuple.At(i) typ := v.Type() @@ -690,8 +692,6 @@ func (sym *symtab) buildTuple(tuple *types.Tuple, varnm string, methvar string) return "", fmt.Errorf("buildTuple: type not handled: %s", typ.String()) } } - bstr += "C.PyGILState_Release(_tuple_gstate)\n" - bstr += "}\n" return bstr, nil } @@ -1128,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 index 68449c4..4bb8e00 100644 --- a/bind/symbols_test.go +++ b/bind/symbols_test.go @@ -11,50 +11,65 @@ import ( "testing" ) -// TestBuildTupleHoldsGIL guards against a regression class already fixed -// once for complex64/complex128 (see needsGILForArgMarshal in gen_func.go): -// generated code that touches a raw *C.PyObject must never run outside a -// PyGILState_Ensure/Release (or equivalent) bracket. +// 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. // -// buildTuple emits PyTuple_New/PyTuple_SetItem -- pure PyObject manipulation -// -- as a bare code fragment with no way to know whether its caller already -// holds the GIL. Its one current caller (addSignatureType's py2go closure) -// happens to bracket it correctly today, but that's a convention external -// to buildTuple, not something buildTuple enforces. This test requires -// buildTuple's own output to be self-bracketing, so a future caller can't -// reintroduce the complex64/128 bug by forgetting to hold the GIL. -func TestBuildTupleHoldsGIL(t *testing.T) { - tuple := types.NewTuple( - types.NewVar(token.NoPos, nil, "i", types.Typ[types.Int]), - types.NewVar(token.NoPos, nil, "s", types.Typ[types.String]), - ) - - got, err := current.buildTuple(tuple, "_fcargs", "_fun_arg") - if err != nil { - t.Fatalf("buildTuple returned error: %v", err) - } +// 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) - ensureCount := strings.Count(got, "C.PyGILState_Ensure()") - releaseCount := strings.Count(got, "C.PyGILState_Release(") - if ensureCount == 0 || releaseCount == 0 { - t.Fatalf("buildTuple output does not self-bracket its PyTuple_* calls "+ - "with PyGILState_Ensure/Release; got:\n%s", got) + if err := current.addSignatureType(nil, nil, sig, 0, "sigtest_id", "sigtest"); err != nil { + t.Fatalf("addSignatureType returned error: %v", err) } - if ensureCount != releaseCount { - t.Fatalf("mismatched PyGILState_Ensure/Release counts (%d vs %d) in "+ - "buildTuple output:\n%s", ensureCount, releaseCount, got) + + 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()") - firstTupleIdx := strings.Index(got, "C.PyTuple_") lastTupleIdx := strings.LastIndex(got, "C.PyTuple_") - releaseIdx := strings.LastIndex(got, "C.PyGILState_Release(") + 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)") - if ensureIdx == -1 || firstTupleIdx == -1 || releaseIdx == -1 { - t.Fatalf("could not locate expected markers in buildTuple output:\n%s", got) + 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) + } } - if !(ensureIdx < firstTupleIdx && lastTupleIdx < releaseIdx) { - t.Fatalf("PyTuple_* calls are not nested inside the "+ - "PyGILState_Ensure/Release bracket in buildTuple output:\n%s", 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) } } From f8d679543003e649994640aac63ee2c4fecfb9d8 Mon Sep 17 00:00:00 2001 From: b-long Date: Thu, 30 Jul 2026 21:16:52 -0400 Subject: [PATCH 8/8] Consolidate argument GIL-conversion logic Also, trims stress test to 500 iterations --- _examples/simple/test_complex_stress.py | 4 +- bind/gen_func.go | 64 +++++++++----- bind/gen_func_test.go | 111 ++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 22 deletions(-) create mode 100644 bind/gen_func_test.go diff --git a/_examples/simple/test_complex_stress.py b/_examples/simple/test_complex_stress.py index 5d9f7cd..80beadf 100644 --- a/_examples/simple/test_complex_stress.py +++ b/_examples/simple/test_complex_stress.py @@ -18,7 +18,7 @@ import simple as pkg -ITERATIONS = 20000 +ITERATIONS = 500 STOP = threading.Event() @@ -51,7 +51,7 @@ def churn(): file=sys.stderr) sys.exit(1) - if i % 500 == 0: + if i % 50 == 0: gc.collect() finally: STOP.set() diff --git a/bind/gen_func.go b/bind/gen_func.go index fcc131a..5ec5067 100644 --- a/bind/gen_func.go +++ b/bind/gen_func.go @@ -228,6 +228,37 @@ 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 @@ -275,19 +306,20 @@ 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. The variadic tail is skipped: its "arg" stands - // in for the whole trailing slice, not a single marshalled scalar. + // 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 { - if !needsGILForArgMarshal(arg.sym) { - continue - } - if fsym.isVariadic && i == len(args)-1 { + anm := pySafeArg(arg.Name(), i) + isVariadicTail := fsym.isVariadic && i == len(args)-1 + expr, needsGIL := argCallExpr(ifchandle, isVariadicTail, anm, arg.sym) + if !needsGIL { continue } - anm := pySafeArg(arg.Name(), i) varnm := fmt.Sprintf("_premarshal%d", i) - g.gofile.Printf("%s := %s(%s)%s\n", varnm, arg.sym.py2go, anm, arg.sym.py2goParenEx) + g.gofile.Printf("%s := %s\n", varnm, expr) premarshalled[i] = varnm } @@ -327,21 +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 premarshalled[i] != "": + isVariadicTail := fsym.isVariadic && i == len(args)-1 + na, needsGIL := argCallExpr(ifchandle, isVariadicTail, anm, arg.sym) + if needsGIL { na = premarshalled[i] - 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 { + 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 0000000..e8fda48 --- /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) + } + }) + } +}