[LLVM-C][OCaml] Add a fast linker binding

[kren1]

Summary

The current LLVM-C/Ocaml linker API is very slow when linking many files
together. This patch exposes and additional API that does not destroy
the Linker object between each linking call.

[whitequark]

LGTM

[kren1]

@modocache could you commit this for me?

[CodaFi]

include/llvm-c/Linker.h

54

Can you document the lifetime of the linked module?

lib/Linker/LinkModules.cpp

617

Can you expose these two defaulted arguments in the bindings as well?

[kren1]

Document lifetime of source, remove unnecessary arguments

[kren1]

lib/Linker/LinkModules.cpp

617

Exposing them is not trivial, because I would have to introduce new types for Flag and a trampoline for the callback. I removed them to be implicit and in line with LLVMLinkModules2. If you really want me to add them, I would prefer to do it as a separate diff.

[CodaFi]

lib/Linker/LinkModules.cpp

617

I realize it’s non-trivial, but our commitment to ABI compatibility makes revising these public interfaces much more difficult after the fact than getting it right up front.

FWIW, the result of the internalize bindings patch you have apply here as well, and you need only provide a thunk to translate the C binding flags to the few linker flags LLVM currently supports. If you’d like, I can generate that binding and tag you in a review, then you can rebase the OCaml bindings on top.

[kren1]

Properly expose the 2 default arguments to linkInModule

[kren1]

Simplify memory management for the callback value

[CodaFi]

include/llvm-c/Linker.h

35

These names should be qualified as above.

typedef enum {

LLVMLinkerFlagsNone = 0,
LLVMLinkerFlagsOverrideFromSrc = 1,
LLVMLinkerFlagsLinkOnlyNeeded = 2

} LLVMLinkerFlags;

[kren1]

Qualify linker flags

[whitequark]

bindings/ocaml/linker/linker_ocaml.c

40

I think the proliferation of non-reentrant/non-threadsafe functions in the API is an issue. (Right now OCaml has a global lock, but a multicore runtime is in works.) The approach in D65138 is better, but still seems error-prone to me; I'll need to take a closer look at the OCaml runtime feature to see if perhaps a better one is possible.

[kren1]

bindings/ocaml/linker/linker_ocaml.c

40

I can't do the D65138 approach, because the callback doesn't pass a context, but it would be easier in this case because there is no need to worry about life times.

[whitequark]

bindings/ocaml/linker/linker_ocaml.c

40

I would personally consider any C API function that does not take a context a (usability) bug, since the LLVM C API is primarily useful for writing bindings from other languages, most of which have some kind of closure mechanism. I think in this case it is clearly justifiable to add an LLVMLinkInModule2 that fixes this.

[kren1]

bindings/ocaml/linker/linker_ocaml.c

40

Ah good point, I'Il do that. Thanks

[kren1]

Pass the closure via context instead of global variable

[kren1]

Any news regarding this patch?

[whitequark]

I won't be able to review this (or other patches) until at least mid-September.

[hiraditya]

Can we get review on this patch.

[whitequark]

@CodaFi Could you please comment on the current state of the patch?

[kren1]

Any news on this?

[whitequark]

@kren1 Do you have any objections to my suggestion of adding LLVMLinkInModule2? I think that's the only remaining issue to address.

[kren1]

@whitequark I think I decided to fold the functionality of LLVMLinkInModule2 into the LLVMLinkInModule. Since I'm adding the function in the first place, I thought there is no need to have the C function without those additional parameters.

Or did I misunderstand what you meant by adding the LLVMLinkInModule2?

bindings/ocaml/linker/linker_ocaml.c

 
 /* llmodule -> llmodule -> unit */
 CAMLprim value llvm_link_modules(LLVMModuleRef Dst, LLVMModuleRef Src) {
   if (LLVMLinkModules2(Dst, Src))
     llvm_raise(*caml_named_value("Llvm_linker.Error"), LLVMCreateMessage("Linking failed"));
 
   return Val_unit;
 }
+
+/* llmodule -> lllinker */
+CAMLprim LLVMLinkerCtx llvm_get_linker(LLVMModuleRef Dest) {
+  return LLVMGetLinkerCtx(Dest);
+}
+
+static value *internalize_trampoline_callback_value = NULL;
+
+void internalize_callback_trampoline(LLVMModuleRef M, const char **names,
+                                     int names_size) {
+  CAMLparam0();
+  CAMLlocal2(names_list, cons);
+  names_list = Val_emptylist;
+  for (int i = 0; i < names_size; i++) {
+    cons = caml_alloc(2, 0);
+    Store_field(cons, 0, caml_copy_string(names[i]));
+    Store_field(cons, 1, names_list);
+    names_list = cons;
+  }
+  caml_callback2(*internalize_trampoline_callback_value, (value)M, names_list);
+  CAMLreturn0;
+}
+
+/* LinkFlags option -> (llmodule -> string list -> unit) option -> lllinker ->
+ * llmodule -> unit */
+CAMLprim value llvm_link_in(value Flags, value Callback, LLVMLinkerCtx Dest,
+                            LLVMModuleRef Src) {
+  CAMLparam2(Flags, Callback);
+
+  unsigned flags;
+  if (Flags == Val_int(0)) {
+    flags = None;
+  } else {
+    flags = Int_val(Field(Flags, 0));
+  }
+
+  void (*callback_ptr)(LLVMModuleRef, const char **, int);
+  if (Callback == Val_int(0)) {
+    callback_ptr = NULL;
+  } else {
+    /*Since this call back is only called while linking happens,
+      its lifetime doesn't need to extend beyong this function.
+      Therefore we don't need any GC trickery  */
+    internalize_trampoline_callback_value = &Field(Callback,0);
+    callback_ptr = &internalize_callback_trampoline;
+  }
+
+  if (LLVMLinkInModule(Dest, Src, flags, callback_ptr))
+    llvm_raise(*caml_named_value("Llvm_linker.Error"),
+               LLVMCreateMessage("Linking failed"));
+
+  CAMLreturn(Val_unit);
+}
+
+/* lllinker -> unit */
+CAMLprim value llvm_linker_dispose(LLVMLinkerCtx L) {
+  LLVMDisposeLinkerCtx(L);
+  return Val_unit;
+}

bindings/ocaml/linker/llvm_linker.ml

 (*===-- llvm_linker.ml - LLVM OCaml Interface ------------------*- OCaml -*-===*
  *
  * Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
  * See https://llvm.org/LICENSE.txt for license information.
  * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  *
  *===----------------------------------------------------------------------===*)
 
 exception Error of string
 
+module LinkerFlags = struct
+  type t =
+    | None
+    | OverrideFromSrc
+    | LinkOnlyNeeded
+    | OverrideAndLinkOnlyNeeded
+end
+
 let () = Callback.register_exception "Llvm_linker.Error" (Error "")
 
 external link_modules' : Llvm.llmodule -> Llvm.llmodule -> unit
                        = "llvm_link_modules"
+
+external get_linker : Llvm.llmodule -> Llvm.lllinker = "llvm_get_linker"
+
+external linker_dispose : Llvm.lllinker -> unit = "llvm_linker_dispose"
+
+
+external link_in :
+     ?flags:LinkerFlags.t
+  -> ?internalizeCallback:(Llvm.llmodule -> string list -> unit)
+  -> Llvm.lllinker
+  -> Llvm.llmodule
+  -> unit
+  = "llvm_link_in"

bindings/ocaml/linker/llvm_linker.mli

 (*===-- llvm_linker.mli - LLVM OCaml Interface -----------------*- OCaml -*-===*
  *
  * Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
  * See https://llvm.org/LICENSE.txt for license information.
  * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  *
  *===----------------------------------------------------------------------===*)
 
 (** Linker.
 
     This interface provides an OCaml API for LLVM bitcode linker,
     the classes in the Linker library. *)
 
 exception Error of string
 
+module LinkerFlags : sig
+type t =
+    None
+  | OverrideFromSrc
+  | LinkOnlyNeeded
+  | OverrideAndLinkOnlyNeeded
+end
+
 (** [link_modules' dst src] links [src] into [dst], raising [Error]
     if the linking fails. The src module is destroyed. *)
-val link_modules' : Llvm.llmodule -> Llvm.llmodule -> unit
+val link_modules' : Llvm.llmodule -> Llvm.llmodule -> unit
+
+(** [get_linker dst] creates a linker context used to link into module [dst].
+    See the [llvm::Linker] class. *)
+val get_linker : Llvm.llmodule -> Llvm.lllinker
+
+(** [link_in dst src] links [src] into [dst], raising [Error] if the linking
+    fails. The [src] module is destroyed. [flags] argument changes the behaviour of 
+    the linker. [internalizeCallback] gets passed the new module and a list of names 
+    of symbols to internalize. See the [llvm::Linker::linkInModule] method.  *)
+val link_in : 
+     ?flags:LinkerFlags.t
+  -> ?internalizeCallback:(Llvm.llmodule -> string list -> unit)
+  -> Llvm.lllinker
+  -> Llvm.llmodule
+  -> unit
+
+(** [linker_dispose linker] frees up linker from [get_linker]. *)
+val linker_dispose : Llvm.lllinker -> unit

bindings/ocaml/llvm/llvm.ml

 (*===-- llvm/llvm.ml - LLVM OCaml Interface -------------------------------===*
  *
  * Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
  * See https://llvm.org/LICENSE.txt for license information.
  * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  *
  *===----------------------------------------------------------------------===*)
 
 
 type llcontext
 type llmodule
+type lllinker
 type lltype
 type llvalue
 type lluse
 type llbasicblock
 type llbuilder
 type llattrkind
 type llattribute
 type llmemorybuffer

bindings/ocaml/llvm/llvm.mli

 (** The top-level container for all LLVM global data. See the
     [llvm::LLVMContext] class. *)
 type llcontext
 
 (** The top-level container for all other LLVM Intermediate Representation (IR)
     objects. See the [llvm::Module] class. *)
 type llmodule
 
+(** A linker used to link in other LLVM IR modules. See the [llvm::Linker]
+    class. *)
+type lllinker
+
 (** Each value in the LLVM IR has a type, an instance of [lltype]. See the
     [llvm::Type] class. *)
 type lltype
 
 (** Any value in the LLVM IR. Functions, instructions, global variables,
     constants, and much more are all [llvalues]. See the [llvm::Value] class.
     This type covers a wide range of subclasses. *)
 type llvalue

include/llvm-c/Linker.h

 
 /* This enum is provided for backwards-compatibility only. It has no effect. */
 typedef enum {
   LLVMLinkerDestroySource = 0, /* This is the default behavior. */
   LLVMLinkerPreserveSource_Removed = 1 /* This option has been deprecated and
                                           should not be used. */
 } LLVMLinkerMode;
 
+typedef enum {
+  None = 0,
+  OverrideFromSrc = 1,
+  LinkOnlyNeeded = 2
+
+} LinkerFlags;
+
 /* Links the source module into the destination module. The source module is
  * destroyed.
  * The return value is true if an error occurred, false otherwise.
  * Use the diagnostic handler to get any diagnostic message.
 */
 LLVMBool LLVMLinkModules2(LLVMModuleRef Dest, LLVMModuleRef Src);
 
+/**
+ * Construct a linker for a module.
+ * @see llvm::Linker(Module&)
+ */
+LLVMLinkerCtx LLVMGetLinkerCtx(LLVMModuleRef Dest);
+
+/**
+ * Link Src into the composite Dest. The source module is destroyed.
+ * Flags argument can be a combination of LinkerFlags enum, see llvm::Linker:Flags.
+ * InternalizeCallback gets called with the new module and an array of strings containing
+ * the names to be internalized. The array size is passed as a third parameter.
+ * @see llvm::Linker::linkInModule
+ */
+LLVMBool LLVMLinkInModule(LLVMLinkerCtx Dest, LLVMModuleRef Src, unsigned Flags,
+                          void (*InternalizeCallback)(LLVMModuleRef,
+                                                      const char **, int));
+
+/**
+ * Destroy a linker.
+ */
+void LLVMDisposeLinkerCtx(LLVMLinkerCtx L);
+
 #ifdef __cplusplus
 }
 #endif
 
 #endif

include/llvm-c/Types.h

 /**
  * Used to pass regions of memory through LLVM interfaces.
  *
  * @see llvm::MemoryBuffer
  */
 typedef struct LLVMOpaqueMemoryBuffer *LLVMMemoryBufferRef;
 
 /**
+ * Used to pass linkers through LLVM interfaces.
+ *
+ * @see llvm::Linker
+ */
+typedef struct LLVMOpaqueLinkerCtx *LLVMLinkerCtx;
+
+/**
  * The top-level container for all LLVM global data. See the LLVMContext class.
  */
 typedef struct LLVMOpaqueContext *LLVMContextRef;
 
 /**
  * The top-level container for all other LLVM Intermediate Representation (IR)
  * objects.
  *

lib/Linker/LinkModules.cpp

 // C API.
 //===----------------------------------------------------------------------===//
 
 LLVMBool LLVMLinkModules2(LLVMModuleRef Dest, LLVMModuleRef Src) {
   Module *D = unwrap(Dest);
   std::unique_ptr<Module> M(unwrap(Src));
   return Linker::linkModules(*D, std::move(M));
 }
+
+DEFINE_SIMPLE_CONVERSION_FUNCTIONS(Linker, LLVMLinkerCtx)
+
+LLVMLinkerCtx LLVMGetLinkerCtx(LLVMModuleRef Dest) {
+  Module *D = unwrap(Dest);
+  return wrap(new Linker(*D));
+}
+
+LLVMBool LLVMLinkInModule(LLVMLinkerCtx Dest, LLVMModuleRef Src, unsigned Flags,
+                          void (*InternalizeCallback)(LLVMModuleRef,
+                                                      const char **, int)) {
+  Linker *L = unwrap(Dest);
+  std::unique_ptr<Module> M(unwrap(Src));
+  if (InternalizeCallback == nullptr)
+    return L->linkInModule(std::move(M), Flags);
+  else {
+    return L->linkInModule(
+        std::move(M), Flags, [=](Module &M, const StringSet<> &names) {
+          unsigned size = names.size();
+          const char **c_names =
+              (const char **)malloc(size * sizeof(const char *));
+          int i = 0;
+          for (StringRef s : names.keys()) {
+            c_names[i] = s.data();
+            i++;
+          }
+          InternalizeCallback(wrap(&M), c_names, size);
+          free(c_names);
+        });
+  }
+}
+
+void LLVMDisposeLinkerCtx(LLVMLinkerCtx L) { delete unwrap(L); }