This is an archive of the discontinued LLVM Phabricator instance.

Initial version of Go bindings.
ClosedPublic

Authored by pcc on Oct 8 2014, 4:37 PM.

Details

Summary

This code is based on the existing LLVM Go bindings project hosted at:
https://github.com/go-llvm/llvm

Note that all contributors to the gollvm project have agreed to relicense
their changes under the LLVM license and submit them to the LLVM project.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 14609.Oct 8 2014, 4:37 PM
pcc retitled this revision from to Initial version of Go bindings..
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added a subscriber: Unknown Object (MLST).
pcc updated this revision to Diff 14752.Oct 10 2014, 3:44 PM
  • Remove unnecessary explicit StringRef conversions
  • Move build script up a level
  • Add entries to .gitignore
pcc updated this revision to Diff 14758.Oct 10 2014, 4:49 PM
  • Add a Go version check
ruiu added a subscriber: ruiu.Oct 10 2014, 5:05 PM

Looks like a straightforward LLVM binding for Go.
Here's my review comments from the perspective of a Go programmer.

bindings/go/llvm/bitreader.go
49 ↗(On Diff #14752)

At line 38 you return Module{}, err. Here you return Module{nil}, err. Are these different?

bindings/go/llvm/dibuilder.go
26 ↗(On Diff #14752)

What is this number?

176 ↗(On Diff #14752)

Do you follow 80 column rule even in Go? We don't have the Go coding style in LLVM, but are you going to set it?

bindings/go/llvm/executionengine.go
40 ↗(On Diff #14752)

I prefer writing

type GenericValue struct {
  ...
}

type ExecutionEngine struct {
  ...
}

instead of grouping them in the type.

87 ↗(On Diff #14752)

You don't need this line

100 ↗(On Diff #14752)

Ditto

119 ↗(On Diff #14752)

Ditto

173 ↗(On Diff #14752)

Remove

bindings/go/llvm/string.go
107 ↗(On Diff #14752)

Remove

bindings/go/llvm/support.go
53 ↗(On Diff #14752)

Throughout this patch, you seems to avoid using defer where possible. Is there any reason behind that decision? It seems to me that this function could be written like this

for i, arg := range args {
  argstr[i] = C.CString(arg)
  defer C.free(unsafe.Pointer(argstr[i]))
}

and

overviewstr := C.CString(overview)
defer C.Free(unsafe.Pointer(ovewviewstr))
bindings/go/llvm/transforms_pmbuilder.go
22 ↗(On Diff #14752)

type PassManagerBuilder struct {

pcc updated this revision to Diff 14825.Oct 13 2014, 2:36 PM
  • Address reviewer comments
bindings/go/llvm/bitreader.go
49 ↗(On Diff #14752)

No. Changed here and elsewhere to be consistent.

bindings/go/llvm/dibuilder.go
26 ↗(On Diff #14752)

It appears to refer to a set of debug info version constants that are no longer used. Removed (and sent D5760 to remove the constants).

176 ↗(On Diff #14752)

To me it seems reasonable to follow the Go coding conventions for Go code in LLVM -- there doesn't seem to be any compelling reason to define our own conventions. I've sent out D5761 which formalizes this.

In this particular case we probably don't need the line break; removed.

bindings/go/llvm/executionengine.go
40 ↗(On Diff #14752)

Done.

87 ↗(On Diff #14752)

Removed.

100 ↗(On Diff #14752)

Ditto

119 ↗(On Diff #14752)

Ditto

173 ↗(On Diff #14752)

Done.

bindings/go/llvm/string.go
107 ↗(On Diff #14752)

Done.

bindings/go/llvm/support.go
53 ↗(On Diff #14752)

That certainly helps with conciseness here; done.

I was not the original author of most of this code, but I agree that it would be useful (and more idiomatic) to start using defer. I went through the code to make it use defer where it made sense.

bindings/go/llvm/transforms_pmbuilder.go
22 ↗(On Diff #14752)

Done.

ruiu accepted this revision.Oct 13 2014, 2:41 PM
ruiu added a reviewer: ruiu.

LGTM. Go code looks good to me.

This revision is now accepted and ready to land.Oct 13 2014, 2:41 PM

First pass at the LLVM C++ and build system side.

bindings/go/llvm/bitwriter.cpp
25 ↗(On Diff #14825)

Is there any particular reason to not add this to the existing C APIs?

Also, if it remains, this file should be named with CamelCase. Probably as GoBitWriter or BitWriterBindings.

bindings/go/llvm/dibuilder.cpp
18 ↗(On Diff #14825)

iostreams aren't used in LLVM

bindings/go/llvm/dibuilder.h
10 ↗(On Diff #14825)

This file too should be GoDIBuilderBindings.{h,cpp} or something CamelCased.

Do you think these should be Go-specific, or generic C APIs for bindings? I'm curious how you see this evolving over time.

bindings/go/llvm/ir.h
24–26 ↗(On Diff #14825)

I really thought we already had the C APIs for this... If not, they make a lot of sense to just add.

bindings/go/llvm/support.h
21–24 ↗(On Diff #14825)

These both seem moderately terrifying to expose in the Go bindings. Why were they needed?

bindings/go/llvm/transforms_instrumentation.h
23–28 ↗(On Diff #14825)

I think it would be reasonable to add an 'Instrumentation.h' to llvm-c/Transforms with these in it.

test/Bindings/Go/go.test
1 ↗(On Diff #14825)

Ow.

Is there any way to factor some of this into the lit.local.cfg?

At the least split it into multiple lines with '\' to smash them together?

pcc updated this revision to Diff 14846.Oct 13 2014, 5:44 PM
pcc edited edge metadata.
  • Address review comments
bindings/go/llvm/bitwriter.cpp
25 ↗(On Diff #14825)

Is there any particular reason to not add this to the existing C APIs?

In general, for this change I tried to avoid modifying the C API, mainly because of the stability policy.

That said, this specific function looks fine to add to the C API. Done in r219643.

Also, if it remains, this file should be named with CamelCase. Probably as GoBitWriter or BitWriterBindings.

From the point of view of a maintainer, it's convenient to be able to find the C++ for foo.go in foo.cpp. Also, the coding standards don't seem to prescribe file names. But this isn't a big deal, I'm happy to rename them.

bindings/go/llvm/dibuilder.cpp
18 ↗(On Diff #14825)

Removed. Not sure how that got there.

bindings/go/llvm/dibuilder.h
10 ↗(On Diff #14825)

There's no technical reason why these couldn't be part of the C API, but because of our C API stability policy, I think they should live here for now.

We could consider changing this policy in the future, for example by defining a core 'stable' subset of the C API, but I feel that that discussion should only happen as and when it needs to (e.g. if we add another set of language bindings that expose debug info).

bindings/go/llvm/ir.h
24–26 ↗(On Diff #14825)

We have LLVMAddFunctionAttr etc, but they take a 32-bit attribute value; in particular, we need a 64-bit attribute value to set some of the sanitizer attributes. We could add a 64-bit variant of this function, but it would probably need its own set of constants.

bindings/go/llvm/support.h
21–24 ↗(On Diff #14825)

These were needed to implement -fload-plugin and -mllvm in the Go frontend; despite the problems with them, it is sometimes useful or convenient for Go frontend users or developers to be able to set llvm::cl flags or load plugins from the command line. I'd imagine that if we expose command line flags through some other mechanism in the future, this API would change as well.

bindings/go/llvm/transforms_instrumentation.h
23–28 ↗(On Diff #14825)

I am not sure if we can consider the APIs for these to be stable yet; in particular, there are some changes for DFSan that I have in mind that might change this API.

test/Bindings/Go/go.test
1 ↗(On Diff #14825)

I wanted to keep the command output of lit.cfg copy-pastable if this test fails (at least in most normal situations).

I didn't know (or had forgotten) that you could use '\' on these lines. Changed.

Here are mostly replies to your comments Peter. Looking at the updated patch now...

bindings/go/llvm/bitwriter.cpp
25 ↗(On Diff #14825)

I think I end up on the side of consistency with other C++ code rather than consistency with the Go code. This is probably because I want to see all of the C++ code moved out of the Go bindings and into a common C interface that other bindings can use. =]

I would suggest normal PascalCase file names, and suffixing with 'Bindings' to avoid collisions with the existing files.

bindings/go/llvm/dibuilder.h
10 ↗(On Diff #14825)

Yes, I agree and having talked to others suspect this would be a widely appreciated change. Clearly it can't happen here though.

However, I would structure the code in anticipation of that change and leave comments in the relevant .h files that these bindings shouldn't be Go-specific and should eventually move to a (somewhat) less stable collection of C APIs for use in creating bindings of LLVM in other languages.

That seem like a good path?

15 ↗(On Diff #14825)

I would probably name these LLVM_BINDINGS_GO_LLVM_DIBUILDERBINDINGS_H for consistency... Not a big deal, but 'GOLLVM' seems hard to intuit without knowing the history of the project. =]

26 ↗(On Diff #14825)

The (arguably bad) LLVM convention for C-APIs has been: 'LLVMFooBarBaz'. Considering you're using 'LLVMDIBuilderRef' above, I think it makes sense to go ahead and follow this convention here. Doesn't much matter whether that lands in this patch or in a follow-up though.

bindings/go/llvm/support.h
21–24 ↗(On Diff #14825)

I'm much more comfortable exposing the command line option parsing (until some glorious day when the -mllvm tunables can be set through a saner API). The plugin loading is somewhat concerning to me... Our plugin support is dodgy at best, and surfacing it to frontends is frankly terrifying... but hey, there you go.

I'm actually rather surprised that the plugin loading is best accomplished through LLVM's pretty terrible shared library loading wrapper rather than through native Go interfaces.

For the commandline flags, I would actually be find to put it into the C API. If it changes (when, I hope), it will just become a no-op and a new interface will be provided for setting such flags.

bindings/go/llvm/transforms_instrumentation.h
23–28 ↗(On Diff #14825)

Sure. Same comments as in the DIBuilder case.

chandlerc accepted this revision.Oct 16 2014, 11:51 AM
chandlerc added a reviewer: chandlerc.

I think with the C interface naming tweaks and some other minor stuff this is ready to go in.

If you'd like me to take another look at anything, let me know, otherwise feel free to commit once you've addressed the comments (or refuted them as wrong headed!).

Thanks!

bindings/go/llvm/ir.go
1 ↗(On Diff #14846)

I know its pedantic, but could you capitalize 'IR' when using it as a normal noun in a sentence? (Clearly the Go APIs and files need to remain lower case, just talking about the comments.)

bindings/go/llvm/target.go
210 ↗(On Diff #14846)

nit: There seem to be a mixture of horizontal rule comment styles... I would pick on and be consistent with it.

But maybe there is some Go style thing going on here that I'm just not aware of. If so, ignore me. =]

pcc added inline comments.Oct 16 2014, 3:58 PM
bindings/go/llvm/bitwriter.cpp
25 ↗(On Diff #14825)

Done.

bindings/go/llvm/dibuilder.h
10 ↗(On Diff #14825)

Sounds good, done.

15 ↗(On Diff #14825)

Done.

26 ↗(On Diff #14825)

Done.

bindings/go/llvm/ir.go
1 ↗(On Diff #14846)

Done.

bindings/go/llvm/support.h
21–24 ↗(On Diff #14825)

I'm actually rather surprised that the plugin loading is best accomplished through LLVM's pretty terrible shared library loading wrapper rather than through native Go interfaces.

Go (the language) does not currently support dynamic loading of packages, so there is no standard library function for this. You can call dlopen directly from Go, but that isn't portable (admittedly LLVM's wrapper isn't much better, but I suppose in theory it would know what the "correct" way of loading LLVM plugins is for any given platform).

For the commandline flags, I would actually be find to put it into the C API. If it changes (when, I hope), it will just become a no-op and a new interface will be provided for setting such flags.

Sounds good. I split this out into r219975.

bindings/go/llvm/target.go
210 ↗(On Diff #14846)

Done.

pcc closed this revision.Oct 16 2014, 3:58 PM
pcc updated this revision to Diff 15052.

Closed by commit rL219976 (authored by @pcc).