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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:46-62
+value to_val(void *ptr) {
+  if ((((value)ptr) & 1) == 0) {
+    return ((value)ptr) + 1;
+  } else {
+    value v = caml_alloc(1, Abstract_tag);
+    *((void **)Data_abstract_val(v)) = ptr;
+    return v;
----------------
nikic wrote:
> jberdine wrote:
> > @nikic this section of code is something that it would be good to get eyes on from someone familiar with the LLVM C API without needing any familiarity with OCaml specifics.
> > 
> > The `to_val` function is used to "encode" pointers returned from the LLVM C API to the client OCaml code, and `from_val` performs the corresponding "decode". What `to_val` does is to test if the pointer is at least 2-byte aligned, and if so encodes it by setting the low bit. If the pointer is not 2-aligned, a wrapper object is allocated and the pointer stored in it. The OCaml runtime system guarantees that the wrapper object will be word aligned, and so the decode done in `from_val` tests the low bit with `Is_long` to distinguish the two cases.
> > 
> > This implementation LGTM (I have not had time yet to check the usages in this diff), but a question for those more familiar with the LLVM C API is whether the case where pointers are not aligned is impossible due to guarantees about alignment? The implementation in this diff is conservative and supports unaligned pointers, but is perhaps not as efficient as possible if there are reasons to know all pointers from LLVM will be aligned.
> I believe it's safe to assume that LLVM allocations are at least 4 byte aligned (we commonly use 2 bits for pointer tags). Only problem would be cases where something other than an allocation base pointer is returned, e.g. some arbitrary substring of a larger string. I don't know if we have any APIs that do something like this though.
m68k's ABI caps alignment at 2 bytes (unless you `__attribute__((aligned(...)))` or `alignas(...)`), FWIW, though there are currently places where LLVM itself breaks as a result of that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400