[llvm-objdump] Fix incomplete relocation output for -D -r mode

[jasonliu]

Summary

This patch intends to fix incomplete relocation printing for XCOFF (potentially for other targets).

[hubert.reinterpretcast]

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

1579

I think this comment is out of date. There might be others in this block.

1589

I think XCOFF32 is working because the pointer length is the same as the instruction length?

[jasonliu]

Fix comments.

[hubert.reinterpretcast]

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

1589

This was mainly a comment about how the output would be displayed. The status quo (relocation prints "in the middle") felt weird to me, but I have now found that it matches the binutils objdump output.

00000000000000c8 <main>:
  c8:   00 00 00 00     .long 0x0
                        c8: R_POS_64    .main
  cc:   00 00 00 00     .long 0x0

Should we add a comment that this checks only the start of the relocation (and not its end) on purpose?

[jasonliu]

Add comments for print in the middle behavior.

[jasonliu]

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

1589

Thanks. Comment added.

[MaskRay]

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

1589

This was mainly a comment about how the output would be displayed. The status quo (relocation prints "in the middle") felt weird to me, but I have now found that it matches the binutils objdump output.

I think the idea is that the relocations that may modify the instruction are printed after the instruction.

[hubert.reinterpretcast]

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

1

I would suggest adding an x86_64 test file (llvm/test/tools/llvm-objdump/MachO/disassemble-relocs-data-x86_64.test) for another object file format that can expose similar issues.

The following, compiled for x86_64-apple-darwin would seem to fit:

extern char s[];
void *p = s + 0x60606060606060;

yaml2obj/obj2yaml does not seem to preserve relocation information for Mach-O, but the output seems to be good enough for the test.

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

1590

Split the sentence using a period instead of using ", and".
s/relocation prints/the relocation print/;

1591

s/. But this matches/, but this is what matches/;

[jhenderson]

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

1591

Actually, I think "but this matches" is a better phrasing (but should be part of the sentence before as you suggest). I'd also use a colon rather than a full stop for the end of the "Note..." bit. I'd recommend the following, with some other minor rewording too:

"... is not the complete data: we might see the relocation printed in the middle of the data, but this matches the binutils objdump output."

[jasonliu]

Added test case for Macho. Adjusted comments as suggested.

[hubert.reinterpretcast]

This LGTM. Thanks.

[MaskRay]

llvm/test/tools/llvm-objdump/MachO/Inputs/relocs-data-macho-x86_64.yaml

3 ↗

(On Diff #256086)

The existing problem with obj2yaml's Mach-O port is that the output is too verbose.

Can you check whether some fields can be deleted and reduce the YAML as much as possible?

[jhenderson]

llvm/test/tools/llvm-objdump/MachO/Inputs/relocs-data-macho-x86_64.yaml

1 ↗

(On Diff #256086)

Is this file used in only one test? If so, it could just be folded into that test.

[jasonliu]

Fold test case into a single file.

[jasonliu]

@MaskRay
I was trying to remove some fields, but I will get " truncated or malformed object" error from "llvm-objdump -D -r" command for those objects.
I suspect after removing some fields, I will need to edit other fields in order for every offset/length to add up correctly.
So I kept the yaml file as it is.

[saugustine]

As written, this test writes to the current directory, which assumes that the current directory is writable, but that is not true in all builds. I have committed 215e6bfcfb5af7713ec348f679c7be4d2f32dc82 to fix.

[MaskRay]

llvm/test/tools/llvm-objdump/MachO/disassemble-relocs-data-x86_64.test

2 ↗

(On Diff #257000)

This should be yaml2obj %s -o %.o

(yaml2obj does not record the source filename in the output, which may make FileCheck tests brittle sometimes.)

[jasonliu]

In D77580#1978986, @saugustine wrote:

As written, this test writes to the current directory, which assumes that the current directory is writable, but that is not true in all builds. I have committed 215e6bfcfb5af7713ec348f679c7be4d2f32dc82 to fix.

@saugustine Thanks for the quick fix.

llvm/test/tools/llvm-objdump/MachO/disassemble-relocs-data-x86_64.test

2 ↗

(On Diff #257000)

Thanks for pointing that out. Addressed in post commit: rGd5143e3f102a91599682fc630205e5c6fec58c76.

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

 # REQUIRES: powerpc-registered-target
 
 # RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
-# RUN:   FileCheck --check-prefixes=CHECK,ONLY-D %s
+# RUN:   FileCheck %s
 
 # RUN: llvm-objdump -D -r %p/Inputs/xcoff-section-headers.o | \
 # RUN:   FileCheck --check-prefixes=CHECK,WITH-R %s
 
 ## xcoff-section-headers.o Compiled with IBM XL C/C++ for AIX, V16.1.0
 ## compiler command: xlc -qtls -o xcoff-section-headers.o -c test.c
 ## test.c:
 ## int a;

 CHECK-NEXT:       14: 00 00 00 01                   <unknown>
 CHECK-NEXT:       18: 00 00 00 0c                   <unknown>
 CHECK-NEXT:       1c: 00 04 66 75                   <unknown>
 CHECK-NEXT:       20: 6e 63 00 00                   xoris 3, 19, 0
 CHECK-NEXT:     ...
 CHECK:        Disassembly of section .data:
 CHECK:        00000080 <func>:
 CHECK-NEXT:       80: 00 00 00 94                   <unknown>
+WITH-R-NEXT:                         00000080:  R_POS        func
 CHECK:        00000084 <a>:
 CHECK-NEXT:       84: 00 00 00 a4                   <unknown>
+WITH-R-NEXT:                         00000084:  R_POS        a
 CHECK:        00000088 <b>:
 CHECK-NEXT:       88: 00 00 00 a0                   <unknown>
+WITH-R-NEXT:                         00000088:  R_POS        b
 CHECK:        0000008c <c>:
 CHECK-NEXT:       8c: 00 00 00 08                   <unknown>
+WITH-R-NEXT:                         0000008c:  R_TLS        c
 CHECK:        00000090 <d>:
-ONLY-D-NEXT:      90: 00 00 00 00                   <unknown>
-WITH-R-NEXT:      ...
+CHECK-NEXT:       90: 00 00 00 00                   <unknown>
+WITH-R-NEXT:                         00000090:  R_TLS        d
 CHECK:        00000094 <func>:
 CHECK-NEXT:       94: 00 00 00 00                   <unknown>
+WITH-R-NEXT:                         00000094:  R_POS        .func
 CHECK-NEXT:       98: 00 00 00 80                   <unknown>
+WITH-R-NEXT:                         00000098:  R_POS        TOC
 CHECK-NEXT:       9c: 00 00 00 00                   <unknown>
 CHECK:        000000a0 <b>:
 CHECK-NEXT:       a0: 00 00 30 39                   <unknown>
 CHECK:        Disassembly of section .bss:
 CHECK:        000000a4 <a>:
 CHECK-NEXT:        ...
 CHECK:        Disassembly of section .tdata:
 CHECK:        00000000 <d>:
 CHECK-NEXT:        0: 40 09 21 f9                   bdnzfl  9, 0x21f8
 CHECK-NEXT:        4: f0 1b 86 6e                   <unknown>
 CHECK:        Disassembly of section .tbss:
 CHECK:        00000008 <c>:
 CHECK-NEXT:        ...

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

for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
 
         PIP.printInst(*IP, Disassembled ? &Inst : nullptr,
                       Bytes.slice(Index, Size),
                       {SectionAddr + Index + VMAAdjustment, Section.getIndex()},
                       outs(), "", *STI, &SP, Obj->getFileName(), &Rels);
         outs() << CommentStream.str();
         Comments.clear();
 
-        // If disassembly has failed, continue with the next instruction, to
-        // avoid analysing invalid/incomplete instruction information.
-        if (!Disassembled) {
-          outs() << "\n";
-          Index += Size;
-          continue;
-        }
-
-        // 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))) {
+        // If disassembly has failed, avoid analysing invalid/incomplete
+        // instruction information. Otherwise, try to resolve the target of a
+        // call, tail call, etc. to a specific symbol.
+        if (Disassembled && MIA &&
+            (MIA->isCall(Inst) || MIA->isUnconditionalBranch(Inst) ||
           uint64_t Target;
           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.
             //
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
               outs() << '>';
             }
           }
         }
         outs() << "\n";
 
         // Hexagon does this in pretty printer
         if (Obj->getArch() != Triple::hexagon) {
-          // Print relocation for instruction.
+          // Print relocation for instruction and data.
           while (RelCur != RelEnd) {
             uint64_t Offset = RelCur->getOffset();
             // If this relocation is hidden, skip it.
             if (getHidden(*RelCur) || SectionAddr + Offset < StartAddress) {
               ++RelCur;
               continue;
             }
 
-            // Stop when RelCur's offset is past the current instruction.
+            // Stop when RelCur's offset is past the current instruction/data.
             if (Offset >= Index + Size)
               break;
 
             // When --adjust-vma is used, update the address printed.
             if (RelCur->getSymbol() != Obj->symbol_end()) {
               Expected<section_iterator> SymSI =
                   RelCur->getSymbol()->getSection();
               if (SymSI && *SymSI != Obj->section_end() &&
                   shouldAdjustVA(**SymSI))
                 Offset += AdjustVMA;
             }