Re: [Xen-devel] [RFC] Generating Go bindings for libxl

[Nicholas Rosbrook <rosbrookn@xxxxxxxxxxxx>]

> All that said, the first question I think is, what the generated code
> needs to look like.  Then, if c-for-go can be configured to do that,
> then we can consider it; otherwise, making our own generator from the
> IDL will be the only option.

Writing a custom Go code generator means that all C symbols used need
to be known at generation time. E.g., the Go code generator needs to know
the signature of libxl_domain_create_new for C.libxl_domain_create_new(...)
to work. Currently, such knowledge is gained by parsing C code, which makes
sense given the nature of cgo. AFAICT, the IDL describes how to generate C types
and boiler-plate functions like libxl_<type>_dispose(). How would the IDL alone 
be able to 
generate valid Go code without significant expansion?

> Out of curiosity, have you looked at the existing in-tree bindings?  Any
> particular opinions?

Yes, my process started by using the existing bindings.

One thing is that they do not conform to basic go standards. For
example: naming conventions, naked returns are used everywhere, and I find
it strange that there is an exported Context variable. But, obviously those are
very minor things and easy to change. See [1] for general information on this.

I also thought it looked very tedious to do by hand, and would be hard to extend
in the future. Hence the search for a cgo generator.

> There are two major differences I note.
> 
> First, is that in your version, there seems to be two layers: libxl.go
> is generated by c-for-go, and contains simple function calls; e.g.:
> domainInfo(), which takes a *Ctx as an argument and calls
> C.libxl_domain_info.  Then you have libxl_wrappers.go, which is written
> manually, defining DomainInfo as a  method on Ctx, and calls domainInfo().
>
> So you're writing the "idiomatic Go" part by hand anyway; I don't really
> see why having a hand-written Go function call an automatically
> generated Go function to call a C function is better than having a
> hand-written Go function call a C function directly.

I'm sure you would agree that writing all of that cgo code by hand was a PITA.

This saved me a lot of time. There is an extra layer, but I didn't need to write
any of the cgo by hand. Adding a wrapper function for the generated Go 
functions,
e.g. domainInfo(), takes a couple minutes and you end up with easy-to-use Go.

But, I think we already agree on the necessity of code generation.

> In fact, there's a Go-like clone of libxl_domain_config, but none for
> the elements of it; DeviceDisk, for instance, is simply defined as
> C.libxl_device_disk, and config->disks simply copied to the Disks
> element of the struct.  That's just all wrong -- it's actually a C
> array; Go can only access the first element of it.  How are you supposed
> to create a domain with more than one disk?

This is simply because my fork is still WIP. If you look at [2], I'm telling 
c-for-go
to not generate a new Go type for libxl_device_disk, whereas at [3] I tell 
c-for-go
how to create libxl_domain_config. 

> Furthermore, these pointers are not re-set to `nil` after <type>.Free()
> is called.  This just seems very dangerous: It would be way to easy to
> introduce a use-after-free bug.

In [4], the C pointer is set to nil inside the call to DomInfo.Free().

> The in-tree bindings generally only create C structures temporarily, and
> do a full marshal and unmarshall into and out of Go structures.  This
> means a lot of copying on every function call.  But it also means that
> the callers can generally treat the Go structures like normal Go
> structures -- they don't have to worry about keeping track of them and
> freeing them or cleaning them up; they can let the GC deal with it, just
> like they deal with everything else.

AFAICT, the generated code provides the ability to do this. The wrappers
just need more discipline to have defer'd dispose/free calls. If the wrapper
hides the calls to freeing/disposing the C pointers, then the caller can still
rely on the garbage collector like normal.

> 1. Keep separate structures, and do a full "deep copy", as the in-tree
> bindings do.  Advantage: Callers can use GC like normal Go functions.
> Structure elements are translated to go-native types. Disadvantage:
> Copying overhead on every function call.

Personally, I'm not worried about optimizing just yet.

> 2. Use C types; do explicit allocate / free.  Advantage: No copying on
> every function call.  Disadvantage: Needing to remember to clean up / no
> GC; can't use Go-native types.

We definitely don't want to export the C types through the Go API.

> 4. Attempt to use SetFinalizer() to automatically do frees / structure
> clean-up [1].  Advantage: No / less copying on every function call, but
> can still treat structures like they'll be GC'd.  Disadvantage: Requires
> careful thinking; GC may not be as effective if C-allocated memory
> greatly exceeds Go-allocated memory; can't use Go-native types for elements.

If we start looking to use the Go runtime, we've gone in the wrong direction.

> c-for-go seems to take the worst bits of #1 and #2: It requires explicit
> allocate / free, but also actually does a full copy of each structure
> whenever one "half" of it changes.

Either way, we need to worry about the allocate/free. But that will at least be
hidden by the wrappers. Not sure how to get around that part in any case.

However, it is not immediately obvious to me how to tell c-for-go not to export
the functions in cgo_helpers.go, which is unfortunate.

> I think I'm coming more and more to the conclusion that I don't like
> what c-for-go produces in libxl's case. :-)
> 
> On the whole, I still think #1 is the best option.  Thoughts?

I agree that doing a full copy and allowing callers to simply rely on Go's 
garbage collector is the best approach. I'm just not entirely convinced that
cannot be accomplished using c-for-go.

-NR

[1] https://github.com/golang/go/wiki/CodeReviewComments
[2] https://github.com/enr0n/xen/blob/libxl-go/tools/golibxl/libxl.yml#L33
[3] https://github.com/enr0n/xen/blob/libxl-go/tools/golibxl/libxl.yml#L68
[4] 
https://github.com/enr0n/xen/blob/libxl-go/tools/golibxl/libxl/cgo_helpers.go#L422
 
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel