code review 9835047: cmd/cgo: Add support for C function pointers

cmd/cgo: Add support for C function pointers

* Add a new kind of Name, "fpvar" which stands for function pointer variable
* When walking the AST, find functions used as expressions and create a new Name object for them
* Track functions which are only used in expr contexts, and avoid generating bridge code for them

[Hierro, 2013-05-31 11:09:47]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[Hierro, 2013-05-31 11:10:42]

Just wanted to add that all the tests in misc/cgo/tests continue to pass
correctly after applying this patch.

[minux1, 2013-05-31 11:40:38]

On Fri, May 31, 2013 at 7:10 PM, <alberto.garcia.hierro@gmail.com> wrote:

> Just wanted to add that all the tests in misc/cgo/tests continue to pass
> correctly after applying this patch.
>
You should add new tests to misc/cgo/test for the new feature.

[albert.strasheim, 2013-05-31 12:09:00]

Fixes issue 3364.

[albert.strasheim, 2013-05-31 12:31:52]

FWIW, I was hoping this code might work now:

package jvm

//#cgo CFLAGS: -I/usr/java/jdk1.7.0_21/include
//#cgo linux CFLAGS: -I/usr/java/jdk1.7.0_21/include/linux
//#cgo linux LDFLAGS: -ljvm -L/usr/java/jdk1.7.0_21/jre/lib/amd64/server
-Wl,-rpath,/usr/java/jdk1.7.0_21/jre/lib/amd64/server
//#include <jni.h>
import "C"
import "unsafe"

func jvm() {
	var vmargs C.JavaVMInitArgs
	vmargs.version = 0x00010001
	C.JNI_GetDefaultJavaVMInitArgs(unsafe.Pointer(&vmargs))
	var jvm *C.JavaVM
	var env unsafe.Pointer
	C.JNI_CreateJavaVM(&jvm, &env, unsafe.Pointer(&vmargs))
	(*jvm).DestroyJavaVM(jvm)
}

but I still get:

cannot call non-function (*jvm).DestroyJavaVM (type *[0]byte)

with your patch.

[Hierro, 2013-05-31 12:56:21]

On 2013/05/31 12:31:52, albert.strasheim wrote:
> FWIW, I was hoping this code might work now:
> 
> package jvm
> 
> //#cgo CFLAGS: -I/usr/java/jdk1.7.0_21/include
> //#cgo linux CFLAGS: -I/usr/java/jdk1.7.0_21/include/linux
> //#cgo linux LDFLAGS: -ljvm -L/usr/java/jdk1.7.0_21/jre/lib/amd64/server
> -Wl,-rpath,/usr/java/jdk1.7.0_21/jre/lib/amd64/server
> //#include <jni.h>
> import "C"
> import "unsafe"
> 
> func jvm() {
> 	var vmargs C.JavaVMInitArgs
> 	vmargs.version = 0x00010001
> 	C.JNI_GetDefaultJavaVMInitArgs(unsafe.Pointer(&vmargs))
> 	var jvm *C.JavaVM
> 	var env unsafe.Pointer
> 	C.JNI_CreateJavaVM(&jvm, &env, unsafe.Pointer(&vmargs))
> 	(*jvm).DestroyJavaVM(jvm)
> }
> 
> but I still get:
> 
> cannot call non-function (*jvm).DestroyJavaVM (type *[0]byte)
> 
> with your patch.

Hi Albert,

This patch lets you pass C function pointers around and store them in variables,
but it doesn't allow calling them from Go code. You still need a C function
which expects a function pointer to call them.

e.g. (from the graphicsmagick binding I'm writing)


fiam@ubuntu:~/gocode/src/magick$ cat bridge.h 
#ifndef BRIDGE_H
#define BRIDGE_H

typedef Image* (*ImageFunc) (const Image *, ExceptionInfo *);
typedef Image* (*ImageRectFunc) (const Image*, const RectangleInfo *,
ExceptionInfo *);

Image * bridge_image_func(ImageFunc f, const Image *im, ExceptionInfo *ex);
Image * bridge_image_rect_func(ImageRectFunc f, const Image *im, const
RectangleInfo *r, ExceptionInfo *ex);

#endif


fiam@ubuntu:~/gocode/src/magick$ cat bridge.c 
#include <magick/api.h>
#include "bridge.h"

Image *
bridge_image_func(ImageFunc f, const Image *im, ExceptionInfo *ex)
{
    return f(im, ex);
}

Image *
bridge_image_rect_func(ImageRectFunc f, const Image *im, const RectangleInfo *r,
ExceptionInfo *ex)
{
    return f(im, r, ex);
}

fiam@ubuntu:~/gocode/src/magick$ cat bridge.go // Ignore the im.Apply() stuff,
that's for transparently handling animations, but it's not relevant for this
example
package magick

// #include <magick/api.h>
// #include "bridge.h"
import "C"

func (im *Image) applyRectFunc(r Rect, what string, f C.ImageRectFunc) (*Image,
error) {
	rect := r.rectangleInfo()
	return im.Apply(func(i *Image) (*Image, error) {
		var ex C.ExceptionInfo
		C.GetExceptionInfo(&ex)
		defer C.DestroyExceptionInfo(&ex)
		res := C.bridge_image_rect_func(f, i.image, rect, &ex)
		return checkImage(res, nil, &ex, what)
	})
}

func (im *Image) applyFunc(what string, f C.ImageFunc) (*Image, error) {
	return im.Apply(func(i *Image) (*Image, error) {
		var ex C.ExceptionInfo
		C.GetExceptionInfo(&ex)
		defer C.DestroyExceptionInfo(&ex)
		res := C.bridge_image_func(f, i.image, &ex)
		return checkImage(res, nil, &ex, what)
	})
}

func (im *Image) fn(what string, f C.ImageFunc) (*Image, error) {
	var ex C.ExceptionInfo
	C.GetExceptionInfo(&ex)
	defer C.DestroyExceptionInfo(&ex)
	res := C.bridge_image_func(f, im.image, &ex)
	return checkImage(res, nil, &ex, what)
}


Then, in Go code I can do stuff like:


func (im *Image) Chop(r Rect) (*Image, error) {
	return im.applyRectFunc(r, "chopping", C.ImageRectFunc(C.ChopImage))
}

func (im *Image) Coalesce() (*Image, error) {
	return im.fn("coalescing", C.ImageFunc(C.CoalesceImages))
}

func (im *Image) Crop(r Rect) (*Image, error) {
	return im.applyRectFunc(r, "cropping", C.ImageRectFunc(C.CropImage))
}

func (im *Image) Deconstruct() (*Image, error) {
	return im.fn("deconstructing", C.ImageFunc(C.DeconstructImages))
}

func (im *Image) Extent(r Rect) (*Image, error) {
	return im.applyRectFunc(r, "extenting", C.ImageRectFunc(C.ExtentImage))
}

func (im *Image) Flatten() (*Image, error) {
	return im.fn("flattening", C.ImageFunc(C.FlattenImages))
}

func (im *Image) Flip() (*Image, error) {
	return im.applyFunc("flipping", C.ImageFunc(C.FlipImage))
}


Of course, if this patch gets accepted I'd really like to add support for
calling C function pointers from Go code, which would also eliminate the need to
generate bridge functions for every C function called from Go (take a look at
*.cgo2.c files in $WORK directory after go build -work in a package that uses
cgo).

Regards,
Alberto

[Hierro, 2013-07-18 21:46:25]

Ping

[rsc, 2013-08-01 18:15:23]

This looks good, and it's great that misc/cgo/test continues to pass, but it
already passes without this CL. Put please add a test of the new functionality.

[Hierro, 2013-08-02 12:22:45]

On 2013/08/01 18:15:23, rsc wrote:
> This looks good, and it's great that misc/cgo/test continues to pass, but it
> already passes without this CL. Put please add a test of the new
functionality.

I've just added a test. PTAL.

Regards,
Alberto

[iant, 2013-08-07 14:19:46]

https://codereview.appspot.com/9835047/diff/19001/misc/cgo/test/fpvar.go
File misc/cgo/test/fpvar.go (right):

https://codereview.appspot.com/9835047/diff/19001/misc/cgo/test/fpvar.go#newc...
misc/cgo/test/fpvar.go:1: package cgotest
This new file needs a 2013 copyright header.

https://codereview.appspot.com/9835047/diff/19001/misc/cgo/test/fpvar.go#newc...
misc/cgo/test/fpvar.go:9: return f();
May as well indent by a tab here and below, to match the other C code.

https://codereview.appspot.com/9835047/diff/19001/misc/cgo/test/fpvar.go#newc...
misc/cgo/test/fpvar.go:34: t.Errorf("want %d, got %d", expected, r1)
Go tests generally do "got %d, want %d", please switch these.

https://codereview.appspot.com/9835047/diff/19001/src/cmd/cgo/out.go
File src/cmd/cgo/out.go (right):

https://codereview.appspot.com/9835047/diff/19001/src/cmd/cgo/out.go#newcode124
src/cmd/cgo/out.go:124: panic(fmt.Errorf("Invalid var kind %q", n.Kind))
s/Invalid/invalid/

[iant, 2013-08-07 14:27:58]

It seems appropriate to add a sentence or two to doc.go about this, e.g., in the
section on implementation details.

[Hierro, 2013-08-07 17:12:39]

Hi Ian,

Thanks for the review. I've added the changes you suggested, as well as a couple
of sentences and an example in doc.go (in the section that gets shown in
golang.org/cmd/cgo).

Regards,
Alberto

[iant, 2013-08-07 17:22:02]

LGTM

Please wait for rsc.

https://codereview.appspot.com/9835047/diff/28001/misc/cgo/test/fpvar.go
File misc/cgo/test/fpvar.go (right):

https://codereview.appspot.com/9835047/diff/28001/misc/cgo/test/fpvar.go#newc...
misc/cgo/test/fpvar.go:5: // This file contains test cases for cgo.
Please change this comment to

// This file contains test cases for cgo with function pointer variables.

[Hierro, 2013-08-07 20:30:08]

On 2013/08/07 17:22:02, iant wrote:
> LGTM
> 
> Please wait for rsc.
> 
> https://codereview.appspot.com/9835047/diff/28001/misc/cgo/test/fpvar.go
> File misc/cgo/test/fpvar.go (right):
> 
>
https://codereview.appspot.com/9835047/diff/28001/misc/cgo/test/fpvar.go#newc...
> misc/cgo/test/fpvar.go:5: // This file contains test cases for cgo.
> Please change this comment to
> 
> // This file contains test cases for cgo with function pointer variables.

Thanks! I've just updated the patch set.

Regards,
Alberto

[rsc, 2013-08-13 16:42:10]

LGTM

[rsc, 2013-08-13 16:42:24]

*** Submitted as https://code.google.com/p/go/source/detail?r=abd44d99e4cf ***

cmd/cgo: Add support for C function pointers

* Add a new kind of Name, "fpvar" which stands for function pointer variable
* When walking the AST, find functions used as expressions and create a new Name
object for them
* Track functions which are only used in expr contexts, and avoid generating
bridge code for them

R=golang-dev, minux.ma, fullung, rsc, iant
CC=golang-dev
https://codereview.appspot.com/9835047

Committer: Russ Cox <rsc@golang.org>