[PATCH] D90831: DebugInfo support for OCaml bindings

[Josh Berdine via Phabricator via llvm-commits]

jberdine requested changes to this revision.
jberdine added a comment.
This revision now requires changes to proceed.

I have checked over this code, and tested it by adapting a system I work on to use this diff plus a few changes to do all the reading of debuginfo from bitcode. The changes I used in the experiment are available on the branch whose tip is here <https://github.com/jberdine/llvm-project/commits/ocaml_api>. With this I have successfully read the debuginfo of every global/function/instruction in every file under llvm/test, as well as several internal codebases. I have not done similar testing of the debuginfo creation side of the added APIs.

The code by and large looks good. I have left several comments inline, some are just nits but a few point to changes that I think are necessary, and another round of review would be a good idea.



================
Comment at: llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c:1-2
+/*===-- debuginfo_ocaml.c - LLVM OCaml Glue ----------------------*- C++
+-*-===*\
+|*                                                                            *|
----------------
nit: formatting


================
Comment at: llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c:346
+CAMLprim LLVMMetadataRef llvm_di_scope_get_file(LLVMMetadataRef Scope) {
+  return LLVMDIScopeGetFile(Scope);
+}
----------------
Functions like this can return NULL, and when NULL is returned as an opaque pointer from C to OCaml, there is nothing that can be done with it on the OCaml side. There is no way to test if it is NULL before passing it back from OCaml to C e.g. to `llvm_di_file_get_directory`. Instead, I think that functions like this need to return an OCaml `option` that is `None` in the `NULL` case and otherwise `Some` of the `LLVMMetadataRef`.

As far as I can tell, all of these functions can return NULL. In order to test this diff I made changes to some of these functions, see [here](https://github.com/jberdine/llvm-project/commit/fb38cc9b2ac3cf150eb05b624b725946e5aefe9e).


================
Comment at: llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c:352-353
+  const char *Directory = LLVMDIFileGetDirectory(File, &Len);
+  value String = caml_alloc_string(Len);
+  memcpy(String_val(String), Directory, Len);
+
----------------
Since `Directory` might be `NULL`, this might call `memcpy` on `NULL`. I think that `Len` will always be `0` in this case, but AFAIU it is still UB to pass invalid pointers to `memcpy`. So I would suggest changing this and other string-returning functions to something along the lines of the suggestion. See [here](https://github.com/jberdine/llvm-project/commit/32bda0b7a08a2ca788d3eea797a396b54d75c0b7).


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.ml:11
+
+type llmetadata
+
----------------
I think that it would be better to define this type in the `Llvm` module instead. This module has to depend on `Llvm` anyhow, and there are other uses of `LLVMMetadataRef` apart from `DebugInfo`. I encountered this when experimenting with extending this diff with support for variables, which lead to needing an OCaml API for `LLVMGlobalCopyAllMetadata`, which seems like it should live in `Llvm`. See [here](https://github.com/jberdine/llvm-project/commit/f9577ada92b4385c0d8b712c954fab0e23a19901) for that extension.


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.ml:13-15
+(*
+ * Source languages known by DWARF.
+ *)
----------------
nit: why not use standard docstrings here and below?


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.ml:159
+
+external get_module_debug_metadata_version : Llvm.llmodule -> unit
+  = "llvm_get_module_debug_metadata_version"
----------------
this returns `int` not `unit`


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:12
+type llmetadata
+
+(*
----------------
I have not worked all the way through the details, but it could improve the usability of these APIs to introduce some types to mirror the C++ type hierarchy. For instance, defining:
```
type lllocation = private llmetadata
type llscope = private llmetadata
type llsubprogram = private llscope
type llfile = private llmetadata
```
would enable using e.g.:
```
val di_location_get_scope : lllocation -> llscope
```
instead of
```
val di_location_get_scope : location:llmetadata -> llmetadata
```
I have experimented with some changes along these lines, see [here](https://github.com/jberdine/llvm-project/commit/7db8605a8ff2ee5a5544fbfaa318f548da495002).


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:160
+
+val get_module_debug_metadata_version : Llvm.llmodule -> unit
+
----------------
`int` not `unit` here too


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

https://reviews.llvm.org/D90831