[PATCH] D136395: Add the ability to verify the .debug_aranges section.
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 25 14:45:43 PDT 2022
dblaikie added inline comments.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:1043-1046
+ // Make sure all .debug_aranges descriptor ranges are found in the
+ // compile unit's ranges.
+ uint32_t DescIdx = 0;
+ for (const auto &Descriptor: ArangeSet.descriptors()) {
----------------
clayborg wrote:
> dblaikie wrote:
> > Pretty sure this won't be true for clang/llvm's debug info - LLVM includes global variable addresses in aranges, but not in CU ranges. We've had various discussions about whether this is correct/useful, and so far people seem to think it is correct and maybe useful? I don't really know. GCC doesn't put global variable addresses in either aranges or cu ranges.
> >
> > (I'm pretty sure it's technically correct, but maybe not all that useful - at least if GCC doesn't do it (pretty sure it's generally correct that CU ranges shouldn't include global variable addresses, at least - so just a question of what should/shouldn't be in aranges))
> > Pretty sure this won't be true for clang/llvm's debug info - LLVM includes global variable addresses in aranges, but not in CU ranges. We've had various discussions about whether this is correct/useful, and so far people seem to think it is correct and maybe useful? I don't really know. GCC doesn't put global variable addresses in either aranges or cu ranges.
>
> Interesting. We should be including it all address ranges for functions and globals in both .debug_aranges and the DW_AT_ranges for the compile unit, or not for either. Otherwise we are making a case for .debug_aranges to continue to exist.
>
> > (I'm pretty sure it's technically correct, but maybe not all that useful - at least if GCC doesn't do it (pretty sure it's generally correct that CU ranges shouldn't include global variable addresses, at least - so just a question of what should/shouldn't be in aranges))
>
> I would vote to include globals in both, but with the hopes to move to DW_AT_ranges and getting rid of .debug_aranges. Since it hasn't been clarified, maybe we can leave this in for now to see if we can detect this issue with GCC? No one is using llvm-dwarfdump's verify result as something that stops a compilation or build that I am aware of, so it would be good to see this inconsistency IMHO.
> We should be including it all address ranges for functions and globals in both .debug_aranges and the DW_AT_ranges for the compile unit, or not for either.
My reading of the spec is that they're different (aranges includes code and data, ranges only includes code) - see below for spec quotations.
> Otherwise we are making a case for .debug_aranges to continue to exist.
Owing to the practical implementation differences/defacto standard GCC provides, I don't think we need to worry about that - the majority of tooling won't be depending on global variables being in aranges.
If anyone knows of anything that depends on aranges that can't be reimplemented with similar performance using CU ranges (so, especially, anything that depends on the data addresses being in aranges - which only clang does, and it doesn't enable aranges by default anyway...) I'd love to hear about it.
> I would vote to include globals in both, but with the hopes to move to DW_AT_ranges and getting rid of .debug_aranges. Since it hasn't been clarified, maybe we can leave this in for now to see if we can detect this issue with GCC? No one is using llvm-dwarfdump's verify result as something that stops a compilation or build that I am aware of, so it would be good to see this inconsistency IMHO.
It won't detect an issue in GCC - more likely an issue in clang where there will be entries in .debug_aranges that aren't in CU ranges.
(but yeah, if you looked further, and believe that CU ranges should include global variable addresses - then verifying that relationship (unrelated to aranges) would show another issue with Clang, and /then/ if you verified aranges against raw debug_info addresses (skipping CU ranges) you'd see the GCC issue, maybe)
The DWARF spec says that aranges entries contain "the beginning address ... of text or data covered by some entry owned by the corresponding CU" which supports the idea of having variable data in aranges. But the DWARF spec says that DW_AT_low_pc/high_pc are about describing "an entity that has a machine code address or range of machine code addresses".
So I believe Clang is technically correct, and aranges should/can contain things that aren't in CU ranges (& cu ranges shouldn't include addresses for global data).
But I don't think a consumer can reasonably rely on this guarantee due to GCC's different choice of what it puts in aranges.
test.cpp:
```
extern int x;
int x = 3;
void f1() { }
```
```
$ g++-12 -g -c test.cpp && llvm-dwarfdump-tot test.o -debug-aranges -debug-info
DW_TAG_compile_unit
...
DW_AT_low_pc (0x0000000000000000)
DW_AT_high_pc (0x0000000000000007)
...
DW_TAG_variable
DW_AT_location (DW_OP_addrx 0x0)
DW_TAG_subprogram
DW_AT_low_pc (0x0000000000000000)
DW_AT_high_pc (0x0000000000000006)
.debug_aranges contents:
Address Range Header: length = 0x0000002c, format = DWARF32, version = 0x0002, cu_offset = 0x00000000, addr_size = 0x08, seg_size = 0x00
[0x0000000000000000, 0x0000000000000007)
```
```
Whereas Clang's aranges:
$ clang++-tot -gdwarf-aranges -g -c test.cpp && llvm-dwarfdump-tot test.o -debug-aranges
...
.debug_aranges contents:
Address Range Header: length = 0x0000003c, format = DWARF32, version = 0x0002, cu_offset = 0x00000000, addr_size = 0x08, seg_size = 0x00
[0x0000000000000000, 0x0000000000000004)
[0x0000000000000000, 0x0000000000000006)
```
(the .debug_info/CU is roughly the same as for GCC - nothing interesting to show there)
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:1078-1088
+ // Now make sure that all CU ranges are in the .debug_aranges set.
+ for (const auto &Range: SortedCURanges) {
+ if (!SortedAranges.contains(Range)) {
+ errorHeader(error());
+ OS << " compile unit range ["
+ << format_hex(Range.start(), AddrPrintSize)
+ << " - " << format_hex(Range.end(), AddrPrintSize)
----------------
clayborg wrote:
> dblaikie wrote:
> > Rather than scanning in both directions (does everything in aranges appear in CU ranges, then does everything in CU ranges appear in aranges) - maybe sort both and do a single scan through & just flag the first place they're inconsistent? (or, if accounting for the "aranges has extra stuff for global variables", skip over those entries if you can until you pick up in the CU ranges - if you get to the end without having visited all the CU ranges then there's something wrong at least)
> > Rather than scanning in both directions (does everything in aranges appear in CU ranges, then does everything in CU ranges appear in aranges) - maybe sort both and do a single scan through & just flag the first place they're inconsistent?
>
> I would personally prefer to see all ranges that are missing to indicate how many times we have issues in the .debug_aranges. If we just flag the first inconsistency then we only know that there was at least one error. The reason I like seeing all of these issues is when thinks fail in a debugger, like "I was looking up address 0x123456 in this binary and it failed", it is nice to see an error in the "llvm-dwarfdump --verify" output that lists the range as having an error so we know this can be the reason it failed. Similar issues with anyone doing symbolication would exist as well.
>
> > (or, if accounting for the "aranges has extra stuff for global variables", skip over those entries if you can until you pick up in the CU ranges - if you get to the end without having visited all the CU ranges then there's something wrong at least)
>
> I am thinking of solutions for the global variable problem. One idea is to try and extract all ranges for any data sections (read only and read + write), and then if we don't find a .debug_aranges range in the DW_AT_ranges, then we check if that address is in the DataRanges and if so, don't produce an error? Do we want to warn?
>
> I am not opposed to seeing the full inconsistency though as if something is in .debug_aranges, it should really also been DW_AT_ranges IMHO. Do you agree? If we already have something that differs where global vars are in .debug_aranges but not in DW_AT_ranges, this should be fixed somehow if we want to get rid of .debug_aranges.
>
>
> I would personally prefer to see all ranges that are missing to indicate how many times we have issues in the .debug_aranges.
Fair enough - could probably still be achieved in a single scan if both lists are sorted first? Do it like a merge deduplication - walk each list so long as its less than the next entry in the other list - skip over any entries that are earlier than the other list, then report all those as missing once you find the next matching entry?
> I am not opposed to seeing the full inconsistency though as if something is in .debug_aranges, it should really also been DW_AT_ranges IMHO. Do you agree?
Nah, I don't think that's something to warn or error on - the spec seems pretty clear that aranges should include data and code addresses and high_pc/low_pc/ranges should only include code addresses.
So, yeah, if you want to implement this on-spec, you'd need to search through all the DIEs looking for code addresses (which is non-trivial, since you have to go look in global variable exprlocs and go scrumping through the operations to pick the address operations out of them - and determining the length you wan tto use to verify against aranges would be non-trivial)
It's complexity like this that is why I'd rather not do any of this work - flag aranges as deprecated and move on.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:1093-1130
+ const DWARFDebugLine::LineTable *LT = DCtx.getLineTableForUnit(CU);
+ if (LT) {
+ for (const auto &Sequence: LT->Sequences) {
+ // If the first address in a sequence is invalid, don't verify it as
+ // it should have been dead stripped.
+ const DWARFDebugLine::Row &Row = LT->Rows[Sequence.FirstRowIndex];
+ if (DWARFAddressIsInvalid(Row.Address.Address, SetHdr.AddrSize))
----------------
clayborg wrote:
> dblaikie wrote:
> > Do we already have something like this for CU ranges? Could we reuse it?
> We don't currently. It would be easy to move this elsewhere so we can use it when checking .debug_line sections where that code would verify against either the DW_TAG_compile_unit's DW_AT_ranges, or in this case maybe it would only check against the .debug_aranges ranges. The hard part there is determining which addresses have the first address in each sequence being invalid as most dead stripping will just zeroing out the start address of a function. So we end up with tons of overlapping address ranges at zero all the time and this info really shouldn't be checked.
Here's the function I was thinking of: https://github.com/llvm/llvm-project/blob/2bdfececef4330b3a6489cdb67c57eb771d5f9e4/llvm/include/llvm/BinaryFormat/Dwarf.h#L780
Looks like for old school .debug_ranges we just subtract 1 from that with a comment to explain: https://github.com/llvm/llvm-project/blob/2bdfececef4330b3a6489cdb67c57eb771d5f9e4/llvm/lib/DebugInfo/DWARF/DWARFDebugRangeList.cpp#L92
So we could roll that into a helper function (either the same helper function with an extra parameter to the helper function or some other way).
Perhaps all this code could just use a higher level abstraction - but I guess maybe we don't have a higher level abstraction for aranges, unlike we do for ranges (see the code above and the general `DIE::getAddressRanges` which returns ranges not including dead code using whatever the appropriate tombstone values are) - these don't support 0 as a tombstone owing to its ambiguity/uncertainty.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136395/new/
https://reviews.llvm.org/D136395
More information about the llvm-commits
mailing list