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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 5 inline comments as done.
MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1117
+  outs() << "\t\t.byte\t" << format_hex(Bytes[0], 4);
+  return 1;
 }
----------------
nickdesaulniers wrote:
> It looks like this code previously advanced the `Index`, but this version doesn't.  Intentional?
Yes. The two loops are merged.  `Index += Size` (in the very end of the `while (Index < Size)` loop) will increment `Index`.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1462-1467
+          DumpARMELFData = Kind == 'd';
+          if (SecondarySTI) {
+            if (Kind == 'a') {
+              STI = PrimaryIsThumb ? SecondarySTI : PrimarySTI;
+              DisAsm = PrimaryIsThumb ? SecondaryDisAsm : PrimaryDisAsm;
+            } else if (Kind == 't') {
----------------
nickdesaulniers wrote:
> 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'`.
`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.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1475-1476
+        if (DumpARMELFData) {
+          Size = dumpARMELFData(SectionAddr, Index, End, Obj, Bytes,
+                                MappingSymbols);
+        } else {
----------------
nickdesaulniers wrote:
> 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`?
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.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1480
+          // them. Otherwise we might want to skip zero bytes we see.
+          if (!DisassembleZeroes) {
+            uint64_t MaxOffset = End - Index;
----------------
nickdesaulniers wrote:
> This looks like its on the wrong side of the `else`. What happens if I disassemble with `-z` for mixed aarch32/aarch64 code?
`-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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79284/new/

https://reviews.llvm.org/D79284