[PATCH] D90831: DebugInfo support for OCaml bindings

[Josh Berdine via Phabricator via llvm-commits]

jberdine added a comment.

I have not had time to complete a full round of review, but made some comments on the latest version.

I would like to think a little more about introducing more informative type aliases for llmetadata. My thinking is that in the worst case there could be one ocaml type for each node in the llvm::Metadata inheritance diagram: https://llvm.org/doxygen/classllvm_1_1Metadata.html . Note that there is no multiple inheritance there. Then for each edge Supertype <-- Subtype there could be an ocaml type alias `type subtype = private supertype`. Then client code could upcast from supertype to subtype using an expression such as `(e : subtype :> supertype)`. To downcast would require a function such as `val get_subtype : supertype -> subtype option` that would optimally be implemented with something like a dyn_cast to check that the argument is actually of the right subclass at runtime. For the purposes of the binding's debuginfo API, it might not be necessary to include the whole inheritance diagram, it might be possible to leave out some nodes. Is this what you were working toward and ran into problems with?



================
Comment at: llvm/bindings/ocaml/debuginfo/CMakeLists.txt:5
+  C        debuginfo_ocaml
+  LLVM     DebugInfoDwarf)
----------------
I'm no cmake guru and am unsure what is the right thing here, or how this ought to be tested. Since DebugInfoDwarf seems to be only one of several DebugInfo* libraries, it is not obvious to me that it is the right choice. Do you understand and have an explanation?


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:1
+(*===-- llvm_analysis.mli - LLVM OCaml Interface --------------*- OCaml -*-===*
+ *
----------------



================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:11
+
+type llmetadata = Llvm.llmetadata
+
----------------
It would be tidier to leave this alias out of the interface of this module and use `Llvm.llmetadata` in place of `llmetadata` below.


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:233-237
+val di_location_get_scope : location:llmetadata -> llmetadata
+
+val di_location_get_inlined_at : location:llmetadata -> llmetadata
+
+val di_scope_get_file : scope:llmetadata -> llmetadata
----------------
Can't the underlying LLVM-C functions for these 3 return null? If so, these should return options. I have seen the last return null for sure.


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:457
+
+val ditype_get_name : llmetadata -> string
+
----------------
Minor naming question: It seems that the naming scheme is not entirely uniform: I don't understand why there is `ditype_...` and then `di_subprogram_...`, etc.


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:469
+
+val get_subprogram : Llvm.llvalue -> llmetadata
+
----------------
Should return option


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:475
+
+val instruction_get_debug_loc : Llvm.llvalue -> llmetadata
+
----------------
Should return option


================
Comment at: llvm/bindings/ocaml/llvm/llvm.mli:367
 
+module ModuleFlagBehaviour :sig
+  type t =
----------------
nit: since the LLVM-C type is LLVMModuleFlagBehavior, it seems better to stick to the existing US spelling with no 'u'


================
Comment at: llvm/bindings/ocaml/llvm/llvm.mli:540-542
+val get_module_flag : llmodule -> string -> llmetadata
+val add_module_flag : llmodule -> ModuleFlagBehaviour.t ->
+                        string -> llmetadata -> unit
----------------
All the other functions in this module are documented. It would be nice to maintain that.


================
Comment at: llvm/bindings/ocaml/llvm/llvm.mli:936
 
+(** Obtain a Value as a Metadata.
+    See the method [llvm::ValueAsMetadata::get()]. *)
----------------
switch Value and Metadata


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:324
+CAMLprim LLVMMetadataRef llvm_get_module_flag(LLVMModuleRef M, value Key) {
+  return LLVMGetModuleFlag(M, String_val(Key), caml_string_length(Key));
+}
----------------
This can return null can't it? If so llvm_get_module_flag should return an option


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:328
+CAMLprim value llvm_add_module_flag(LLVMModuleRef M,
+                                    LLVMModuleFlagBehavior Behaviour, value Key,
+                                    LLVMMetadataRef Val) {
----------------
nit: non-'u' spelling consistency here too


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

https://reviews.llvm.org/D90831