LLVM C DIBuilder Creation APIs
Needs ReviewPublic

Authored by harlanhaskins on Apr 21 2017, 12:40 PM.

Details

Summary

This is an initial set of LLVM DIBuilder APIs that can create builders,

compile units, files, and scopes, and introspect the debug info inside
modules.
harlanhaskins created this revision.Apr 21 2017, 12:40 PM

arc...picked some wrong branches here. I'll fix this diff πŸ˜…

Fix merge head

Fix 80-column limit for LLVMDWARFSourceLanguage

Add back remanining files (sorry for the email spam!)

Hi, you can put "HEAD^" in the file ".git/arc/default-relative-commit" , this ensure that only the comit you are currently on is sent to phabricator.

whitequark added inline comments.Apr 21 2017, 12:50 PM
lib/IR/DebugInfo.cpp
704

I believe our policy for all new LLVM-C code is to use pointer/length pairs for all array arguments, including strings.

harlanhaskins added inline comments.Apr 21 2017, 12:51 PM
lib/IR/DebugInfo.cpp
704

Ah! Yes, I had forgotten. Thanks!

  • Fix 80-column limit for LLVMDWARFSourceLanguage
  • Pass string parameters as pointer/length pairs

Ideally we'd like to have some test with this. The way it is usually done is to use the echo test: it reads a module using the C API and then recreate another identical module using the same C API. This ensure end to end testing. However this doesn't provide any reading facility so that me be more work than you are willing to put into this.

include/llvm-c/DebugInfo.h
64

Maybe having 2 function rather than a bool in the API that end up being a uint8_t ?

66

The doc should specify if it finalize or not when disposing.

91

DWOId yields notthing relevant on Google. Maybe this needs to be explained a bit more.

107

It hasn't been enforced consistently int he past, so that may not be immediately obvious, but we usually pass a pointer and a length for string in the newer API. This avoid useless calls to strlen and works even if the string isn't zero terminated which is a plus. Keep in mind that the C API is often used to bind to other languages, so while this may not be the most idiomatic thing to do in C, this ends up fitting our use case better.

lib/IR/DebugInfo.cpp
678

There is a facility to unwrap subclasses. You can see LLVMValueRef for how it is done. You shouldn't need to write this.

harlanhaskins added inline comments.Apr 21 2017, 1:17 PM
include/llvm-c/DebugInfo.h
66

This begs the question: Should it just call finalize itself?

107

This has been fixed in a later revision πŸ‘

lib/IR/DebugInfo.cpp
678

Thanks! I was going off of the Rust implementation initially.

harlanhaskins added inline comments.Apr 21 2017, 1:21 PM
include/llvm-c/DebugInfo.h
91

I'll be completely honest here, this has stumped me for a while. I don't think there's even a facility within the wrappers to create a "split skeleton" compile unit...

aprantl added inline comments.Apr 21 2017, 1:30 PM
include/llvm-c/DebugInfo.h
11

Can you doxygenify this comment, too?

53

We have autobrief enabled in the doxygen configuration. Please don't use \brief any more.

Could you also add some unit tests for the new interface?

Wallbraker requested changes to this revision.Sun, Apr 23, 2:24 PM

Hey cool that you are doing this work, finally!

See inline comments (also click the done tag on the comments when those are fixed, please).

Regarding the tests, only since this API is only about creating debug info I'm okay with just a creation test, this API already have a special status.

include/llvm-c/DebugInfo.h
107

Please use size_t for the the string sizes, no need to go to 64bit on 32bit systems.

This revision now requires changes to proceed.Sun, Apr 23, 2:24 PM
  • Fix 80-column limit for LLVMDWARFSourceLanguage
  • Address comments
harlanhaskins marked 9 inline comments as done.Fri, May 19, 11:00 AM
  • uint64_t -> size_t for string lengths
harlanhaskins marked an inline comment as done.Fri, May 19, 11:13 AM
  • Add simple debuginfo tests
  • Fix unused variable warning

This is looking good and I like where this is going. I have a few nits in comments.

Where I'm more doubtful is for the test. The way we've tested the C API in the past is by reading a module and then re-emiting a clone of it on the output. This patch doesn't have the ability to read debug infos, so that's a bit compromised. One way forward is obviously to add reading support for the elements we add emission support from. I'm not sure how much work that would be, so I'm not sure how practical this is within the scope of that diff. If that's too hard then an alternative should be chosen, like emitting a given module and checking its output. This is where this patch is going, however, it doesn't looks like this patch actually checks the output so that's a problem.

include/llvm-c/DebugInfo.h
31

These id needs to persist across versions, and this is not ensuring this. You can look at other enums in Core.h to see how it is done. The basic idea is to list all the enum values here, and then use the macro option to generate the mapping between exposed values and internal llvm values.

58

Please use LLVMBool

109

LLVMBool for flags.

whitequark requested changes to this revision.Fri, May 19, 12:39 PM
whitequark added inline comments.
include/llvm-c/DebugInfo.h
104

size_t ProducerLen

105

LLVMBool IsOptimized

108

Are these last two also booleans? LLVMBool too then

129

What's the value for "line/column not available"?

lib/IR/DebugInfo.cpp
708

Ditto

709

LLVMBool IsOptimized

This revision now requires changes to proceed.Fri, May 19, 12:39 PM

(Having unit tests will avoid

include/llvm-c/DebugInfo.h
129

Compiler-generated locations (which are different from empty locations) are using line 0.

  • uint8_t -> LLVMBool for flags
  • ProducerLen should be size_t
harlanhaskins marked 7 inline comments as done.Sat, May 20, 11:52 AM
harlanhaskins added inline comments.
include/llvm-c/DebugInfo.h
129

Should this be documented?