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

Josh Berdine via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 16 07:53:25 PST 2023


jberdine added inline comments.


================
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.              *|
-|*                                                                            *|
----------------
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.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:36
+value to_val(void *ptr) {
+  assert((((value)ptr) & 1) == 0);
+  return ((value)ptr) | 1;
----------------
It is common practice elsewhere in the LLVM codebase to add an informative message to assertions, in a way that does not affect the meaning:


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:592-604
+value llvm_param_types(value FunTy) {
+  CAMLparam1(FunTy);
+  CAMLlocal1(Tys);
+  unsigned Length = LLVMCountParamTypes(Type_val(FunTy));
+  LLVMTypeRef *Temp = malloc(sizeof(LLVMTypeRef) * Length);
+  LLVMGetParamTypes(Type_val(FunTy), Temp);
+  Tys = caml_alloc_tuple(Length);
----------------
To take this function as a specific example, I think that the suggested change would be a much more minimal change that yields code that does not suffer from naked pointers, is safe in a sequential context, and is probably safe (up to the best of my understanding) in a parallel context.


================
Comment at: llvm/test/Bindings/OCaml/debuginfo.ml:293
   let arg_var = Llvm_debuginfo.dibuild_create_parameter_variable dibuilder ~scope:fun_di
-    ~name:"my_arg" ~argno:0 ~file:file_di ~line:10 ~ty
+    ~name:"my_arg" ~argno:1 ~file:file_di ~line:10 ~ty
     ~always_preserve:false flags_zero
----------------
alan wrote:
> When I ran the tests with assertions enabled, the OCaml debuginfo test at `llvm/test/Bindings/OCaml/debuginfo.ml` failed with the message:
> 
> ```
> File "/home/alan/llvm-project/debug/test/Bindings/OCaml/Output/debuginfo.ml.tmp/Testsuite.ml", line 1:
> Warning 70 [missing-mli]: Cannot find interface file.
> File "/home/alan/llvm-project/debug/test/Bindings/OCaml/Output/debuginfo.ml.tmp/debuginfo.ml", line 1:
> Warning 70 [missing-mli]: Cannot find interface file.
> executable: /home/alan/llvm-project/llvm/lib/IR/DIBuilder.cpp:809: llvm::DILocalVariable* llvm::DIBuilder::createParameterVariable(llvm::DIScope*, llvm::StringRef, unsigned int, llvm::DIFile*, unsigned int, llvm::DIType*, bool, llvm::DINode::DIFlags, llvm::DINodeArray): Assertion `ArgNo && "Expected non-zero argument number for parameter"' failed.
> FileCheck error: '<stdin>' is empty.
> FileCheck command line:  /home/alan/llvm-project/debug/bin/FileCheck /home/alan/llvm-project/llvm/test/Bindings/OCaml/debuginfo.ml
> ```
> 
> The docs for createParameterVariable (https://llvm.org/doxygen/classllvm_1_1DIBuilder.html#afa7c4efcb5c40c8527c13759f10ef5ed) state that the index is 1-based, so the test code with `argno:0` seems to be wrong?
I think you're right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136400



More information about the llvm-commits mailing list