Skip to content

Add support for verifying .debug_names in split DWARF for CUs and TUs. #101775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 14, 2024
Merged

Add support for verifying .debug_names in split DWARF for CUs and TUs. #101775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c84ff2
Add support for verifying .debug_names in split DWARF for CUs and TUs.
clayborg Aug 2, 2024
c6febf0
Add tests.
clayborg Aug 5, 2024
dacdf67
Add missing test files.
clayborg Aug 5, 2024
36079f2
Add a comment for a FIXME for a follow up PR.
clayborg Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,9 @@ class DWARFContext : public DIContext {
DWARFCompileUnit *getDWOCompileUnitForHash(uint64_t Hash);
DWARFTypeUnit *getTypeUnitForHash(uint16_t Version, uint64_t Hash, bool IsDWO);

/// Return the DWARF unit that includes an offset (relative to .debug_info).
DWARFUnit *getUnitForOffset(uint64_t Offset);

/// Return the compile unit that includes an offset (relative to .debug_info).
DWARFCompileUnit *getCompileUnitForOffset(uint64_t Offset);

Expand Down
7 changes: 5 additions & 2 deletions llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1510,9 +1510,12 @@ DWARFUnitVector &DWARFContext::getDWOUnits(bool Lazy) {
return State->getDWOUnits(Lazy);
}

DWARFUnit *DWARFContext::getUnitForOffset(uint64_t Offset) {
return State->getNormalUnits().getUnitForOffset(Offset);
}

DWARFCompileUnit *DWARFContext::getCompileUnitForOffset(uint64_t Offset) {
return dyn_cast_or_null<DWARFCompileUnit>(
State->getNormalUnits().getUnitForOffset(Offset));
return dyn_cast_or_null<DWARFCompileUnit>(getUnitForOffset(Offset));
}

DWARFCompileUnit *DWARFContext::getCompileUnitForCodeAddress(uint64_t Address) {
Expand Down
66 changes: 63 additions & 3 deletions llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1626,8 +1626,63 @@ unsigned DWARFVerifier::verifyNameIndexEntries(
UnitOffset = NI.getCUOffset(*CUIndex);
if (!UnitOffset)
continue;
uint64_t DIEOffset = *UnitOffset + *EntryOr->getDIEUnitOffset();
DWARFDie DIE = DCtx.getDIEForOffset(DIEOffset);
// For split DWARF entries we need to make sure we find the non skeleton
// DWARF unit that is needed and use that's DWARF unit offset as the
// DIE offset to add the DW_IDX_die_offset to.
DWARFUnit *DU = DCtx.getUnitForOffset(*UnitOffset);
if (DU == nullptr || DU->getOffset() != *UnitOffset) {
// If we didn't find a DWARF Unit from the UnitOffset, or if the offset
// of the unit doesn't match exactly, report an error.
ErrorCategory.Report(
"Name Index entry contains invalid CU or TU offset", [&]() {
error() << formatv("Name Index @ {0:x}: Entry @ {1:x} contains an "
"invalid CU or TU offset {1:x}.\n",
NI.getUnitOffset(), EntryID, *UnitOffset);
});
++NumErrors;
continue;
}
// This function will try to get the non skeleton unit DIE, but if it is
// unable to load the .dwo file from the .dwo or .dwp, it will return the
// unit DIE of the DWARFUnit in "DU". So we need to check if the DWARFUnit
// has a .dwo file, but we couldn't load it.

// FIXME: Need a follow up patch to fix usage of
// DWARFUnit::getNonSkeletonUnitDIE() so that it returns an empty DWARFDie
// if the .dwo file isn't available and clean up other uses of this function
// call to properly deal with it. It isn't clear that getNonSkeletonUnitDIE
// will return the unit DIE of DU if we aren't able to get the .dwo file,
// but that is what the function currently does.
DWARFDie NonSkeletonUnitDie = DU->getNonSkeletonUnitDIE();
if (DU->getDWOId() && DU->getUnitDIE() == NonSkeletonUnitDie) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this be a more clear check?
DWARFUnit *DWOCU =
DwarfUnit->getNonSkeletonUnitDIE(false, AbsolutePath).getDwarfUnit();
if (!DWOCU->isDWOUnit()) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the .dwo file isn't found, DU->getNonSkeletonUnitDIE() returns the skeleton compile unit, and then NonSkeletonUnitDie would say that it isn't a DWO file. We need to ask the skeleton unit if it has a DWO ID, and if it does and the DIEs are the same, then we know we didn't find the .dwo file (in a .dwo or .dwp). So for a normal DW_TAG_compile_unit the DU->getNonSkeletonUnitDIE() will return the same DIE and DU won't have a DWO ID.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making getNonSkeletonUnitDIE return a null DWARFDIE if this is a skeleton unit but the split full unit can't be found seems reasonable - I doubt the behavior was intentional/explicitly desired, just falls out by accident

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is intentional and expected behavior because as it if want all DIEs from a skeleton unit or a normal CU or TU, you call this API to try to get the DIEs regardless of if it is a skeleton unit or not.

  DWARFDie getNonSkeletonUnitDIE(bool ExtractUnitDIEOnly = true,
                                 StringRef DWOAlternativeLocation = {}) {
    parseDWO(DWOAlternativeLocation);
    return DWO ? DWO->getUnitDIE(ExtractUnitDIEOnly)
               : getUnitDIE(ExtractUnitDIEOnly);
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is kind of weird when one calls getNonSkeletonUnitDIE, and gets SkeletonUnitDIE.
Poking around seems like only place this logic is used is in LVDWARFReader::createScopes(), and maybe in dwarfdump::collectStatsForObjectFile.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is intentional and expected behavior because as it if want all DIEs from a skeleton unit or a normal CU or TU, you call this API to try to get the DIEs regardless of if it is a skeleton unit or not.

The behavior of returning the non-split unit DIE makes sense given the name, but the behavior of returning the skeleton unit DIE if the split unit can't be found seems surprising/unintentional.

Returning the non-split unit DIE allows the "calling this function regardless of if it is a skeleton unit or not" work out, which seems good - but returning the skeleton unit DIE when the split unit isn't found seems like it'd be bad.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. So you want me to fix this as part of this commit? Or in a separate commit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that it's a current behavior, maybe it's best to discuss it in follow up PR, and not let it block this one? :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, leave a FIXME at this call site and on the function itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the FIXME

ErrorCategory.Report("Unable to get load .dwo file", [&]() {
error() << formatv("Name Index @ {0:x}: Entry @ {1:x} unable to load "
".dwo file \"{2}\" for DWARF unit @ {3:x}.\n",
NI.getUnitOffset(), EntryID,
dwarf::toString(DU->getUnitDIE().find(
{DW_AT_dwo_name, DW_AT_GNU_dwo_name})),
*UnitOffset);
});
++NumErrors;
continue;
}
DWARFUnit *NonSkeletonUnit = NonSkeletonUnitDie.getDwarfUnit();
uint64_t DIEOffset =
NonSkeletonUnit->getOffset() + *EntryOr->getDIEUnitOffset();
const uint64_t NextUnitOffset = NonSkeletonUnit->getNextUnitOffset();
// DIE offsets are relative to the specified CU or TU. Make sure the DIE
// offsets is a valid relative offset.
if (DIEOffset >= NextUnitOffset) {
ErrorCategory.Report("NameIndex relative DIE offset too large", [&]() {
error() << formatv("Name Index @ {0:x}: Entry @ {1:x} references a "
"DIE @ {2:x} when CU or TU ends at {3:x}.\n",
NI.getUnitOffset(), EntryID, DIEOffset,
NextUnitOffset);
});
continue;
}
DWARFDie DIE = NonSkeletonUnit->getDIEForOffset(DIEOffset);

if (!DIE) {
ErrorCategory.Report("NameIndex references nonexistent DIE", [&]() {
error() << formatv("Name Index @ {0:x}: Entry @ {1:x} references a "
Expand All @@ -1637,7 +1692,12 @@ unsigned DWARFVerifier::verifyNameIndexEntries(
++NumErrors;
continue;
}
if (DIE.getDwarfUnit()->getOffset() != *UnitOffset) {
// Only compare the DIE we found's DWARFUnit offset if the DIE lives in
// the DWARFUnit from the DW_IDX_comp_unit or DW_IDX_type_unit. If we are
// using split DWARF, then the DIE's DWARFUnit doesn't need to match the
// skeleton unit.
if (DIE.getDwarfUnit() == DU &&
DIE.getDwarfUnit()->getOffset() != *UnitOffset) {
ErrorCategory.Report("Name index contains mismatched CU of DIE", [&]() {
error() << formatv(
"Name Index @ {0:x}: Entry @ {1:x}: mismatched CU of "
Expand Down
99 changes: 99 additions & 0 deletions llvm/test/tools/llvm-dwarfdump/Inputs/verify_split_dwarf_debug_names_exe.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
--- !ELF
FileHeader:
Class: ELFCLASS64
Data: ELFDATA2LSB
Type: ET_EXEC
Machine: EM_X86_64
ProgramHeaders:
- Type: PT_PHDR
Flags: [ PF_R ]
VAddr: 0x200040
Align: 0x8
Offset: 0x40
- Type: PT_LOAD
Flags: [ PF_R ]
FirstSec: .eh_frame
LastSec: .eh_frame
VAddr: 0x200000
Align: 0x1000
Offset: 0x0
- Type: PT_LOAD
Flags: [ PF_X, PF_R ]
FirstSec: .text
LastSec: .text
VAddr: 0x201160
Align: 0x1000
Offset: 0x160
- Type: PT_GNU_STACK
Flags: [ PF_W, PF_R ]
Align: 0x0
Offset: 0x0
Sections:
- Name: .eh_frame
Type: SHT_PROGBITS
Flags: [ SHF_ALLOC ]
Address: 0x200120
AddressAlign: 0x8
Content: 1400000000000000017A5200017810011B0C0708900100001C0000001C000000201000000F00000000410E108602430D064A0C070800000000000000
- Name: .text
Type: SHT_PROGBITS
Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
Address: 0x201160
AddressAlign: 0x10
Content: 554889E5C745FC0000000031C05DC3
- Name: .debug_abbrev
Type: SHT_PROGBITS
AddressAlign: 0x1
Content: 014A00101772171B257625111B12067317000000
- Name: .debug_info
Type: SHT_PROGBITS
AddressAlign: 0x1
Content: 24000000050004080000000048DE180E7C9CA7750100000000080000000001000F00000008000000
- Name: .debug_str_offsets
Type: SHT_PROGBITS
AddressAlign: 0x1
Content: 0C000000050000000900000000000000
- Name: .debug_names
Type: SHT_PROGBITS
AddressAlign: 0x4
Content: 8800000005000000010000000000000000000000030000000300000019000000080000004C4C564D30373030000000000100000002000000030000008973880B6A7F9A7C3080880B140000000B0000001000000000000000060000000C0000000113031304190000022E031304190000032403130419000000013900000000021A0000000003350000000000
- Name: .comment
Type: SHT_PROGBITS
Flags: [ SHF_MERGE, SHF_STRINGS ]
AddressAlign: 0x1
EntSize: 0x1
Content: 004C696E6B65723A204C4C442031392E302E30202868747470733A2F2F6769746875622E636F6D2F636C6179626F72672F6C6C766D2D70726F6A6563742E67697420353562313431303839356236396438653435313766366132613239373862343134623465636163332900636C616E672076657273696F6E2031392E302E30676974202868747470733A2F2F6769746875622E636F6D2F636C6179626F72672F6C6C766D2D70726F6A6563742E67697420353562313431303839356236396438653435313766366132613239373862343134623465636163332900
- Name: .debug_line
Type: SHT_PROGBITS
AddressAlign: 0x1
Content: 590000000500080037000000010101FB0E0D00010101010000000100000101011F010000000003011F020F051E0102000000006A7F8D4D5A8D13A0F16154AB8A56B134040000090260112000000000001605030AAE060B2E0202000101
- Name: .debug_line_str
Type: SHT_PROGBITS
Flags: [ SHF_MERGE, SHF_STRINGS ]
AddressAlign: 0x1
EntSize: 0x1
Content: 2E006D61696E2E63707000
Symbols:
- Name: main.cpp
Type: STT_FILE
Index: SHN_ABS
- Name: main
Type: STT_FUNC
Section: .text
Binding: STB_GLOBAL
Value: 0x201160
Size: 0xF
DWARF:
debug_str:
- main.dwo
- .
- main
- int
- Foo
debug_addr:
- Length: 0xC
Version: 0x5
AddressSize: 0x8
Entries:
- Address: 0x201160
...
37 changes: 37 additions & 0 deletions llvm/test/tools/llvm-dwarfdump/Inputs/verify_split_dwarf_debug_names_main_dwo.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
--- !ELF
FileHeader:
Class: ELFCLASS64
Data: ELFDATA2LSB
Type: ET_REL
Machine: EM_X86_64
SectionHeaderStringTable: .strtab
Sections:
- Name: .debug_str_offsets.dwo
Type: SHT_PROGBITS
Flags: [ SHF_EXCLUDE ]
AddressAlign: 0x1
Content: 24000000050000000000000005000000090000000B0000000D00000011000000810000008A000000
- Name: .debug_str.dwo
Type: SHT_PROGBITS
Flags: [ SHF_EXCLUDE, SHF_MERGE, SHF_STRINGS ]
AddressAlign: 0x1
EntSize: 0x1
Content: 6D61696E00696E740066007800466F6F00636C616E672076657273696F6E2031392E302E30676974202868747470733A2F2F6769746875622E636F6D2F636C6179626F72672F6C6C766D2D70726F6A6563742E676974203535623134313038393562363964386534353137663661326132393738623431346234656361633329006D61696E2E637070006D61696E2E64776F00
- Name: .debug_info.dwo
Type: SHT_PROGBITS
Flags: [ SHF_EXCLUDE ]
AddressAlign: 0x1
Content: 46000000050005080000000048DE180E7C9CA77501052100060702000F000000015600000535000000030291780200063900000000040105040505040400010603350000000002000000
- Name: .debug_abbrev.dwo
Type: SHT_PROGBITS
Flags: [ SHF_EXCLUDE ]
AddressAlign: 0x1
Content: 01110125251305032576250000022E01111B1206401803253A0B3B0B49133F190000033400021803253A0B3B0B4913000004240003253E0B0B0B0000051301360B03250B0B3A0B3B0B0000060D00032549133A0B3B0B380B000000
- Type: SectionHeaderTable
Sections:
- Name: .strtab
- Name: .debug_str_offsets.dwo
- Name: .debug_str.dwo
- Name: .debug_info.dwo
- Name: .debug_abbrev.dwo
...
4 changes: 2 additions & 2 deletions llvm/test/tools/llvm-dwarfdump/X86/debug-names-verify-entries.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

# CHECK: error: Name Index @ 0x0: Unable to get string associated with name 1.
# CHECK: error: Name Index @ 0x0: Entry @ 0x73 contains an invalid CU index (47).
# CHECK: error: Name Index @ 0x0: Entry @ 0x79 references a non-existing DIE @ 0x3fa.
# CHECK: error: Name Index @ 0x0: Entry @ 0x85: mismatched CU of DIE @ 0x30: index - 0x0; debug_info - 0x1e.
# CHECK: error: Name Index @ 0x0: Entry @ 0x79 references a DIE @ 0x3fa when CU or TU ends at 0x1e.
# CHECK: error: Name Index @ 0x0: Entry @ 0x85 references a DIE @ 0x30 when CU or TU ends at 0x1e.
# CHECK: error: Name Index @ 0x0: Entry @ 0x8b: mismatched Tag of DIE @ 0x17: index - DW_TAG_subprogram; debug_info - DW_TAG_variable.
# CHECK: error: Name Index @ 0x0: Entry @ 0x91: mismatched Name of DIE @ 0x35: index - foo; debug_info - bar, _Z3bar.
# CHECK: error: Name Index @ 0x0: Name 2 (foo): Incorrectly terminated entry list.
Expand Down
32 changes: 32 additions & 0 deletions llvm/test/tools/llvm-dwarfdump/verify_split_dwarf_debug_names.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Verify we can successfully verify .debug_names with split DWARF.
#
# RUN: rm -rf %t1/
# RUN: mkdir %t1
# RUN: yaml2obj %p/Inputs/verify_split_dwarf_debug_names_exe.yaml > %t1/a.out
# RUN: yaml2obj %p/Inputs/verify_split_dwarf_debug_names_main_dwo.yaml > %t1/main.dwo
# RUN: cd %t1
# RUN: llvm-dwarfdump --verify %t1/a.out | FileCheck %s

# CHECK: Verifying unit: 1 / 1
# CHECK: Verifying dwo Units...
# CHECK: Verifying .debug_line...
# CHECK: Verifying .debug_str_offsets...
# CHECK: Verifying .debug_names...
# CHECK: No errors.

# Now verify if we remove the "main.dwo" file that we get an error letting us
# know that the .dwo file was not able to be found.
# RUN: rm %t1/main.dwo
# RUN: not llvm-dwarfdump --verify %t1/a.out | FileCheck --check-prefix=NODWO %s

# NODWO: Verifying unit: 1 / 1
# NODWO: Verifying dwo Units...
# NODWO: Verifying .debug_line...
# NODWO: Verifying .debug_str_offsets...
# NODWO: Verifying .debug_names...
# NODWO: error: Name Index @ 0x0: Entry @ 0x79 unable to load .dwo file "main.dwo" for DWARF unit @ 0x0.
# NODWO: error: Name Index @ 0x0: Entry @ 0x7f unable to load .dwo file "main.dwo" for DWARF unit @ 0x0.
# NODWO: error: Name Index @ 0x0: Entry @ 0x85 unable to load .dwo file "main.dwo" for DWARF unit @ 0x0.
# NODWO: error: Aggregated error counts:
# NODWO: error: Unable to get load .dwo file occurred 3 time(s).
# NODWO: Errors detected.
Loading