@wolfgangp do you need any help with this revision before you commit it?

@vleschuk: Let me know what you think about this. I understand your suggestion but this seems a bit simpler to me.

LGTM

@jhenderson Here is the variant where dumpWarning() lives in DWARFContext. Patch became much simpler and smaller. What do you think?

• Removed all functions from Support library in order not to tempt other Support users
• dumpWarning() now lives in DWARFContext
• Had to get rid of default callbacks in DebugLine and DebugAddr to avoid circular dependency with DWARFContext

In D51033#1208987, @jhenderson wrote:

I'm thinking a bit more about this and I'm concerned that by adding these functions to Error.h, people will be more likely to use them from libraries, which can lead to various issues when the client of the library doesn't want to report errors and warnings the same way as this implements. The warn function was really only intended as a default implementation so that we didn't have to propagate things all the way out of DWARFContext.cpp, for the single client (llvm-dwarfdump) to consume.

• Changed names of functions from Display* to dump*
• Changed comments
• Got rid of unrelated whitespace change
• Manually fixed indentation when clang-format suggested smth strange

In D51033#1207437, @jhenderson wrote:

I'm not convinced "Display" is the right term for the message. "Print" or "Dump" would be much more typical for reporting messages.

@jhenderson Please take a look when you have time.

• Changed error codes to more appropriate ones
• Fixed formatting

In D49964#1186975, @jhenderson wrote:

I just noticed that llvmObject has an object_error equivalent to std::errc, with a make_error_code method allowing us to turn them into std::error_code for use with the new function. In particular, there are unexpected_eof and parser_failed errors, which might be more appropriate for certain situations in this file. What do you think?

In D49964#1200459, @jhenderson wrote:

Hi @vleschuk, I just wanted to quickly check-in on this to see if you need any help with making further improvements as suggested?

Ping. Any questions?

Closed by rL338447

In D49676#1182470, @jhenderson wrote:

Okay, I think I'm happy with this, but please let the others give their feedback and thumbs up too.

@aprantl , @dblaikie , @probinson, do you have any objections on this?

Have you considered writing unit tests for this at all? I don't think they're strictly necessary, but they would be nice.

I thought about it, I will give it more thought when we have more mature implementation.

• Fixed spacing in tests
• Minor grammar fixes in comments

• Fixed comments and gaps between lines.
• Moved get-by-index implementation from header to cpp.
• Added default value to DumpOptions.

Does anyone have more questions/suggestions/objections on this revision?

• Added FIXME comment for version mismatch situation
• Added check for the version <= 5 and corresponding test.

• Adopted producer test to match recent changes (got rid of "params" from "Addr Section params" message).
• Removed explicit return from test sample
• Moved header emission to AddressPool class

As mentioned in https://reviews.llvm.org/D50005 changed "header" message:

This revision implements support for generating DWARFv5 .debug_addr section. It is dependent on https://reviews.llvm.org/D49676 as we need working consumer to execute test.

• Remove unnecessary warnings from tests
• Separated test for 64bit addresses
• Explicit length invalidation in clear()
• Explicit clear() before extraction
• Style and comment fixes
• Rename debug_addr_size_not_multiple.s ==> debug_addr_address_size_not_multiple.s and debug_addr_size_not_multiple.s ==> debug_addr_address_size_not_multiple.s

• More verbose error messages
• Fixes for comment style
• New test for the situation when section size too small for given length

In D49676#1179057, @dblaikie wrote:

Also - I know for DW_AT_ranges attributes we have some advanced dumping in some situations that can print section names and the like, I think? Could we reuse that here too so the dumped addresses also have section name+offset, perhaps?

Sorry, actually I don't see this functionality for DW_AT_ranges, could you please tell me where I missed it?

Oh, sure - it was added here: https://reviews.llvm.org/D36313

Changed errc::function_not_supported to errc::not_supported.

• Added more tests to verify all generated error messages
• Added invalidateLength() method to DebugAddrTable class to mark the point when we need to stop extraction
• Added support for dumping 64bit addresses

• Used new createStringError() function
• Shortened tests and added new trivial test-case w/o .debug_addr section
• Used getMaxVersion() to guess which version of .debug_addr we are looking at
• Fixed comment and style issues

Added overloaded version of createStringError() function which is not variadic, also added corresponding test.

Added explicit std::error_code param to createStringError() function.

In D49824#1176004, @JDevlieghere wrote:

Can you add a test for the case where no Vals are provided? I remember something about that requiring an overload without the variadic template arguments.

• Added support for pre-DWARFv5 .debug_addr format (e.g. w/o header)
• Shortened .debug_addr consumer tests
• Moved createError() implementation to llvm::Support
• Relocated dumping above range_list dump
• Moved table length verification inside AddrTable class
• Renamed length() -> getLength()
• Shortened comments
• Fixed mistype in error message

In D49676#1172206, @dblaikie wrote:

What happens when dumping a file using pre-standard debug_addr? (is the version of the CU checked to make sure that the debug_addr section has a header, etc) I know this is super annoying - every time there's a section that lacks its own self-describing, versioned header, it hurts the ability to dump that section in the future :/ (in any object file that contains CUs of the old version that might have a contribution from that section, and thus cause entries in the section without a self-describing/standalone/versioned header)

In D49676#1173164, @jhenderson wrote:

Not specific to your change, but I noticed that a lot of the DWARFContext code uses 32-bit offsets. Would it not make sense where we are writing new stuff, to use 64-bit offsets, since DWARF does support this?

Sorry, I missed it too =) Please commit and I will reconfigure buildbot.

Hello. I can restart in nearest two hours as it's Sunday.

LGTM

LGTM

LGTM