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/bind/gen_func.go b/bind/gen_func.go index 8f2e606e..75aacfd7 100644 --- a/bind/gen_func.go +++ b/bind/gen_func.go @@ -263,7 +263,18 @@ func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) { // release GIL g.gofile.Printf("_saved_thread := C.PyEval_SaveThread()\n") - if !rvIsErr && nres != 2 { + // Use defer only when there is no go2py return conversion that calls + // Python C API (which requires the GIL to already be reacquired). + // For go2py returns (excluding handle types) and error/multi-return cases, + // we restore explicitly. Handle types use handleFromPtr which is pure Go + // and doesn't need the GIL, so they can keep using defer. + nresForDefer := nres + // hasRetCvt fires when go2py != "" and NOT (hasHandle && !isPtrOrIface). + // Suppress defer in exactly those cases so the explicit restore below is the only one. + if nres > 0 && res[0].sym.go2py != "" && !(res[0].sym.hasHandle() && !res[0].sym.isPtrOrIface()) { + nresForDefer = 2 // treat like nres==2: no defer, explicit restore + } + if !rvIsErr && nresForDefer != 2 { // reacquire GIL after return g.gofile.Printf("defer C.PyEval_RestoreThread(_saved_thread)\n") } @@ -357,8 +368,8 @@ if __err != nil { g.pywrap.Printf("_%s.%s(", pkgname, mnm) } - hasRetCvt := false hasAddrOfTmp := false + hasRetCvt := false if nres > 0 { ret := res[0] switch { @@ -370,8 +381,10 @@ if __err != nil { hasAddrOfTmp = true g.gofile.Printf("cret := ") case ret.sym.go2py != "": + // Assign to cret first; we'll restore the GIL before calling go2py + // so that Python C API functions inside go2py run with the GIL held. hasRetCvt = true - g.gofile.Printf("return %s(", ret.sym.go2py) + g.gofile.Printf("cret := ") default: g.gofile.Printf("return ") } @@ -394,11 +407,6 @@ if __err != nil { } else { funCall = fmt.Sprintf("%s(%s)", fsym.GoFmt(), strings.Join(callArgs, ", ")) } - if hasRetCvt { - ret := res[0] - funCall += fmt.Sprintf(")%s", ret.sym.go2pyParenEx) - } - if nres == 0 { g.gofile.Printf("if boolPyToGo(goRun) {\n") g.gofile.Indent() @@ -413,6 +421,13 @@ if __err != nil { g.gofile.Printf("%s\n", funCall) } + if hasRetCvt { + // Restore GIL before calling go2py, which may call Python C API. + ret := res[0] + g.gofile.Printf("C.PyEval_RestoreThread(_saved_thread)\n") + g.gofile.Printf("return %s(cret)%s\n", ret.sym.go2py, ret.sym.go2pyParenEx) + } + if rvIsErr || nres == 2 { g.gofile.Printf("\n") // reacquire GIL diff --git a/bind/gen_map.go b/bind/gen_map.go index f65111fa..a7fef2b3 100644 --- a/bind/gen_map.go +++ b/bind/gen_map.go @@ -317,7 +317,11 @@ otherwise parameter is a python list that we copy from g.gofile.Outdent() g.gofile.Printf("}\n\n") - g.pybuild.Printf("mod.add_function('%s_elem', retval('%s'), [param('%s', 'handle'), param('%s', '_ky')])\n", slNm, esym.cpyname, PyHandle, ksym.cpyname) + if esym.go2py == "C.CString" { + g.pybuild.Printf("add_checked_string_function(mod, '%s_elem', retval('%s'), [param('%s', 'handle'), param('%s', '_ky')])\n", slNm, esym.cpyname, PyHandle, ksym.cpyname) + } else { + g.pybuild.Printf("mod.add_function('%s_elem', retval('%s'), [param('%s', 'handle'), param('%s', '_ky')])\n", slNm, esym.cpyname, PyHandle, ksym.cpyname) + } // contains g.gofile.Printf("//export %s_contains\n", slNm) diff --git a/bind/gen_slice.go b/bind/gen_slice.go index 8df9dedb..9233245f 100644 --- a/bind/gen_slice.go +++ b/bind/gen_slice.go @@ -345,7 +345,11 @@ otherwise parameter is a python list that we copy from caller_owns_ret = ", caller_owns_return=True" transfer_ownership = ", transfer_ownership=False" } - g.pybuild.Printf("mod.add_function('%s_elem', retval('%s'%s), [param('%s', 'handle'), param('int', 'idx')])\n", slNm, esym.cpyname, caller_owns_ret, PyHandle) + if esym.go2py == "C.CString" { + g.pybuild.Printf("add_checked_string_function(mod, '%s_elem', retval('%s'), [param('%s', 'handle'), param('int', 'idx')])\n", slNm, esym.cpyname, PyHandle) + } else { + g.pybuild.Printf("mod.add_function('%s_elem', retval('%s'%s), [param('%s', 'handle'), param('int', 'idx')])\n", slNm, esym.cpyname, caller_owns_ret, PyHandle) + } if slc.isSlice() { g.gofile.Printf("//export %s_subslice\n", slNm) diff --git a/bind/gen_struct.go b/bind/gen_struct.go index b50ddfc5..9aab31e4 100644 --- a/bind/gen_struct.go +++ b/bind/gen_struct.go @@ -227,7 +227,11 @@ func (g *pyGen) genStructMemberGetter(s *Struct, i int, f types.Object) { g.gofile.Outdent() g.gofile.Printf("}\n\n") - g.pybuild.Printf("mod.add_function('%s', retval('%s'), [param('%s', 'handle')])\n", cgoFn, ret.cpyname, PyHandle) + if ret.go2py == "C.CString" { + g.pybuild.Printf("add_checked_string_function(mod, '%s', retval('%s'), [param('%s', 'handle')])\n", cgoFn, ret.cpyname, PyHandle) + } else { + g.pybuild.Printf("mod.add_function('%s', retval('%s'), [param('%s', 'handle')])\n", cgoFn, ret.cpyname, PyHandle) + } } func (g *pyGen) genStructMemberSetter(s *Struct, i int, f types.Object) { 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..896ad120 100644 --- a/main_test.go +++ b/main_test.go @@ -12,6 +12,7 @@ import ( "os/exec" "path/filepath" "reflect" + "runtime" "sort" "strings" "testing" @@ -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,11 @@ 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)") + // Skip for some Go versions due to CGO issue + // See: https://github.com/go-python/gopy/issues/370 + if strings.HasPrefix(runtime.Version(), "go1.23") { + t.Skip("Skipping due to CGO issue (see https://github.com/go-python/gopy/issues/370)") + } // t.Parallel() path := "_examples/cgo" testPkg(t, pkg{