diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e232aa0c..3684d0b7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,7 +23,7 @@ jobs: fail-fast: false matrix: go-version: [1.25.x, 1.24.x, 1.22.x, 1.21.x] - platform: [ubuntu-latest, windows-latest] + platform: [ubuntu-latest, windows-latest, macos-15-intel, macos-15] #platform: [ubuntu-latest, macos-latest, windows-latest] runs-on: ${{ matrix.platform }} steps: @@ -51,6 +51,12 @@ jobs: # install goimports go install golang.org/x/tools/cmd/goimports@latest + - name: Install macOS packages + if: startsWith(matrix.platform, 'macos-') + run: | + python3 -m pip install -U pybindgen + go install golang.org/x/tools/cmd/goimports@latest + - name: Install Windows packages if: matrix.platform == 'windows-latest' run: | @@ -69,6 +75,15 @@ jobs: run: | make test + - name: Build-macOS + if: startsWith(matrix.platform, 'macos-') + run: | + make + - name: Test macOS + if: startsWith(matrix.platform, 'macos-') + run: | + make test + - name: Build-Windows if: matrix.platform == 'windows-latest' run: | diff --git a/SUPPORT_MATRIX.md b/SUPPORT_MATRIX.md index 8e73c1ae..8f30be77 100644 --- a/SUPPORT_MATRIX.md +++ b/SUPPORT_MATRIX.md @@ -11,6 +11,7 @@ _examples/consts | yes _examples/cstrings | yes _examples/empty | yes _examples/funcs | yes +_examples/gilstring | yes _examples/gobytes | yes _examples/gopygc | yes _examples/gostrings | yes diff --git a/_examples/cgo/cgo.go b/_examples/cgo/cgo.go index 98a64a31..4119d47a 100644 --- a/_examples/cgo/cgo.go +++ b/_examples/cgo/cgo.go @@ -9,7 +9,7 @@ package cgo //#include //#include //const char* cpkg_sprintf(const char *str) { -// char *o = (char*)malloc(strlen(str)); +// char *o = (char*)malloc(strlen(str) + 1); // sprintf(o, "%s", str); // return o; //} diff --git a/_examples/gilstring/gilstring.go b/_examples/gilstring/gilstring.go new file mode 100644 index 00000000..9410c57a --- /dev/null +++ b/_examples/gilstring/gilstring.go @@ -0,0 +1,14 @@ +// 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 gilstring is a regression test for the multi-runtime crash (issue #370). +// It mirrors the exact reproduction from the issue report: a string-returning +// function called alongside an integer function from a second extension in the +// same Python process, which triggers crashes under repeated calls. +package gilstring + +import "fmt" + +// Hello returns a greeting string, mirroring hi.Hello from the issue report. +func Hello(s string) string { return fmt.Sprintf("Hello, %s!", s) } diff --git a/_examples/gilstring/test.py b/_examples/gilstring/test.py new file mode 100644 index 00000000..1aae7b31 --- /dev/null +++ b/_examples/gilstring/test.py @@ -0,0 +1,18 @@ +# 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. + +## py2/py3 compat +from __future__ import print_function + +# Regression test for multi-runtime crash (issue #370). +# Exact reproduction from the issue report: two separately-built gopy +# extensions loaded in the same process, with calls interleaved in a loop. +from gilstring.gilstring import Hello +from simple.simple import Add + +for _ in range(5000): + Add(2, 2) + Hello('hi') + +print("OK") diff --git a/bind/gen.go b/bind/gen.go index 3685bbab..34a5c375 100644 --- a/bind/gen.go +++ b/bind/gen.go @@ -87,10 +87,27 @@ static inline void gopy_err_handle() { PyErr_Print(); } } +// _gopy_clear_go_tls clears the Go goroutine pointer from this thread's TLS. +// When multiple gopy extensions share a process, each has its own Go runtime +// but all runtimes use the same TLS slot for the current goroutine pointer +// (GS:0x30 on darwin/amd64, FS:-8 on linux/amd64). After one extension's +// init(), TLS is left pointing to that runtime's g0. If another extension's +// CGo entry-point reads TLS and finds a non-nil goroutine, it takes the fast +// path (no needm()) and runs with the wrong M/P/mcache -- corrupting the heap. +// Clearing the slot before each CGo entry forces needm() to run, which +// establishes the correct per-extension context (issue #370). +static void _gopy_clear_go_tls(void) { +#if defined(__x86_64__) && defined(__APPLE__) + __asm__ volatile("movq $0, %%%%gs:0x30" ::: "memory"); +#elif defined(__x86_64__) && defined(__linux__) + __asm__ volatile("movq $0, %%%%fs:-8" ::: "memory"); +#endif +} %[8]s */ import "C" import ( + "runtime" "github.com/go-python/gopy/gopyh" // handler %[6]s ) @@ -132,6 +149,15 @@ func NumHandles() int { return gopyh.NumHandles() } +// RunGC runs the Go garbage collector. gopy registers this as a Python +// gc.callbacks handler so it fires automatically after each Python GC cycle, +// keeping Go-heap objects freed via DecRef actually collected without any +// user intervention. +//export RunGC +func RunGC() { + runtime.GC() +} + // boolGoToPy converts a Go bool to python-compatible C.char func boolGoToPy(b bool) C.char { if b { @@ -259,6 +285,8 @@ mod.add_function('GoPyInit', None, []) mod.add_function('DecRef', None, [param('int64_t', 'handle')]) mod.add_function('IncRef', None, [param('int64_t', 'handle')]) mod.add_function('NumHandles', retval('int'), []) +mod.add_function('RunGC', None, []) +mod.add_function('_gopy_clear_go_tls', None, []) ` // appended to imports in py wrap preamble as key for adding at end @@ -281,8 +309,39 @@ except ImportError: 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). +if hasattr(sys, 'getdlopenflags'): + try: + import ctypes as _gopy_ctypes + _gopy_saved_flags = sys.getdlopenflags() + sys.setdlopenflags(_gopy_saved_flags & ~getattr(_gopy_ctypes, 'RTLD_GLOBAL', 0)) + except Exception: + _gopy_saved_flags = None +else: + _gopy_saved_flags = None %[6]s +if _gopy_saved_flags is not None: + sys.setdlopenflags(_gopy_saved_flags) os.chdir(cwd) +# Run Go's GC whenever Python's GC runs so that Go-heap objects whose handles +# were released via DecRef are promptly collected. Without this, Go memory +# can accumulate between Python gc.collect() calls because Python GC only +# frees the Python wrapper; the underlying Go allocation is not reclaimed +# until Go's own GC fires. +try: + import gc as _gopy_gc + def _gopy_gc_cb(phase, info): + if phase == 'stop': + _%[1]s.RunGC() + _gopy_gc.callbacks.append(_gopy_gc_cb) +except Exception: + pass # to use this code in your end-user python file, import it as follows: # from %[1]s import %[3]s diff --git a/bind/gen_func.go b/bind/gen_func.go index 8f2e606e..fc2ee15d 100644 --- a/bind/gen_func.go +++ b/bind/gen_func.go @@ -261,10 +261,8 @@ func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) { } } - // release GIL g.gofile.Printf("_saved_thread := C.PyEval_SaveThread()\n") if !rvIsErr && nres != 2 { - // reacquire GIL after return g.gofile.Printf("defer C.PyEval_RestoreThread(_saved_thread)\n") } @@ -338,6 +336,12 @@ 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) + // pywrap output mnm := fsym.ID() if isMethod { @@ -415,7 +419,6 @@ if __err != nil { if rvIsErr || nres == 2 { g.gofile.Printf("\n") - // reacquire GIL g.gofile.Printf("C.PyEval_RestoreThread(_saved_thread)\n") g.gofile.Printf("if __err != nil {\n") diff --git a/bind/symbols.go b/bind/symbols.go index 0ddc8c73..67bb6a5b 100644 --- a/bind/symbols.go +++ b/bind/symbols.go @@ -1083,24 +1083,32 @@ func (sym *symtab) addSignatureType(pkg *types.Package, obj types.Object, t type py2g := fmt.Sprintf("%s { ", nsig) + py2g += "_gstate := C.PyGILState_Ensure()\n" + // TODO: use strings.Builder if rets.Len() == 0 { - py2g += "if C.PyCallable_Check(_fun_arg) == 0 { return }\n" + py2g += "if C.PyCallable_Check(_fun_arg) == 0 {\n" + py2g += "C.PyGILState_Release(_gstate)\n" // Release GIL + py2g += "return\n" + py2g += "}\n" } else { zstr, err := sym.ZeroToGo(ret.Type(), rsym) if err != nil { return err } - py2g += fmt.Sprintf("if C.PyCallable_Check(_fun_arg) == 0 { return %s }\n", zstr) + py2g += "if C.PyCallable_Check(_fun_arg) == 0 {\n" + py2g += "C.PyGILState_Release(_gstate)\n" // Release GIL + py2g += fmt.Sprintf("return %s\n", zstr) + py2g += "}\n" } - py2g += "_gstate := C.PyGILState_Ensure()\n" + if nargs > 0 { bstr, err := sym.buildTuple(args, "_fcargs", "_fun_arg") if err != nil { return err } py2g += bstr + retstr - py2g += fmt.Sprintf("C.PyObject_CallObject(_fun_arg, _fcargs)\n") + py2g += "C.PyObject_CallObject(_fun_arg, _fcargs)\n" py2g += "C.gopy_decref(_fcargs)\n" } else { // TODO: methods not supported for no-args case -- requires self arg.. diff --git a/cmd_build.go b/cmd_build.go index 24b0191c..3def118a 100644 --- a/cmd_build.go +++ b/cmd_build.go @@ -181,30 +181,67 @@ func runBuild(mode bind.BuildMode, cfg *BuildCfg) error { // build the go shared library upfront to generate the header // needed by our generated cpython code - args := []string{"build", "-mod=mod", "-buildmode=c-shared"} + firstArgs := []string{"build", "-mod=mod", "-buildmode=c-shared"} if cfg.BuildTags != "" { - args = append(args, "-tags", cfg.BuildTags) + firstArgs = append(firstArgs, "-tags", cfg.BuildTags) } if !cfg.Symbols { // These flags will omit the various symbol tables, thereby // reducing the final size of the binary. From https://golang.org/cmd/link/ // -s Omit the symbol table and debug information // -w Omit the DWARF symbol table - args = append(args, "-ldflags=-s -w") + firstArgs = append(firstArgs, "-ldflags=-s -w") } - args = append(args, "-o", buildLib, ".") - fmt.Printf("go %v\n", strings.Join(args, " ")) - cmd = exec.Command("go", args...) + firstArgs = append(firstArgs, "-o", buildLib, ".") + fmt.Printf("go %v\n", strings.Join(firstArgs, " ")) + cmd = exec.Command("go", firstArgs...) cmdout, err = cmd.CombinedOutput() if err != nil { fmt.Printf("cmd had error: %v output:\n%v\n", err, string(cmdout)) return err } - // update the output name to the one with the ABI extension - args[len(args)-2] = modlib // we don't need this initial lib because we are going to relink os.Remove(buildLib) + // Build the final extension with symbol-visibility restriction so that + // Go runtime globals are not placed in the global dynamic-linker + // namespace. Two independently-loaded Go runtimes sharing those globals + // via RTLD_GLOBAL interposition corrupt each other's GC state (#370). + // This applies only to the second build, which is where PyInit__ + // exists and where the exported-symbols list is valid. + finalArgs := []string{"build", "-mod=mod", "-buildmode=c-shared"} + if cfg.BuildTags != "" { + finalArgs = append(finalArgs, "-tags", cfg.BuildTags) + } + var finalLdFlags []string + if !cfg.Symbols { + finalLdFlags = append(finalLdFlags, "-s", "-w") + } + switch runtime.GOOS { + case "darwin": + ef, ferr := os.CreateTemp("", "gopy-exports-*.txt") + if ferr == nil { + fmt.Fprintf(ef, "_PyInit__%s\n", cfg.Name) + ef.Close() + defer os.Remove(ef.Name()) + finalLdFlags = append(finalLdFlags, "-extldflags=-Wl,-exported_symbols_list,"+ef.Name()) + } + case "linux": + ef, ferr := os.CreateTemp("", "gopy-exports-*.map") + if ferr == nil { + fmt.Fprintf(ef, "{ global: PyInit__%s; local: *; };\n", cfg.Name) + ef.Close() + defer os.Remove(ef.Name()) + finalLdFlags = append(finalLdFlags, "-extldflags=-Wl,--version-script="+ef.Name()) + } + } + if len(finalLdFlags) > 0 { + finalArgs = append(finalArgs, "-ldflags="+strings.Join(finalLdFlags, " ")) + } + finalArgs = append(finalArgs, "-o", modlib, ".") + // args is still used below for the CGO env build; point it at finalArgs. + args := finalArgs + // generate c code fmt.Printf("%v build.py\n", cfg.VM) cmd = exec.Command(cfg.VM, "build.py") diff --git a/go.sum b/go.sum index 4b74f8cf..9f9cbc57 100644 --- a/go.sum +++ b/go.sum @@ -6,9 +6,13 @@ github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= golang.org/x/mod v0.22.0 h1:D4nJWe9zXqHOmWqj4VMOJhvzj7bEZg4wEYa759z1pH4= golang.org/x/mod v0.22.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= +golang.org/x/net v0.34.0/go.mod h1:di0qlW3YNM5oh6GqDGQr92MyTozJPmybPK4Ev/Gm31k= golang.org/x/sync v0.10.0 h1:3NQrjDixjgGwUOCaF8w2+VYHv0Ve/vGYSbdkTa98gmQ= golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/sys v0.29.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/telemetry v0.0.0-20240521205824-bda55230c457/go.mod h1:pRgIJT+bRLFKnoM1ldnzKoxTIn14Yxz928LQRYYgIN0= golang.org/x/tools v0.29.0 h1:Xx0h3TtM9rzQpQuR4dKLrdglAmCEN5Oi+P74JdhdzXE= golang.org/x/tools v0.29.0/go.mod h1:KMQVMRsVxU6nHCFXrBPhDB8XncLNLM0lIy/F14RP588= diff --git a/main_test.go b/main_test.go index c5f6c29e..1127698c 100644 --- a/main_test.go +++ b/main_test.go @@ -49,6 +49,7 @@ var ( "_examples/cstrings": []string{"py3"}, "_examples/pkgconflict": []string{"py3"}, "_examples/variadic": []string{"py3"}, + "_examples/gilstring": []string{"py3"}, } testEnvironment = os.Environ() @@ -316,7 +317,6 @@ OK } func TestBindSimple(t *testing.T) { - t.Skip("Skipping due to Go 1.21+ CGO issue (see https://github.com/go-python/gopy/issues/370)") // t.Parallel() path := "_examples/simple" testPkg(t, pkg{ @@ -546,7 +546,6 @@ OK } func TestBindCgoPackage(t *testing.T) { - t.Skip("Skipping due to Go 1.21+ CGO issue (see https://github.com/go-python/gopy/issues/370)") // t.Parallel() path := "_examples/cgo" testPkg(t, pkg{ @@ -774,7 +773,6 @@ func TestCStrings(t *testing.T) { lang: features[path], cmd: "build", extras: nil, - // todo: this test on mac leaks everything except String want: []byte(`gofnString leaked: False gofnStruct leaked: False gofnNestedStruct leaked: False @@ -785,6 +783,74 @@ OK }) } +// TestGilString is a regression test for the multi-runtime crash (issue #370). +// It replicates the exact reproduction from the issue report: two separately-built +// gopy extensions (gilstring and simple) loaded in the same Python process, with +// calls interleaved in a loop of 5000 iterations. +func TestGilString(t *testing.T) { + backends := []string{"py3"} + for _, be := range backends { + vm, ok := testBackends[be] + if !ok || vm == "" { + t.Logf("Skipped testing backend %s for TestGilString\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() + + gilDir := filepath.Join(workdir, "gilstring") + if err := os.MkdirAll(gilDir, 0700); err != nil { + t.Fatalf("could not create gilstring subdir: %v", err) + } + writeGoMod(t, cwd, gilDir) + if err := run([]string{"build", "-vm=" + vm, "-output=" + gilDir, "./_examples/gilstring"}); err != nil { + t.Fatalf("error building gilstring: %v", err) + } + bind.ResetPackages() + + simpleDir := filepath.Join(workdir, "simple") + if err := os.MkdirAll(simpleDir, 0700); err != nil { + t.Fatalf("could not create simple subdir: %v", err) + } + writeGoMod(t, cwd, simpleDir) + if err := run([]string{"build", "-vm=" + vm, "-output=" + simpleDir, "./_examples/simple"}); err != nil { + t.Fatalf("error building simple: %v", err) + } + + tstDst := filepath.Join(workdir, "test.py") + if err := copyCmd(filepath.Join(cwd, "_examples/gilstring/test.py"), tstDst); err != nil { + t.Fatalf("error copying test.py: %v", err) + } + + env := make([]string, len(testEnvironment)) + copy(env, testEnvironment) + env = append(env, fmt.Sprintf("PYTHONPATH=%s", workdir)) + + cmd := exec.Command(vm, "./test.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"