Page MenuHomePhabricator

DebugInfo support for OCaml bindings
ClosedPublic

Authored by vaivaswatha on Nov 5 2020, 12:57 AM.

Details

Summary

The OCaml bindings do not currently wrap the DebugInfo C APIs.

This patch adds many DebugInfo functions to the OCaml bindings, but not all. In particular, the ones related to Variables, expressions and a couple other are missing. These can be safely added at a later time (just as I'm adding a couple functions in the core bindings right now, which were previously missing).

The OCaml bindings do not seem to have any testsuite, so I only have my local testing with a project I'm working on. If there's an easy way to add tests, I'll be happy to do it.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Maybe ask in Discord to get reviewers?

jberdine requested changes to this revision.Mar 12 2021, 3:51 PM

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

llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c
2–3

nit: formatting

347

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.

353–354

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.

llvm/bindings/ocaml/debuginfo/llvm_debuginfo.ml
12

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 for that extension.

14–16

nit: why not use standard docstrings here and below?

160

this returns int not unit

llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli
13

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.

161

int not unit here too

This revision now requires changes to proceed.Mar 12 2021, 3:51 PM
jberdine added inline comments.Mar 13 2021, 3:08 PM
llvm/bindings/ocaml/debuginfo/CMakeLists.txt
6

Note that I don't think this builds on main, in particular after https://github.com/llvm/llvm-project/commit/95537f450814c378fcb9d446dadcabc1385a5903 .

vaivaswatha updated this revision to Diff 330500.EditedMar 14 2021, 1:49 AM

Thank you very much @jberdine for the review, especially for the ready to incorporate changes in your fork (which I just cherry-picked).

The updated patch incorporates all (but one - see below) review comments by @jberdine.

improve the usability of these APIs to introduce some types to mirror the C++ type hierarchy. For instance, defining:
type llsubprogram = private llscope

This won't work because a scope can be either a subprogram or a lexical block. We would need something like type llscope = | Subprogram of llmetadata | LexicalBlock of llmetadata.

However my attempt at implementing this whole comment didn't quite succeed. For example, A file can be a scope too in the argument to dibuild_create_function, but dibuild_create_debug_location cannot have a file scope.

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?

llvm/bindings/ocaml/debuginfo/CMakeLists.txt
6

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?

llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli
2
12

It would be tidier to leave this alias out of the interface of this module and use Llvm.llmetadata in place of llmetadata below.

234–238

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.

458

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.

470

Should return option

476

Should return option

llvm/bindings/ocaml/llvm/llvm.mli
373

nit: since the LLVM-C type is LLVMModuleFlagBehavior, it seems better to stick to the existing US spelling with no 'u'

550–556

All the other functions in this module are documented. It would be nice to maintain that.

949

switch Value and Metadata

llvm/bindings/ocaml/llvm/llvm_ocaml.c
345

This can return null can't it? If so llvm_get_module_flag should return an option

349

nit: non-'u' spelling consistency here too

vaivaswatha marked 20 inline comments as done.EditedMar 15 2021, 12:13 AM

@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 find 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.

llvm/bindings/ocaml/debuginfo/CMakeLists.txt
6

ah my bad. It's supposed to be "Core" because the basic debug info code is in the IR/DebugInfo.cpp

llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli
234–238

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.

458

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.

vaivaswatha updated this revision to Diff 330566.EditedMar 15 2021, 12:14 AM
vaivaswatha marked 3 inline comments as done.

Address all but one review comment.

The types that we'll need to model include, at least the following:

DIType and its 5 subclasses
DILocalScope and its 4 (transitive) subclasses
DIVariable and its two subclasses
So if I understand correctly we'll have 5 + 5 + 2 upcasts and the same number of downcast functions b/w each of these 14 types (aliases).

All three of the above derive from DIScope, so that means more upcast and downcast utility functions.

I think we should defer this work to a separate patch / review where the focus is just on strongly typing the interface (or recapturing the C++ type hierarchy - whichever way you see it). Such a patch could fix the identical problem that's there in the core IR too, where the entire llvm::Value hierarchy is represented as llvalue in the OCaml interface (save for the exception made for llbasicblock).

@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 find 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.

The above comment was added just before I uploaded the reworked patch, so it gets hidden by the review tool. I'm writing this so that it appears without having to "show older comments" and you don't miss it.

jberdine accepted this revision.Mar 15 2021, 6:20 AM

Thanks @vaivaswatha for making all these changes, and so promptly!

I agree that the question of making the debuginfo types in the ocaml api safer is leading too far afield. If there was a clear path forward, it would be nice to take it from the beginning before making such changes will break backward compatibility, but the situation is not so clear. Particularly with the current loose types, it would be helpful if the llvm_debuginfo.mli file was documented. For example, it is not obvious that Llvm_debuginfo.get_subprogram should only be called on functions. It would be good if this new api were documented along the lines of the llvm_ocaml.mli file. For example, from

(** [element_type ty] returns the element type of the pointer, vector, or array
    type [ty]. See the method [llvm::SequentialType::get]. *)
val element_type : lltype -> lltype

the reader has a chance to know the assumptions on the argument, and the reference to the underlying method is useful for more information.

Otherwise I have made only minor comments that are mostly entirely discretionary.

So this LGTM. I'm happy to give feedback on any documentation, etc. that gets added, but functionally this looks good and doesn't need another round of review.

llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c
21

I'm not sure what the preferred approach is, but this is not needed since llvm_ocaml.h includes it.

169

why |?

342

I guess clang-format should be run

354–360

Totally your call, but how you moved ptr_to_option to llvm_ocaml.h makes me think that this code is a good candidate for a small helper function there too.

890

If llmetadata type aliases are not added, this comment should be updated. Also, this and the next 2 functions have these comments following the pattern in llvm_ocaml.c, but it would be good if this file was uniform with itself to either have them or not. I personally find them to be useful, but they are a bit of maintenance overhead to keep in sync.

llvm/bindings/ocaml/debuginfo/llvm_debuginfo.ml
12

Your call, but this alias can be removed now that it is not in the mli, and it would be more uniform with the treatment of e.g. Llvm.llvalue.

llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli
2

seems to still have the wrong file name

478

extremely minor nit, but just note that e.g. llvm_instr_get_opcode uses the 'instr' abbreviation

This revision is now accepted and ready to land.Mar 15 2021, 6:20 AM
vaivaswatha marked 8 inline comments as done.

Thank you very much @jberdine . I've incorporated all of your suggestions with this latest diff. I haven't yet added the documentation and will do that by tomorrow. Please have a final look at the latest changes so that with my update tomorrow it should only be comments over each function and (hopefully) no code changes.

llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c
169

This is strange. I can't recall why I used |=. It's likely that it was an overlook / typo. I've fixed this now. Thank you for catching this !

vaivaswatha added inline comments.Mar 15 2021, 9:23 AM
llvm/bindings/ocaml/llvm/llvm_ocaml.h
18

The include guards for this header is missing and I'll add them along with the documentation update coming up next.

jberdine added inline comments.Mar 15 2021, 1:54 PM
llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c
169

Maybe it was just left over from copy/paste from ..._set below

llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli
2
llvm/bindings/ocaml/llvm/llvm_ocaml.c
47–55

I'm sorry for the poor suggestion before, but looking again I think this code needs to use CAMLlocal, etc.

vaivaswatha updated this revision to Diff 330886.EditedMar 15 2021, 11:11 PM

@jberdine: I have now updated the patch with documentation for the newly added APIs, inline. Further:

  1. I removed the function llmetadata_null (only from the public interface). I'm not sure where it's required except to clear the metadata in instr_set_debug_loc.
  2. Consequent to (1), I changed the interface of instr_set_debug_loc to take llmetadata option.
  3. Minor: Moved the declarations of 3 functions that you had added (which came here because I cherry-picked your commits) from the end to somewhere in between where the order of the function declarations match that of the C API.
  4. I had missed including llvm_ocaml.h in llvm_ocaml.c. Fixed this now. Also added previously missing header guard to the header.
  5. The fix to cstr_to_string to use CAML* declarations.
  6. In all references to LLVM functions in the documentation, I've referred to the C-API rather than the C++ one because the C-API has a more detailed explanation of the arguments to the functions.

Fix clang-tidy warning on header guard name convention.

With this, there are now the following clang-tidy issues still reported:

  1. error: 'caml/memory.h' file not found. This has been around already for llvm_ocaml.c (and now we see it for the new file llvm_debuginfo.c).
  2. warning: invalid case style for variable 'value' and similar warnings for the variable and function name casing convention used. I'm choosing to ignore this because the convention I've used is on par with what was already used in the LLVM OCaml interface.
  3. warning: invalid case style for variable 'i': for (int i = 0; i < NumEntries; i++) {. This is a bit grey because both conventions are used in the file already. So I've stuck to using the smaller case as similar loops (when integers are iterated with) have this convention.
vaivaswatha marked 3 inline comments as done.Mar 15 2021, 11:44 PM

Looks good, thanks! I made a few very minor comments. I agree that the current state of clang-tidy is good. I don't know how to let it know about the OCaml-specific headers, and that e.g. value is a type declared there and it's capitalization is not up for discussion.

llvm/bindings/ocaml/debuginfo/llvm_debuginfo.mli
199
208
245
248–274

These 8 don't mention their LLVM-C counterparts

260
278
281–293

Maybe change these to refer to the C apis instead of the C++ ones for consistency with the rest of this module.

444–446

Mention LLVMDIBuilderCreateObjectPointerType?

448–461

These also don't mention their LLVM-C counterparts.

552–594

These don't mention their LLVM-C counterparts.

vaivaswatha marked 6 inline comments as done.

I've updated the patch addressing some of your comments. In particular, I didn't add a reference to the C-API to the functions where I had already provided the explanations inline (and the C-API comment doesn't add more value).

Thank you @jberdine for the detailed review and helping through this patch. Shall I commit it?

jberdine accepted this revision.Mar 16 2021, 12:01 PM

Thanks @vaivaswatha for dealing with all my nitpicking! Yes, I've made a final pass over this and it looks good. I'd recheck the build before committing given the CI build failure.

vaivaswatha added a comment.EditedMar 16 2021, 12:17 PM

Thanks @vaivaswatha for dealing with all my nitpicking! Yes, I've made a final pass over this and it looks good. I'd recheck the build before committing given the CI build failure.

I checked ninja check-all and ninja check-llvm-bindings-ocaml locally and they both go fine. The CI build shown above in this page, if you click on it, shows that the builds succeed and the error is only because of clang-tidy. Since these messages are something that were already there before for the files, I assumed that it's ok to commit. I'm not sure what's the right thing to do.

Ah, I see, the failure is just tidy, go for it then. I don't know if it can be configured, but clang-tidy cannot be obeyed on this code without breaking the build.

This revision was landed with ongoing or failed builds.Mar 16 2021, 9:46 PM
This revision was automatically updated to reflect the committed changes.

@jberdine: after commit, I found this failure emailed to me llvm_ocaml.c:function cstr_to_string: error: undefined reference to 'caml_alloc_initialized_string'. Do you happen to know how I can fix it, because it works on my Ubuntu 20.04 system. What OS do you have? (because you reported that it worked too). A link to one of the build fails I got over email: https://lab.llvm.org/buildbot/#/builders/16/builds/7919

@jberdine: after commit, I found this failure emailed to me llvm_ocaml.c:function cstr_to_string: error: undefined reference to 'caml_alloc_initialized_string'. Do you happen to know how I can fix it, because it works on my Ubuntu 20.04 system. What OS do you have? (because you reported that it worked too). A link to one of the build fails I got over email: https://lab.llvm.org/buildbot/#/builders/16/builds/7919

I've now switched to using the following code instead in cstr_to_string:

if (Str) {
  String = caml_alloc_string(Len);
  memcpy(String_val(Str), Str, Len);
} else {
  String = caml_alloc_string(0);
}

@jberdine: after commit, I found this failure emailed to me llvm_ocaml.c:function cstr_to_string: error: undefined reference to 'caml_alloc_initialized_string'. Do you happen to know how I can fix it, because it works on my Ubuntu 20.04 system. What OS do you have? (because you reported that it worked too). A link to one of the build fails I got over email: https://lab.llvm.org/buildbot/#/builders/16/builds/7919

I've now switched to using the following code instead in cstr_to_string:

if (Str) {
  String = caml_alloc_string(Len);
  memcpy(String_val(Str), Str, Len);
} else {
  String = caml_alloc_string(0);
}

That fixes it: https://github.com/llvm/llvm-project/commit/f7be9db6220cb39f0eaa12d2af3abedf0d86c303

I see, it seems some of the builds are still using a version of OCaml older than 4.06 (from 11-2017). The difference between using caml_alloc_initialized_string and the code with memcpy is about const char * versus char *. If OCaml is configured with immutable strings (the default starting with 4.10), then String_val returns a const char *. I guess that supporting such old versions will necessitate compilation warnings about const.