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

[Alan via Phabricator via llvm-commits]

alan added a comment.

I have some comments to add about the latest revision. The `caml_alloc_tuple_uninit` is actually not an OCaml runtime function, but a helper function that the LLVM binding code on the main branch defines as follows:

  value caml_alloc_tuple_uninit(mlsize_t wosize) {
    if (wosize <= Max_young_wosize) {
      return caml_alloc_small(wosize, 0);
    } else {
      return caml_alloc_shr(wosize, 0);
    }
  }

The documentation for `caml_alloc_small` and `caml_alloc_shr` state the following: https://v2.ocaml.org/manual/intfc.html#ss:c-low-level-gc-harmony

> We now give the GC rules corresponding to the low-level allocation functions `caml_alloc_small` and `caml_alloc_shr`. You can ignore those rules if you stick to the simplified allocation function `caml_alloc`.
> Rule 5  After a structured block (a block with tag less than `No_scan_tag`) is allocated with the low-level functions, all fields of this block must be filled with well-formed values before the next allocation operation. If the block has been allocated with `caml_alloc_small`, filling is performed by direct assignment to the fields of the block:
>
>   Field(v, n) = vn;
>
> If the block has been allocated with `caml_alloc_shr`, filling is performed through the `caml_initialize` function:
>
>   caml_initialize(&Field(v, n), vn);
>
> The next allocation can trigger a garbage collection. The garbage collector assumes that all structured blocks contain well-formed values. Newly created blocks contain random data, which generally do not represent well-formed values.

The binding code on the main branch does not follow these rules; when it calls `caml_alloc_tuple_uninit`, it always uses `Field(v, n)` to initialize the members, without checking if the amount allocated is greater than `Max_young_wosize`. Therefore, any code that hits the `caml_alloc_shr` case uses the OCaml runtime API incorrectly. I'm also not a fan of the `caml_` naming scheme, which misleads the reader into thinking the function comes from the OCaml runtime API.

Instead of keeping this function and checking `Max_young_wosize` everywhere it's used to determine whether to use `Field(v, n)` or `caml_initialize(&Field(v, n), vn)`, I just removed it and used `caml_alloc_tuple` everywhere, which I think results in the less intrusive diff. Allocations using `caml_alloc_tuple` should initialize their members with `Store_field(v, n, vn)` instead of `Field(v, n) = vn`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400