This is an archive of the discontinued LLVM Phabricator instance.

[OCaml] Expose the LLVM diagnostic handler
ClosedPublic

Authored by jketema on Apr 8 2016, 2:34 AM.

Details

Summary

This might need some iteration and more than the accidental test cases it has now, but I'd like to know if I'm on the right path.

Diff Detail

Event Timeline

jketema updated this revision to Diff 53004.Apr 8 2016, 2:34 AM
jketema retitled this revision from to [OCaml] Expose the LLVM diagnostic handler.
jketema updated this object.
jketema added a reviewer: whitequark.
jketema added a subscriber: llvm-commits.
whitequark added inline comments.Apr 8 2016, 2:45 AM
bindings/ocaml/llvm/llvm.ml
320

Just description and severity.

bindings/ocaml/llvm/llvm.mli
427

There is no need to have an 'a here since OCaml already has closures; the context argument in C is closure emulation in essence.

430

Accept an option in set_diagnostic_handler instead of using two functions.

bindings/ocaml/llvm/llvm_ocaml.c
187

There is caml_raise_out_of_memory and I think we should use it.

test/Bindings/OCaml/linker.ml
21

Is this (and elsewhere in test/) actually used in tests?

Thanks for the feedback so far!

bindings/ocaml/llvm/llvm.mli
427

I had that originally, just wasn't quite sure whether that was desired in this case. I'll remove the 'a

bindings/ocaml/llvm/llvm_ocaml.c
187

I looked for something like that but couldn't find it. Thanks for the pointer.

test/Bindings/OCaml/linker.ml
21

It's used in the sense that currently the test crashes, because it tries to read invalid bitcode but no handler is installed. Having diagnostic handlers actually fixes the tests updated in this patch.

It's not used in the sense that there's no FileCheck for the printed text. I could make the handler just be empty instead?

whitequark added inline comments.Apr 8 2016, 3:00 AM
test/Bindings/OCaml/linker.ml
21

Why not add the check?

jketema updated this revision to Diff 53171.Apr 10 2016, 5:35 AM

Updated patch based on @whitequark 's feedback. I've also added a separate test to test the diagnostic handler functionality.

whitequark added inline comments.Apr 10 2016, 5:45 AM
bindings/ocaml/llvm/llvm.ml
317

Just module Diagnostic, I think.

bindings/ocaml/llvm/llvm_ocaml.c
135

You don't need all this ceremony. LLVMDiagnosticInfoRef is a void* and so is a value and OCaml's runtime is defined to not do anything special with pointers that it doesn't control. So just cast LLVMDiagnosticInfoRef to value.

178

I would merge these two functions into just llvm_set_diagnostic_handler; there are a few examples of how to handle incoming option values in this codebase.

test/Bindings/OCaml/diagnostic_handler.ml
32

No check for the proper message and severity? This can hide a bug where the severity or message are mishandled.

jketema updated this revision to Diff 53176.Apr 10 2016, 6:20 AM

Addressed new comments.

whitequark accepted this revision.Apr 10 2016, 6:25 AM
whitequark edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Apr 10 2016, 6:25 AM

Thanks for reviewing. Note that this doesn't re-enable the binding tests, as one test is still broken after these fixes: the core.ml test tries to build old-style debug information, and the new style is not even exposed in llvm-c. Maybe that part of core.ml should be disabled (or even removed?).

Yes, please remove those parts and re-enable the tests.

This revision was automatically updated to reflect the committed changes.