This is an archive of the discontinued LLVM Phabricator instance.

Fix the verification of DIEs with DW_AT_ranges.
ClosedPublic

Authored by clayborg on May 14 2020, 1:33 PM.

Details

Summary

There was a bug when processing the DW_AT_ranges for a DIE where as soon as an overlapping address range was found, we would stop processing and further ranges in a DW_AT_ranges set of address ranges. This means the DIE, usually a DW_AT_compile_unit, would end up with a truncated list of address ranges. This would cause spurious errors to be emitted stating that a DIE was not contained in a parent's address ranges.

This has now been fixed and added a fix and a test.

Diff Detail

Event Timeline

clayborg created this revision.May 14 2020, 1:33 PM
Herald added a project: Restricted Project. · View Herald Transcript

The verification for:

0x0000000b: DW_TAG_compile_unit
              DW_AT_name        ("/tmp/main.c")
              DW_AT_language    (DW_LANG_C_plus_plus)
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_ranges      (0x00000000
                 [0x0000000000000000, 0x0000000000000020)
                 [0x0000000000000000, 0x0000000000000030)
                 [0x0000000000001000, 0x0000000000002000))
              DW_AT_stmt_list   (0x00000000)

0x00000022:   DW_TAG_subprogram
                DW_AT_name      ("stripped1")
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x0000000000000020)

0x00000033:   DW_TAG_subprogram
                DW_AT_name      ("stripped2")
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x0000000000000030)

0x00000048:   DW_TAG_subprogram
                DW_AT_name      ("main")
                DW_AT_low_pc    (0x0000000000001000)
                DW_AT_high_pc   (0x0000000000002000)

0x00000059:   NULL

Now looks like:

$ ../debug/bin/llvm-dwarfdump --verify /tmp/a.out
Verifying /tmp/a.out:   file format Mach-O 64-bit x86-64
Verifying .debug_abbrev...
Verifying .debug_info Unit Header Chain...
error: DIE has overlapping ranges in DW_AT_ranges attribute: [0x0000000000000000, 0x0000000000000020) and [0x0000000000000000, 0x0000000000000030)
0x0000000b:   DW_TAG_compile_unit
                DW_AT_name      ("/tmp/main.c")
                DW_AT_language  (DW_LANG_C_plus_plus)
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_ranges    (0x00000000
                   [0x0000000000000000, 0x0000000000000020)
                   [0x0000000000000000, 0x0000000000000030)
                   [0x0000000000001000, 0x0000000000002000))
                DW_AT_stmt_list (0x00000000)

error: DIEs have overlapping address ranges:
0x00000033: DW_TAG_subprogram
              DW_AT_name        ("stripped2")
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_high_pc     (0x0000000000000030)

0x00000022: DW_TAG_subprogram
              DW_AT_name        ("stripped1")
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_high_pc     (0x0000000000000020)

Verifying .debug_info references...
Verifying .debug_types Unit Header Chain...
Errors detected.

I'm not able to understand the description of the original bug, perhaps you could show the original behavior for your test case for comparison?

llvm/include/llvm/DebugInfo/DWARF/DWARFAddressRange.h
58

Probably could use a more explicit name - "union_if_intersecting"?

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
48

No need to have the llvm:: qualification here.

416

Where have you seen -1? Both gold and lld use 0, binutils ld... is more complicated, but I don't think it uses -1? hmm, maybe it does... hmm, at least for ranges it uses 1: [0x0000000000000001, 0x0000000000000001) which is different from gold and lld but for the address it uses zero:

DW_AT_low_pc    (0x0000000000000000)
DW_AT_high_pc   (0x0000000000000006)
clayborg marked 2 inline comments as done.May 14 2020, 6:59 PM

I'm not able to understand the description of the original bug, perhaps you could show the original behavior for your test case for comparison?

I will improve the description and improve the test to include some checks to make sure something is not there.

Basically if the DW_TAG_compile_unit had a DW_AT_ranges that had overlapping ranges, there was a break statement that would stop it from gathering all of the valid ranges for the compile unit. Then and DIEs whose address ranges didn't get added to the CU's ranges would emit an error. Here is a real example with some names changed:

error: DIE has overlapping address ranges: [0x0000000000000000, 0x0000000000000008) and [0x0000000000000000, 0x000000000000001e)
error: DIE address ranges are not contained in its parent's ranges:
0x0002022e: DW_TAG_compile_unit
              DW_AT_producer	("...")
              DW_AT_language	(DW_LANG_C_plus_plus)
              DW_AT_name	("...")
              DW_AT_stmt_list	(0x00004f60)
              DW_AT_comp_dir	(".")
              DW_AT_GNU_pubnames	(true)
              DW_AT_low_pc	(0x0000000000000000)
              DW_AT_ranges	(0x000010b0
                 [0x000045c4, 0x000045e8)
                 [0x00000000, 0x0000001e)
                 [0x00000000, 0x00000008)
                 [0x000049bc, 0x000049c0)
                 [0x000049c0, 0x00004a08)
                 [0x00004a08, 0x00004a0e))

0x0002f1f1:   DW_TAG_subprogram
                DW_AT_low_pc	(0x00000000000049c0)
                DW_AT_high_pc	(0x0000000000004a08)
                DW_AT_frame_base	(DW_OP_reg13)
                DW_AT_object_pointer	(0x0002f214)
                DW_AT_linkage_name	("_ZNSt12__shared_ptr...")
                DW_AT_specification	(0x00022eef)

But note that this range [0x49c0-0x4a08) IS in the DW_AT_ranges of the CU. So even if there are overlapping address ranges, we need to report them all and then shoe the DIE. I also modified the warning message to let you know that there are ovarlapping DW_AT_ranges in a DIE to make this more clear what is overlapping. So this diff is fixing this incorrect error reporting for "DIE address ranges are not contained in its parent's ranges".

llvm/include/llvm/DebugInfo/DWARF/DWARFAddressRange.h
58

that works

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
416

I believe Paul Robinson stated that the Sony linker does this. Paul did I remember correctly?

clayborg edited the summary of this revision. (Show Details)May 14 2020, 7:02 PM

I updated the description to better explain the fix.

clayborg marked an inline comment as done.May 14 2020, 7:04 PM
clayborg added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
413

This was the problem "break;" that would stop parsing each range in "Ranges" and cause the address ranges for a DIE with DW_AT_ranges to be truncated. We still want a complete address range list for verification purposes.

Makes rough sense, but I'll leave it to folks who've worked more closely on the validator to chime in/approve (already cc'd)

clayborg updated this revision to Diff 264144.May 14 2020, 7:38 PM

Fixed test case to properly check that we do not see a "error: DIE address ranges are not contained in its parent's ranges:" string in the output. The old llvm-dwarfdump --verify would have this incorrect error.

clayborg updated this revision to Diff 264145.May 14 2020, 7:41 PM

Rename union_range to union_if_intersecting and and remove llvm:: qualification on None.

clayborg marked 2 inline comments as done.May 14 2020, 7:42 PM

All issues should be resolved. Test case was improved to verify old behavior doesn't persist by adding a CHECK-NOT in the test case.

jhenderson added inline comments.May 15 2020, 1:24 AM
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
416

Not got time to do a full review myself today, but just chiming in to confirm that the Sony linker patches low_pc to -1 for stripped stuff in .debug_info and .debug_aranges. For .debug_ranges we actually use -2/-2 pairs because -1 is a special value in that section.

I vaguely recall that there was some discussion on the mailing list about this a couple of years ago, and that there may be some other users of this approach too.

Ping in case anyone has time to help move this forward.

I don't know if you've noticed, but you might be interested in following @Higuoxing's GSOC project to implement DWARF support for ELF yaml2obj. That would allow you to write this test using ELF, and omit 99% of the noise (to be clear, I'm not suggesting waiting, but it should help you in the future). If you've got any specific comments on his proposal, there's an RFC that's just gone up on the mailing list. I'd hope that some of the principles in that project (auto-generating most of the debug data) might even be more generically useful for Mach-O in the future.

llvm/include/llvm/DebugInfo/DWARF/DWARFAddressRange.h
58

Shouldn't it be unionIfIntersecting?

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
35–44

I think this and the similar code below be simplified slightly:

DWARFAddressRange Range(*Pos);
if (Pos->union_if_intersecting(R))
  return Range;

Alternatively, get rid of the "if intersecting" part of the union function (perhaps make it an assertion instead, but I'm not sure).

416

Some more related thoughts:

A range of (0, 0) is the end marker, whilst (-1, ...) is a base address selection entry. I don't know how these are represented (if at all) in the table. Either way, as mentioned (-2, -2) is a range we use for deadstripped code, so if that can be handled, we'd be very grateful! Last time I checked (admittedly quite a long while ago) DWARF verification didn't work for us because of stripped stuff (and possibly other reasons, but we haven't bothered digging further yet).

llvm/test/tools/llvm-dwarfdump/X86/verify_overlapping_cu_ranges.yaml
68

Maybe it would be better to add --implicit-check-not=error: to FileCheck's invocation. That way, all errors will be detected, regardless of position, and will avoid this check passing spuriously in the future should the text of the message ever change.

clayborg marked 4 inline comments as done.May 21 2020, 11:41 AM
clayborg added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFAddressRange.h
58

yep, I will change it

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
39

True. I will do rename to "union" since we already know it intersects.

416

My next patch will be to calculate all of the read + execute section ranges and verify all addresses against this. If the address ranges aren't in the read+execute ranges, then ignore the address (on fully linked input files only, not .o files). This will help clear out incorrect errors that find overlapping address ranges. That should be enough.

llvm/test/tools/llvm-dwarfdump/X86/verify_overlapping_cu_ranges.yaml
68

that is a good idea. I haven't used that flag before.

jhenderson added inline comments.May 22 2020, 12:32 AM
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
39

Hehe, just realised that union might not be an option for keyword reasons... Maybe something like merge would be okay.

clayborg updated this revision to Diff 271540.Jun 17 2020, 5:56 PM
clayborg marked an inline comment as done.

Simplified logic in DieRangeInfo::insert() and switch DWARFAddressRange::union_if_intersecting() to DWARFAddressRange::merge().

Higuoxing added inline comments.Jun 17 2020, 7:40 PM
llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h
64

"if a DIE" -> "of a DIE" ?

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
402

I think the variable name GotError is a little bit ambiguous here. What about renaming it to InvalidOrOverlapped or something else?

clayborg updated this revision to Diff 271559.Jun 17 2020, 8:18 PM

Fixed "if a DIE" to be "of a DIE" and renamed GotError to DumpDieAfterError.

clayborg marked 2 inline comments as done.Jun 17 2020, 8:18 PM
clayborg added inline comments.
llvm/test/tools/llvm-dwarfdump/X86/verify_overlapping_cu_ranges.yaml
68

Actually can't include this because there is an error before this on line 44

clayborg marked an inline comment as done.Jun 17 2020, 8:19 PM
clayborg added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
402

Yeah, I need something to dump the DIE once if we find any issues with its attributes and have already emitted an error message.

jhenderson added inline comments.Jun 18 2020, 5:56 AM
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
416

By the way, LLD is about to start patching -1/-2 for .debug data addresses to indicate "ignore me". See D81784.

llvm/test/tools/llvm-dwarfdump/X86/verify_overlapping_cu_ranges.yaml
68

--implicit-check-not only fails if the string is found and it wasn't matched by an explicit check somewhere else. Consequently, the line 44 check will mean --implicit-check-not should still work. It will only not work if there are other errors for this test case which aren't being checked for.

clayborg updated this revision to Diff 271803.Jun 18 2020, 11:59 AM
clayborg marked an inline comment as done.

Use "--implicit-check-not=error:" in test to ensure no other errors are found.

clayborg marked an inline comment as done.Jun 18 2020, 11:59 AM
clayborg added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
416

That is great. The next patch that excludes all ranges that are not in sections with read + execute will easily weed those out and also not report errors when we find those ranges.

jhenderson accepted this revision.Jun 19 2020, 12:49 AM

LGTM, with one small comment.

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
416

-1 probably should be -2 in this context, since -1 is for base address selection entries. In the Sony linker and the LLD patch, we use -2 for .debug_ranges and dead values (-1 is used elsewhere where it can be).

This revision is now accepted and ready to land.Jun 19 2020, 12:49 AM
MaskRay added inline comments.Jun 19 2020, 9:51 AM
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
416

For .debug_ranges

  • GNU ld: [1,1)
  • gold and LLD before D81988: [addend0, addend1) ( addend0 is very likely 0 )
  • LLD since D81988: [-2,-2)
This revision was automatically updated to reflect the committed changes.