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

[Josh Berdine via Phabricator via llvm-commits]

jberdine added a comment.

Thanks all, it is great to get more eyes on this!



================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:45
+                       "treats them as integers");
+  return (void *)(v ^ 1);
 }
----------------
jrtc27 wrote:
> Kakadu wrote:
> > alan wrote:
> > > Kakadu wrote:
> > > > Could you add a comment why these two functions are needed? I always though that out-of-heap values should be wrapped into Abstract_tag. At least the manual says that https://v2.ocaml.org/manual/intfc.html#ss:c-outside-head
> > > > 
> > > > Sorry for asking stupid questions, I was teleported there from https://discuss.ocaml.org/t/proposal-care-more-about-ocaml-bindings-for-popular-libraries/11451
> > > When I first wrote this patch, my original approach was to wrap LLVM values in an OCaml block of Abstract_tag. In a discussion with @jberdine, he said that I could assume that all pointers from LLVM are 2-byte aligned, and set the low bit to 1 when exposing them to OCaml (https://github.com/llvm/llvm-project/issues/58134#issuecomment-1271544642). The OCaml GC distinguishes pointers from integers from the low bit; the low bit of integers is 1. Therefore, the OCaml GC will treat out-of-OCaml-heap LLVM pointers as integers.
> > > 
> > > The OCaml documentation that you link also recommends this representation:
> > > 
> > > > For pointers that are at least 2-aligned (the low bit is guaranteed to be zero), we have yet another valid representation as an OCaml tagged integer. 
> > > 
> > > This approach avoids an extra allocation. This design choice is documented in the comments and assert messages. Is there anything else I need to add to make it clear for people unfamiliar with the code?
> > Sorry, I forgot that there is an optimization to avoid Abstract_tag.  It would be great to have a comment explicitly saying that. 
> Pointers are not integers.
> Pointers are not integers.

>From the perspective of the FFI, bit patterns with the lsb set are not interpreted as pointers that the runtime system should follow. It happens that integers in OCaml are represented with bit patterns where the lsb is set, so it is common, perhaps imprecise, usage to refer to pointers where their lsb has been set as "integers".


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:61
 
 value cstr_to_string(const char *Str, mlsize_t Len) {
   if (!Str)
----------------
Kakadu wrote:
> alan wrote:
> > Kakadu wrote:
> > > Am I understanding right... LLVM uses non-zero-terminated strings under the hood, so this function is need?
> > > 
> > > In case of zero terminated string it would be better to use caml_copy_string https://github.com/ocaml/ocaml/blob/5.0/runtime/alloc.c#L208
> > This code is what exists on the main branch and my patch does not touch it. The code passes the tests; changing this function is not the business of my patch.
> > 
> > It seems that this function accepts NULL pointers and handles the NULL case by allocating a zero-length OCaml-heap-allocated string.
> Yeah, you are right. The caml_alloc_string+memcpy could be probably be replaced by caml_copy_string, but it could be a topic for a distinct patch....
Yes, this code is old, and the same as the implementation of `caml_copy_string` except for also treating NULL as an empty string.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:663
+  unsigned Length = LLVMCountStructElementTypes(Type_val(StructTy));
+  LLVMTypeRef *Temp = alloc_temp(Length);
+  LLVMGetStructElementTypes(Type_val(StructTy), Temp);
----------------
Kakadu wrote:
> alan wrote:
> > Kakadu wrote:
> > > Could you double check that there is no bug here? See my comment about alloc_temp. 
> > This code allocates a temporary, intermediate array as a buffer to store the output of `LLVMGetStructElementTypes`. Then, it copies them over to an OCaml-heap-allocated array (which internally have the same representation as OCaml tuples) and copies over the elements, wrapping them by setting the low bit to 1.
> Yes, but it looks like alloc_temp accepts array as an argument, but you pass `unsigned Length` here...
Yes, `alloc_temp` should be replaced by a call to `malloc` here. I made a similar comment on D136537 a while ago but didn't notice that I hadn't submitted it. :-(


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:13-15
-|* Note that these functions intentionally take liberties with the CAMLparamX *|
-|* macros, since most of the parameters are not GC heap objects.              *|
-|*                                                                            *|
----------------
alan wrote:
> jberdine wrote:
> > A high-level question about this diff is whether you really want to change the bindings in this way. The current situation is that they intentionally take a low-level view of the FFI and have been written with that in mind and reviewed from that perspective. There is probably more chance of introducing bugs by changing this than by just adding the encoding of naked pointers and retaining the use of the low-level FFI. My personal preference would be to keep the existing low level FFI. But if such a change from the low-level to the high-level FFI is done, it should be done as a separate diff that does only that.
> I thought about this a long time, even before you left this comment. The OCaml manual instructs you to use the `CAMLparam`, `CAMLlocal`, and `CAMLreturn` macros for all FFI functions (https://v2.ocaml.org/manual/intfc.html#ss:c-simple-gc-harmony). It does not give any nuances, like "You don't need to register unboxed integers with `CAMLparam`" or "You don't need to use the macros if you never call the OCaml runtime." When I first wrote this patch, I just added all the macros to be extra safe.
> 
> I've been going through the code locally now and removing the macros, and so far the tests still pass, but there are several spots where I was unsure if removing the macro would be unsafe. (Example: Where one of the arguments is a pointer into the OCaml-managed heap, such as an OCaml string or an `Abstract_tag`ged LLVM value for purposes of custom finalizer, and the function calls the OCaml runtime by allocating something.) I don't want a heisenbug where the code usually passes the tests, but mysteriously segfaults in some user's program if the GC kicks in at the wrong time and deallocates memory that shouldn't, because the binding code plays fast and loose with the macros.
> 
> Then, we have the issue with the diagnostic handler, meaning that the OCaml runtime could kick in at many points that may not be obvious, but the diagnostic handler could violate so many assumptions about C and C++ resource cleanup, as we've discussed, that I don't know if it should factor in to the decision whether to use the macros.
> 
> When I was thinking about whether to use the macros or drop them, my view was mainly about efficiency versus readability, where I viewed the macros as sacrificing slight efficiency (because the code spends instructions registering values with the GC, when this may not be necessary) for readability (because the code follows the rules of the OCaml FFI manual to the letter). I do not share your view that using the macros would *introduce* bugs or make the code harder to review; I always thought the macros make the code safer and easier to review. It's better to just use the macro than to think, "Do we need to use a macro here? Does the GC need to know about this value before we call `caml_alloc_whatever`?"
My main point is not so much about whether or not to most desirable final situation involves abiding by the simplified FFI rules, but that the changes to do so are large and orthogonal to the goal of changing the bindings to avoid naked pointers. Changing to the simplified FFI rules is a judgement call that has pros and cons, while eliminating naked pointers is a hard requirement going forward. That is why I think the two things should be done as two separate diffs.

As for potentially introducing bugs, maybe I'm just paranoid, but large changes are hard to review and not as precise when bisecting issues that are discovered later. More specifically, adding `CAMLlocal` and associated macros publish more locations as GC roots, and the associated changes involve more allocations. Going all the way to strictly following the simplified rules should be fine, but if there are any oversights and we only get part of the way there, it seems entirely possible to introduce bugs.

The diagnostic handler API is limited in the sense that the handler functions must not access the OCaml runtime system. But following the simplified FFI rules will not be enough to enable supporting arbitrary handler functions. That would be another orthogonal change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400