bind: replace patch-leak fix solution with a built-in PyBindGen code gen

Using the same approach to add exception checking to function calls, this commit replaces the entire patch-leak tool with built-in PyBindGen logic that cleans up the string allocations

Author: justinfx

README.md

@@ -179,7 +179,7 @@ package hi exposes a few Go functions to be wrapped and used from Python.
 ```sh
 $ gopy build -output=out -vm=`which python3` github.com/go-python/gopy/_examples/hi
 $ ls out
-Makefile  __init__.py  __pycache__/  _hi.so*  build.py  go.py  hi.c  hi.go  hi.py  hi_go.h  hi_go.so  patch-leaks.go
+Makefile  __init__.py  __pycache__/  _hi.so*  build.py  go.py  hi.c  hi.go  hi.py  hi_go.h  hi_go.so
 ```
 
 

bind/gen.go

@@ -210,23 +210,37 @@ class CheckedFunction(Function):
     def __init__(self, *a, **kw):
         super(CheckedFunction, self).__init__(*a, **kw)
         self._failure_expression = kw.get('failure_expression', '')
+        self._failure_cleanup = kw.get('failure_cleanup', '')
 
     def set_failure_expression(self, expr):
         self._failure_expression = expr
 
+    def set_failure_cleanup(self, expr):
+        self._failure_cleanup = expr
+
     def generate_call(self):
         super(CheckedFunction, self).generate_call()
         check = "PyErr_Occurred()"
         if self._failure_expression:
             check = "{} && {}".format(self._failure_expression, check)
-        self.before_call.write_error_check(check)
+        failure_cleanup = self._failure_cleanup or None
+        self.before_call.write_error_check(check, failure_cleanup)
 
 def add_checked_function(mod, name, retval, params, failure_expression='', *a, **kw):
     fn = CheckedFunction(name, retval, params, *a, **kw)
     fn.set_failure_expression(failure_expression)
     mod._add_function_obj(fn)
     return fn
 
+def add_checked_string_function(mod, name, retval, params, failure_expression='', *a, **kw):
+    fn = CheckedFunction(name, retval, params, *a, **kw)
+    cleanup = 'free(retval);'
+    fn.set_failure_cleanup(cleanup)
+    fn.after_call.add_cleanup_code(cleanup)
+    fn.set_failure_expression(failure_expression)
+    mod._add_function_obj(fn)
+    return fn
+
 mod = Module('_%[1]s')
 mod.add_include('"%[1]s_go.h"')
 mod.add_function('GoPyInit', None, [])
@@ -351,8 +365,6 @@ build:
 	# use pybindgen to build the %[1]s.c file which are the CPython wrappers to cgo wrappers..
 	# note: pip install pybindgen to get pybindgen if this fails
 	$(PYTHON) build.py
-	# patch storage leaks in pybindgen output
-	go run patch-leaks.go %[1]s.c
 	# build the _%[1]s$(LIBEXT) library that contains the cgo and CPython wrappers
 	# generated %[1]s.py python wrapper imports this c-code package
 	$(GCC) %[1]s.c %[6]s %[1]s_go$(LIBEXT) -o _%[1]s$(LIBEXT) $(CFLAGS) $(LDFLAGS) -fPIC --shared -w
@@ -394,91 +406,11 @@ build:
 	# use pybindgen to build the %[1]s.c file which are the CPython wrappers to cgo wrappers..
 	# note: pip install pybindgen to get pybindgen if this fails
 	$(PYTHON) build.py
-	# patch storage leaks in pybindgen output
-	go run patch-leaks.go %[1]s.c
 	# build the executable
 	- rm %[1]s_go$(LIBEXT)
 	$(GOBUILD) -o py%[1]s
 	
 `
-
-	patchLeaksPreamble = `// patch-leaks.go for post-processing %[1]s.
-// File is generated by gopy. Do not edit.
-// %[2]s
-// +build ignore
-
-package main
-
-import (
-	"bufio"
-	"bytes"
-	"io/ioutil"
-	"log"
-	"os"
-	"strings"
-)
-
-const (
-	cStringLine = "    py_retval = Py_BuildValue((char *) \"s\", retval);"
-)
-var cstringFunctions = []string{
-`
-
-	patchLeaksPostamble = `
-}
-
-func isCString(line string, names []string) bool {
-	for _, cfn := range names {
-		if strings.HasPrefix(line, cfn) {
-			return true
-		}
-	}
-	return false
-}
-
-func patchCString(line string, out *bytes.Buffer) bool {
-	out.WriteString(line)
-	out.Write([]byte{'\n'})
-	switch line {
-	case "}":
-		return false
-	case cStringLine:
-		out.WriteString("    free(retval);\n")
-		return false
-	}
-	return true
-}
-
-func main() {
-	file := os.Args[1]
-	buf, err := ioutil.ReadFile(file)
-	if err != nil {
-		log.Fatal(err)
-	}
-	sc := bufio.NewScanner(bytes.NewBuffer(buf))
-	obuf := &bytes.Buffer{}
-	var cstring bool
-	for sc.Scan() {
-		line := sc.Text()
-		if cstring {
-			cstring = patchCString(line, obuf)
-			continue
-		}
-		cstring = isCString(line, cstringFunctions)
-		obuf.WriteString(line)
-		obuf.Write([]byte{'\n'})
-	}
-
-	if err := sc.Err(); err != nil {
-		log.Fatal(err)
-	}
-
-	err = ioutil.WriteFile(file, obuf.Bytes(), 0644)
-	if err != nil {
-		log.Fatal(err)
-	}
-}
- `
 )
 
 // thePyGen is the current pyGen which is needed in symbols to lookup
@@ -570,7 +502,6 @@ func (g *pyGen) genPre() {
 		g.makefile = &printer{buf: new(bytes.Buffer), indentEach: []byte("\t")}
 	}
 	g.genGoPreamble()
-	g.genLeaksPreamble()
 	g.genPyBuildPreamble()
 	if !NoMake {
 		g.genMakefile()
@@ -593,9 +524,7 @@ func (g *pyGen) genPrintOut(outfn string, pr *printer) {
 func (g *pyGen) genOut() {
 	g.pybuild.Printf("\nmod.generate(open('%v.c', 'w'))\n\n", g.cfg.Name)
 	g.gofile.Printf("\n\n")
-	g.genLeaksPostamble()
 	g.genPrintOut(g.cfg.Name+".go", g.gofile)
-	g.genPrintOut("patch-leaks.go", g.leakfile)
 	g.genPrintOut("build.py", g.pybuild)
 	if !NoMake {
 		g.makefile.Printf("\n\n")
@@ -669,14 +598,6 @@ func (g *pyGen) genGoPreamble() {
 	g.gofile.Printf("\n// --- generated code for package: %[1]s below: ---\n\n", g.cfg.Name)
 }
 
-func (g *pyGen) genLeaksPreamble() {
-	g.leakfile.Printf(patchLeaksPreamble, g.cfg.Name, g.cfg.Cmd)
-}
-
-func (g *pyGen) genLeaksPostamble() {
-	g.leakfile.Printf(patchLeaksPostamble)
-}
-
 func (g *pyGen) genPyBuildPreamble() {
 	g.pybuild.Printf(PyBuildPreamble, g.cfg.Name, g.cfg.Cmd)
 }

bind/gen_func.go

@@ -10,51 +10,10 @@ import (
 	"strings"
 )
 
-// genPatchLeakFuncName outputs the name of either the function or the class
-// 'getter' represented by its arguments that need to have a memory
-// leak induced by the interaction of pybindgen and gopy.
-// These leaks occur because the cgo bindings use C.CString
-// to allocate a string on the C heap, as in:
-//
-//	return C.CString(cstrings.StringValue(C.GoString(s), int(n)))
-//
-// but the calling pybindgen code then takes a copy of this
-// newly allocated string indirectly by calling Py_BuildValue (see below)
-// without freeing the copy returned by the function or getter method.
-// Arguably pybindgen should provide a means of directly requesting
-// this behaviour (simularly to the transfer_ownership options it provides),
-// but as of now there is no means of doing so. As a work around the
-// output of pybindgen is modified to insert the free in the appropriate
-// functions identified by genPatchLeakFuncName.
-//
-// The patch-leaks.go (see gen.go) file patches these code fragments
-// to insert free(retval) immediately after the call to PyBuildValue.
-// The patch-leaks.go generated file will perform this patch for
-// functions whose names are embedded in it by genPatchLeakFuncName.
-//
-//  .....
-//  retval = cstrings_StringValue(s, n);
-//  py_retval = Py_BuildValue((char *) "s", retval);
-//  .....
-//
-// This patching is required for top level functions that return
-// strings directly and also for getters that return strings that
-// are embedded in maps, structs etc.
-// NOTE that it's ok to output the same function name multiple times (eg.
-// for a shared structs).
-func (g *pyGen) genPatchLeakFuncName(ret *Var, fsym *Func) {
-	if ret == nil || fsym == nil || ret.GoType() == nil {
-		return
-	}
-	g.recurse(ret.GoType(), "_wrap__"+fsym.pkg.Name()+"_", fsym.ID())
-}
-
 func (g *pyGen) recurse(gotype types.Type, prefix, name string) {
 	switch t := gotype.(type) {
 	case *types.Basic:
-		if t.Kind() == types.String {
-			g.leakfile.Printf("    %q,\n", prefix+name)
-		}
+		// pass
 	case *types.Map:
 		g.recurse(t.Elem(), prefix, "Map_"+t.Key().String()+"_string_elem")
 	case *types.Named:
@@ -149,21 +108,38 @@ func (g *pyGen) genFuncSig(sym *symbol, fsym *Func) bool {
 		wpArgs = append(wpArgs, "goRun=False")
 	}
 
+	// When building the pybindgen builder code, we start with
+	// a function that adds function calls with exception checking.
+	// But given specific return types, we may want to add more
+	// behavior to the wrapped function code gen.
+	addFuncName := "add_checked_function"
+	if len(res) > 0 {
+		ret := res[0]
+		switch t := ret.GoType().(type) {
+		case *types.Basic:
+			// string return types need special memory leak patches
+			// to free the allocated char*
+			if t.Kind() == types.String {
+				addFuncName = "add_checked_string_function"
+			}
+		}
+	}
+
 	switch {
 	case isMethod:
 		mnm := sym.id + "_" + fsym.GoName()
 
 		g.gofile.Printf("\n//export %s\n", mnm)
 		g.gofile.Printf("func %s(", mnm)
 
-		g.pybuild.Printf("add_checked_function(mod, '%s', ", mnm)
+		g.pybuild.Printf("%s(mod, '%s', ", addFuncName, mnm)
 
 		g.pywrap.Printf("def %s(", gname)
 	default:
 		g.gofile.Printf("\n//export %s\n", fsym.ID())
 		g.gofile.Printf("func %s(", fsym.ID())
 
-		g.pybuild.Printf("add_checked_function(mod, '%s', ", fsym.ID())
+		g.pybuild.Printf("%s(mod, '%s', ", addFuncName, fsym.ID())
 
 		g.pywrap.Printf("def %s(", gname)
 	}
@@ -180,8 +156,6 @@ func (g *pyGen) genFuncSig(sym *symbol, fsym *Func) bool {
 			))
 		}
 
-		g.genPatchLeakFuncName(ret, fsym)
-
 		if sret.cpyname == "PyObject*" {
 			g.pybuild.Printf("retval('%s', caller_owns_return=True)", sret.cpyname)
 		} else {

cmd_build.go

@@ -84,17 +84,6 @@ func gopyRunCmdBuild(cmdr *commander.Command, args []string) error {
 	return runBuild("build", cfg)
 }
 
-func patchLeaks(cfile string) error {
-	fmt.Printf("go run patch-leaks.go %v # patch memory leaks in pybindgen output\n", cfile)
-	cmd := exec.Command("go", "run", "patch-leaks.go", cfile)
-	cmdout, err := cmd.CombinedOutput()
-	if err != nil {
-		fmt.Printf("cmd had error: %v  output:\n%v\n", err, string(cmdout))
-		return err
-	}
-	return nil
-}
-
 // runBuild calls genPkg and then executes commands to build the resulting files
 // exe = executable mode to build an executable instead of a library
 // mode = gen, build, pkg, exe
@@ -155,10 +144,6 @@ func runBuild(mode bind.BuildMode, cfg *BuildCfg) error {
 			return err
 		}
 
-		if err := patchLeaks(cfg.Name + ".c"); err != nil {
-			return err
-		}
-
 		err = os.Remove(cfg.Name + "_go" + libExt)
 
 		fmt.Printf("go build -o py%s\n", cfg.Name)
@@ -212,11 +197,6 @@ func runBuild(mode bind.BuildMode, cfg *BuildCfg) error {
 			return err
 		}
 
-		// patch c code
-		if err := patchLeaks(cfg.Name + ".c"); err != nil {
-			return err
-		}
-
 		cflags := strings.Fields(strings.TrimSpace(pycfg.CFlags))
 		cflags = append(cflags, "-fPIC", "-Ofast")
 		if include, exists := os.LookupEnv("GOPY_INCLUDE"); exists {