[PATCH] Initial version of Go bindings.

[Chandler Carruth]

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

================
Comment at: bindings/go/llvm/bitwriter.cpp:25
@@ +24,3 @@
+
+LLVMMemoryBufferRef gollvm_WriteBitcodeToMemoryBuffer(LLVMModuleRef M) {
+  std::string Data;
----------------
pcc wrote:
> chandlerc wrote:
> > 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.
> > 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.
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.

================
Comment at: bindings/go/llvm/dibuilder.h:10
@@ +9,3 @@
+//
+// This file defines C bindings for the DIBuilder class.
+//
----------------
pcc wrote:
> chandlerc wrote:
> > 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.
> 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).
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?

================
Comment at: bindings/go/llvm/dibuilder.h:15
@@ +14,3 @@
+#ifndef GOLLVM_LLVM_DIBUILDER_H
+#define GOLLVM_LLVM_DIBUILDER_H
+
----------------
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. =]

================
Comment at: bindings/go/llvm/dibuilder.h:26
@@ +25,3 @@
+
+LLVMDIBuilderRef gollvm_NewDIBuilder(LLVMModuleRef m);
+
----------------
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.

================
Comment at: bindings/go/llvm/support.h:21-24
@@ +20,6 @@
+
+void gollvm_LoadLibraryPermanently(const char *Filename, char **ErrMsg);
+
+void gollvm_ParseCommandLineOptions(int argc, const char *const *argv,
+                                    const char *Overview);
+
----------------
pcc wrote:
> chandlerc wrote:
> > These both seem moderately terrifying to expose in the Go bindings. Why were they needed?
> 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.
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.

================
Comment at: bindings/go/llvm/transforms_instrumentation.h:23-28
@@ +22,8 @@
+
+void gollvm_AddAddressSanitizerFunctionPass(LLVMPassManagerRef PM);
+void gollvm_AddAddressSanitizerModulePass(LLVMPassManagerRef PM);
+void gollvm_AddThreadSanitizerPass(LLVMPassManagerRef PM);
+void gollvm_AddMemorySanitizerPass(LLVMPassManagerRef PM);
+void gollvm_AddDataFlowSanitizerPass(LLVMPassManagerRef PM,
+                                     const char *ABIListFile);
+
----------------
pcc wrote:
> chandlerc wrote:
> > I think it would be reasonable to add an 'Instrumentation.h' to llvm-c/Transforms with these in it.
> 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.
Sure. Same comments as in the DIBuilder case.

http://reviews.llvm.org/D5684