[PATCH] D90831: DebugInfo support for OCaml bindings

[Vaivaswatha Nagaraj via Phabricator via llvm-commits]

vaivaswatha marked 20 inline comments as done.
vaivaswatha added a comment.

@jberdine I have addressed all your comments, except for the one on stricter type aliases for Metadata.

> Is this what you were working toward and ran into problems with?

No, I only attempted something simpler along the lines of your experimental change. Nothing so much as having conversion functions between them.

I need to think what's the best way to model the C++ type hierarchy for Metadata here in OCaml. For example, there is `llvalue` and `llbasicblock` already in `Llvml.ml`, but they just rely on, internally in `Core.cpp`, `cast<>` throwing a runtime error (at least that's what I understood of it).

> that would optimally be implemented with something like a dyn_cast

We don't have `dyn_cast` available in the C-API (I couldn't any uses of it). So we might need to reestablish the subtyping from scratch using the `MetadataKind` enum values. I'm not sure that will be easy to maintain though. I can give it a try.



================
Comment at: llvm/bindings/ocaml/debuginfo/CMakeLists.txt:5
+  C        debuginfo_ocaml
+  LLVM     DebugInfoDwarf)
----------------
jberdine wrote:
> 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?
ah my bad. It's supposed to be "Core" because the basic debug info code is in the `IR/DebugInfo.cpp`


================
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
----------------
jberdine wrote:
> 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.
It looks like the last two can, but not the first one. (https://llvm.org/docs/LangRef.html#dilocation). Scope is mandatory for a location. So I'll make it an optional return for the latter two.


================
Comment at: llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli:457
+
+val ditype_get_name : llmetadata -> string
+
----------------
jberdine wrote:
> 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.
You're right. While the DIBuilder related functions have a prefix `dibuild` (similar to how all `llbuilder`  functions start with `build_` in Llvm.ml), the same doesn't apply to metadata types (I thought it did at first). I'll fix these.


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

https://reviews.llvm.org/D90831