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

[Alan via Phabricator via llvm-commits]

alan added a comment.

There were some parts where you had commented about `CAMLparam` and which I had previously chosen to keep the macro because the function called the OCaml runtime, such as by raising an OCaml exception or allocating from the OCaml heap, and I wanted to be careful. However, I went and accepted your suggestions of removing them because I don't think the roots are necessary for one of two reasons:

- The local variable only exists to store the result of an OCaml heap allocation, so before the call to the OCaml runtime, it does not contain a root
- The local variable only exists to store data to pass to an LLVM API call, and by the point that the OCaml runtime is called, the local variable no longer needs to be live.

However, there is one point where I kept the `CAMLparam` and left a comment at, in which the function must register one of its parameters as a root as it makes an OCaml heap allocation and the argument of course needs to be live throughout the function call.

I feel a little nervous about removing the `CAMLparam` macros because I'm worried about mistakes.



================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:46
 
+value to_val(void *ptr) {
+  assert((((value)ptr) & 1) == 0 &&
----------------
jberdine wrote:
> alan wrote:
> > jberdine wrote:
> > > It might not hurt to add a comment before `to_val` and `from_val` saying that they encode pointers to LLVM objects as OCaml tagged integers, following the 3rd encoding suggested by the OCaml FFI documentation at https://ocaml.org/releases/5.0/htmlman/intfc.html#ss:c-outside-head . I don't know how stable such urls are expected to be though.
> > I added comments to the header file explaining how these functions work. Should I put additional comments in the source file?
> The comment in the header is good but I think since it discusses the implementation details of the encoding, I would have put it in the .c and used a comment in the header that just said that the functions implement an encoding without specifying which one. But I have not checked if the uses in other files need to know about details of the encoding. Those are the things I think of, but go with your preference.
The reason I would rather have the comments in the header is that if the functions were to be used for some LLVM type that is not 2-byte aligned, or the implementation of some type changed so that it is no longer 2-byte aligned, the code will break. (In that case, the specific type that is not 2-byte aligned should have a dedicated `alloc_` function, just like the ones that already exist for types that need custom finalizers.) Therefore, I think that the 2-byte alignment aspect is a contract that users of the functions need to know about, not an implementation detail.


================
Comment at: llvm/bindings/ocaml/target/target_ocaml.c:148
+value llvm_datalayout_offset_of_element(value Ty, value Index, value DL) {
+  CAMLparam1(DL);
+  CAMLreturn(caml_copy_int64(
----------------
jberdine wrote:
> left over
This root is necessary. `DataLayout.t` is an OCaml custom block, not a pointer with the low bit set. `Int64.t` is also boxed. So, this function allocates on the OCaml heap and the GC needs to know that `DL` is live.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136400/new/

https://reviews.llvm.org/D136400