[objdump] Fix disassembly for call/branch instructions for ELF binaries built with -ffunction-sections

[davidb]

Summary

When disassembling relocatable ELF object files built with -ffunction-sections, calls and branches to symbols that live in different text sections cannot be resolved correctly. You could add -r to dump the relocations inbetweeen the disassembly, however this is not widely known by users of objdump, and the default behaviour has been confusing many users of the tool. This behaviour differs from the gnu binutils... but I'm not sure that is really a bad thing.

Outstanding question about the symbol addend? I couldn't find out hor to access that from RelocationRef.

Note that the XCOFF fix has been split into a separate patch.

[MaskRay]

llvm/test/tools/llvm-objdump/X86/demangle.s

14

The addend is incorrectly dropped.

llvm/test/tools/llvm-objdump/X86/elf-disassemble-relocs.test

10–11

This is not correct. whiz is undefined. It is incorrect to symbolize with an undefined symbol.

objdump -d does not print the symbol:

5:   e8 00 00 00 00          callq  0xa
                      6: R_X86_64_PLT32       whiz+0x1

llvm/test/tools/llvm-objdump/X86/section-filter-relocs.test

13

foo is undefined. It is not correct to symbolize with an undefined symbol. The problem is that the display can get addends wrong.

For disassembly of object files, I think users should realize that they need to use -d -r.

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

3 ↗

(On Diff #245362)

You can move XCOFF support to a separate change.

[davidb]

llvm/test/tools/llvm-objdump/X86/section-filter-relocs.test

13

It is not correct to symbolize with an undefined symbol.

What do you mean by symbolize? Are you saying it is incorrect to print the name of the symbol that the call or branch instruction is to be patched to when linked? How is <.text+0x5> more correct?that seems like garbage to me...

In the -ffunction-section case, these symbols are defined, but live in a separate section, and therefore require a relocation. The result is the same as if the symbol was undefined. Would it be ok if defined symbols were displayed, but undefined ones left to print garbage?

For disassembly of object files, I think users should realize that they need to use -d -r.

This is pretty much what is being asked in the OP. And I'm not sure I agree. When a user is compiling ELF binaries with -ffunction-sections, then -d is pretty much useless. Worse than useless in the case that on some platforms it will pick the symbol name for that particular section, so it looks like a genuine call

[davidb]

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

3 ↗

(On Diff #245362)

Was submitted before as D74823

[jhenderson]

Looks like you've got changes related to D74823 mixed in here?

[davidb]

In D74824#1890859, @jhenderson wrote:

Looks like you've got changes related to D74823 mixed in here?

Yes I put a note in the original description mentioning that this has been split to a separate change.

[jhenderson]

In D74824#1891127, @davidb wrote:

In D74824#1890859, @jhenderson wrote:

Looks like you've got changes related to D74823 mixed in here?

Yes I put a note in the original description mentioning that this has been split to a separate change.

It would be clearer if you made them as two separate diffs, with the one that needs to be second based on the first one, so that Phabricator only shows the differences related to this patch (in other words, each Phabricator review represents one git commit). You can also indicate the dependency by saying "Depends on DXXXXXX" in the patch description, and Phabricator will then link the two patches together.

[davidb]

rebase

[davidb]

In D74824#1891171, @jhenderson wrote:

In D74824#1891127, @davidb wrote:

In D74824#1890859, @jhenderson wrote:

Looks like you've got changes related to D74823 mixed in here?

Yes I put a note in the original description mentioning that this has been split to a separate change.

It would be clearer if you made them as two separate diffs, with the one that needs to be second based on the first one, so that Phabricator only shows the differences related to this patch (in other words, each Phabricator review represents one git commit). You can also indicate the dependency by saying "Depends on DXXXXXX" in the patch description, and Phabricator will then link the two patches together.

Done. I'm aware that there is more required to do on this change, but I don't want to continue until we answer the question about handling undefined symbols, or symbols defined in different text sections... First impression is that current behaviour that matches GNU is preferable.

[jhenderson]

On the references to undefined symbols, I don't have a strong opinion either way. It is nice to see the symbol name, as produced in this case, and I don't know how important GNU compatibility is here. On the other hand, if you print with inline relocations, you will get the symbol name from that, so maybe it's unnecessary?

[MaskRay]

I think symbolizing an undefined symbol as if it is defined is fine.

llvm/tools/llvm-objdump/llvm-objdump.cpp

1463

Addends may be important for "range extension thunks with addends"

https://bugs.llvm.org/show_bug.cgi?id=40438

I think a sensible approach is to shift r_addend as if r_offset is moved to the start address of the instruction.

[MaskRay]

Request changes as this may regress disassembly for "range extension thunks with addends" (see inline comment)

llvm/test/tools/llvm-objdump/X86/demangle.s

 # RUN: llvm-mc %s -filetype=obj -triple=x86_64-pc-linux -o %t
 # RUN: llvm-objdump -t -r --demangle %t | FileCheck %s
 
 ## Check we demangle symbols when printing symbol table.
 # CHECK:      SYMBOL TABLE:
 # CHECK-NEXT: 0000000000000000 g     F .text           00000000 foo()
 
 ## Check we demangle symbols when printing relocations.
 # CHECK:      000000000000001 R_X86_64_PLT32 foo()-0x4
 
 ## Check the case when relocations are inlined into disassembly.
 # RUN: llvm-objdump -d -r --demangle %t | FileCheck %s --check-prefix=INLINE
 # INLINE:      foo():
-# INLINE-NEXT:  0: {{.*}}  callq   0 <_Z3foov+0x5>
+# INLINE-NEXT:  0: {{.*}}  callq   0 <_Z3foov>
 # INLINE-NEXT:  0000000000000001:  R_X86_64_PLT32 foo()-0x4
 
 .text
 .globl _Z3foov
 .type _Z3foov,@function
 _Z3foov:
  callq _Z3foov@PLT

llvm/test/tools/llvm-objdump/X86/elf-disassemble-relocs.test

 ## Show that --disassemble + --reloc prints relocations inline and does not dump
 ## the relocation sections.
 
 # RUN: yaml2obj %s --docnum=1 -o %t1.o
 # RUN: llvm-objdump %t1.o -d -r | FileCheck %s --implicit-check-not="RELOCATION RECORDS"
 
-# CHECK:      0: e8 00 00 00 00                callq   0 <.text+0x5>
+# CHECK:      0: e8 00 00 00 00                callq   0 <foo>
 # CHECK-NEXT:          0000000000000001:  R_X86_64_PC32        foo-0x4
 # CHECK-NEXT:          0000000000000002:  R_X86_64_NONE        bar+0x8
-# CHECK-NEXT: 5: e8 00 00 00 00                callq   0 <.text+0xa>
-# CHECK-NEXT:          0000000000000006:  R_X86_64_PLT32       foo+0x1
+# CHECK-NEXT: 5: e8 00 00 00 00                callq   0 <whiz>
+# CHECK-NEXT:          0000000000000006:  R_X86_64_PLT32       whiz+0x1
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
Relocations:
         Symbol: foo
         Type:   R_X86_64_PC32
         Addend: -4
       - Offset: 2
         Symbol: bar
         Type:   R_X86_64_NONE
         Addend: 8
       - Offset: 6
-        Symbol: foo
+        Symbol: whiz
         Type:   R_X86_64_PLT32
         Addend: 1
 Symbols:
   - Name: foo
   - Name: bar
+  - Name: whiz
 
 ## Check we report an error if the relocated section identified by the
 ## sh_info field of a relocation section is invalid.
 
 # RUN: yaml2obj %s --docnum=2 -o %t2.o
 # RUN: not llvm-objdump %t2.o --disassemble --reloc 2>&1 | FileCheck %s -DFILE=%t2.o --check-prefix=ERR
 
 # ERR: error: '[[FILE]]': section (1): failed to get a relocated section: invalid section index: 255

llvm/test/tools/llvm-objdump/X86/elf-disassemble-symbol-references.yaml

- Name:    fourth
     Value:   0x400A
 
 # RUN: yaml2obj %s --docnum=2 -o %t.o
 # RUN: llvm-objdump %t.o -d | FileCheck %s --check-prefix=REL
 
 # REL:      Disassembly of section .text1:
 # REL-EMPTY:
 # REL-NEXT: 0000000000000000 .text1:
-# REL-NEXT:        0: e8 00 00 00 00                callq   0 <.text1+0x5>
+# REL-NEXT:        0: e8 00 00 00 00                callq   0 <sym3>
 # REL-EMPTY:
 # REL-NEXT: Disassembly of section .text2:
 # REL-EMPTY:
 # REL-NEXT: 0000000000000000 .text2:
 # REL-NEXT:        0: e8 00 00 00 00                callq   0 <sym2>
 # REL-EMPTY:
 # REL-NEXT: Disassembly of section .text3:
 # REL-EMPTY:

llvm/test/tools/llvm-objdump/X86/section-filter-relocs.test

 ## Test that --section works correctly for -d with -r.
 # RUN: yaml2obj %s -o %t.o
 
 ## Show that only the specified sections are disassembled.
 ## Also show that no relocation sections are dumped because none are
 ## specified.
 # RUN: llvm-objdump -d -r %t.o --section=.text --section=.rodata \
 # RUN:   | FileCheck %s --check-prefixes=DISASM,RELOC --implicit-check-not=.text2
 
 # DISASM:       Disassembly of section .text:
 # DISASM-EMPTY:
 # DISASM-NEXT:  0000000000000400 .text:
-# DISASM-NEXT:  400: e8 00 00 00 00                callq   0 <.text+0x5>
+# DISASM-NEXT:  400: e8 00 00 00 00                callq   0 <foo>
 # RELOC-NEXT:                      00000401:  R_X86_64_PC32        foo+0x1
 # RELOC-NEXT:                      00000401:  R_X86_64_GOT32       foo
 # DISASM:       Disassembly of section .rodata:
 # DISASM-EMPTY:
 # DISASM-NEXT:  0000000000000000 .rodata:
 # DISASM-NEXT:  0: 00 00                           addb    %al, (%rax)
 # RELOC-NEXT:              0000000000000000:  R_X86_64_NONE        foo
 # DISASM-NEXT:  2: 00 00                           addb    %al, (%rax)

llvm/tools/llvm-objdump/llvm-objdump.cpp

static bool isArmElf(const ObjectFile *Obj) {
   const auto *Elf = dyn_cast<ELFObjectFileBase>(Obj);
   return Elf && Elf->getEMachine() == ELF::EM_ARM;
 }
 
 static bool hasMappingSymbols(const ObjectFile *Obj) {
   return isArmElf(Obj) || isAArch64Elf(Obj);
 }
 
+static void printBranchTarget(StringRef TargetName, uint64_t Displacement) {
+  outs() << " <" << TargetName;
+  if (Displacement)
+    outs() << "+0x" << Twine::utohexstr(Displacement);
+  outs() << '>';
+}
+
 static void printRelocation(StringRef FileName, const RelocationRef &Rel,
                             uint64_t Address, bool Is64Bits) {
   StringRef Fmt = Is64Bits ? "\t\t%016" PRIx64 ":  " : "\t\t\t%08" PRIx64 ":  ";
   SmallString<16> Name;
   SmallString<32> Val;
   Rel.getTypeName(Name);
   if (Error E = getRelocationValueString(Rel, Val))
     reportError(std::move(E), FileName);
static void disassembleObject(const Target *TheTarget, const ObjectFile *Obj,
                               SourcePrinter &SP, bool InlineRelocs) {
   const MCSubtargetInfo *STI = PrimarySTI;
   MCDisassembler *DisAsm = PrimaryDisAsm;
   bool PrimaryIsThumb = false;
   if (isArmElf(Obj))
     PrimaryIsThumb = STI->checkFeatures("+thumb-mode");
 
   std::map<SectionRef, std::vector<RelocationRef>> RelocMap;
-  if (InlineRelocs)
-    RelocMap = getRelocsMap(*Obj);
   bool Is64Bits = Obj->getBytesInAddress() > 4;
 
   // Create a mapping from virtual address to symbol name.  This is used to
   // pretty print the symbols while disassembling.
   std::map<SectionRef, SectionSymbolsTy> AllSymbols;
   SectionSymbolsTy AbsoluteSymbols;
   const StringRef FileName = Obj->getFileName();
   const MachOObjectFile *MachO = dyn_cast<const MachOObjectFile>(Obj);
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
         }
 
         // When -z or --disassemble-zeroes are given we always dissasemble
         // them. Otherwise we might want to skip zero bytes we see.
         if (!DisassembleZeroes) {
           uint64_t MaxOffset = End - Index;
           // For -reloc: print zero blocks patched by relocations, so that
           // relocations can be shown in the dump.
-          if (RelCur != RelEnd)
+          if (InlineRelocs && RelCur != RelEnd)
             MaxOffset = RelCur->getOffset() - Index;
 
           if (size_t N =
                   countSkippableZeroBytes(Bytes.slice(Index, MaxOffset))) {
             outs() << "\t\t..." << '\n';
             Index += N;
             continue;
           }
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
         // If disassembly has failed, continue with the next instruction, to
         // avoid analysing invalid/incomplete instruction information.
         if (!Disassembled) {
           outs() << "\n";
           Index += Size;
           continue;
         }
 
+        // Find the next relocation
+        while (RelCur != RelEnd &&
+               (getHidden(*RelCur) ||
+                SectionAddr + RelCur->getOffset() < StartAddress)) {
+          ++RelCur;
+        }
+
         // Try to resolve the target of a call, tail call, etc. to a specific
         // symbol.
         if (MIA && (MIA->isCall(Inst) || MIA->isUnconditionalBranch(Inst) ||
                     MIA->isConditionalBranch(Inst))) {
           uint64_t Target;
-          if (MIA->evaluateBranch(Inst, SectionAddr + Index, Size, Target)) {
+          if (RelCur != RelEnd) {
+              auto const Sym = RelCur->getSymbol();
+              auto Name = Sym->getName();
+              if (Name)
+                printBranchTarget(*Name, 0 /* would expect this to be symbol addend? */);
+          }
+          else if (MIA->evaluateBranch(Inst, SectionAddr + Index, Size, Target)) {
             // In a relocatable object, the target's section must reside in
             // the same section as the call instruction or it is accessed
             // through a relocation.
             //
             // In a non-relocatable object, the target may be in any section.
             //
             // N.B. We don't walk the relocations in the relocatable case yet.
             auto *TargetSectionSymbols = &Symbols;
             if (!Obj->isRelocatableObject()) {
               auto It = partition_point(
                   SectionAddresses,
                   [=](const std::pair<uint64_t, SectionRef> &O) {
                     return O.first <= Target;
                   });
               if (It != SectionAddresses.begin()) {
                 --It;
                 TargetSectionSymbols = &AllSymbols[It->second];
               } else {
                 TargetSectionSymbols = &AbsoluteSymbols;
               }
             }
-
             // Find the last symbol in the section whose offset is less than
             // or equal to the target. If there isn't a section that contains
             // the target, find the nearest preceding absolute symbol.
             auto TargetSym = partition_point(
                 *TargetSectionSymbols,
                 [=](const SymbolInfoTy &O) {
                   return O.Addr <= Target;
                 });
             if (TargetSym == TargetSectionSymbols->begin()) {
               TargetSectionSymbols = &AbsoluteSymbols;
               TargetSym = partition_point(
                   AbsoluteSymbols,
                   [=](const SymbolInfoTy &O) {
                     return O.Addr <= Target;
                   });
             }
             if (TargetSym != TargetSectionSymbols->begin()) {
               --TargetSym;
               uint64_t TargetAddress = TargetSym->Addr;
               StringRef TargetName = TargetSym->Name;
-              outs() << " <" << TargetName;
-              uint64_t Disp = Target - TargetAddress;
-              if (Disp)
-                outs() << "+0x" << Twine::utohexstr(Disp);
-              outs() << '>';
+              printBranchTarget (TargetName, Target - TargetAddress);
             }
           }
         }
         outs() << "\n";
 
         // Hexagon does this in pretty printer
-        if (Obj->getArch() != Triple::hexagon) {
+        if (InlineRelocs && Obj->getArch() != Triple::hexagon) {
           // Print relocation for instruction.
           while (RelCur != RelEnd) {
             uint64_t Offset = RelCur->getOffset();
             // If this relocation is hidden, skip it.
             if (getHidden(*RelCur) || SectionAddr + Offset < StartAddress) {
               ++RelCur;
               continue;
             }