Skip to content

Commit ef7b4bc

Browse files
b-longb-long
committed
WIP
1 parent 462a29d commit ef7b4bc

7 files changed

Lines changed: 51 additions & 14 deletions

File tree

‎_examples/cgo/cgo.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ package cgo
99
//#include <string.h>
1010
//#include <stdlib.h>
1111
//const char* cpkg_sprintf(const char *str) {
12-
// char *o = (char*)malloc(strlen(str));
12+
// char *o = (char*)malloc(strlen(str) + 1);
1313
// sprintf(o, "%s", str);
1414
// return o;
1515
//}

‎bind/gen_func.go‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,18 @@ func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) {
263263

264264
// release GIL
265265
g.gofile.Printf("_saved_thread := C.PyEval_SaveThread()\n")
266-
if !rvIsErr && nres != 2 {
266+
// Use defer only when there is no go2py return conversion that calls
267+
// Python C API (which requires the GIL to already be reacquired).
268+
// For go2py returns (excluding handle types) and error/multi-return cases,
269+
// we restore explicitly. Handle types use handleFromPtr which is pure Go
270+
// and doesn't need the GIL, so they can keep using defer.
271+
nresForDefer := nres
272+
// hasRetCvt fires when go2py != "" and NOT (hasHandle && !isPtrOrIface).
273+
// Suppress defer in exactly those cases so the explicit restore below is the only one.
274+
if nres > 0 && res[0].sym.go2py != "" && !(res[0].sym.hasHandle() && !res[0].sym.isPtrOrIface()) {
275+
nresForDefer = 2 // treat like nres==2: no defer, explicit restore
276+
}
277+
if !rvIsErr && nresForDefer != 2 {
267278
// reacquire GIL after return
268279
g.gofile.Printf("defer C.PyEval_RestoreThread(_saved_thread)\n")
269280
}
@@ -357,8 +368,8 @@ if __err != nil {
357368
g.pywrap.Printf("_%s.%s(", pkgname, mnm)
358369
}
359370

360-
hasRetCvt := false
361371
hasAddrOfTmp := false
372+
hasRetCvt := false
362373
if nres > 0 {
363374
ret := res[0]
364375
switch {
@@ -370,8 +381,10 @@ if __err != nil {
370381
hasAddrOfTmp = true
371382
g.gofile.Printf("cret := ")
372383
case ret.sym.go2py != "":
384+
// Assign to cret first; we'll restore the GIL before calling go2py
385+
// so that Python C API functions inside go2py run with the GIL held.
373386
hasRetCvt = true
374-
g.gofile.Printf("return %s(", ret.sym.go2py)
387+
g.gofile.Printf("cret := ")
375388
default:
376389
g.gofile.Printf("return ")
377390
}
@@ -394,11 +407,6 @@ if __err != nil {
394407
} else {
395408
funCall = fmt.Sprintf("%s(%s)", fsym.GoFmt(), strings.Join(callArgs, ", "))
396409
}
397-
if hasRetCvt {
398-
ret := res[0]
399-
funCall += fmt.Sprintf(")%s", ret.sym.go2pyParenEx)
400-
}
401-
402410
if nres == 0 {
403411
g.gofile.Printf("if boolPyToGo(goRun) {\n")
404412
g.gofile.Indent()
@@ -413,6 +421,13 @@ if __err != nil {
413421
g.gofile.Printf("%s\n", funCall)
414422
}
415423

424+
if hasRetCvt {
425+
// Restore GIL before calling go2py, which may call Python C API.
426+
ret := res[0]
427+
g.gofile.Printf("C.PyEval_RestoreThread(_saved_thread)\n")
428+
g.gofile.Printf("return %s(cret)%s\n", ret.sym.go2py, ret.sym.go2pyParenEx)
429+
}
430+
416431
if rvIsErr || nres == 2 {
417432
g.gofile.Printf("\n")
418433
// reacquire GIL

‎bind/gen_map.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,11 @@ otherwise parameter is a python list that we copy from
317317
g.gofile.Outdent()
318318
g.gofile.Printf("}\n\n")
319319

320-
g.pybuild.Printf("mod.add_function('%s_elem', retval('%s'), [param('%s', 'handle'), param('%s', '_ky')])\n", slNm, esym.cpyname, PyHandle, ksym.cpyname)
320+
if esym.go2py == "C.CString" {
321+
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)
322+
} else {
323+
g.pybuild.Printf("mod.add_function('%s_elem', retval('%s'), [param('%s', 'handle'), param('%s', '_ky')])\n", slNm, esym.cpyname, PyHandle, ksym.cpyname)
324+
}
321325

322326
// contains
323327
g.gofile.Printf("//export %s_contains\n", slNm)

‎bind/gen_slice.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,11 @@ otherwise parameter is a python list that we copy from
345345
caller_owns_ret = ", caller_owns_return=True"
346346
transfer_ownership = ", transfer_ownership=False"
347347
}
348-
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)
348+
if esym.go2py == "C.CString" {
349+
g.pybuild.Printf("add_checked_string_function(mod, '%s_elem', retval('%s'), [param('%s', 'handle'), param('int', 'idx')])\n", slNm, esym.cpyname, PyHandle)
350+
} else {
351+
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)
352+
}
349353

350354
if slc.isSlice() {
351355
g.gofile.Printf("//export %s_subslice\n", slNm)

‎bind/gen_struct.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,11 @@ func (g *pyGen) genStructMemberGetter(s *Struct, i int, f types.Object) {
227227
g.gofile.Outdent()
228228
g.gofile.Printf("}\n\n")
229229

230-
g.pybuild.Printf("mod.add_function('%s', retval('%s'), [param('%s', 'handle')])\n", cgoFn, ret.cpyname, PyHandle)
230+
if ret.go2py == "C.CString" {
231+
g.pybuild.Printf("add_checked_string_function(mod, '%s', retval('%s'), [param('%s', 'handle')])\n", cgoFn, ret.cpyname, PyHandle)
232+
} else {
233+
g.pybuild.Printf("mod.add_function('%s', retval('%s'), [param('%s', 'handle')])\n", cgoFn, ret.cpyname, PyHandle)
234+
}
231235
}
232236

233237
func (g *pyGen) genStructMemberSetter(s *Struct, i int, f types.Object) {

‎go.sum‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,13 @@ github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
66
github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
77
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
88
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
9+
github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
910
golang.org/x/mod v0.22.0 h1:D4nJWe9zXqHOmWqj4VMOJhvzj7bEZg4wEYa759z1pH4=
1011
golang.org/x/mod v0.22.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY=
12+
golang.org/x/net v0.34.0/go.mod h1:di0qlW3YNM5oh6GqDGQr92MyTozJPmybPK4Ev/Gm31k=
1113
golang.org/x/sync v0.10.0 h1:3NQrjDixjgGwUOCaF8w2+VYHv0Ve/vGYSbdkTa98gmQ=
1214
golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
15+
golang.org/x/sys v0.29.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
16+
golang.org/x/telemetry v0.0.0-20240521205824-bda55230c457/go.mod h1:pRgIJT+bRLFKnoM1ldnzKoxTIn14Yxz928LQRYYgIN0=
1317
golang.org/x/tools v0.29.0 h1:Xx0h3TtM9rzQpQuR4dKLrdglAmCEN5Oi+P74JdhdzXE=
1418
golang.org/x/tools v0.29.0/go.mod h1:KMQVMRsVxU6nHCFXrBPhDB8XncLNLM0lIy/F14RP588=

‎main_test.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"os/exec"
1313
"path/filepath"
1414
"reflect"
15+
"runtime"
1516
"sort"
1617
"strings"
1718
"testing"
@@ -316,7 +317,6 @@ OK
316317
}
317318

318319
func TestBindSimple(t *testing.T) {
319-
t.Skip("Skipping due to Go 1.21+ CGO issue (see https://github.com/go-python/gopy/issues/370)")
320320
// t.Parallel()
321321
path := "_examples/simple"
322322
testPkg(t, pkg{
@@ -546,7 +546,13 @@ OK
546546
}
547547

548548
func TestBindCgoPackage(t *testing.T) {
549-
t.Skip("Skipping due to Go 1.21+ CGO issue (see https://github.com/go-python/gopy/issues/370)")
549+
// Skip for some Go versions due to CGO issue
550+
// See: https://github.com/go-python/gopy/issues/370
551+
if strings.HasPrefix(runtime.Version(), "go1.21") ||
552+
strings.HasPrefix(runtime.Version(), "go1.23") ||
553+
strings.HasPrefix(runtime.Version(), "go1.24") {
554+
t.Skip("Skipping due to CGO issue (see https://github.com/go-python/gopy/issues/370)")
555+
}
550556
// t.Parallel()
551557
path := "_examples/cgo"
552558
testPkg(t, pkg{

0 commit comments

Comments
 (0)