[PATCH] D136400: [ocaml-llvm] Migrate from naked pointers in preparation for OCaml 5

[Alan via Phabricator via llvm-commits]

alan added a comment.

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 fields by direct assignment to the field.

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.



================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:2395
 
-    value Tmp = caml_alloc_small(2, 0);
-    Field(Tmp, 0) = Hd;
-    Field(Tmp, 1) = Tl;
-    Tl = Tmp;
+    List = caml_alloc_small(2, 0);
+    Store_field(List, 0, Hd);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400