Skip to content

[lldb] Tolerate multiple compile units with the same DWO ID #100577

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 1 commit into from
Aug 12, 2024
Merged

[lldb] Tolerate multiple compile units with the same DWO ID #100577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 13 additions & 13 deletions lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,14 @@ void DWARFUnit::ExtractUnitDIEIfNeeded() {
*m_dwo_id, m_first_die.GetOffset()));
return; // Can't fetch the compile unit from the dwo file.
}
// If the skeleton compile unit gets its unit DIE parsed first, then this
// will fill in the DWO file's back pointer to this skeleton compile unit.
// If the DWO files get parsed on their own first the skeleton back link
// can be done manually in DWARFUnit::GetSkeletonCompileUnit() which will
// do a reverse lookup and cache the result.
dwo_cu->SetSkeletonUnit(this);

// Link the DWO unit to this object, if it hasn't been linked already (this
// can happen when we have an index, and the DWO unit is parsed first).
if (!dwo_cu->LinkToSkeletonUnit(*this)) {
SetDwoError(Status::createWithFormat(
"multiple compile units with Dwo ID {0:x16}", *m_dwo_id));
Comment on lines +104 to +105
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we detect these empty .dwo files and not warn if they contain no data? Detection would probably need to be done in LinkToSkeletonUnit(...) and it can return true if it detects two .dwo files being linked that are both empty? Not sure if this will help stop errors from being displayed in cases where it doesn't matter?

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 don't think there's an easy way to do that. The problem is that the compile units aren't really empty (llvm already skips empty units). They could have arbitrarily many type definitions (typically enums, as those can't be homed) inside them, so we'd have to check if that's all they contain.

I've also thought about looking at the DW_AT_ranges attribute, but that doesn't cover variables, so we would misclassify compile units that only define variables.

The upshot of all this is that since the units don't contain any code, it's pretty hard (maybe impossible?) to actually end up "inside" them. Therefore, I'm not sure if the user would ever see this kind of error, and all that they'll do is slightly increase the error statistics (but then again, maybe we want this to show up in the statistics?)

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any thoughts on what I wrote above, Greg?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the delay. Lets start with this and see how things go and if we ever see this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. With #100375, I think the only way this can show up is with old compiler versions. New versions should no longer generate identical hashes even in these cases (of course, it's still possible to create a hash collision deliberately, but accidental ones should not be happening).

return;
}

DWARFBaseDIE dwo_cu_die = dwo_cu->GetUnitDIEOnly();
if (!dwo_cu_die.IsValid()) {
Expand Down Expand Up @@ -718,13 +720,11 @@ DWARFCompileUnit *DWARFUnit::GetSkeletonUnit() {
return llvm::dyn_cast_or_null<DWARFCompileUnit>(m_skeleton_unit);
}

void DWARFUnit::SetSkeletonUnit(DWARFUnit *skeleton_unit) {
// If someone is re-setting the skeleton compile unit backlink, make sure
// it is setting it to a valid value when it wasn't valid, or if the
// value in m_skeleton_unit was valid, it should be the same value.
assert(skeleton_unit);
assert(m_skeleton_unit == nullptr || m_skeleton_unit == skeleton_unit);
m_skeleton_unit = skeleton_unit;
bool DWARFUnit::LinkToSkeletonUnit(DWARFUnit &skeleton_unit) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No real reason to pass in a reference here since we want the pointer. The call of this function is calling with dwo_cu->LinkToSkeletonUnit(*this). Can we change DWARFUnit &skeleton_unit back to DWARFUnit *skeleton_unit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A reference implies (and documents) non-nullness of the argument. This removes the need for the assert(skeleton_unit); inside the function and makes it clear to the caller that it must pass a valid object. I don't strongly about it, but I think that's worth an extra * at the call site.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense.

if (m_skeleton_unit && m_skeleton_unit != &skeleton_unit)
return false;
m_skeleton_unit = &skeleton_unit;
return true;
}

bool DWARFUnit::Supports_DW_AT_APPLE_objc_complete_type() {
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ class DWARFUnit : public UserID {
/// both cases correctly and avoids crashes.
DWARFCompileUnit *GetSkeletonUnit();

void SetSkeletonUnit(DWARFUnit *skeleton_unit);
bool LinkToSkeletonUnit(DWARFUnit &skeleton_unit);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this was renamed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It just seemed to me like Set does not capture very well the new semantics of the function (a typical setter function just sets the corresponding field and does not second-guess the callers intentions). A nice side benefit is that we can be sure to catch all callers of the api to make sure they're not silently ignoring the result.

I can revert it if you feel strongly about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with the change, makes sense.


bool Supports_DW_AT_APPLE_objc_complete_type();

Expand Down
142 changes: 142 additions & 0 deletions lldb/test/Shell/SymbolFile/DWARF/x86/dwp-hash-collision.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
## Test that lldb handles (mainly, that it doesn't crash) the situation where
## two skeleton compile units have the same DWO ID (and try to claim the same
## split unit from the DWP file. This can sometimes happen when the compile unit
## is nearly empty (e.g. because LTO has optimized all of it away).

# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s --defsym MAIN=0 > %t
# RUN: llvm-mc -triple=x86_64-pc-linux -filetype=obj %s > %t.dwp
# RUN: %lldb %t -o "image lookup -t my_enum_type" \
# RUN: -o "image dump separate-debug-info" -o exit | FileCheck %s

## Check that we're able to access the type within the split unit (no matter
## which skeleton unit it ends up associated with). Completely ignoring the unit
## might also be reasonable.
# CHECK: image lookup -t my_enum_type
# CHECK: 1 match found
# CHECK: name = "my_enum_type", byte-size = 4, compiler_type = "enum my_enum_type {
# CHECK-NEXT: }"
#
## Check that we get some indication of the error.
# CHECK: image dump separate-debug-info
# CHECK: Dwo ID Err Dwo Path
# CHECK: 0xdeadbeefbaadf00d E multiple compile units with Dwo ID 0xdeadbeefbaadf00d

.set DWO_ID, 0xdeadbeefbaadf00d

## The main file.
.ifdef MAIN
.section .debug_abbrev,"",@progbits
.byte 1 # Abbreviation Code
.byte 74 # DW_TAG_compile_unit
.byte 0 # DW_CHILDREN_no
.byte 0x76 # DW_AT_dwo_name
.byte 8 # DW_FORM_string
.byte 0 # EOM(1)
.byte 0 # EOM(2)
.byte 0 # EOM(3)


.section .debug_info,"",@progbits
.irpc I,01
.Lcu_begin\I:
.long .Ldebug_info_end\I-.Ldebug_info_start\I # Length of Unit
.Ldebug_info_start\I:
.short 5 # DWARF version number
.byte 4 # DWARF Unit Type
.byte 8 # Address Size (in bytes)
.long .debug_abbrev # Offset Into Abbrev. Section
.quad DWO_ID # DWO id
.byte 1 # Abbrev [1] DW_TAG_compile_unit
.ascii "foo"
.byte '0' + \I
.asciz ".dwo\0" # DW_AT_dwo_name
.Ldebug_info_end\I:
.endr

.else
## DWP file starts here.

.section .debug_abbrev.dwo,"e",@progbits
.LAbbrevBegin:
.byte 1 # Abbreviation Code
.byte 17 # DW_TAG_compile_unit
.byte 1 # DW_CHILDREN_yes
.byte 37 # DW_AT_producer
.byte 8 # DW_FORM_string
.byte 19 # DW_AT_language
.byte 5 # DW_FORM_data2
.byte 0 # EOM(1)
.byte 0 # EOM(2)
.byte 2 # Abbreviation Code
.byte 4 # DW_TAG_enumeration_type
.byte 0 # DW_CHILDREN_no
.byte 3 # DW_AT_name
.byte 8 # DW_FORM_string
.byte 73 # DW_AT_type
.byte 19 # DW_FORM_ref4
.byte 11 # DW_AT_byte_size
.byte 11 # DW_FORM_data1
.byte 0 # EOM(1)
.byte 0 # EOM(2)
.byte 4 # Abbreviation Code
.byte 36 # DW_TAG_base_type
.byte 0 # DW_CHILDREN_no
.byte 3 # DW_AT_name
.byte 8 # DW_FORM_string
.byte 62 # DW_AT_encoding
.byte 11 # DW_FORM_data1
.byte 11 # DW_AT_byte_size
.byte 11 # DW_FORM_data1
.byte 0 # EOM(1)
.byte 0 # EOM(2)
.byte 0 # EOM(3)
.LAbbrevEnd:
.section .debug_info.dwo,"e",@progbits
.LCUBegin:
.Lcu_begin1:
.long .Ldebug_info_end1-.Ldebug_info_start1 # Length of Unit
.Ldebug_info_start1:
.short 5 # DWARF version number
.byte 5 # DWARF Unit Type
.byte 8 # Address Size (in bytes)
.long 0 # Offset Into Abbrev. Section
.quad DWO_ID # DWO id
.byte 1 # Abbrev [1] DW_TAG_compile_unit
.asciz "Hand-written DWARF" # DW_AT_producer
.short 12 # DW_AT_language
.byte 2 # Abbrev [2] DW_TAG_enumeration_type
.asciz "my_enum_type" # DW_AT_name
.long .Lint-.Lcu_begin1 # DW_AT_type
.byte 4 # DW_AT_byte_size
.Lint:
.byte 4 # Abbrev [4] DW_TAG_base_type
.asciz "int" # DW_AT_name
.byte 5 # DW_AT_encoding
.byte 4 # DW_AT_byte_size
.byte 0 # End Of Children Mark
.Ldebug_info_end1:
.LCUEnd:
.section .debug_cu_index, "", @progbits
## Header:
.short 5 # Version
.short 0 # Padding
.long 2 # Section count
.long 1 # Unit count
.long 2 # Slot count
## Hash Table of Signatures:
.quad 0
.quad DWO_ID
## Parallel Table of Indexes:
.long 0
.long 1
## Table of Section Offsets:
## Row 0:
.long 1 # DW_SECT_INFO
.long 3 # DW_SECT_ABBREV
## Row 1:
.long 0 # Offset in .debug_info.dwo
.long 0 # Offset in .debug_abbrev.dwo
## Table of Section Sizes:
.long .LCUEnd-.LCUBegin # Size in .debug_info.dwo
.long .LAbbrevEnd-.LAbbrevBegin # Size in .debug_abbrev.dwo
.endif
Loading