Skip to content

[LLD] Tombstone LocalTU entry in .debug_names #70701

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

Closed
wants to merge 12 commits into from
Closed

[LLD] Tombstone LocalTU entry in .debug_names #70701

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d2665cc
[LLD] Tombstone LocalTU entry in .debug_names
ayermolo Oct 16, 2023
792155d
fixup! [LLD] Tombstone LocalTU entry in .debug_names
ayermolo Oct 30, 2023
854a281
fixup! fixup! [LLD] Tombstone LocalTU entry in .debug_names
ayermolo Oct 30, 2023
b716d70
fixup! fixup! fixup! [LLD] Tombstone LocalTU entry in .debug_names
ayermolo Nov 2, 2023
19fa33e
fixup! fixup! fixup! fixup! [LLD] Tombstone LocalTU entry in .debug_n…
ayermolo Nov 2, 2023
a69878b
fixup! fixup! fixup! fixup! fixup! [LLD] Tombstone LocalTU entry in .…
ayermolo Nov 2, 2023
8e5c0b7
fixup! fixup! fixup! fixup! fixup! fixup! [LLD] Tombstone LocalTU ent…
ayermolo Nov 2, 2023
be18f4f
fixup! fixup! fixup! fixup! fixup! fixup! fixup! [LLD] Tombstone Loca…
ayermolo Nov 3, 2023
e25f409
fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! [LLD] Tombsto…
ayermolo Nov 3, 2023
03afbda
fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! [LLD] …
ayermolo Nov 6, 2023
036dbef
Refactored tests. Consolidated into one test
ayermolo Nov 29, 2023
1dfdf18
Removed redundant test.
ayermolo Nov 29, 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
28 changes: 18 additions & 10 deletions lld/ELF/InputSection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@
#include "llvm/Support/Compression.h"
#include "llvm/Support/Endian.h"
#include "llvm/Support/xxhash.h"
#include <algorithm>
#include <mutex>
Copy link
Member

Choose a reason for hiding this comment

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

We should keep the headers. mutex is used by std::mutex in this file.

#include <vector>

using namespace llvm;
Expand Down Expand Up @@ -886,8 +884,7 @@ void InputSection::relocateNonAlloc(uint8_t *buf, ArrayRef<RelTy> rels) {
const unsigned bits = sizeof(typename ELFT::uint) * 8;
const TargetInfo &target = *elf::target;
const bool isDebug = isDebugSection(*this);
const bool isDebugLocOrRanges =
isDebug && (name == ".debug_loc" || name == ".debug_ranges");
const bool isDebugNames = isDebug && name == ".debug_names";
const bool isDebugLine = isDebug && name == ".debug_line";
std::optional<uint64_t> tombstone;
for (const auto &patAndValue : llvm::reverse(config->deadRelocInNonAlloc))
Expand All @@ -896,6 +893,16 @@ void InputSection::relocateNonAlloc(uint8_t *buf, ArrayRef<RelTy> rels) {
break;
}

const uint64_t debugTombstone = StringSwitch<uint64_t>(name)
.Case(".debug_ranges", 1)
.Case(".debug_loc", 1)
.Case(".debug_names", llvm::maxUIntN(32))
.Default(0);
// If -z dead-reloc-in-nonalloc= is specified, respect it.
if (!tombstone && isDebug)
tombstone = debugTombstone;
else if (tombstone)
tombstone = SignExtend64<bits>(*tombstone);
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is sign extended to 64 bits, but then we have a 32 bit relocation, I'd expect this fails/errors out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the same as original logic. It would go into if condition if tombstone is specified, and always extend.
bits == 64
So at least on ELF64 this is no-op. So TBH I am not sure why it's there.

template <unsigned B> constexpr inline int64_t SignExtend64(uint64_t x) {
  static_assert(B > 0, "Bit width can't be 0.");
  static_assert(B <= 64, "Bit width out of range.");
  return int64_t(x << (64 - B)) >> (64 - B);
}

for (const RelTy &rel : rels) {
RelType type = rel.getType(config->isMips64EL);

Expand All @@ -917,8 +924,7 @@ void InputSection::relocateNonAlloc(uint8_t *buf, ArrayRef<RelTy> rels) {
if (expr == R_NONE)
continue;

if (tombstone ||
(isDebug && (type == target.symbolicRel || expr == R_DTPREL))) {
if (tombstone) {
// Resolve relocations in .debug_* referencing (discarded symbols or ICF
// folded section symbols) to a tombstone value. Resolving to addend is
// unsatisfactory because the result address range may collide with a
Expand Down Expand Up @@ -947,12 +953,14 @@ void InputSection::relocateNonAlloc(uint8_t *buf, ArrayRef<RelTy> rels) {
//
// TODO To reduce disruption, we use 0 instead of -1 as the tombstone
// value. Enable -1 in a future release.

// Extend to 64bit MAX for 64 bit relocations, LocalTU, in .debug_names.
if (isDebugNames && type == target.symbolicRel)
tombstone = SignExtend64<32>(*tombstone);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this only handles one case of 64 bit relocations, but there might be others? & I'm not sure isDebugNames is the special case here - shouldn't it be known by the relocation, rather than the section the relocation is in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, made it more generic.


auto *ds = dyn_cast<Defined>(&sym);
if (!sym.getOutputSection() || (ds && ds->folded && !isDebugLine)) {
// If -z dead-reloc-in-nonalloc= is specified, respect it.
const uint64_t value = tombstone ? SignExtend64<bits>(*tombstone)
: (isDebugLocOrRanges ? 1 : 0);
target.relocateNoSym(bufLoc, type, value);
target.relocateNoSym(bufLoc, type, *tombstone);
continue;
}
}
Expand Down
397 changes: 397 additions & 0 deletions lld/test/ELF/Inputs/dwarf5-64-debug-names-type-comdat-helper.s
View file
Open in desktop

Large diffs are not rendered by default.

384 changes: 384 additions & 0 deletions lld/test/ELF/Inputs/dwarf5-64-debug-names-type-comdat-main.s
View file
Open in desktop

Large diffs are not rendered by default.

410 changes: 410 additions & 0 deletions lld/test/ELF/Inputs/dwarf5-debug-names-type-comdat-helper.s
View file
Open in desktop

Large diffs are not rendered by default.

397 changes: 397 additions & 0 deletions lld/test/ELF/Inputs/dwarf5-debug-names-type-comdat-main.s
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 3 additions & 5 deletions lld/test/ELF/debug-dead-reloc.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
# CHECK-NEXT: 0000 {{.*}}000 00000000 {{.*}}000 00000000
# CHECK-NEXT: 0010 00000000 00000000 {{.*}}000 00000000
# CHECK: Contents of section .debug_foo:
# CHECK-NEXT: 0000 00000000 00000000 08000000 00000000
# CHECK-NEXT: 0010 00000000 00000000 08000000 00000000
# CHECK-NEXT: 0000 00000000 00000000 00000000 00000000
# CHECK-NEXT: 0010 00000000 00000000 00000000 00000000

# REL: Relocations [
# REL-NEXT: .rela.text {
Expand Down Expand Up @@ -80,8 +80,6 @@ group:
.section .debug_foo
.quad .text.1+8

## We only deal with DW_FORM_addr. Don't special case short-range absolute
## relocations. Treat them like regular absolute relocations referencing
## discarded symbols, which are resolved to the addend.
## Treat short-range absolute relocations the same as DW_FORM_addr.
.long .text.1+8
.long 0
20 changes: 20 additions & 0 deletions lld/test/ELF/x86-64-dwarf5-64-debug-names-type-comdat.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// REQUIRES: x86
Copy link
Member

Choose a reason for hiding this comment

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

We should place the DWARF64 test into the other file.

https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer#i-dont-know-an-existing-test-can-be-enhanced applies

// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/dwarf5-64-debug-names-type-comdat-main.s -o %t.o
// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/dwarf5-64-debug-names-type-comdat-helper.s -o %t1.o
// RUN: ld.lld %t.o %t1.o -o %t1
// RUN: llvm-readelf --relocs %t.o | FileCheck --check-prefix=RELOCMAIN %s
// RUN: llvm-dwarfdump --debug-names %t1 | FileCheck %s

// Test checks that LLD tombstones TU section that was de-duplicated using COMDAT to the maxium value.
// For some reason llvm-dwarfdump doesn't print out full 64bitt offset at the moment.
// Working around it by checking that relocation is 64 bit.

// RELOCMAIN: rela.debug_names
// RELOCMAIN-NEXT: Offset
// RELOCMAIN-NEXT: R_X86_64_64
// RELOCMAIN-SAME: .debug_info + 0
// RELOCMAIN-NEXT: R_X86_64_64
// RELOCMAIN-SAME: .debug_info + 0

// CHECK: LocalTU[0]: 0x00000000
// CHECK: LocalTU[0]: 0xffffffffffffffff
10 changes: 10 additions & 0 deletions lld/test/ELF/x86-64-dwarf5-debug-names-type-comdat.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// REQUIRES: x86
Copy link
Member

Choose a reason for hiding this comment

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

In newer tests we make the comment (non-RUN non-CHECK) marker outstanding. We typically use # RUN: # CHECK: and ## Regular comments.

// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/dwarf5-debug-names-type-comdat-main.s -o %t.o
// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/dwarf5-debug-names-type-comdat-helper.s -o %t1.o
// RUN: ld.lld %t.o %t1.o -o %t1
// RUN: llvm-dwarfdump --debug-names %t1 | FileCheck %s

// Test checks that LLD tombstones TU section that was de-duplicated using COMDAT to the maxium value.

// CHECK: LocalTU[0]: 0x00000000
// CHECK: LocalTU[0]: 0xffffffff