[llvm-objdump][ARM] Print inline relocations when dumping ARM data

[MaskRay]

Summary

Fixes PR44357

For ARM ELF, regions covered by data mapping symbols $d are dumped as .byte, .short or .word but inline relocations are not printed. This patch merges its loop into the normal instruction printing loop so that inline relocations are printed.

[rovka]

Hi, I like that we're looping in only one place now, but I have 2 nitpicks:

1. The formatting changes are pretty distracting, I think you should commit them separately.
2. The commit message should explain what the problem is and why this change fixes it. Just referencing a bug number, where there haven't even been any discussions, is not very helpful.

Thanks!

[MaskRay]

In D79284#2017321, @rovka wrote:

Hi, I like that we're looping in only one place now, but I have 2 nitpicks:

1. The formatting changes are pretty distracting, I think you should commit them separately.
2. The commit message should explain what the problem is and why this change fixes it. Just referencing a bug number, where there haven't even been any discussions, is not very helpful.

Thanks!

The else branch of if (DumpARMELFData) { increases indentation, thus causing some minor code/comment formatting. Phabricator is smart enough to hide indentation changes, though.

[nickdesaulniers]

Thanks for the patch! This will allow me to use llvm-objdump again for aarch32!

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

1124

It looks like this code previously advanced the Index, but this version doesn't. Intentional?

1460–1461

Can we not dumpARMELFData as soon as we see a SectionKind of 'd'? I'm curious if we could eliminate DumpARMELFData, or if there's some complicated case where CheckARMELFData could be false, yet DumpARMELFData is true?

1461–1466

Would this be simpler as a switch (Kind) or even switch (getMappingSymbolKind(MappingSymbols, Index)? Then the two cases for 't' and 'a' could have the same body, and you could have another case for 'd'.

1465

This looks like its on the wrong side of the else. What happens if I disassemble with -z for mixed aarch32/aarch64 code?

[MaskRay]

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

1124

Yes. The two loops are merged. Index += Size (in the very end of the while (Index < Size) loop) will increment Index.

1460–1461

There are two alternatives:

• dumpARMELFData
• regular instruction printing. See getInstruction+printInst below.

They are followed by (complex) relocation printing and Index += Size. The idea of the patch is to let dumpARMELFData reuse the existing relocation printing.

1461–1466

SecondarySTI is not always initialized (if the +mclass feature (I don't know what it is) is enabled). I tried but a switch would be more complex.

1465

-z is part of instruction printing and I do not intend to change that.

Long runs of ARM zeroes will be dumped as a number of .word 0 directives.

[nickdesaulniers]

Cool, thanks for the additional info, LGTM.

[efriedma]

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

1461–1466

(+mclass indicates a CPU that only supports Thumb mode.)

[MaskRay]

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

1461–1466

Thanks!

llvm/test/tools/llvm-objdump/ELF/ARM/disassemble-code-data-mix.s

-@RUN: llvm-mc -triple arm-unknown-linux -filetype=obj %s | llvm-objdump -d - | FileCheck %s
+@ RUN: llvm-mc -triple arm-unknown-linux -filetype=obj %s -o %t
+@ RUN: llvm-objdump -d %t | FileCheck %s
+@ RUN: llvm-objdump -d -r %t | FileCheck --check-prefixes=CHECK,RELOC %s
 
   .cpu arm7tdmi
   .global myInt
   .data
   .align  2
   .type myInt, %object
   .size myInt, 4
 myInt:
.L3:
   .word myInt
   .size main, .-main
         .global myStr
         .type myStr, %object
 myStr:
         .string "test string"
 
 
-@CHECK:     .word   0x00000000
-@CHECK-DAG: 74 65 73 74 20 73 74 72         test str
+@ CHECK:       .word   0x00000000
+@ RELOC-NEXT:    R_ARM_ABS32  myInt
+@ CHECK-EMPTY:
+@ CHECK-NEXT:  <myStr>:
+@ CHECK-NEXT:    74 65 73 74 20 73 74 72         test str

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

auto It =
       });
   // Return zero for any address before the first mapping symbol; this means
   // we should use the default disassembly mode, depending on the target.
   if (It == MappingSymbols.begin())
     return '\x00';
   return (It - 1)->second;
 }
 
-static uint64_t
-dumpARMELFData(uint64_t SectionAddr, uint64_t Index, uint64_t End,
-               const ObjectFile *Obj, ArrayRef<uint8_t> Bytes,
+static uint64_t dumpARMELFData(uint64_t SectionAddr, uint64_t Index,
+                               uint64_t End, const ObjectFile *Obj,
+                               ArrayRef<uint8_t> Bytes,
                                ArrayRef<MappingSymbolPair> MappingSymbols) {
   support::endianness Endian =
       Obj->isLittleEndian() ? support::little : support::big;
-  while (Index < End) {
-    outs() << format("%8" PRIx64 ":", SectionAddr + Index);
-    outs() << "\t";
-    if (Index + 4 <= End) {
-      dumpBytes(Bytes.slice(Index, 4), outs());
-      outs() << "\t.word\t"
-             << format_hex(
-                    support::endian::read32(Bytes.data() + Index, Endian), 10);
-      Index += 4;
-    } else if (Index + 2 <= End) {
-      dumpBytes(Bytes.slice(Index, 2), outs());
-      outs() << "\t\t.short\t"
-             << format_hex(
-                    support::endian::read16(Bytes.data() + Index, Endian), 6);
-      Index += 2;
-    } else {
-      dumpBytes(Bytes.slice(Index, 1), outs());
-      outs() << "\t\t.byte\t" << format_hex(Bytes[0], 4);
-      ++Index;
-    }
-    outs() << "\n";
-    if (getMappingSymbolKind(MappingSymbols, Index) != 'd')
-      break;
-  }
-  return Index;
+  outs() << format("%8" PRIx64 ":\t", SectionAddr + Index);
+           << format_hex(support::endian::read32(Bytes.data() + Index, Endian),
+                         10);
+    return 4;
+  }
+  if (Index + 2 <= End) {
+           << format_hex(support::endian::read16(Bytes.data() + Index, Endian),
+                         6);
+    return 2;
+  }
+  return 1;
 }
 
 static void dumpELFData(uint64_t SectionAddr, uint64_t Index, uint64_t End,
                         ArrayRef<uint8_t> Bytes) {
   // print out data up to 8 bytes at a time in hex and ascii
   uint8_t AsciiData[9] = {'\0'};
   uint8_t Byte;
   int NumBytes = 0;
 
   for (; Index < End; ++Index) {
     if (NumBytes == 0)
       outs() << format("%8" PRIx64 ":", SectionAddr + Index);
     Byte = Bytes.slice(Index)[0];
     outs() << format(" %02x", Byte);
     AsciiData[NumBytes] = isPrint(Byte) ? Byte : '.';
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
           dumpELFData(SectionAddr, Index, End, Bytes);
           Index = End;
         }
       }
 
       bool CheckARMELFData = hasMappingSymbols(Obj) &&
                              Symbols[SI].Type != ELF::STT_OBJECT &&
                              !DisassembleAll;
+      bool DumpARMELFData = false;
       while (Index < End) {
         // ARM and AArch64 ELF binaries can interleave data and text in the
         // same section. We rely on the markers introduced to understand what
         // we need to dump. If the data marker is within a function, it is
         // denoted as a word/short etc.
-        if (CheckARMELFData &&
-            getMappingSymbolKind(MappingSymbols, Index) == 'd') {
-          Index = dumpARMELFData(SectionAddr, Index, End, Obj, Bytes,
-                                 MappingSymbols);
-          continue;
+        if (CheckARMELFData) {
+          char Kind = getMappingSymbolKind(MappingSymbols, Index);
+          DumpARMELFData = Kind == 'd';
+          if (SecondarySTI) {
+            if (Kind == 'a') {
+              STI = PrimaryIsThumb ? SecondarySTI : PrimarySTI;
+              DisAsm = PrimaryIsThumb ? SecondaryDisAsm : PrimaryDisAsm;
+            } else if (Kind == 't') {
+              STI = PrimaryIsThumb ? PrimarySTI : SecondarySTI;
+              DisAsm = PrimaryIsThumb ? PrimaryDisAsm : SecondaryDisAsm;
+            }
+          }
         }
 
+        if (DumpARMELFData) {
+          Size = dumpARMELFData(SectionAddr, Index, End, Obj, Bytes,
+                                MappingSymbols);
+        } 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;
 
             if (size_t N =
                     countSkippableZeroBytes(Bytes.slice(Index, MaxOffset))) {
               outs() << "\t\t..." << '\n';
               Index += N;
               continue;
             }
           }
 
-        if (SecondarySTI) {
-          if (getMappingSymbolKind(MappingSymbols, Index) == 'a') {
-            STI = PrimaryIsThumb ? SecondarySTI : PrimarySTI;
-            DisAsm = PrimaryIsThumb ? SecondaryDisAsm : PrimaryDisAsm;
-          } else if (getMappingSymbolKind(MappingSymbols, Index) == 't') {
-            STI = PrimaryIsThumb ? PrimarySTI : SecondarySTI;
-            DisAsm = PrimaryIsThumb ? PrimaryDisAsm : SecondaryDisAsm;
-          }
-        }
-
           // Disassemble a real instruction or a data when disassemble all is
           // provided
           MCInst Inst;
-        bool Disassembled = DisAsm->getInstruction(
-            Inst, Size, Bytes.slice(Index), SectionAddr + Index, CommentStream);
+          bool Disassembled =
+              DisAsm->getInstruction(Inst, Size, Bytes.slice(Index),
+                                     SectionAddr + Index, CommentStream);
           if (Size == 0)
             Size = 1;
 
-        PIP.printInst(*IP, Disassembled ? &Inst : nullptr,
-                      Bytes.slice(Index, Size),
-                      {SectionAddr + Index + VMAAdjustment, Section.getIndex()},
-                      outs(), "", *STI, &SP, Obj->getFileName(), &Rels);
+          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, avoid analysing invalid/incomplete
-        // instruction information. Otherwise, try to resolve the target address
-        // (jump target or memory operand address) and print it on the right of
-        // the instruction.
+          // instruction information. Otherwise, try to resolve the target
+          // address (jump target or memory operand address) and print it on the
+          // right of the instruction.
           if (Disassembled && MIA) {
             uint64_t Target;
             bool PrintTarget =
                 MIA->evaluateBranch(Inst, SectionAddr + Index, Size, Target);
             if (!PrintTarget)
               if (Optional<uint64_t> MaybeTarget =
-                    MIA->evaluateMemoryOperandAddress(Inst, SectionAddr + Index,
-                                                      Size)) {
+                      MIA->evaluateMemoryOperandAddress(
+                          Inst, SectionAddr + Index, Size)) {
                 Target = *MaybeTarget;
                 PrintTarget = true;
                 outs() << "  # " << Twine::utohexstr(Target);
               }
             if (PrintTarget) {
               // 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.
-            // In that case, locate the section(s) containing the target address
-            // and find the symbol in one of those, if possible.
+              // In that case, locate the section(s) containing the target
+              // address and find the symbol in one of those, if possible.
               //
               // N.B. We don't walk the relocations in the relocatable case yet.
               std::vector<const SectionSymbolsTy *> TargetSectionSymbols;
               if (!Obj->isRelocatableObject()) {
                 auto It = llvm::partition_point(
                     SectionAddresses,
                     [=](const std::pair<uint64_t, SectionRef> &O) {
                       return O.first <= Target;
                     });
                 uint64_t TargetSecAddr = 0;
                 while (It != SectionAddresses.begin()) {
                   --It;
                   if (TargetSecAddr == 0)
                     TargetSecAddr = It->first;
                   if (It->first != TargetSecAddr)
                     break;
                   TargetSectionSymbols.push_back(&AllSymbols[It->second]);
                 }
               } else {
                 TargetSectionSymbols.push_back(&Symbols);
               }
               TargetSectionSymbols.push_back(&AbsoluteSymbols);
 
-            // Find the last symbol in the first candidate section whose offset
-            // is less than or equal to the target. If there are no such
-            // symbols, try in the next section and so on, before finally using
-            // the nearest preceding absolute symbol (if any), if there are no
-            // other valid symbols.
+              // Find the last symbol in the first candidate section whose
+              // offset is less than or equal to the target. If there are no
+              // such symbols, try in the next section and so on, before finally
+              // using the nearest preceding absolute symbol (if any), if there
+              // are no other valid symbols.
               const SymbolInfoTy *TargetSym = nullptr;
-            for (const SectionSymbolsTy *TargetSymbols : TargetSectionSymbols) {
+              for (const SectionSymbolsTy *TargetSymbols :
+                   TargetSectionSymbols) {
                 auto It = llvm::partition_point(
                     *TargetSymbols,
                     [=](const SymbolInfoTy &O) { return O.Addr <= Target; });
                 if (It != TargetSymbols->begin()) {
                   TargetSym = &*(It - 1);
                   break;
                 }
               }
 
               if (TargetSym != nullptr) {
                 uint64_t TargetAddress = TargetSym->Addr;
                 std::string TargetName = TargetSym->Name.str();
                 if (Demangle)
                   TargetName = demangle(TargetName);
 
                 outs() << " <" << TargetName;
                 uint64_t Disp = Target - TargetAddress;
                 if (Disp)
                   outs() << "+0x" << Twine::utohexstr(Disp);
                 outs() << '>';
               }
             }
           }
+        }
         outs() << "\n";
 
         // 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();
             // If this relocation is hidden, skip it.