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

[Josh Berdine via Phabricator via llvm-commits]

jberdine added inline comments.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:1949-1950
+value llvm_call_site_attrs(value F, value Index) {
+  CAMLparam0();
+  CAMLlocal2(Temp, Array);
+  unsigned Count = LLVMGetCallSiteAttributeCount(Value_val(F), Int_val(Index));
----------------
these no longer need to be added


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:46
 
+value to_val(void *ptr) {
+  assert((((value)ptr) & 1) == 0 &&
----------------
alan wrote:
> 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.
Yes, that makes sense.


================
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(
----------------
alan wrote:
> 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.
I understand that `DL` is a custom block, but its value is only used before the allocation in `caml_copy_int64`. This is the same before and after this diff, or am I overlooking something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400