[ELF] - Stop producing broken entries in .debug_ranges section

[grimar]

Summary

Address range lists are contained in a separate section called .debug_ranges.
In short it contains start/end address range pairs.
zero/zero pair means the end of range list. Values of this section
in object files are relocatable.

Perviously LLD would generate wrong output if relocation applied
was against discarded section. LLD would write addend of relocation in that case,
what produced broken ranges, like (0x00000000 - [Addend value]), for example.

According to DWARF specification, it is safe to produce range entries with
same begining and end address: "A range list entry (but not a base address selection or end of list entry) whose beginning and
ending addresses are equal has no effect because the size of the range covered by such an
entry is zero"

GNU LD in such case produces stub entry with start/end = 1.
(https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=e4067dbb2a3368dbf908b39c5435c84d51abc9f3)

I suggest to do the same. I am planning to use it for .gdb_index, D31424 description contains information about issue I faced,
this patch should fix it and that allows to use relocated output for generating .gdb_index section in final without additional parsing
of relocations.

Probably we want to do the same for other debug sections (but place zero instead of 1, that is what ld do, though I did not investigate that deeper),
but now I faced problems only with .debug_ranges, so patch changes behavior for that section only.

[dblaikie]

Does GNU ld (binutils, or gold, or both?) actually special case this relocation in this section? (ie: a small hand-written assembly file with only debug_ranges ends up with different contents if you rename the section to debug_foo_ranges? (zeros in the latter case, 1s in the original case)

[grimar]

In D31464#713161, @dblaikie wrote:

Does GNU ld (binutils, or gold, or both?) actually special case this relocation in this section? (ie: a small hand-written assembly file with only debug_ranges ends up with different contents if you rename the section to debug_foo_ranges? (zeros in the latter case, 1s in the original case)

In my tests only GNU ld handled that, gold generated broken entry. And yes, GNU ld handles this section separatelly by name, see:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/reloc.c;h=4e95d85f0b94aacb5995c6f2307f94e6ede206fb;hb=e4067dbb2a3368dbf908b39c5435c84d51abc9f3#l1591

[jhenderson]

Generally looks okay to me. I've made a few inline comments.

This approach works for .debug_ranges, because we are working with pairs of addresses, but we have to be more careful with other DWARF sections, because on some architectures, address zero is a valid address and should not be used, and we cannot simply set the size to zero, because we don't know where that is (if anywhere at all). gdb recognizes zero as special, if the architecture does not allow zero addresses, but otherwise it is not treated as special, and so we end up with (potentially) overlapping debug info entries which can cause problems with the debugger. Similarly, .debug_aranges uses an address and a fixed size, which we have to update correctly. There's a related discussion on the LLDB mailing list I started about handling GC'ed sections in the debugger: http://www.mail-archive.com/[e-mail]/msg04140.html. The general agreement is that we should use a special non-zero address. I would suggest -1 (i.e. 0xFFFFFFFF in 32-bit DWARF addresses and 0xFFFFFFFFFFFFFFFF in 64-bit DWARF addresses), but we should have that discussion on the mailing lists.

ELF/InputSection.cpp

545

Should be called isInDiscardedSection, because it is working with a symbol not a section. "isDiscardedSection" suggests to me that it is asking if a section has been discarded or not, and so would take an InputSection or section index.

568

Is there anywhere else that we check section names? If so, I'd personally prefer if we set some flag on the section early rather than doing string comparisons on every input section to test the name multiple times.

591–596

A few grammar nits and minor improvements to this comment. I suggest the following:

"If an address range list contains an entry that belongs to a discarded section, we should perform a fix-up. We cannot write zero as the start and end address because such a pair is an end of range list marker in the .debug_ranges section. We put a placeholder value equal to 1, which is fine because a range list entry whose beginning and end address are equal has no effect, because the size of the range is zero in such case."

[jhenderson]

Also, one more thing: .debug_ranges can contain "base address selection entries" which have a -1 and address pair, hence why we cannot use -1 in this situation. I'm not sure how the address is set, as I have never seen a real-life example of this, but I'm wondering if there's some possible conflict with this patch, i.e. we might end up with a -1, 1 base selection entry, if there is a relocation to a discarded section used here. I have no idea if this is really something we need to worry about though (I think probably not).

[grimar]

In D31464#713928, @jhenderson wrote:

Generally looks okay to me. I've made a few inline comments.

This approach works for .debug_ranges, because we are working with pairs of addresses, but we have to be more careful with other DWARF sections, because on some architectures, address zero is a valid address and should not be used, and we cannot simply set the size to zero, because we don't know where that is (if anywhere at all). gdb recognizes zero as special, if the architecture does not allow zero addresses, but otherwise it is not treated as special, and so we end up with (potentially) overlapping debug info entries which can cause problems with the debugger. Similarly, .debug_aranges uses an address and a fixed size, which we have to update correctly. There's a related discussion on the LLDB mailing list I started about handling GC'ed sections in the debugger: http://www.mail-archive.com/[e-mail]/msg04140.html. The general agreement is that we should use a special non-zero address. I would suggest -1 (i.e. 0xFFFFFFFF in 32-bit DWARF addresses and 0xFFFFFFFFFFFFFFFF in 64-bit DWARF addresses), but we should have that discussion on the mailing lists.

Interesting. I worried about other sections and so this patch touches only .debug_ranges. Thanks for your comments !

ELF/InputSection.cpp

545

I agree.

568

This method is isolated and used for doing relocations for non allocatable things like debug sections.
I would not let this a bit wierd but reasonable logic to go outside.

[jhenderson]

Could we reduce the size of the test by removing many of the debug info attributes? I don't know what dwarfdump requires, but I expect we could get rid of basically all the DW_AT_... entries except the DW_AT_ranges tag.

test/ELF/debug-ranges-reloc.s

5

We only need the -debug-dump=ranges, since we are only interested in the .debug_ranges output.

12

No need for this line.

14–16

I think we could get rid of these lines (similarly below in main).

[grimar]

• Addressed review comments.

[grimar]

In D31464#715325, @jhenderson wrote:

Could we reduce the size of the test by removing many of the debug info attributes? I don't know what dwarfdump requires, but I expect we could get rid of basically all the DW_AT_... entries except the DW_AT_ranges tag.

I tried to reduce the testcase as much as possible in updated diff.

ELF/InputSection.cpp

591–596

Fixed. thanks !

test/ELF/debug-ranges-reloc.s

5

It does not work. Looks llvm-dwarfdump has an issue here (it does not dump anything with =ranges only for this testcase), I'll try to investigate and may be prepare patch for it separatelly. I suggest to go with =all for now.

12

Removed.

14–16

Removed.

[jhenderson]

LGTM, with the inline comments. One of @ruiu or @rafael should probably still approve it though.

ELF/InputSection.cpp

545

I think this can be a const reference.

591–596

Sorry, one more grammar nit I missed - the last bit should say "in such a case."

test/ELF/debug-ranges-reloc.s

5

I just ran into this problem as well! Fixing dwarfdump would be great.

[grimar]

ELF/InputSection.cpp

545

We usually do not use const references in LLD.
Its a bit complicated. I think no const is consistent with code and ok here.

591–596

Thanks !

[grimar]

Its not yet clear how much it is really needed, abandoning for now.

ELF/InputSection.h

public:
   // to. The writer sets a value.
   uint64_t OutSecOff = 0;
 
   static bool classof(const SectionBase *S);
 
   InputSectionBase *getRelocatedSection();
 
   template <class ELFT, class RelTy>
-  void relocateNonAlloc(uint8_t *Buf, llvm::ArrayRef<RelTy> Rels);
+  void relocateNonAlloc(uint8_t *Buf, llvm::ArrayRef<RelTy> Rels,
+                        StringRef Name);
 
   // Used by ICF.
   uint32_t Class[2] = {0, 0};
 
   // Called by ICF to merge two input sections.
   void replace(InputSection *Other);
 
 private:

ELF/InputSection.cpp

getRelocTargetVA(uint32_t Type, int64_t A, typename ELFT::uint P,
   case R_TLSLD:
     return In<ELFT>::Got->getTlsIndexOff() + A - In<ELFT>::Got->getSize();
   case R_TLSLD_PC:
     return In<ELFT>::Got->getTlsIndexVA() + A - P;
   }
   llvm_unreachable("Invalid expression");
 }
 
+static bool isInDiscardedSection(SymbolBody &Body) {
+  DefinedRegular *D = dyn_cast<DefinedRegular>(&Body);
+  if (!D || !D->isSection())
+    return false;
+
+  SectionBase *IS = D->Section;
+  if (auto *ISB = dyn_cast_or_null<InputSectionBase>(IS))
+    IS = ISB->Repl;
+  return IS == &InputSection::Discarded || !IS->Live;
+}
+
 // This function applies relocations to sections without SHF_ALLOC bit.
 // Such sections are never mapped to memory at runtime. Debug sections are
 // an example. Relocations in non-alloc sections are much easier to
 // handle than in allocated sections because it will never need complex
 // treatement such as GOT or PLT (because at runtime no one refers them).
 // So, we handle relocations for non-alloc sections directly in this
 // function as a performance optimization.
 template <class ELFT, class RelTy>
-void InputSection::relocateNonAlloc(uint8_t *Buf, ArrayRef<RelTy> Rels) {
+void InputSection::relocateNonAlloc(uint8_t *Buf, ArrayRef<RelTy> Rels,
+                                    StringRef Name) {
   typedef typename ELFT::uint uintX_t;
+  bool IsDebugRanges = Name == ".debug_ranges";
+
   for (const RelTy &Rel : Rels) {
     uint32_t Type = Rel.getType(Config->IsMips64EL);
     uint64_t Offset = getOffset(Rel.r_offset);
     uint8_t *BufLoc = Buf + Offset;
     int64_t Addend = getAddend<ELFT>(Rel);
     if (!RelTy::IsRela)
       Addend += Target->getImplicitAddend(BufLoc, Type);
 
     SymbolBody &Sym = this->getFile<ELFT>()->getRelocTargetSym(Rel);
     RelExpr Expr = Target->getRelExpr(Type, Sym);
     if (Expr == R_NONE)
       continue;
     if (Expr != R_ABS) {
       error(this->getLocation<ELFT>(Offset) + ": has non-ABS reloc");
       return;
     }
 
     uintX_t AddrLoc = this->OutSec->Addr + Offset;
     uint64_t SymVA = 0;
     if (!Sym.isTls() || Out::TlsPhdr)
       SymVA = SignExtend64<sizeof(uintX_t) * 8>(
           getRelocTargetVA<ELFT>(Type, Addend, AddrLoc, Sym, R_ABS));
+    // If an address range list contains an entry that belongs to a discarded
+    // section, we should perform a fix-up. We cannot write zero as the start
+    // and end address because such a pair is an end of range list marker in the
+    // .debug_ranges section. We put a placeholder value equal to 1, which is
+    // fine because a range list entry whose beginning and end address are equal
+    // has no effect, because the size of the range is zero in such case.
+    if (IsDebugRanges && isInDiscardedSection(Sym))
+      SymVA = 1;
     Target->relocateOne(BufLoc, Type, SymVA);
   }
 }
 
 template <class ELFT> elf::ObjectFile<ELFT> *InputSectionBase::getFile() const {
   return cast_or_null<elf::ObjectFile<ELFT>>(File);
 }
 
 template <class ELFT>
 void InputSectionBase::relocate(uint8_t *Buf, uint8_t *BufEnd) {
   // scanReloc function in Writer.cpp constructs Relocations
   // vector only for SHF_ALLOC'ed sections. For other sections,
   // we handle relocations directly here.
   auto *IS = dyn_cast<InputSection>(this);
   if (IS && !(IS->Flags & SHF_ALLOC)) {
     if (IS->AreRelocsRela)
-      IS->relocateNonAlloc<ELFT>(Buf, IS->template relas<ELFT>());
+      IS->relocateNonAlloc<ELFT>(Buf, IS->template relas<ELFT>(), IS->Name);
     else
-      IS->relocateNonAlloc<ELFT>(Buf, IS->template rels<ELFT>());
+      IS->relocateNonAlloc<ELFT>(Buf, IS->template rels<ELFT>(), IS->Name);
     return;
   }
 
   typedef typename ELFT::uint uintX_t;
   const unsigned Bits = sizeof(uintX_t) * 8;
   for (const Relocation &Rel : Relocations) {
     uint64_t Offset = getOffset(Rel.Offset);
     uint8_t *BufLoc = Buf + Offset;

test/ELF/debug-ranges-reloc.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t1.o
+# RUN: ld.lld %t1.o -o %t -gc-sections
+# RUN: llvm-dwarfdump -debug-dump=all %t | FileCheck %s
+
+# CHECK:     .debug_ranges contents:
+# CHECK-NEXT: 00000000 0000000000000001 0000000000000001
+# CHECK-NEXT: 00000000 0000000000201000 0000000000201002
+# CHECK-NEXT: 00000000 <End of list>
+
+.section .text._Z3aaav,"ax",@progbits
+.Lfunc_begin0:
+nop
+nop
+.Lfunc_end0:
+
+.section .text.start,"ax",@progbits
+.globl _start
+_start:
+.Lfunc_begin1:
+nop
+nop
+.Lfunc_end1:
+
+.section .debug_abbrev,"",@progbits
+ .byte 85                     # DW_AT_ranges
+
+.section .debug_info,"",@progbits
+.Lcu_begin0:
+ .long 12                      # Length of Unit
+ .short 4                      # DWARF version number
+ .long 0                       # Offset Into Abbrev. Section
+ .byte 8                       # Address Size (in bytes)
+ .byte 1                       # Abbrev [1] 0xb:0x25 DW_TAG_compile_unit
+ .long .Ldebug_ranges0         # DW_AT_ranges
+
+.section .debug_ranges,"",@progbits
+.Ldebug_ranges0:
+ .quad .Lfunc_begin0
+ .quad .Lfunc_end0
+ .quad .Lfunc_begin1
+ .quad .Lfunc_end1
+ .quad 0
+ .quad 0