Add support for verifying .debug_names in split DWARF for CUs and TUs. #101775
Conversation
This patch fixes .debug_names verification for split DWARF with no type units. It will print out an error for any name entries where we can't locate the .dwo file. It finds the non skeleton unit and correctly figures out the DIE offset in the .dwo file. If the non skeleton unit is found and yet the skeleton unit has a DWO ID, an error will be emitted showing we couldn't access the non-skeleton compile unit.
|
@llvm/pr-subscribers-debuginfo Author: Greg Clayton (clayborg) ChangesThis patch fixes .debug_names verification for split DWARF with no type units. It will print out an error for any name entries where we can't locate the .dwo file. It finds the non skeleton unit and correctly figures out the DIE offset in the .dwo file. If the non skeleton unit is found and yet the skeleton unit has a DWO ID, an error will be emitted showing we couldn't access the non-skeleton compile unit. Full diff: https://github.com/llvm/llvm-project/pull/101775.diff 3 Files Affected:
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
index d800850450eb3..455ccaab74d7e 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h
@@ -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);
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFContext.cpp b/llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
index f36399ed00a6e..045f5e219d34a 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
@@ -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) {
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
index 8ff6ebd230025..7c6da712d1276 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
@@ -1626,8 +1626,44 @@ 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.
+ DWARFDie NonSkeletonUnitDie = DU->getNonSkeletonUnitDIE();
+ if (DU->getDWOId() && DU->getUnitDIE() == NonSkeletonUnitDie) {
+ 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();
+ DWARFDie DIE = NonSkeletonUnit->getDIEForOffset(DIEOffset);
+
if (!DIE) {
ErrorCategory.Report("NameIndex references nonexistent DIE", [&]() {
error() << formatv("Name Index @ {0:x}: Entry @ {1:x} references a "
@@ -1637,7 +1673,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 "
|
|
I don't have tests yet as I am looking for the right way to create them. If anyone has any input one what I should do, please let me know. I was thinking of having tests that do:
Since we can't use clang and or lld in llvm tests, I am basically looking for the best way to add test for this. If anyone can provide guidance on what the best way to do this is, I am all ears. |
|
You can check the existing dwo/dwp test coverage (one easy way to find test coverage, is to break the code - like just cause the dwp or dwo lookup code to consistently fail to find the dwo/dwp file) - these days I think we go with yaml or assembly for most tests. To test for just DWO files, you could checkin assembly or yaml files, maybe using the new https://llvm.org/docs/TestingGuide.html#elaborated-tests functionality to make it easier to regenerate as needed. |
|
When I've needed to do that, I just created wrote the dwp section in assembly, the format is pretty simple, so you could probably just take an existing asm output from clang and slap a debug_[ct]u_index section at the back. You can use an existing DWP test as a template (e.g. test/DebugInfo/X86/dwp-v5-tu.s) Generally I'd recommend assembly instead of yaml as test sources, because they're easier to read in a code review (i.e., without running the input through |
This adds a test and fixes another test to check for DIE offsets being out of range for a compile unit as this was triggered by some of the changes made for the non skeleton compile unit.
|
This is now fully tested. Before this patch errors would be emitted when verifying .debug_names in split DWARF because the DIE offsets wouldn't match up. This patch makes sure that works and only tests .dwo files as .dwp is just another way to access .dwo files. Let me know if there is anything missing. I have more patches needed to allow us to enable .debug_names with split DWARF, but we can't until we can verify the data is good. |
| // 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. | ||
| DWARFDie NonSkeletonUnitDie = DU->getNonSkeletonUnitDIE(); | ||
| if (DU->getDWOId() && DU->getUnitDIE() == NonSkeletonUnitDie) { |
There was a problem hiding this comment.
would this be a more clear check?
DWARFUnit *DWOCU =
DwarfUnit->getNonSkeletonUnitDIE(false, AbsolutePath).getDwarfUnit();
if (!DWOCU->isDWOUnit()) {
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree. So you want me to fix this as part of this commit? Or in a separate commit?
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
Sure, leave a FIXME at this call site and on the function itself.
There was a problem hiding this comment.
I added the FIXME
|
Anything else needed for this PR? |
|
Ok, I added the FIXME. Anything else needed for this patch? |
|
Does anyone have time to help move this along? I have more patches coming to finish up the verification of .debug_names. |
|
Sorry for the late ping Looks like the new verification error triggers in some of the tests on the DWARFv5 x86 macOS bots: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake-matrix/628/execution/node/79/log/ Seeing a bunch of these in the log: Trying to repro this locally now. This is what one of the clang commands looks like: Note how we're compiling with |
|
So looks like the offending DWARF is this: Note that the first CU has a AFAIU, this happens when we compile with , we do have a Any idea on the best way forward here? @adrian-prantl @clayborg Maybe |
This fixes a verifier error uncovered as a result of llvm#101775. When compiling with `-gmodules`, Clang will create a skeleton CU that contains a `DW_AT_dwo_id` and a `DW_AT_dwo_name` corresponding to the path of the `.pcm` that will contain type definitions referenced in the non-skeleton CU (see the [gmodules LLDB docs](https://lldb.llvm.org/resources/extensions.html) for more details). The non-skeleton CU will also contain a `DW_AT_dwo_id` that matches that of the skeleton. `dsymutil` effectively undoes the `-gmodules` work, replacing all the module type references with definitions. I.e., we no longer create a skeleton `.dwo` CU. Prior to this patch `dsymutil` did not strip out the `DW_AT_dwo_id` on the non-skeleton CU, now (since llvm#101775) causing verification errors such as: ``` Verifying .debug_names... error: Name Index @ 0x0: Entry @ 0x9a unable to load .dwo file "None" for DWARF unit @ 0x0. error: output verification failed for x86_64 make: *** [a.out.dSYM] Error 1 ``` Because the verifier sees the DWO ID but can't find a matching `.dwo` unit. This patch simply strips the `DW_AT_dwo_id` from the main CU.
|
Proposed fix in: #105186 |
This fixes a verifier error uncovered as a result of #101775. When compiling with `-gmodules`, Clang will create a skeleton CU that contains a `DW_AT_dwo_id` and a `DW_AT_dwo_name` corresponding to the path of the `.pcm` that carries the type definitions referenced in the non-skeleton CU (see the [gmodules LLDB docs](https://lldb.llvm.org/resources/extensions.html) for more details). The non-skeleton CU will also contain a `DW_AT_dwo_id` that matches that of the skeleton. `dsymutil` effectively undoes the `-gmodules` work, replacing all the module type references with definitions. I.e., we no longer create a skeleton `.dwo` CU. Prior to this patch `dsymutil` did not strip out the `DW_AT_dwo_id` on the non-skeleton CU. This now (since #101775) causes verification errors such as: ``` Verifying .debug_names... error: Name Index @ 0x0: Entry @ 0x9a unable to load .dwo file "None" for DWARF unit @ 0x0. error: output verification failed for x86_64 make: *** [a.out.dSYM] Error 1 ``` ...because the verifier sees the DWO ID but can't find a matching `.dwo` unit. This patch simply strips the `DW_AT_dwo_id` from the main CU.
This fixes a verifier error uncovered as a result of llvm#101775. When compiling with `-gmodules`, Clang will create a skeleton CU that contains a `DW_AT_dwo_id` and a `DW_AT_dwo_name` corresponding to the path of the `.pcm` that carries the type definitions referenced in the non-skeleton CU (see the [gmodules LLDB docs](https://lldb.llvm.org/resources/extensions.html) for more details). The non-skeleton CU will also contain a `DW_AT_dwo_id` that matches that of the skeleton. `dsymutil` effectively undoes the `-gmodules` work, replacing all the module type references with definitions. I.e., we no longer create a skeleton `.dwo` CU. Prior to this patch `dsymutil` did not strip out the `DW_AT_dwo_id` on the non-skeleton CU. This now (since llvm#101775) causes verification errors such as: ``` Verifying .debug_names... error: Name Index @ 0x0: Entry @ 0x9a unable to load .dwo file "None" for DWARF unit @ 0x0. error: output verification failed for x86_64 make: *** [a.out.dSYM] Error 1 ``` ...because the verifier sees the DWO ID but can't find a matching `.dwo` unit. This patch simply strips the `DW_AT_dwo_id` from the main CU.
This patch fixes .debug_names verification for split DWARF with no type units. It will print out an error for any name entries where we can't locate the .dwo file. It finds the non skeleton unit and correctly figures out the DIE offset in the .dwo file. If the non skeleton unit is found and yet the skeleton unit has a DWO ID, an error will be emitted showing we couldn't access the non-skeleton compile unit.