[llvm-ocaml] Migrate from naked pointers to prepare for OCaml 5

[alan]

Wrap data in CAMLlocal1

[nikic]

Nice work! Unfortunately I don't know enough about ocaml to meaningfully review this.

[jberdine]

llvm/bindings/ocaml/llvm/llvm_ocaml.c

53–69

@nikic this section of code is something that it would be good to get eyes on from someone familiar with the LLVM C API without needing any familiarity with OCaml specifics.

The to_val function is used to "encode" pointers returned from the LLVM C API to the client OCaml code, and from_val performs the corresponding "decode". What to_val does is to test if the pointer is at least 2-byte aligned, and if so encodes it by setting the low bit. If the pointer is not 2-aligned, a wrapper object is allocated and the pointer stored in it. The OCaml runtime system guarantees that the wrapper object will be word aligned, and so the decode done in from_val tests the low bit with Is_long to distinguish the two cases.

This implementation LGTM (I have not had time yet to check the usages in this diff), but a question for those more familiar with the LLVM C API is whether the case where pointers are not aligned is impossible due to guarantees about alignment? The implementation in this diff is conservative and supports unaligned pointers, but is perhaps not as efficient as possible if there are reasons to know all pointers from LLVM will be aligned.

[nikic]

llvm/bindings/ocaml/llvm/llvm_ocaml.c

53–69

I believe it's safe to assume that LLVM allocations are at least 4 byte aligned (we commonly use 2 bits for pointer tags). Only problem would be cases where something other than an allocation base pointer is returned, e.g. some arbitrary substring of a larger string. I don't know if we have any APIs that do something like this though.

[jrtc27]

llvm/bindings/ocaml/llvm/llvm_ocaml.c

53–69

m68k's ABI caps alignment at 2 bytes (unless you __attribute__((aligned(...))) or alignas(...)), FWIW, though there are currently places where LLVM itself breaks as a result of that

[alan]

Eliminate unsafe uses of caml_alloc_small

[alan]

Replace more returns with CAMLreturns in debuginfo

[alan]

The original code defined caml_alloc_tuple_uninit, which either called caml_alloc_small or caml_alloc_shr. Then, the original code would initialize the allocation by passing the pointer to the LLVM C API function.

According to the OCaml docs: https://v2.ocaml.org/manual/intfc.html#ss:c-low-level-gc-harmony

Initialization of caml_alloc_small allocations should be done with Field(v, n) = v_n, and initialization of caml_alloc_shr allocations should be done with caml_initialize(&Field(v, n), v_n). So, the usage in the original code was incorrect.

Furthermore, caml_alloc_small and caml_alloc_shr must be initialized before the next allocation. In my patch, to_val is called when initializing the allocation. Assuming that all LLVM pointers are 2-bit aligned, to_val should not allocate, but we are not sure that all pointers are 2-bit aligned. If to_val allocates, then the usage of caml_alloc_small and caml_alloc_shr are unsafe.

(Also, I'm still getting the hang of Phabricator... I had left a comment on the code this morning, but realized that it wasn't visible for other people until I made this comment, after I was able to make the necessary changes.)

llvm/bindings/ocaml/llvm/llvm_ocaml.c

2392–2395

I just found out that using Store_field here is unsafe, but I'm going to work and can't fix this until I come back home.

[alan]

Remove outdated comments

[alan]

For what it's worth, in my local repository, I redefined to_val to the following:

value to_val(void *ptr) {
  if ((((value)ptr) & 1) == 0) {
    return ((value)ptr) + 1;
  } else {
    return NULL;
  }
}

and all the tests passed on OCaml 5 beta1.

These are all the LLVM types that previously used naked pointers and now use the from_val/to_val conversions.

#define ExecutionEngine_val(v) ((LLVMExecutionEngineRef)from_val(v))
#define DiagnosticInfo_val(v) ((LLVMDiagnosticInfoRef)from_val(v))
#define Context_val(v) ((LLVMContextRef)from_val(v))
#define Attribute_val(v) ((LLVMAttributeRef)from_val(v))
#define Module_val(v) ((LLVMModuleRef)from_val(v))
#define Metadata_val(v) ((LLVMMetadataRef)from_val(v))
#define Type_val(v) ((LLVMTypeRef)from_val(v))
#define Value_val(v) ((LLVMValueRef)from_val(v))
#define Use_val(v) ((LLVMUseRef)from_val(v))
#define BasicBlock_val(v) ((LLVMBasicBlockRef)from_val(v))
#define MemoryBuffer_val(v) ((LLVMMemoryBufferRef)from_val(v))
#define PassManager_val(v) ((LLVMPassManagerRef)from_val(v))
#define TargetMachine_val(v) ((LLVMTargetMachineRef)from_val(v))

Glancing at their implementations:

• https://llvm.org/doxygen/ExecutionEngine_2ExecutionEngine_8h_source.html
• https://llvm.org/doxygen/DiagnosticInfo_8h_source.html
• https://llvm.org/doxygen/LLVMContext_8h_source.html
• https://llvm.org/doxygen/Attributes_8h_source.html
• https://llvm.org/doxygen/Module_8h_source.html
• https://llvm.org/doxygen/Metadata_8h_source.html
• https://llvm.org/doxygen/Type_8h_source.html
• https://llvm.org/doxygen/Value_8h_source.html
• https://llvm.org/doxygen/classllvm_1_1Use.html
• https://llvm.org/doxygen/BasicBlock_8h_source.html
• https://llvm.org/doxygen/MemoryBuffer_8h_source.html
• https://llvm.org/doxygen/PassManager_8h_source.html
• https://llvm.org/doxygen/Target_2TargetMachine_8h_source.html

I think it's safe to say that all are 2-bit aligned or more.

• Should I omit registering locals with CAMLparam if the value is known to not be an OCaml heap allocation (e.g. it is an Int or a Bool)?
• Should I switch the implementations of to_val and from_val to assume 2-bit alignment, and not register LLVM objects with CAMLparam?
• Should I omit CAMLparam and CAMLreturn entirely if the function does not allocate in the OCaml heap?

[alan]

This comment was removed by alan.

[alan]

Rewrite commit message

[alan]

The original binding code on the main branch often skipped using CAMLparamX and CAMLreturn because many of the functions did not manipulate OCaml heap objects. When converting the bindings to eliminate naked pointers, I followed all the rules at https://v2.ocaml.org/manual/intfc.html because I assumed that to_val might allocate on the OCaml heap. Now, I have a stacked diff at https://reviews.llvm.org/D136537 that assumes that all pointers that LLVM provides OCaml are at least 2-bit aligned, and therefore don't require an allocation on the OCaml heap. This diff also:

• Skips CAMLparamX and CAMLreturn entirely if the function does not allocate on the OCaml heap
• Only registers OCaml boxed values with CAMLparamX (i.e. it does not pass an int or a bool to CAMLparamX)

The code passes all tests on OCaml 5.0~beta1. This stacked diff is closer to the original code on the main branch. However, it relies on more assumptions that could possibly change, or I may have gotten wrong, so it is more brittle. I will leave it up to the reviewers to decide if the performance benefit is worth these changes.

[alan]

I'm concerned about the diagnostic handler-related functions. An arbitrary OCaml function can be set as the handler, so any LLVM function that invokes the diagnostic handler anywhere may allocate on the OCaml heap, therefore triggering the GC. (This weakens some assumptions that I thought allowed me to omit some instances of CAMLparam in my stacked diff.) What's more, any such LLVM function may throw an OCaml exception, which will unwind the stack, meaning:

• Calls to free in these bindings will not be reached. (The fix would be to allocate on the OCaml heap using Abstract_tag.)
• Assumptions about resource cleanup deeper in the LLVM code will be violated (e.g. C++ destructors may not run).

Is the diagnostic handler feature safe?

[alan]

Check for NULL in alloc_temp

[alan]

Rebase

[alan]

Ping @jberdine

[alan]

Ping @jberdine

[alan]

Starting from the 20th, I will be recovering from a major medical operation. For an unknown length of time, possibly several months, I will be incapacitated from working on this patch.

[jberdine]

Only a partial review so far.

llvm/bindings/ocaml/llvm/llvm.ml

444

Missed wrapping this function to decode args and encode result.

513

Missed wrapping this function to decode args and encode result.