Skip to content

[DebugInfo] Separate error generation from reporting in DWARFHeaderUnit::extract #68242

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 3 commits into from
Oct 18, 2023
Merged

[DebugInfo] Separate error generation from reporting in DWARFHeaderUnit::extract #68242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f162e71
[DebugInfo] Separate error generation from reporting in DWARFHeaderUn…
bulbazord Oct 4, 2023
9ec5638
[DebugInfo] Add new test
bulbazord Oct 17, 2023
46f8bac
[DebugInfo] Create new error instead of joining existing errors
bulbazord Oct 17, 2023
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
4 changes: 2 additions & 2 deletions llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ class DWARFUnitHeader {
/// Note that \p SectionKind is used as a hint to guess the unit type
/// for DWARF formats prior to DWARFv5. In DWARFv5 the unit type is
/// explicitly defined in the header and the hint is ignored.
bool extract(DWARFContext &Context, const DWARFDataExtractor &debug_info,
uint64_t *offset_ptr, DWARFSectionKind SectionKind);
Error extract(DWARFContext &Context, const DWARFDataExtractor &debug_info,
Copy link
Contributor

@felipepiovezan felipepiovezan Oct 5, 2023

Choose a reason for hiding this comment

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

It's nice that now we can tell from the signature the meaning of the returned value (previously we couldn't and there was no documentation about it)

uint64_t *offset_ptr, DWARFSectionKind SectionKind);
// For units in DWARF Package File, remember the index entry and update
// the abbreviation offset read by extract().
bool applyIndexEntry(const DWARFUnitIndex::Entry *Entry);
Expand Down
14 changes: 10 additions & 4 deletions llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,12 @@ void fixupIndexV4(DWARFContext &C, DWARFUnitIndex &Index) {
DWARFDataExtractor Data(DObj, S, C.isLittleEndian(), 0);
while (Data.isValidOffset(Offset)) {
DWARFUnitHeader Header;
if (!Header.extract(C, Data, &Offset, DWARFSectionKind::DW_SECT_INFO)) {
if (Error ExtractionErr = Header.extract(
C, Data, &Offset, DWARFSectionKind::DW_SECT_INFO)) {
logAllUnhandledErrors(
createError("Failed to parse CU header in DWP file"), errs());
createError("Failed to parse CU header in DWP file: " +
toString(std::move(ExtractionErr))),
errs());
Map.clear();
break;
}
Expand Down Expand Up @@ -149,9 +152,12 @@ void fixupIndexV5(DWARFContext &C, DWARFUnitIndex &Index) {
uint64_t Offset = 0;
while (Data.isValidOffset(Offset)) {
DWARFUnitHeader Header;
if (!Header.extract(C, Data, &Offset, DWARFSectionKind::DW_SECT_INFO)) {
if (Error ExtractionErr = Header.extract(
C, Data, &Offset, DWARFSectionKind::DW_SECT_INFO)) {
logAllUnhandledErrors(
createError("Failed to parse unit header in DWP file"), errs());
createError("Failed to parse CU header in DWP file: " +
toString(std::move(ExtractionErr))),
errs());
break;
}
bool CU = Header.getUnitType() == DW_UT_split_compile;
Expand Down
79 changes: 34 additions & 45 deletions llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,11 @@ void DWARFUnitVector::addUnitsImpl(
if (!Data.isValidOffset(Offset))
return nullptr;
DWARFUnitHeader Header;
if (!Header.extract(Context, Data, &Offset, SectionKind))
if (Error ExtractErr =
Header.extract(Context, Data, &Offset, SectionKind)) {
Context.getWarningHandler()(std::move(ExtractErr));
return nullptr;
}
if (!IndexEntry && IsDWO) {
const DWARFUnitIndex &Index = getDWARFUnitIndex(
Context, Header.isTypeUnit() ? DW_SECT_EXT_TYPES : DW_SECT_INFO);
Expand Down Expand Up @@ -244,10 +247,10 @@ Expected<uint64_t> DWARFUnit::getStringOffsetSectionItem(uint32_t Index) const {
return DA.getRelocatedValue(ItemSize, &Offset);
}

bool DWARFUnitHeader::extract(DWARFContext &Context,
const DWARFDataExtractor &debug_info,
uint64_t *offset_ptr,
DWARFSectionKind SectionKind) {
Error DWARFUnitHeader::extract(DWARFContext &Context,
const DWARFDataExtractor &debug_info,
uint64_t *offset_ptr,
DWARFSectionKind SectionKind) {
Offset = *offset_ptr;
Error Err = Error::success();
IndexEntry = nullptr;
Expand Down Expand Up @@ -277,72 +280,58 @@ bool DWARFUnitHeader::extract(DWARFContext &Context,
} else if (UnitType == DW_UT_split_compile || UnitType == DW_UT_skeleton)
DWOId = debug_info.getU64(offset_ptr, &Err);

if (Err) {
Context.getWarningHandler()(joinErrors(
if (Err)
return joinErrors(
createStringError(
errc::invalid_argument,
"DWARF unit at 0x%8.8" PRIx64 " cannot be parsed:", Offset),
std::move(Err)));
return false;
}
std::move(Err));

// Header fields all parsed, capture the size of this unit header.
assert(*offset_ptr - Offset <= 255 && "unexpected header size");
Size = uint8_t(*offset_ptr - Offset);
uint64_t NextCUOffset = Offset + getUnitLengthFieldByteSize() + getLength();

if (!debug_info.isValidOffset(getNextUnitOffset() - 1)) {
Context.getWarningHandler()(
createStringError(errc::invalid_argument,
"DWARF unit from offset 0x%8.8" PRIx64 " incl. "
"to offset 0x%8.8" PRIx64 " excl. "
"extends past section size 0x%8.8zx",
Offset, NextCUOffset, debug_info.size()));
return false;
}
if (!debug_info.isValidOffset(getNextUnitOffset() - 1))
return createStringError(errc::invalid_argument,
"DWARF unit from offset 0x%8.8" PRIx64 " incl. "
"to offset 0x%8.8" PRIx64 " excl. "
"extends past section size 0x%8.8zx",
Offset, NextCUOffset, debug_info.size());

if (!DWARFContext::isSupportedVersion(getVersion())) {
Context.getWarningHandler()(createStringError(
if (!DWARFContext::isSupportedVersion(getVersion()))
return createStringError(
errc::invalid_argument,
"DWARF unit at offset 0x%8.8" PRIx64 " "
"has unsupported version %" PRIu16 ", supported are 2-%u",
Offset, getVersion(), DWARFContext::getMaxSupportedVersion()));
return false;
}
Offset, getVersion(), DWARFContext::getMaxSupportedVersion());

// Type offset is unit-relative; should be after the header and before
// the end of the current unit.
if (isTypeUnit() && TypeOffset < Size) {
Context.getWarningHandler()(
createStringError(errc::invalid_argument,
"DWARF type unit at offset "
"0x%8.8" PRIx64 " "
"has its relocated type_offset 0x%8.8" PRIx64 " "
"pointing inside the header",
Offset, Offset + TypeOffset));
return false;
}
if (isTypeUnit() &&
TypeOffset >= getUnitLengthFieldByteSize() + getLength()) {
Context.getWarningHandler()(createStringError(
if (isTypeUnit() && TypeOffset < Size)
return createStringError(errc::invalid_argument,
"DWARF type unit at offset "
"0x%8.8" PRIx64 " "
"has its relocated type_offset 0x%8.8" PRIx64 " "
"pointing inside the header",
Offset, Offset + TypeOffset);

if (isTypeUnit() && TypeOffset >= getUnitLengthFieldByteSize() + getLength())
return createStringError(
errc::invalid_argument,
"DWARF type unit from offset 0x%8.8" PRIx64 " incl. "
"to offset 0x%8.8" PRIx64 " excl. has its "
"relocated type_offset 0x%8.8" PRIx64 " pointing past the unit end",
Offset, NextCUOffset, Offset + TypeOffset));
return false;
}
Offset, NextCUOffset, Offset + TypeOffset);

if (Error SizeErr = DWARFContext::checkAddressSizeSupported(
getAddressByteSize(), errc::invalid_argument,
"DWARF unit at offset 0x%8.8" PRIx64, Offset)) {
Context.getWarningHandler()(std::move(SizeErr));
return false;
}
"DWARF unit at offset 0x%8.8" PRIx64, Offset))
return SizeErr;

// Keep track of the highest DWARF version we encounter across all units.
Context.setMaxVersionIfGreater(getVersion());
return true;
return Error::success();
}

bool DWARFUnitHeader::applyIndexEntry(const DWARFUnitIndex::Entry *Entry) {
Expand Down
73 changes: 73 additions & 0 deletions llvm/test/tools/llvm-dwp/X86/cu_tu_units_manual_v5_invalid.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# This test checks that llvm-dwarfdump correctly reports errors when parsing
# DWARF Unit Headers in DWP files

# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o %t.o \
# RUN: -split-dwarf-file=%t.dwo -dwarf-version=5
# RUN: llvm-dwp %t.dwo -o %t.dwp
# RUN: llvm-dwarfdump -debug-info -debug-cu-index -debug-tu-index \
# RUN: -manaully-generate-unit-index %t.dwp 2>&1 | FileCheck %s

## Note: In order to check whether the type unit index is generated
## there is no need to add the missing DIEs for the structure type of the type unit.

# CHECK-NOT: .debug_info.dwo contents:

# CHECK-DAG: .debug_cu_index contents:
# CHECK: Failed to parse CU header in DWP file: DWARF unit at offset 0x00000000 has unsupported version 6, supported are 2-5
Copy link
Collaborator

@dwblaikie dwblaikie Oct 17, 2023

Choose a reason for hiding this comment

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

I'd probably still include the error: prefix here to make it a bit more explicit how this is being printed

Copy link
Member Author

@bulbazord bulbazord Oct 17, 2023

Choose a reason for hiding this comment

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

They're not emitted with error: unfortunately. The raw output of llvm-dwarfdump is:

$PROJECTS/llvm-project/build/test/tools/llvm-dwp/X86/Output/cu_tu_units_manual_v5_invalid.s.tmp.dwp:     file format elf64-x86-64
warning: DWARF unit at offset 0x00000000 has unsupported version 6, supported are 2-5

.debug_cu_index contents:
Failed to parse CU header in DWP file: DWARF unit at offset 0x00000000 has unsupported version 6, supported are 2-5
version = 5, units = 1, slots = 2

Index Signature          INFO                                     ABBREV
----- ------------------ ---------------------------------------- ------------------------
    1 0x10001450c58e1dbe [0x0000000000000036, 0x000000000000004b) [0x00000000, 0x00000010)

.debug_tu_index contents:
Failed to parse CU header in DWP file: DWARF unit at offset 0x00000000 has unsupported version 6, supported are 2-5
version = 5, units = 2, slots = 4

Index Signature          INFO                                     ABBREV
----- ------------------ ---------------------------------------- ------------------------
    1 0x4e834ea939695c24 [0x0000000000000000, 0x000000000000001b) [0x00000000, 0x00000010)
    4 0x89a49a5d44b29ee7 [0x000000000000001b, 0x0000000000000036) [0x00000000, 0x00000010)

If you consider the output of the test I added but without my changes, you end up with:

.debug_cu_index contents:
warning: DWARF unit at offset 0x00000000 has unsupported version 6, supported are 2-5
Failed to parse unit header in DWP file

I think the lack of a warning in my test is definitely problematic, but that's because fixupIndexV{4,5} isn't reporting the error correctly. I'd like to address that in a follow-up.

Copy link
Collaborator

@jh7370 jh7370 Oct 19, 2023

Choose a reason for hiding this comment

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

I don't know if this is what you meant, but the "Failed to parse..." message probably should have an error: prefix, but that should be added by the error handler (using WithColor::error etc).


# CHECK-DAG: .debug_tu_index contents:
# CHECK: Failed to parse CU header in DWP file: DWARF unit at offset 0x00000000 has unsupported version 6, supported are 2-5

.section .debug_info.dwo,"e",@progbits
.long .Ldebug_info_dwo_end0-.Ldebug_info_dwo_start0 # Length of Unit
.Ldebug_info_dwo_start0:
.short 6 # DWARF version number
.byte 6 # DWARF Unit Type (DW_UT_split_type)
.byte 8 # Address Size (in bytes)
.long 0 # Offset Into Abbrev. Section
.quad 5657452045627120676 # Type Signature
.long 25 # Type DIE Offset
.byte 2 # Abbrev [2] DW_TAG_type_unit
.byte 3 # Abbrev [3] DW_TAG_structure_type
.byte 0 # End Of Children Mark
.Ldebug_info_dwo_end0:
.section .debug_info.dwo,"e",@progbits
.long .Ldebug_info_dwo_end1-.Ldebug_info_dwo_start1 # Length of Unit
.Ldebug_info_dwo_start1:
.short 6 # DWARF version number
.byte 6 # DWARF Unit Type (DW_UT_split_type)
.byte 8 # Address Size (in bytes)
.long 0 # Offset Into Abbrev. Section
.quad -8528522068957683993 # Type Signature
.long 25 # Type DIE Offset
.byte 4 # Abbrev [4] DW_TAG_type_unit
.byte 5 # Abbrev [5] DW_TAG_structure_type
.byte 0 # End Of Children Mark
.Ldebug_info_dwo_end1:
.section .debug_info.dwo,"e",@progbits
.long .Ldebug_info_dwo_end2-.Ldebug_info_dwo_start2 # Length of Unit
.Ldebug_info_dwo_start2:
.short 6 # DWARF version number
.byte 5 # DWARF Unit Type (DW_UT_split_compile)
.byte 8 # Address Size (in bytes)
.long 0 # Offset Into Abbrev. Section
.quad 1152943841751211454
.byte 1 # Abbrev [1] DW_TAG_compile_unit
.Ldebug_info_dwo_end2:
.section .debug_abbrev.dwo,"e",@progbits
.byte 1 # Abbreviation Code
.byte 17 # DW_TAG_compile_unit
.byte 0 # DW_CHILDREN_no
.byte 0 # EOM(1)
.byte 0 # EOM(2)
.byte 2 # Abbreviation Code
.byte 65 # DW_TAG_type_unit
.byte 1 # DW_CHILDREN_yes
.byte 0 # EOM
.byte 0 # EOM
.byte 4 # Abbreviation Code
.byte 65 # DW_TAG_type_unit
.byte 1 # DW_CHILDREN_yes
.byte 0 # EOM
.byte 0 # EOM
.byte 0 # EOM
6 changes: 5 additions & 1 deletion llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2170,7 +2170,11 @@ TEST(DWARFDebugInfo, TestDWARF64UnitLength) {
DWARFDataExtractor Data(Obj, Sec, /* IsLittleEndian = */ true,
/* AddressSize = */ 4);
uint64_t Offset = 0;
EXPECT_FALSE(Header.extract(*Context, Data, &Offset, DW_SECT_INFO));
ASSERT_THAT_ERROR(
Header.extract(*Context, Data, &Offset, DW_SECT_INFO),
FailedWithMessage(
"DWARF unit from offset 0x00000000 incl. to offset "
"0x1122334455667794 excl. extends past section size 0x00000018"));
// Header.extract() returns false because there is not enough space
// in the section for the declared length. Anyway, we can check that
// the properties are read correctly.
Expand Down