Skip to content

Fix GetDIE is outside of its CU error from .debug_names #157574

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix GetDIE is outside of its CU error from .debug_names #157574

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
8 changes: 6 additions & 2 deletions lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,12 @@ DebugNamesDWARFIndex::GetNonSkeletonUnit(const DebugNames::Entry &entry) const {
unit_offset = entry.getLocalTUOffset();
if (unit_offset) {
if (DWARFUnit *cu = m_debug_info.GetUnitAtOffset(DIERef::Section::DebugInfo,
*unit_offset))
return &cu->GetNonSkeletonUnit();
*unit_offset)) {
DWARFUnit &ret = cu->GetNonSkeletonUnit();
Copy link
Contributor

@dmpots dmpots Sep 9, 2025

Choose a reason for hiding this comment

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

It seems suprising that the GetNonSkeletonUnit() call can actually return a skeleton unit. Maybe a better approach here would be to model the fact that we can actually fail to find the non-skeleton unit by returning an Expected

llvm::Expected<DWARFUnit&> DWARFUnit::GetNonSkeletonUnit()

I did a quick search and see other
examples where it looks like we might hit this same bug.

Copy link
Contributor Author

@jeffreytan81 jeffreytan81 Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with this in general. Actually, that's the original approach I suggested while discussing with Greg.

Unfortunately, there are many existing other callers/code paths of DWARFUnit::GetNonSkeletonUnit are expecting the default behavior to fallback to return skeleton unit if failing to find dwo files. Changing DWARFUnit::GetNonSkeletonUnit's semantics requiring auditing all other callers/code paths to ensure the behaviors are expected which is a much bigger task than I thought. Yesterday, I tried to change all callers of DWARFUnit::GetNonSkeletonUnit to use new API/semantics, it is failing several tests. Some tests are related with apple debug names, -gmodules flag, PCH modules containing CU with only dwo_id without dwo_name (resulting in dwo error) which I am not feeling comfortable/justified to fix.

Overall, I feel fixing this known code path is safer (not failing any tests) with better scope to reason about.

Copy link
Contributor

@dmpots dmpots Sep 11, 2025

Choose a reason for hiding this comment

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

Thanks for trying out that approach. Do you have a WIP patch you can push to a branch somewhere? Might be something we want to tackle later.

Your fix seems targeted and reasonable given the amount of code relying on existing behavior.

if (ret.IsSkeletonUnit())
return nullptr;
return &ret;
}
}
return nullptr;
}
Expand Down
19 changes: 19 additions & 0 deletions lldb/test/Shell/SymbolFile/DWARF/dwo-miss-getdie-ouside-cu-error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/// Check that LLDB does not emit "GetDIE for DIE {{0x[0-9a-f]+}} is outside of its CU"
/// error message when user is searching for a matching symbol from .debug_names
/// and fail to locate the corresponding .dwo file.

/// -gsplit-dwarf is supported only on Linux.
Copy link
Member

@Michael137 Michael137 Sep 9, 2025

Choose a reason for hiding this comment

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

Might be wrong, but isn't it also supported on Windows?

Copy link
Contributor Author

@jeffreytan81 jeffreytan81 Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I copied this from dwo-missing-error.test and trusted it:
https://github.com/llvm/llvm-project/blob/main/lldb/test/Shell/SymbolFile/DWARF/dwo-missing-error.test#L5-L6

Since I do not have a windows machine to test so feels safer to limit for Linux. Let me know if you find otherwise and I can update the comment.

// REQUIRES: system-linux

// RUN: %clang_host -g -gsplit-dwarf -gpubnames -gdwarf-5 %s -o main
Copy link
Contributor

@dmpots dmpots Sep 11, 2025

Choose a reason for hiding this comment

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

Thanks the test looks much easier to understand now.

I think there is a problem with where it is writing the files though. The lit tests to not automatically get a unique build directory for the test outputs, but uses a shared directory for all the tests in that directory. That means writing and deleting files can interfere with other tests in the directory.

We can use the %t to create unique a temporary directory for the test.

// RUN: mkdir -p %t.dir
// RUN: %clang_host -g -gsplit-dwarf -gpubnames -gdwarf-5 %s -o %t.dir/main
// RUN: rm %t.dir/*.dwo
// RUN: %lldb --no-lldbinit %t.dir/main \
// RUN:   -o "b main" --batch 2>&1 | FileCheck %s

/// Remove the DWO file away from the expected location so that LLDB won't find the DWO next to the binary.
// RUN: rm *.dwo
// RUN: %lldb --no-lldbinit main \
// RUN: -o "b main" --batch 2>&1 | FileCheck %s

// CHECK: warning: {{.*}}main unable to locate separate debug file (dwo, dwp). Debugging will be degraded.
// CHECK-NOT: main GetDIE for DIE {{0x[0-9a-f]+}} is outside of its CU {{0x[0-9a-f]+}}

int num = 5;

int main(void) { return 0; }
Loading