[llvm-ocaml] Assume pointers are at least 2-bit aligned

[alan]

Summary

This diff assumes that all pointers that LLVM gives to OCaml are at least 2-bit aligned, removing the branch that allocates/unwraps the OCaml Abstract_tag block.

The current binding code on the main branch skips a lot of CAMLparamX and CAMLreturn boilerplate because the wrapped code did not allocate. My parent patch added the CAMLparamX and CAMLreturn boilerplate because I assumed the code might allocate. This patch undoes that, since it again assumes that there is no allocation.

Furthermore, if the code does allocate, I only register boxed values (such as strings) with CAMLlocalX, skipping unboxed values (such as ints).

[alan]

Testing...

[alan]

Add missing return

[alan]

I'm extremely puzzled by https://github.com/llvm/llvm-project/blob/main/llvm/test/Bindings/OCaml/ext_exc.ml. When I test on OCaml 4.14, all tests pass, but when I test on OCaml 5.0~beta1, this test hangs forever. All tests on the parent patch pass on OCaml 5.0~beta1.

When I look at the backtrace with gdb, the test hangs in the function LLVMCreateMemoryBufferWithSTDIN. I think that the code is reading from stdin? Is the code supposed to signal an error instead?

[alan]

Don't use CAML macros in iterator functions

[alan]

Add missing CAMLreturn

[alan]

Fix missing CAMLreturn causing hanging tests

[alan]

Convert the rest of the files

[alan]

https://discuss.ocaml.org/t/relaxed-rules-for-binding-a-c-library/10693/ seems to imply that the shortcuts that this diff makes are still safe.

[alan]

Assume all functions may allocate on the OCaml heap

[alan]

Rebase onto parent patch

[alan]

Rebase

[jberdine]

Just a few initial comments so far, I have read and tested this some, but need to do more. Also, I think it would be easier to review if this diff and D136400 were combined into a single diff.

llvm/bindings/ocaml/llvm/llvm_ocaml.c

35–37

It may not matter, but I would be more comfortable to use bitwise operations to set and clear the low bit of pointers. And I think it would be better to add an assertion to test that the low bit is clear, as a way to more explicitly document the assumption, and to give us a chance to catch it in a debug build if it is ever violated.

260–262

CAMLparam and CAMLreturn do not need to be added (by the composition of this diff and its parent) here, right? There are other functions that seem to be in a similar situation, but asking about one example to check my understanding.

645

alloc_temp expects an ocaml block, not a size

1850

[alan]

I tried to figure this out myself, but I'm inexperienced with Phabricator and only messed things up. I've edited the parent diff, https://reviews.llvm.org/D136400. How do I rebase this patch so that it applies to the latest version of the parent? I somehow accidentally rebased the parent patch instead.

[alan]

llvm/bindings/ocaml/llvm/llvm_ocaml.c

260–262

In this diff, I decided to take some liberties by not registering a value with CAMLparam if it is known to not be a GC'd block (for example, if it is an int, or a pointer from LLVM that is represented by setting the low bit). The original bindings on main take this type of liberty a lot by omitting CAMlparam entirely in many places. In my parent diff, I added the CAMlparams and registered all values with CAMlparam, including ones that the GC doesn't need to track, to be safe, but here, I decided to relax which values I registered.

Is this a good choice? Registering all values probably makes the code clearer and reduces room for error (e.g. forgetting to register something), choosing not to register things like ints and 2-bit aligned LLVM pointers with the low bit set probably slightly increases performance.

[alan]

I've manually applied the main change of this diff (to assume that all pointers from LLVM are 2-bit aligned) to https://reviews.llvm.org/D136400 manually.

[jberdine]

Argh, I made these comments some time ago and failed to submit them.

llvm/bindings/ocaml/llvm/llvm_ocaml.c

581

Here and in a number of other cases the result of malloc is not checked for NULL.

645

llvm/bindings/ocaml/llvm/llvm_ocaml.h

 #define Val_none Val_int(0)
 #define Some_val(v) Field(v, 0)
 #define Tag_some 0
 #define Is_none(v) ((v) == Val_none)
 #define Is_some(v) Is_block(v)
 value caml_alloc_some(value);
 #endif
 
+/* ASSUME: Pointer is at least 2-bit aligned! Does not allocate on the OCaml
+   heap. */
 value to_val(void *ptr);
 
 void *from_val(value v);
 
 void *alloc_temp(value Elements);
 
 #define DiagnosticInfo_val(v) ((LLVMDiagnosticInfoRef)from_val(v))
 #define Context_val(v) ((LLVMContextRef)from_val(v))

llvm/bindings/ocaml/llvm/llvm_ocaml.c

 value caml_alloc_some(value v) {
   CAMLparam1(v);
   value Some = caml_alloc_small(1, 0);
   Field(Some, 0) = v;
   CAMLreturn(Some);
 }
 #endif
 
-value to_val(void *ptr) {
-  if ((((value)ptr) & 1) == 0) {
-    return ((value)ptr) + 1;
-  } else {
-    value v = caml_alloc(1, Abstract_tag);
-    *((void **)Data_abstract_val(v)) = ptr;
-    return v;
-  }
-}
+value to_val(void *ptr) { return ((value)ptr) + 1; }
 
-void *from_val(value v) {
-  if (Is_long(v)) {
-    return (void *)(v - 1);
-  } else {
-    return *((void **)Data_abstract_val(v));
-  }
-}
+void *from_val(value v) { return (void *)(v - 1); }
 
 value llvm_string_of_message(char *Message) {
   value String = caml_copy_string(Message);
   LLVMDisposeMessage(Message);
 
   return String;
 }
 
unsigned Kind = LLVMGetEnumAttributeKindForName(String_val(Name),
                                                   caml_string_length(Name));
   if (Kind == 0)
     caml_raise_with_arg(*caml_named_value("Llvm.UnknownAttribute"), Name);
   CAMLreturn(Val_int(Kind));
 }
 
 /* llcontext -> int -> int64 -> llattribute */
 value llvm_create_enum_attr_by_kind(value C, value Kind, value Value) {
   CAMLparam3(C, Kind, Value);
   CAMLreturn(to_val(LLVMCreateEnumAttribute(Context_val(C), Int_val(Kind),
                                             Int64_val(Value))));
 }
 
 /* llattribute -> bool */
 value llvm_is_enum_attr(value A) {
   CAMLparam1(A);
   CAMLreturn(Val_int(LLVMIsEnumAttribute(Attribute_val(A))));
 }
 
value llvm_return_type(value FunTy) {
   CAMLreturn(to_val(Type));
 }
 
 /* lltype -> lltype array */
 value llvm_param_types(value FunTy) {
   CAMLparam1(FunTy);
   CAMLlocal1(Tys);
   unsigned Length = LLVMCountParamTypes(Type_val(FunTy));
   LLVMTypeRef *Temp = malloc(sizeof(LLVMTypeRef) * Length);
   LLVMGetParamTypes(Type_val(FunTy), Temp);
   Tys = caml_alloc_tuple(Length);
   for (unsigned I = 0; I < Length; ++I) {
     Store_field(Tys, I, to_val(Temp[I]));
   }
   free(Temp);
   CAMLreturn(Tys);
 }
value llvm_struct_name(value Ty) {
   CAMLreturn(cstr_to_string_option(CStr, Len));
 }
 
 /* lltype -> lltype array */
 value llvm_struct_element_types(value StructTy) {
   CAMLparam1(StructTy);
   CAMLlocal1(Tys);
   unsigned Length = LLVMCountStructElementTypes(Type_val(StructTy));
   LLVMTypeRef *Temp = alloc_temp(sizeof(LLVMTypeRef) * Length);
   LLVMGetStructElementTypes(Type_val(StructTy), Temp);
   Tys = caml_alloc_tuple(Length);
   for (unsigned I = 0; I < Length; ++I) {
     Store_field(Tys, I, to_val(Temp[I]));
   }
   free(Temp);
   CAMLreturn(Tys);
 }
value llvm_is_thread_local(value GlobalVar) {
   CAMLparam1(GlobalVar);
   CAMLreturn(Val_bool(LLVMIsThreadLocal(Value_val(GlobalVar))));
 }
 
 /* bool -> llvalue -> unit */
 value llvm_set_thread_local(value IsThreadLocal, value GlobalVar) {
   CAMLparam2(IsThreadLocal, GlobalVar);
   LLVMSetThreadLocal(Value_val(GlobalVar), Bool_val(IsThreadLocal));
   CAMLreturn(Val_unit);
 }
 
 /* llvalue -> ThreadLocalMode.t */
 value llvm_thread_local_mode(value GlobalVar) {
   CAMLparam1(GlobalVar);
   CAMLreturn(Val_int(LLVMGetThreadLocalMode(Value_val(GlobalVar))));
 }