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

[Alan via Phabricator via llvm-commits]

alan added a comment.

Oh no, I did something wrong with Arcanist. I ran `arc diff` and thought it would push a revision to this diff, but it instead made a new patch. I feel very frustrated with Arcanist, because it isn't explicit with diff IDs, and sometimes it surprises me about what patch it pushes to. I need to figure out what's wrong with the repo state on my computer and how I can fix this problem.



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


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:1955-1967
+value llvm_call_site_attrs(value F, value Index) {
+  CAMLparam0();
+  CAMLlocal2(Temp, Array);
+  unsigned Length = LLVMGetCallSiteAttributeCount(Value_val(F), Int_val(Index));
+  Temp = caml_alloc(1, Abstract_tag);
+  Array = caml_alloc_tuple_uninit(Length);
+  LLVMGetCallSiteAttributes(Value_val(F), Int_val(Index),
----------------
jberdine wrote:
> Could you double-check this one. `Temp` is allocated on the caml heap unlike other functions, and as written `F` and `Index` will need to be registered as roots.
`F` is an `llvalue`, which is represented using the low bit tagging scheme, and `Index` is an `int`, so both are unboxed and neither is a root. Is this reasoning correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400