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

[Alan via Phabricator via llvm-commits]

alan added inline comments.


================
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:
> alan wrote:
> > jberdine wrote:
> > > 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?
> > `DL` is a parameter, so it needs to be live when the function ends. Otherwise, a caller could pass a `DataLayout.t` value, and that value could be invalidated by the GC during the function call.
> I'm sorry but I still don't understand. Suppose the GC runs when `caml_copy_int64` allocates, and happens to move the value `DL` points to. Then note that `DL` is no longer needed since it is used only to compute the argument of `LLVMOffsetOfElement` and in particular is dead after the allocation in `caml_copy_int64`. Also, if the caller of `llvm_datalayout_offset_of_element` needs what it passed as the `DL` argument, then it will be live and a root in the caller, and the GC will update the caller's pointer when moving the block pointed to by `DL`. So I don't see the issue, but I am not sure I understand the point you are making. Do you see what I mean?
You're right, I made a mistake. I didn't consider that if the caller still needed to keep `DL` around, the caller stack frame would already have `DL` as a root, so this function doesn't need to register it as a root. Thank you for reviewing my code and pointing this out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400