[DWARF] support for .debug_addr (consumer)

[vleschuk]

Summary

This patch implements basic support for parsing and dumping DWARFv5 .debug_addr section. There is still work to be done, further patches are on their way but I think it would be better to review and commit the "sceleton" first.

What is currently missing:

• symbol resolution (we dump only addresses)
• support for non-zero segment selector size
• support for 64bit dwarf

[probinson]

cc llvm-commits. Please do this when first creating the review.

Repeating the description here so it appears on the list:

This patch implements basic support for parsing and dumping DWARFv5 .debug_addr section. There is still work to be done, further patches are on their way but I think it would be better to review and commit the "sceleton" first.

[dblaikie]

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)

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?

[jhenderson]

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?

I haven't looked at the details of the new address table class. I'll look at that later.

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

63

I'm wondering if this should return an Error instance instead of Optional for the out-of-range case. It seems like a non-existent index request would indicate a malformed section, right?

79

Missing trailing ')'

lib/DebugInfo/DWARF/DWARFContext.cpp

254

I see that this is practically identical to dumpRnglistsSection below (and it may well be similar to other stuff too). Perhaps we should generalise the mechanism for extracting and dumping a given DWARF section, made up of distinct sub-sections/tables?

262–266

I'm not particularly comfortable with the DWARFContext knowing that a Length 0 is the indicator of whether the parsing can continue. I had similar issues with the .debug_line behaviour until I added a parser to wrap the extraction process. My personal inclination (but others may disagree) is that the AddrTable should have a "validLength()" method or similar, that can be queried.

506–511

This should probably not happen in between .debug_ranges and .debug_rnglists, just because those two sections are effectively equivalent.

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

26

I feel like this function is something we use repeatedly throughout the DWARF library, and quite probably elsewhere too. That's probably an indication that it needs moving into a support library.

test/tools/llvm-dwarfdump/X86/debug_addr_X86.s

10 ↗

(On Diff #156809)

There's a lot of unnecessary noise in this test. Could you remove everything apart from what is actually needed to test the section dumping, please. You can even use explicit addresses in the .debug_addr section, so that there doesn't need to be any labels.

test/tools/llvm-dwarfdump/X86/debug_addr_empty.s

8

Why .debug_rnglists here?

test/tools/llvm-dwarfdump/X86/debug_addr_invalid.s

10

Again, most of this is probably unnecessary.

[probinson]

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

51

DataSize and AddrCount look like they don't need to be class members.

71

The extra detail after the first sentence of this comment is all correct, but I think too much. How DWARF handles initial-length is consistent across all sections, there's no need to describe it.

85

getLength would be more consistent with LLVM style, not to mention the rest of the getters in this class.

lib/DebugInfo/DWARF/DWARFContext.cpp

28

LLVM style has the include files sorted by name, so this needs to be moved.

test/tools/llvm-dwarfdump/X86/debug_addr_X86.s

2 ↗

(On Diff #156809)

If all you're dumping is .debug_addr, the test doesn't need line-table directives or .cfi directives or even any actual code. It just needs a couple of labels in the .text section separated by a known number of bytes. This will keep the test focused on exactly what it's trying to exercise.

test/tools/llvm-dwarfdump/X86/debug_addr_empty.s

8

Why this section directive?

test/tools/llvm-dwarfdump/X86/debug_addr_invalid.s

3

As with the other test, you don't need to bother with line-table or .cfi directives or actual code.

[vleschuk]

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?

I agree, but this requires refactoring of existing code first: for example at least DataExtractor should be able to work with 64bit offset. I'd prefer not to change it in this revision.

[vleschuk]

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

51

Actually they need: DataSize is used in getter when we are working with headerless .debug_addr section (pre-DWARFv5 implementation) (see new diff when I upload it.

AddrCount is computed in extract() method and used later in dump() method. The easiest way is to save in private class data.

63

I think that returning None is also a good indication of an error. Also non-existent index can indicate an error in upper-level logic. And just IMHO for getters like this returning a requested data as function result looks better that returning it in its argument:

Error getAddrEntry(uint32_t Index, uint64_t &Result);

lib/DebugInfo/DWARF/DWARFContext.cpp

254

I thought about it too, but after that I implemented support for pre-DWARFv5 .debug_addr and it required additional parameters for extract() method. I'd prefer to leave this refactoring out of scope of this revision.

262–266

Done.

506–511

Agree, moved it above .debug_ranges.

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

26

Done for .debug_addr, getting rid of implementations in other DWARF classes requires separate NFC commit.

[jhenderson]

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

63

Sorry, I should have said "Expected<uint64_t>":
Expected<uint64_t> getAddrEntry(uint32_t Index);

(Error would be used where the return type would be void or where bool or an error code would be returned). To me, Optional indicates that a "no result" is a normal and reasonable usage of this interface, whereas Expected indicates that it is bad usage of the function (whether due to malformed input or due to bad upper-level logic). It also ensures that the result must be tested for error, and cannot be ignored.

[vleschuk]

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)

Added support for pre-DWARFv5 format. Thanks for pointing this out.

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?

[vleschuk]

• 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

[probinson]

include/llvm/DebugInfo/DWARF/DWARFContext.h

330

No, this is a bad idea. While we can reasonably expect address size to be the same across all units, the same is not true for DWARF version. Please remove this method and solve the problem another way (see comment where this is called in DWARFContext.cpp).

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

51

I should think Addrs.size() would be identical to the recorded AddrCount, meaning AddrCount is redundant.

include/llvm/Support/Error.h

1132 ↗

(On Diff #157210)

Because the intent is for this to be useful more widely, it should be committed separately. That way if you have to revert this patch, the utility template function remains in place for other users.

lib/DebugInfo/DWARF/CMakeLists.txt

18

This wants to be in alphabetical order.

lib/DebugInfo/DWARF/DWARFContext.cpp

485

The .debug_str_offsets section has the exact same problem, please use the same solution.

491–492

given that getCUAddrSize() is now implementing the for loop, shouldn't you delete this one? Similarly delete the comment as you moved it to the new function.

1615

While it is mechanically possible for address size to vary across CUs, in practice I think we are not going to see this come up within a single object file. The address size field is repeated across various DWARF headers (at least in v5) to make it easier to dump them independently, not to enable varying the address size.

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

2

Needs a module header comment.

60

else goes on the same line as the preceding brace.

There's a very handy tool, clang-format-diff, which will solve this sort of thing for you.

113

Addrs.size() should work here, then you don't need AddrCount.

[jhenderson]

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

25

Nit: unnecessary comment characters.

56

Please don't use unnecessary abbreviations in comments: addrs -> addresses (here and elsewhere).

87

This should be a static method, in my opinion, but better would be to leave it as a member and just test the class's Header Length field directly. In the latter case, you might want to rename the function to "hasValidLength"

89

length of array -> length of the array

include/llvm/Support/Error.h

1126 ↗

(On Diff #157210)

This should probably be unit tested.

lib/DebugInfo/DWARF/DWARFContext.cpp

254

Okay, that's reasonable.

491–492

Do you need this code comment duplicated in full in both places?

1616

This FIXME should not be here.

1617

LLVM style uses capitalized variables i.e. addr -> Addr

(here and elsewhere)

test/tools/llvm-dwarfdump/X86/debug_addr_X86.s

1 ↗

(On Diff #157210)

You don't need "X86" in the name of this test, because it is implied by the folder directory. Unless you want to distinguish between x86 and x86_64 (i.e. 4 byte and 8 byte addresses), which you may want to do anyway (but don't currently).

22–23 ↗

(On Diff #157210)

If you do .long 1234 etc instead of .long .Lfunc_begin0, you can remove the .text section entirely, and not have to worry about knowing the size of nops etc.

test/tools/llvm-dwarfdump/X86/debug_addr_dwarf4.s

11

This looks to have the same issue as the other tests. Please remove everything that isn't needed. I think you'll find that even if the .debug_info etc sections are required, you can probably remove a number of the tags.

test/tools/llvm-dwarfdump/X86/debug_addr_empty.s

2

There's another test case probably needed, similar to this one: no .debug_addr section at all.

test/tools/llvm-dwarfdump/X86/debug_addr_invalid.s

22

I'd change this to be "unsupported" rather than invalid, since technically, 1 is a valid segment selector size.

[vleschuk]

include/llvm/Support/Error.h

1126 ↗

(On Diff #157210)

Created separate review with simple unit test: https://reviews.llvm.org/D49824 .

1132 ↗

(On Diff #157210)

Created separate review with simple unit test: https://reviews.llvm.org/D49824 .

[vleschuk]

• 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

[jhenderson]

include/llvm/BinaryFormat/Dwarf.def

874

These look to be in alphabetical order for the most part too, so you should probably move this.

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

67

I'm wondering whether errc::invalid_argument might be more applicable. It looks like argument_out_of_domain is more to do with Mathematical functions, which this isn't.

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

28

Again, I'd say this should be invalid_argument (also applies to other uses of argument_out_of_domain below).

34

Please don't warn directly in library functions like this! And even if you do, please use WithColor, to ensure it is consistent.

Different executables will have different styles for how they handle warnings (some might want them printing, others might want to ignore them, and yet more might want them to be treated as a failure). Since this warning doesn't prevent continuation, I recommend you use some sort of callback. You can see similar things in the DWARFDebugLine::LineTable class, for what I mean, and would recommend.

44

There is errc::not_supported, which I'd recommend for this.

72

It may be worth saying that Version != 5 is not supported (except for the special case)?

80

This should probably be not_supported too.

93

not_supported again.

125

Are you going to do 64-bit addresses immediately after this?

test/tools/llvm-dwarfdump/X86/debug_addr_absent.s

5

You'll want to redirect stderr to stdout too, if you are going to check for warnings etc. I'd also go as far as to say that you should do CHECK-NOT: {{.}} at the end to show that we printed nothing else.

test/tools/llvm-dwarfdump/X86/debug_addr_dwarf4.s

21

I think you can delete this label.

test/tools/llvm-dwarfdump/X86/debug_addr_empty.s

2

And one more I thought of: an addr section with a header but no members. You might want to add that to the basic test case, rather than as a new file, as that will show what happens when there are multiple tables in the same file.

test/tools/llvm-dwarfdump/X86/debug_addr_invalid.s

6

You should probably add tests for each of the different error messages you can emit, at least reasonably. The ones that I can see are where the section is too small for a length field, the section is shorter than the (otherwise valid) length field (both as too short for the header and too short for the supposed payload), if the version is not specified (i.e. Version == 0), mismatching or invalid versions, and mismatching addresses (essentially that's all the ones that are not "not_supported".

You might also want tests for the currently unsupported cases, such as DWARF64 too.

24

You should probably make this a valid section, given a non-zero segment_selector size.

[probinson]

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

125

Other parts of the library (e.g. DWARFFormValue) unconditionally print a 64-bit address, regardless of the target address size. This probably should be considered a bug in DWARFFormValue but if you print a 64-bit value here, at least it would be consistent with the rest of the library.

[vleschuk]

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

44

There is only errc::function_not_supported. I used this one.

[jhenderson]

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

44

There is only errc::function_not_supported. I used this one.

Nope, errc::not_supported does definitely exist. Go to https://en.cppreference.com/w/cpp/error/errc and search for ENOTSUP for reference.

If it isn't there, then your standard library is not C++11 compliant...

[vleschuk]

• 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

[vleschuk]

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

44

Indeed, sorry for that. Committed rL338114: [Support] Bring std::errc::not_supported to llvm::errc.

[vleschuk]

Changed errc::function_not_supported to errc::not_supported.

[jhenderson]

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

70

It might be worth making this message a little more verbose, to mention the fact that it's out of range of the address table at offset x, if possible.

92

Nit: missing full stop.

lib/DebugInfo/DWARF/DWARFContext.cpp

253

Since .debug_addr can pre-date DWARF v5, you should probably just remove this comment (or at least the bit to do with the version).

258

"CB" is not an obvious abbreviation to me. Please just call it "WarnCallback" or similar (ditto in the actual extract function).

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

53

Missing full stop.

55

You might as well use the existing function for this.

But actually, here and elsewhere below, I suggest you just assume that the Length is valid. I'd only invalidate it when the version is unsupported, DWARF64 is detected or the section is too short for a length field. That way the caller can just skip the rest of the table and attempt to read another one a bit later, which might work.

72

It might have lost track. Which error message shows this exactly?

88

We do support DWARF version 5 for now as well as... -> We support DWARF version 5 and the...

table which -> table, which

header -> a header

series of -> a series of

93

Probably worth putting here where the version is suggested from.

98

Here and elsewhere, use the PRIu8 and similar instead of explicit "hhu" when the type is uint8_t etc.

test/tools/llvm-dwarfdump/X86/debug_addr.s

23

It would be nice to have a test case that doesn't produce this warning.

[dblaikie]

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

[vleschuk]

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

Thank you David! If you don't mind I will implement this in a separate revision: there is already a lot on the plate for this one :-)

[vleschuk]

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

72

Example:

# ERR: section is not large enough to contain a .debug_addr table of length 0x10 at offset 0x0
# ERR-NOT: {{.}}                                                                
                                                                                
# too small section to contain section of given length                          
  .section  .debug_addr,"",@progbits                                            
.Ldebug_addr0:                                                                  
  .long 12 # unit_length                                                        
  .short 5 # version                                                            
  .byte 4 # address_size                                                        
  .byte 0 # segment_selector_size

I added a test for this.

test/tools/llvm-dwarfdump/X86/debug_addr.s

23

We already have on: see debug_addr_version_mismatch.s - the first table produces the warning, the second one doesn't.

[vleschuk]

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

[jhenderson]

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

74

Nit: Since this and the function before are multi-line definitions, I'd suggest putting blank lines either side of them.

lib/DebugInfo/DWARF/DWARFContext.cpp

1618

Nit: I think this needs to be the explicit type, rather than auto, based on the coding style guide.

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

17

I'd call invalidateLength here (yes, the reinitializing of HeaderData renders this redundant, but that then means you are relying on knowing that the value 0 for Length is the method used for knowing if the Length is invalid).

26

I think the common thing to do in other parts of the DWARF library is to call clear() here, to ensure we don't try to extract into an already-filled in table.

37

This should probably say "DWARF version is not defined in a CU" etc, since the Version is defined in the table usually.

55

Do you have any comments on the second half of this inline comment? Because I don't think that's been done.

72

Sorry, I think I meant this point here:

It may be worth saying that Version != 5 is not supported (except for the special case)?

I don't see this being done at all.

86

implementation -> implementations

93

by DWARF -> by the DWARF.

But if I've understood the behaviour correctly this may be a bad error message to have, actually. Consider the case where we have both DWARF4 and DWARF5 style debug info tables in a single file (e.g. an executable). Won't this result in a version mismatch error? Or am I misunderstanding how this code works? If so, it probably needs comments explaining what is meant by "SuggestedVersion" and where it typically might come from. It might help to rename it "UnitVersion" as well.

Nit: by DWARF -> by the DWARF...

155

Don't use auto here, as per the style guide.

test/tools/llvm-dwarfdump/X86/debug_addr.s

23

Right, in that case, I don't think you should check the warning in this test. In fact, it might be worth crafting a small .debug_info unit to suppress the warning in this basic test case.

test/tools/llvm-dwarfdump/X86/debug_addr_size_mismatch.s

1 ↗

(On Diff #157834)

This test should be renamed to debug_addr_address_size_mismatch or similar, since the size could be referring to several different things.

[vleschuk]

lib/DebugInfo/DWARF/DWARFContext.cpp

1618

Actually this will differ from the style used in this particular file. I think we should leave auto here.

[vleschuk]

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

55

Actually I don't see how it might work. The caller relies on the fact that length value from the header is valid and parses the rest of the section based on this assumption. In this case the length field is definitely invalid and telling caller that everything is ok will result in incorrect parsing. Here is an example:

.Ldebug_addr0:                                                                  
  .long 1 # unit_length (should be 12 here)                                                        
  .short 5 # version                                                            
  .byte 4 # address_size                                                        
  .byte 0 # segment_selector_size                                               
  .long 0x00000000                                                              
  .long 0x00000001

if (!AddrTable.hasValidLength())                                          
  break; // If we do not break here we will increase Offset by 5 and not by 16 -> incorrect result.                                                                   
uint64_t Length = AddrTable.getLength();                                  
Offset = TableOffset + Length;

72

I am sorry, I think now that's me who lost track here? Are talking about the comment below?

// We support DWARF version 5 for now as well as pre-DWARF5 ...

93

You understood it correctly. But there is no ideal way to handle this. I used the same approach here as used in extraction of DebugStrOffsets, CU "suggests" MaxVersion found and version mismatch is treated as error: in case of version mismatch one can't tell exactly how to parse the header.

[vleschuk]

• 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

[vleschuk]

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

"Addr Section params" -> "Addr Section".

[jhenderson]

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

55

I think this comes down to "who should we trust more?". What do we do in other cases in the DWARF library?

72

I'll reset this by commenting on the line in question...

91

Before this check, we should check that the Version is a supported Version. The only ones supported are <= 5 (I would argue 4 and 5 explicitly, but I'm not sure at what point pre-standardisated .debug_addr tables started getting emitted, so the lower bound might be wrong). So if we encounter a Version >= 6, we should emit an unsupported error, because in that case, we can't guarantee that our parser can handle it. I'd then assume the Length is valid and jump to the next table when iterating over it.

93

Okay, I think I get you now. I think it shouldn't block this going in, in its first form, but I think I'd consider it a bug. Consider the following case:

• test.o has been compiled with DWARF version 5
• test2.o has been compiled with DWARF version 4 (and has a .debug_addr table)
• test.elf is test.o and test2.o linked together.
• llvm-dwarfdump will fail to dump both .debug_addr tables, because there will always be a mismatch.

Such a scenario is far from unlikely. It's quite common for static libraries built with older compiler versions (and therefore potentially different DWARF versions) to be linked together with objects or libraries from newer compilers.

By my understanding of the DWARF5 standard, the correct way to associate a .debug_addr table with a .debug_info table is to look at the DW_AT_addr_base attribute in the info table. In this case, each of the two .debug_info tables would point at a different .debug_addr table.

[vleschuk]

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

55

The most similar case is .debug_rnglists implementation where the same "mechanism" (break if length is invalid) is used, however the RnglistsTable implementation doesn't use it all: there is know explicit invalidation in case of error (I think we should work on it and see if there are any problems).

In our case I suggest to leave invalidation here: we assume that the header is correct and read the Length value from it, after that we see that our data contradicts our assumption and either Length is invalid or the section is malformed. In any case it is better to report an error than produce potentially invalid output.

93

I think we can put a FIXME mark here for now and get back to it later if you don't mind.

[vleschuk]

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

[vleschuk]

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

55

there is know explicit invalidation

there is no explicit invalidation

[JDevlieghere]

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

56

Nit: a newline between these functions would improve readability, especially with the doxygen comments.

64

Please give the DIDumpOptions a default value to make the argument optional. (i.e. DIDumpOptions DumpOpts = {})

67

Does this have to be implemented in the header?

lib/DebugInfo/DWARF/DWARFContext.cpp

1612

Nit: s/fact/theory/

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

121

Please use a full sentences here.

156

I think a raw_string_ostream might better communicate your intentions of building up a string.

[vleschuk]

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

156

I don't think so, in this situation usage of additional ostreams to build a format string will result in more code which will be less readable. I'd prefer to keep current code.

[vleschuk]

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

[jhenderson]

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

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

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

96–99

I'd move this FIXME to where the UnitVersion is calculated in DWARFContext.cpp.

I also think the last two lines of this comment make no sense on their own, and can probably just be deleted.

Nit: as error, however the -> as an error. However, the

Aside: you don't need to manually wrap comments, because clang-format will do it for you at the correct column width.

test/tools/llvm-dwarfdump/X86/debug_addr_unsupported_version.s

20–24 ↗

(On Diff #158213)

The spacing here (and in other tests) is a little all over the place. Please tidy it up a little.

[vleschuk]

• Fixed spacing in tests
• Minor grammar fixes in comments

[vleschuk]

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.

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

96–99

I'd move this FIXME to where the UnitVersion is calculated in DWARFContext.cpp.

I think it better to leave it here where the actual comparison is performed.

I also think the last two lines of this comment make no sense on their own, and can probably just be deleted.

Agreed.

Aside: you don't need to manually wrap comments, because clang-format will do it for you at the correct column width.

Yep, sorry, always forget to use this tool, need to hotkey it to my vimrc =)

[jhenderson]

Related aside to my comments regarding version mismatches: is it likely that future versions of DWARF (e.g. DWARF 6) will update the version number of the .debug_addr tables without changing anything in them? I know in previous versions of DWARF, some sections didn't increase in version number in line with the overall standard version. If not, then the version mismatch check may not make sense anyway.

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

96–99

But the error message will still want to exist, I believe, so the issue is finding the right CU, which is not done here. If somebody where to want to call this from another place, they might copy the existing call site, and will not be aware of the FIXME in that instance, resulting in them copying potentially dangerous code.

[jhenderson]

In D49676#1182530, @vleschuk wrote:

• Fixed spacing in tests

The space between directive/value/comment still look quite messy to me. Could you fix that too, please.

[aprantl]

Sorry about the late reply, I've been on vacation.

include/llvm/DebugInfo/DIContext.h

157

This looks like the wrong place to store this. Many dwarf sections have version numbers that are independent form one another. For example on Darwin we often use dwarf 4 debug_info and dwarf 2 debug_line in the same file.

158

Why is this a property of the dump options and not of whatever object that is being dumped?

[vleschuk]

include/llvm/DebugInfo/DIContext.h

158

I agree with the comment above about version, but address size should be the same for every section, am I right?

[dblaikie]

In D49676#1179229, @vleschuk wrote:

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

Thank you David! If you don't mind I will implement this in a separate revision: there is already a lot on the plate for this one :-)

Yeah, better as a separate patch in any case. And there's lots of things we can improve in that regard anyway - many places addresses show up & how they could be rendered more informatively with things like this.

[aprantl]

lgtm with the two parameters moved from DIDumpOptions into explict arguments of dumpAddrSection.

[vleschuk]

Closed by rL338447

[jhenderson]

In D49676#1183890, @vleschuk wrote:

Closed by rL338447

Hi @vleschuk, you didn't address my final comments before committing this, as far as I can see. Please could you go back and fix them.

Additionally, please remember to follow the guidelines at https://llvm.org/docs/Phabricator.html#committing-a-change when committing a change reviewed via Phabricator, specifically the bit about "Differential Revision" as that auto-closes the review and syncs up the diff.

[aprantl]

It looks like my request wasn't addressed either. Can you please fix this soon?

[aprantl]

@vleschuk, It looks like you *still* haven't addressed my comment

lgtm with the two parameters moved from DIDumpOptions into explict arguments of dumpAddrSection.

Can you please get this done ASAP?

include/llvm/BinaryFormat/Dwarf.def

 HANDLE_DWARF_SECTION(DebugMacro, ".debug_macro", "debug-macro")
 HANDLE_DWARF_SECTION(DebugNames, ".debug_names", "debug-names")
 HANDLE_DWARF_SECTION(DebugPubnames, ".debug_pubnames", "debug-pubnames")
 HANDLE_DWARF_SECTION(DebugPubtypes, ".debug_pubtypes", "debug-pubtypes")
 HANDLE_DWARF_SECTION(DebugGnuPubnames, ".debug_gnu_pubnames", "debug-gnu-pubnames")
 HANDLE_DWARF_SECTION(DebugGnuPubtypes, ".debug_gnu_pubtypes", "debug-gnu-pubtypes")
 HANDLE_DWARF_SECTION(DebugRanges, ".debug_ranges", "debug-ranges")
 HANDLE_DWARF_SECTION(DebugRnglists, ".debug_rnglists", "debug-rnglists")
+HANDLE_DWARF_SECTION(DebugAddr, ".debug_addr", "debug-addr")
 HANDLE_DWARF_SECTION(DebugStr, ".debug_str", "debug-str")
 HANDLE_DWARF_SECTION(DebugStrOffsets, ".debug_str_offsets", "debug-str-offsets")
 HANDLE_DWARF_SECTION(DebugCUIndex, ".debug_cu_index", "debug-cu-index")
 HANDLE_DWARF_SECTION(DebugTUIndex, ".debug_tu_index", "debug-tu-index")
 // Vendor extensions.
 HANDLE_DWARF_SECTION(AppleNames, ".apple_names", "apple-names")
 HANDLE_DWARF_SECTION(AppleTypes, ".apple_types", "apple-types")
 HANDLE_DWARF_SECTION(AppleNamespaces, ".apple_namespaces", "apple-namespaces")

include/llvm/DebugInfo/DIContext.h

#undef HANDLE_DWARF_SECTION
   DIDT_UUID = 1 << DIDT_ID_UUID,
 };
 
 /// Container for dump options that control which debug information will be
 /// dumped.
 struct DIDumpOptions {
   unsigned DumpType = DIDT_All;
   unsigned RecurseDepth = -1U;
+  uint16_t Version = 0; // DWARF version to assume when extracting.
+  uint8_t AddrSize = 4; // Address byte size to assume when extracting.
   bool ShowAddresses = true;
   bool ShowChildren = false;
   bool ShowParents = false;
   bool ShowForm = false;
   bool SummarizeTypes = false;
   bool Verbose = false;
   bool DisplayRawContents = false;
 

include/llvm/DebugInfo/DWARF/DWARFContext.h

public:
   create(const StringMap<std::unique_ptr<MemoryBuffer>> &Sections,
          uint8_t AddrSize, bool isLittleEndian = sys::IsLittleEndianHost);
 
   /// Loads register info for the architecture of the provided object file.
   /// Improves readability of dumped DWARF expressions. Requires the caller to
   /// have initialized the relevant target descriptions.
   Error loadRegisterInfo(const object::ObjectFile &Obj);
 
+  /// Get address size from CUs.
+  /// TODO: refactor compile_units() to make this const.
+  uint8_t getCUAddrSize();
+
 private:
   /// Return the compile unit which contains instruction with provided
   /// address.
   DWARFCompileUnit *getCompileUnitForAddress(uint64_t Address);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFCONTEXT_H

include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h

public:
   }
 
   /// Extracts a DWARF-encoded pointer in \p Offset using \p Encoding.
   /// There is a DWARF encoding that uses a PC-relative adjustment.
   /// For these values, \p AbsPosOffset is used to fix them, which should
   /// reflect the absolute address of this pointer.
   Optional<uint64_t> getEncodedPointer(uint32_t *Offset, uint8_t Encoding,
                                        uint64_t AbsPosOffset = 0) const;
+
+  size_t size() const { return Section == nullptr ? 0 : Section->Data.size(); }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARFDATAEXTRACTOR_H

include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h

+//===- DWARFDebugAddr.h -------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===------------------------------------------------------------------===//
+
+#ifndef LLVM_DEBUGINFO_DWARFDEBUGADDR_H
+#define LLVM_DEBUGINFO_DWARFDEBUGADDR_H
+
+#include "llvm/BinaryFormat/Dwarf.h"
+#include "llvm/DebugInfo/DIContext.h"
+#include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include <cstdint>
+#include <map>
+#include <vector>
+
+namespace llvm {
+
+class Error;
+class raw_ostream;
+
+/// A class representing an address table as specified in DWARF v5.
+/// The table consists of a header followed by an array of address values from
+/// .debug_addr section.
+class DWARFDebugAddrTable {
+public:
+  struct Header {
+    /// The total length of the entries for this table, not including the length
+    /// field itself.
+    uint32_t Length = 0;
+    /// The DWARF version number.
+    uint16_t Version = 5;
+    /// The size in bytes of an address on the target architecture. For
+    /// segmented addressing, this is the size of the offset portion of the
+    /// address.
+    uint8_t AddrSize;
+    /// The size in bytes of a segment selector on the target architecture.
+    /// If the target system uses a flat address space, this value is 0.
+    uint8_t SegSize = 0;
+  };
+
+private:
+  dwarf::DwarfFormat Format;
+  uint32_t HeaderOffset;
+  Header HeaderData;
+  uint32_t DataSize = 0;
+  std::vector<uint64_t> Addrs;
+
+public:
+  void clear();
+  /// Extract an entire table, including all addresses.
+  Error extract(DWARFDataExtractor Data, uint32_t *OffsetPtr,
+                uint16_t Version, uint8_t AddrSize);
+  /// Look up an address based on a given index.
+  uint32_t getHeaderOffset() const { return HeaderOffset; }
+  uint8_t getAddrSize() const { return HeaderData.AddrSize; }
+  void dump(raw_ostream &OS, DIDumpOptions DumpOpts) const;
+  /// Return the address based on a given index.
+  Expected<uint64_t> getAddrEntry(uint32_t Index) const {
+    if (Index < Addrs.size())
+      return Addrs[Index];
+    return createStringError(errc::argument_out_of_domain,
+                             "Index %" PRIu32 " is out of range");
+  }
+  /// Return the size of the table header including the length
+  /// but not including the addresses.
+  uint8_t getHeaderSize() const {
+    switch (Format) {
+    case dwarf::DwarfFormat::DWARF32:
+      return 8; // 4 + 2 + 1 + 1
+    case dwarf::DwarfFormat::DWARF64:
+      return 16; // 12 + 2 + 1 + 1
+    }
+    llvm_unreachable("Invalid DWARF format (expected DWARF32 or DWARF64)");
+  }
+
+  /// Returns the length of this table, including the length field, or 0 if the
+  /// length has not been determined (e.g. because the table has not yet been
+  /// parsed, or there was a problem in parsing).
+  uint32_t getLength() const;
+
+  /// Verify that the given length is valid for this table.
+  bool hasValidLength() const { return getLength() != 0; } 
+
+  /// Returns the length of the array of addresses.
+  uint32_t getDataSize() const;
+};
+
+} // end namespace llvm
+
+#endif // LLVM_DEBUGINFO_DWARFDEBUGADDR_H
+

lib/DebugInfo/DWARF/CMakeLists.txt

 add_llvm_library(LLVMDebugInfoDWARF
   DWARFAbbreviationDeclaration.cpp
   DWARFAddressRange.cpp
   DWARFAcceleratorTable.cpp
   DWARFCompileUnit.cpp
   DWARFContext.cpp
   DWARFDataExtractor.cpp
   DWARFDebugAbbrev.cpp
+  DWARFDebugAddr.cpp
   DWARFDebugArangeSet.cpp
   DWARFDebugAranges.cpp
   DWARFDebugFrame.cpp
   DWARFDebugInfoEntry.cpp
   DWARFDebugLine.cpp
   DWARFDebugLoc.cpp
   DWARFDebugMacro.cpp
   DWARFDebugPubTable.cpp
   DWARFDebugRangeList.cpp
   DWARFDebugRnglists.cpp
   DWARFDie.cpp
   DWARFExpression.cpp
   DWARFFormValue.cpp
   DWARFGdbIndex.cpp
   DWARFListTable.cpp
   DWARFTypeUnit.cpp
   DWARFUnitIndex.cpp
   DWARFUnit.cpp
   DWARFVerifier.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/DebugInfo/DWARF
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/DebugInfo
   )

lib/DebugInfo/DWARF/DWARFContext.cpp

 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h"
 #include "llvm/DebugInfo/DWARF/DWARFCompileUnit.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugAbbrev.h"
+#include "llvm/DebugInfo/DWARF/DWARFDebugAddr.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugArangeSet.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugAranges.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugFrame.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugLine.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugLoc.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugMacro.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugPubTable.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugRangeList.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugRnglists.h"
 #include "llvm/DebugInfo/DWARF/DWARFDie.h"
 #include "llvm/DebugInfo/DWARF/DWARFFormValue.h"
 #include "llvm/DebugInfo/DWARF/DWARFGdbIndex.h"
 #include "llvm/DebugInfo/DWARF/DWARFSection.h"
 #include "llvm/DebugInfo/DWARF/DWARFUnitIndex.h"
 #include "llvm/DebugInfo/DWARF/DWARFVerifier.h"
 #include "llvm/MC/MCRegisterInfo.h"
while (offset < size) {
       const char *S = StrData.getCStr(&StringOffset);
       if (S)
         OS << format("\"%s\"", S);
       OS << "\n";
     }
   }
 }
 
+// Dump the .debug_addr section (DWARF v5).
+static void dumpAddrSection(raw_ostream &OS, DWARFDataExtractor &AddrData,
+                            DIDumpOptions DumpOpts) {
+    uint32_t Offset = 0;
+    while (AddrData.isValidOffset(Offset)) {
+      DWARFDebugAddrTable AddrTable;
+      uint32_t TableOffset = Offset;
+      if (Error Err = AddrTable.extract(AddrData, &Offset, DumpOpts.Version,
+                                        DumpOpts.AddrSize)) {
+        WithColor::error() << toString(std::move(Err)) << '\n';
+        // Keep going after an error, if we can, assuming that the length field
+        // could be read. If it couldn't, stop reading the section.
+        if (!AddrTable.hasValidLength())
+          break;
+        uint64_t Length = AddrTable.getLength();
+        Offset = TableOffset + Length;
+      } else {
+        AddrTable.dump(OS, DumpOpts);
+      }
+    }
+}
+
 // Dump the .debug_rnglists or .debug_rnglists.dwo section (DWARF v5).
 static void dumpRnglistsSection(raw_ostream &OS,
                                 DWARFDataExtractor &rnglistData,
                                 DIDumpOptions DumpOpts) {
   uint32_t Offset = 0;
   while (rnglistData.isValidOffset(Offset)) {
     llvm::DWARFDebugRnglistTable Rnglists;
     uint32_t TableOffset = Offset;
if (shouldDump(Explicit, ".debug_line_str", DIDT_ID_DebugLineStr,
     while (const char *s = strData.getCStr(&offset)) {
       OS << format("0x%8.8x: \"", strOffset);
       OS.write_escaped(s);
       OS << "\"\n";
       strOffset = offset;
     }
   }
 
+  if (shouldDump(Explicit, ".debug_addr", DIDT_ID_DebugAddr,
+                 DObj->getAddrSection().Data)) {
+    DWARFDataExtractor AddrData(*DObj, DObj->getAddrSection(),
+                                   isLittleEndian(), 0);
+    DumpOpts.Version = getMaxVersion();
+    DumpOpts.AddrSize = getCUAddrSize();
+    dumpAddrSection(OS, AddrData, DumpOpts);
+  }
+
   if (shouldDump(Explicit, ".debug_ranges", DIDT_ID_DebugRanges,
                  DObj->getRangeSection().Data)) {
-    // In fact, different compile units may have different address byte
-    // sizes, but for simplicity we just use the address byte size of the
-    // last compile unit (there is no easy and fast way to associate address
-    // range list and the compile unit it describes).
-    // FIXME: savedAddressByteSize seems sketchy.
-    uint8_t savedAddressByteSize = 0;
-    for (const auto &CU : compile_units()) {
-      savedAddressByteSize = CU->getAddressByteSize();
-      break;
-    }
+    uint8_t savedAddressByteSize = getCUAddrSize();
     DWARFDataExtractor rangesData(*DObj, DObj->getRangeSection(),
                                   isLittleEndian(), savedAddressByteSize);
     uint32_t offset = 0;
     DWARFDebugRangeList rangeList;
     while (rangesData.isValidOffset(offset)) {
       if (Error E = rangeList.extract(rangesData, &offset)) {
         WithColor::error() << toString(std::move(E)) << '\n';
         break;  
       }
       rangeList.dump(OS);
     }
   }
 
   if (shouldDump(Explicit, ".debug_rnglists", DIDT_ID_DebugRnglists,
                  DObj->getRnglistsSection().Data)) {
     DWARFDataExtractor RnglistData(*DObj, DObj->getRnglistsSection(),
                                    isLittleEndian(), 0);
     dumpRnglistsSection(OS, RnglistData, DumpOpts);
   }
 
   if (shouldDump(ExplicitDWO, ".debug_rnglists.dwo", DIDT_ID_DebugRnglists,
                  DObj->getRnglistsDWOSection().Data)) {
     DWARFDataExtractor RnglistData(*DObj, DObj->getRnglistsDWOSection(),
                                    isLittleEndian(), 0);
     dumpRnglistsSection(OS, RnglistData, DumpOpts);
   }
 
Error DWARFContext::loadRegisterInfo(const object::ObjectFile &Obj) {
   std::string TargetLookupError;
   const Target *TheTarget =
       TargetRegistry::lookupTarget(TT.str(), TargetLookupError);
   if (!TargetLookupError.empty())
     return make_error<StringError>(TargetLookupError, inconvertibleErrorCode());
   RegInfo.reset(TheTarget->createMCRegInfo(TT.str()));
   return Error::success();
 }
+
+uint8_t DWARFContext::getCUAddrSize() {
+  // In fact, different compile units may have different address byte
+  // sizes, but for simplicity we just use the address byte size of the
+  // last compile unit. In practice the address size field is repeated across
+  // various DWARF headers (at least in version 5) to make it easier to dump
+  // them independently, not to enable varying the address size.
+  uint8_t Addr = 0;
+  for (const auto &CU : compile_units()) {
+    Addr = CU->getAddressByteSize();
+    break;
+  }
+  return Addr;
+}

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

+//===- DWARFDebugAddr.cpp -------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/DebugInfo/DWARF/DWARFDebugAddr.h"
+#include "llvm/BinaryFormat/Dwarf.h"
+#include "llvm/DebugInfo/DWARF/DWARFUnit.h"
+
+using namespace llvm;
+
+void DWARFDebugAddrTable::clear() {
+  HeaderData = {};
+  Addrs.clear();
+}
+
+Error DWARFDebugAddrTable::extract(DWARFDataExtractor Data,
+                                   uint32_t *OffsetPtr,
+                                   uint16_t Version,
+                                   uint8_t AddrSize) {
+  HeaderOffset = *OffsetPtr;
+  // Read and verify the length field.
+  if (!Data.isValidOffsetForDataOfSize(*OffsetPtr, sizeof(uint32_t)))
+    return createStringError(errc::argument_out_of_domain,
+                       "section is not large enough to contain a "
+                       ".debug_addr table length at offset 0x%"
+                       PRIx32 "\n", *OffsetPtr);
+  uint16_t SuggestedVersion;
+  if (Version == 0) {
+    errs() << "WARNING: CU version is not defined, assuming version 5\n";
+    SuggestedVersion = 5;
+  } else {
+    SuggestedVersion = Version;
+  }
+  // TODO: Add support for DWARF64.
+  Format = dwarf::DwarfFormat::DWARF32;
+  if (SuggestedVersion >= 5) {
+    HeaderData.Length = Data.getU32(OffsetPtr);
+    if (HeaderData.Length == 0xffffffffu)
+      return createStringError(errc::invalid_argument,
+          "DWARF64 is not supported in .debug_addr at offset 0x%" PRIx32 "\n",
+          HeaderOffset);
+    if (HeaderData.Length + sizeof(uint32_t) < sizeof(Header))
+      return createStringError(errc::argument_out_of_domain,
+                         ".debug_addr table at offset 0x%" PRIx32
+                         " has too small length (0x%" PRIx32
+                         ") to contain a complete header\n",
+                         HeaderOffset, getLength());
+    uint32_t End = HeaderOffset + getLength();
+    if (!Data.isValidOffsetForDataOfSize(HeaderOffset, End - HeaderOffset))
+      return createStringError(errc::argument_out_of_domain,
+          "section is not large enough to contain a .debug_addr table "
+          "of length 0x%" PRIx32 " at offset 0x%" PRIx32 "\n",
+          getLength(), HeaderOffset);
+
+    HeaderData.Version = Data.getU16(OffsetPtr);
+    HeaderData.AddrSize = Data.getU8(OffsetPtr);
+    HeaderData.SegSize = Data.getU8(OffsetPtr);
+    DataSize = getDataSize();
+  } else {
+    HeaderData.Version = SuggestedVersion;
+    HeaderData.AddrSize = AddrSize;
+    // TODO: Support for non-zero SegSize.
+    HeaderData.SegSize = 0;
+    DataSize = Data.size();
+  }
+
+  // Perform basic validation of the remaining header fields.
+  if (HeaderData.Version != SuggestedVersion)
+    return createStringError(errc::invalid_argument,
+                       ".debug_addr table version %" PRIu16
+                       " in table at offset 0x% " PRIx32
+                       " is different from CU version 0x" PRIu16 "\n",
+                       HeaderData.Version, HeaderOffset, SuggestedVersion);
+  if (HeaderData.AddrSize != 4 && HeaderData.AddrSize != 8)
+    return createStringError(errc::invalid_argument,
+                       ".debug_addr table at offset 0x%" PRIx32
+                       " has unsupported address size %hhu\n",
+                       HeaderOffset, HeaderData.AddrSize);
+  if (HeaderData.AddrSize != AddrSize && AddrSize != 0)
+    return createStringError(errc::invalid_argument,
+                       ".debug_addr table at offset 0x%" PRIx32
+                       " has address size %hhu"
+                       " which is different from CU address size %hhu\n",
+                       HeaderOffset, HeaderData.AddrSize, AddrSize);
+
+  // TODO: add support for non-zero SegSize
+  if (HeaderData.SegSize != 0)
+    return createStringError(errc::invalid_argument,
+                       ".debug_addr table at offset 0x%" PRIx32
+                       " has unsupported segment selector size %" PRIu8 "\n",
+                       HeaderOffset, HeaderData.SegSize);
+  if (DataSize % HeaderData.AddrSize != 0)
+    return createStringError(errc::invalid_argument,
+                       ".debug_addr table at offset 0x%" PRIx32
+                       " contains data of size %" PRIu32
+                       " which is not a multiple of addr size %" PRIu8 "\n",
+                       HeaderOffset, DataSize, HeaderData.AddrSize);
+  Data.setAddressSize(HeaderData.AddrSize);
+  uint32_t AddrCount = DataSize / HeaderData.AddrSize;
+  for (uint32_t I = 0; I < AddrCount; ++I)
+    if (HeaderData.AddrSize == 4)
+      Addrs.push_back(Data.getU32(OffsetPtr));
+    else
+      Addrs.push_back(Data.getU64(OffsetPtr));
+  return Error::success();
+}
+
+void DWARFDebugAddrTable::dump(raw_ostream &OS, DIDumpOptions DumpOpts) const {
+  if (DumpOpts.Verbose)
+    OS << format("0x%8.8" PRIx32 ": ", HeaderOffset);
+  OS << format("Addr Section params: length = 0x%8.8" PRIx32
+               ", version = 0x%4.4" PRIx16 ", "
+               "addr_size = 0x%2.2" PRIx8 ", seg_size = 0x%2.2" PRIx8 "\n",
+               HeaderData.Length, HeaderData.Version, HeaderData.AddrSize,
+               HeaderData.SegSize);
+
+  if (Addrs.size() > 0) {
+    OS << "Addrs: [";
+    for (const auto &Addr : Addrs) {
+      OS << format("\n0x%8.8" PRIx32, Addr); // TODO: 64bit addrs
+      if (DumpOpts.Verbose)
+        OS << format(" => 0x%8.8" PRIx32,
+                     Addr + HeaderOffset + sizeof(HeaderData));
+    }
+    OS << "\n]\n";
+  }
+}
+
+uint32_t DWARFDebugAddrTable::getLength() const {
+  if (HeaderData.Length == 0)
+    return 0;
+  // TODO: DWARF64 support.
+  return HeaderData.Length + sizeof(uint32_t);
+}
+
+uint32_t DWARFDebugAddrTable::getDataSize() const {
+  if (DataSize != 0)
+    return DataSize;
+  if (getLength() == 0)
+    return 0;
+  return getLength() - getHeaderSize();
+}

test/tools/llvm-dwarfdump/X86/debug_addr.s

+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o %t.o
+# RUN: llvm-dwarfdump --debug-addr %t.o | FileCheck %s
+
+# CHECK: .debug_addr contents    
+# CHECK-NEXT:     length = 0x0000000c, version = 0x0005, addr_size = 0x04, seg_size = 0x00
+# CHECK-NEXT:     Addrs: [
+# CHECK-NEXT:     0x00000000
+# CHECK-NEXT:     0x00000001
+
+  .section  .debug_addr,"",@progbits
+.Ldebug_addr0:
+  .long 12 # unit_length = .short + .byte + .byte + .long + .long
+  .short 5 # version
+  .byte 4 # address_size
+  .byte 0 # segment_selector_size
+  .long 0x00000000
+  .long 0x00000001

test/tools/llvm-dwarfdump/X86/debug_addr_absent.s

+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
+# RUN: llvm-dwarfdump -debug-addr - | FileCheck %s
+# CHECK: .debug_addr contents:
+# CHECK-NOT: Addr
+# CHECK-NOT: error:

test/tools/llvm-dwarfdump/X86/debug_addr_dwarf4.s

+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o %t.o
+# RUN: llvm-dwarfdump --debug-addr %t.o | FileCheck %s
+
+# CHECK: .debug_addr contents
+# CHECK-NEXT:     length = 0x00000000, version = 0x0004, addr_size = 0x04, seg_size = 0x00
+# CHECK-NEXT:     Addrs: [
+# CHECK-NEXT:     0x00000000
+# CHECK-NEXT:     0x00000001
+
+  .section  .debug_abbrev,"",@progbits
+  .byte 1                       # Abbreviation Code
+  .section  .debug_info,"",@progbits
+.Lcu_begin0:
+  .long 7                       # Length of Unit
+  .short  4                     # DWARF version number
+  .long .debug_abbrev           # Offset Into Abbrev. Section
+  .byte 4                       # Address Size (in bytes)
+  .section  .debug_addr,"",@progbits
+  .long 0x00000000
+  .long 0x00000001
+.Lline_table_start0:

test/tools/llvm-dwarfdump/X86/debug_addr_empty.s

+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
+# RUN: llvm-dwarfdump -debug-addr - | FileCheck %s
+# CHECK: .debug_addr contents:
+# CHECK-NOT: Addr
+# CHECK-NOT: error:
+
+.section .debug_addr,"",@progbits

test/tools/llvm-dwarfdump/X86/debug_addr_invalid.s

+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o %t.o
+# RUN: llvm-dwarfdump --debug-addr %t.o 2>&1 | FileCheck %s
+
+# CHECK: unsupported address size 3
+# CHECK: unsupported segment selector size 1
+# CHECK: is not a multiple of addr size 4
+
+# invalid addr size
+  .section  .debug_addr,"",@progbits
+.Ldebug_addr1:
+  .long 12 # unit_length = .short + .byte + .byte + .long + .long
+  .short 5 # version
+  .byte 3 # address_size
+  .byte 0 # segment_selector_size
+  .long 0x00000000
+  .long 0x00000001
+
+# unsupported segment selector size
+  .section  .debug_addr,"",@progbits
+.Ldebug_addr2:
+  .long 12 # unit_length = .short + .byte + .byte + .long + .long
+  .short 5 # version
+  .byte 4 # address_size
+  .byte 1 # segment_selector_size
+  .long 0x00000000
+  .long 0x00000001
+
+# invalid length
+  .section  .debug_addr,"",@progbits
+.Ldebug_addr3:
+  .long 11 # unit_length = .short + .byte + .byte + .long + .long
+  .short 5 # version
+  .byte 4 # address_size
+  .byte 0 # segment_selector_size
+  .long 0x00000000
+  .long 0x00000001