[llvm-objdump] Fix 'llvm-objdump -dr' for executables with relocations

[maksfb]

Summary

Print relocations interleaved with disassembled instructions for
executables with relocatable sections, e.g. those built with "-Wl,-q".

pr/51684

[nickdesaulniers]

Thanks @maksfb , this fixes the issue reported in https://bugs.llvm.org/show_bug.cgi?id=51684. Perhaps you can add pr/51684 to the description+commit message just as a cross reference?

Perhaps it would be good to wait for an additional code review from folks more knowledgeable than myself about this code before merging?

[MaskRay]

llvm/test/tools/llvm-objdump/ELF/exec-relocations.test

54 ↗

(On Diff #369761)

Use just puts

Versioned symbols are not encoded this way.

57 ↗

(On Diff #369761)

This is not needed

[MaskRay]

llvm/test/tools/llvm-objdump/ELF/exec-relocations.test

41 ↗

(On Diff #369761)

The content should be simplified. Having one byte should be sufficient.

Every long stream of opaque bytes if a potential future maintenance burden.

[maksfb]

Address comments.

[MaskRay]

It would be good to wait for @jhenderson as well.

llvm/test/tools/llvm-objdump/ELF/exec-relocations.test

65 ↗

(On Diff #369803)

remove unused main

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

1294

// In executable and shared objects, r_offset holds a virtual address. Subtract SectionAddr from the r_offset field of a relocation to get the section offset.

[jhenderson]

llvm/test/tools/llvm-objdump/ELF/exec-relocations.test

1 ↗

(On Diff #369803)

I think this test needs moving into llvm-objdump/X86, as it disassembles X86 and therefore requires that target. You can maybe rename it elf-disassemble-relocs-exec.test to mirror the existing elf-disassemble-relocs.test found therein.

17 ↗

(On Diff #369803)

You don't need a specific entry point to be defined for this test. This can be omitted.

19 ↗

(On Diff #369803)

You don't need this program header. It shouldn't directly impact the disassembly + relocations.

28 ↗

(On Diff #369803)

There's no need for this alignment value. Just omit it.

33 ↗

(On Diff #369803)

If memory serves me rightly, yaml2obj automatically sets the Address values, based on the program header address value the section is in. I might be misremembering though, so maybe best do some experimentation where one or both sections are missing the address, and see what the output section addresses look like.

35 ↗

(On Diff #369803)

You should be able to simplify this content, so only a small amount is needed (specifically for the two instructions being checked, plus maybe a simple nop instruction before and after, to show relocations aren't printed in the wrong place).

45 ↗

(On Diff #369803)

You can omit this. yaml2obj sets relocation section Link fields to .symtab by default.

54 ↗

(On Diff #369803)

No need to quote the name.

62 ↗

(On Diff #369803)

You don't need to quote the name.

[maksfb]

Address comments.

[jhenderson]

One small suggestion, otherwise this looks fine.

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

31

Consider adding a leading nop too. You need to show that a relocation isn't printed before its correct location (in other words, the leading nop would have no associated relocation).

[maksfb]

Add a leading nop for the test case.

[nickdesaulniers]

thanks for the quick fix; does this still need to be cherry-picked to release/13.x to close out https://bugs.llvm.org/show_bug.cgi?id=51684?

[maksfb]

In D109016#2988172, @nickdesaulniers wrote:

thanks for the quick fix; does this still need to be cherry-picked to release/13.x to close out https://bugs.llvm.org/show_bug.cgi?id=51684?

No problem. Good point, ideally it should be cherry-picked. Do you know what's the logistics for that?

[jhenderson]

In D109016#2988197, @maksfb wrote:

In D109016#2988172, @nickdesaulniers wrote:

thanks for the quick fix; does this still need to be cherry-picked to release/13.x to close out https://bugs.llvm.org/show_bug.cgi?id=51684?

No problem. Good point, ideally it should be cherry-picked. Do you know what's the logistics for that?

I think https://llvm.org/docs/HowToReleaseLLVM.html#merge-requests is the official documentation, although I wouldn't be surprised if it's somewhat out-of-date, given it seems to be using SVN revision numbers! The essence though is that the release manager needs to be advised of which commit to cherry-pick.

[maksfb]

In D109016#2988733, @jhenderson wrote:

In D109016#2988197, @maksfb wrote:

In D109016#2988172, @nickdesaulniers wrote:

thanks for the quick fix; does this still need to be cherry-picked to release/13.x to close out https://bugs.llvm.org/show_bug.cgi?id=51684?

No problem. Good point, ideally it should be cherry-picked. Do you know what's the logistics for that?

I think https://llvm.org/docs/HowToReleaseLLVM.html#merge-requests is the official documentation, although I wouldn't be surprised if it's somewhat out-of-date, given it seems to be using SVN revision numbers! The essence though is that the release manager needs to be advised of which commit to cherry-pick.

Thanks. I see the commit was cherry-picked. Likely because the bug was marked as blocking the release.

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

+## Check that 'llvm-objdump -dr' correctly prints relocations in executables.
+
+# RUN: yaml2obj --docnum=1 %s -o %t
+# RUN: llvm-objdump -dr %t | FileCheck %s
+
+# CHECK:       400000: 90                            nop
+# CHECK-NEXT:  400001: bf 10 00 40 00                movl    $4194320, %edi
+# CHECK-NEXT:                0000000000400002:  R_X86_64_32  .rodata
+# CHECK-NEXT:  400006: e8 fc fe ff ff                callq   0x3fff07
+# CHECK-NEXT:                0000000000400007:  R_X86_64_PLT32       puts-0x4
+# CHECK-NEXT:  40000b: 90                            nop
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+ProgramHeaders:
+  - Type:            PT_LOAD
+    Flags:           [ PF_X, PF_R ]
+    FirstSec:        .text
+    LastSec:         .rodata
+    VAddr:           0x400000
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x400000
+    AddressAlign:    0x10
+    Content:         90BF10004000E8FCFEFFFF90
+  - Name:            .rodata
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x8
+    Content:         00
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Flags:           [ SHF_INFO_LINK ]
+    AddressAlign:    0x8
+    Info:            .text
+    Relocations:
+      - Offset:          0x400002
+        Symbol:          .rodata
+        Type:            R_X86_64_32
+        Addend:          0
+      - Offset:          0x400007
+        Symbol:          puts
+        Type:            R_X86_64_PLT32
+        Addend:          -4
+Symbols:
+  - Name:            .rodata
+    Type:            STT_SECTION
+    Section:         .rodata
+    Value:           0x400628
+  - Name:            puts
+    Type:            STT_FUNC
+    Binding:         STB_GLOBAL
+...

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

for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
 
     SmallString<40> Comments;
     raw_svector_ostream CommentStream(Comments);
 
     uint64_t VMAAdjustment = 0;
     if (shouldAdjustVA(Section))
       VMAAdjustment = AdjustVMA;
 
+    // In executable and shared objects, r_offset holds a virtual address.
+    // Subtract SectionAddr from the r_offset field of a relocation to get
+    // the section offset.
+    uint64_t RelAdjustment = Obj->isRelocatableObject() ? 0 : SectionAddr;
     uint64_t Size;
     uint64_t Index;
     bool PrintedSection = false;
     std::vector<RelocationRef> Rels = RelocMap[Section];
     std::vector<RelocationRef>::const_iterator RelCur = Rels.begin();
     std::vector<RelocationRef>::const_iterator RelEnd = Rels.end();
     // Disassemble symbol by symbol.
     for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
       std::string SymbolName = Symbols[SI].Name.str();
       if (Demangle)
         SymbolName = demangle(SymbolName);
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
         } else {
           // 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)
-              MaxOffset = RelCur->getOffset() - Index;
+              MaxOffset = std::min(RelCur->getOffset() - RelAdjustment - Index,
+                                   MaxOffset);
 
             if (size_t N =
                     countSkippableZeroBytes(Bytes.slice(Index, MaxOffset))) {
               FOS << "\t\t..." << '\n';
               Index += N;
               continue;
             }
           }
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
         emitPostInstructionInfo(FOS, *Ctx.getAsmInfo(), *STI,
                                 CommentStream.str(), LVP);
         Comments.clear();
 
         // Hexagon does this in pretty printer
         if (Obj->getArch() != Triple::hexagon) {
           // Print relocation for instruction and data.
           while (RelCur != RelEnd) {
-            uint64_t Offset = RelCur->getOffset();
+            uint64_t Offset = RelCur->getOffset() - RelAdjustment;
             // If this relocation is hidden, skip it.
             if (getHidden(*RelCur) || SectionAddr + Offset < StartAddress) {
               ++RelCur;
               continue;
             }
 
             // Stop when RelCur's offset is past the disassembled
             // instruction/data. Note that it's possible the disassembled data