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

Sat Feb 18 14:07:50 PST 2023

alan added inline comments.

================
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);
----------------
jberdine wrote:
> 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. :-(
Thank you for the catch @Kakadu! That function wasn't tested, and when I added a test, it segfaulted. I've pushed a fix. With a patch this big, it's easy for mistakes to slip under the radar, so it's important to have a lot of eyes reviewing it,

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400