[PATCH] D71453: [llvm-objdump] Print target address instead of call/j/br pc-rel immediate

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/MC/MCInstrDesc.h:280
 
+  /// Return true if this is an indirect call
+  bool isIndirectCall() const { return Flags & (1ULL << MCID::IndirectCall); }
----------------
Nit: missing trailing full stop.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:341
+cl::opt<bool>
+    PrintTargetAddress("print-tgt-addr",
+                       cl::desc("Print target address instead of immediate "
----------------
"tgt" is not an obvious abbreviation for "target" to me. I'd recommend just "print-target-addr" or "print-target-address".


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:680
 static void printRelocation(StringRef FileName, const RelocationRef &Rel,
-                            uint64_t Address, bool Is64Bits) {
+                            uint64_t Address, bool Is64Bits, raw_ostream &OS) {
   StringRef Fmt = Is64Bits ? "\t\t%016" PRIx64 ":  " : "\t\t\t%08" PRIx64 ":  ";
----------------
This change seems unnecessary for the rest of the change?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1493
+          if (PrintTargetAddress) {
+            auto splitInstStream = InstPrintStream.str().rsplit('\t');
+            outs() << splitInstStream.first << '\t' << Twine::utohexstr(Target)
----------------
LLVM style is to use upper-case for variable names (i.e. splitInstStream -> SplitInstStream). It's also not immediately obvious what `auto` represents here, so consider replacing it with the full type.

That all being said, I'm not especially keen on this approach, as it seems to tie the code here to how the instructions are printed inside the corresponding instruction printer. It feels like something that could break very easily to me. That being said, I don't have a clear suggestion for an alternative at this point. @MaskRay might do.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1494-1498
+            outs() << splitInstStream.first << '\t' << Twine::utohexstr(Target)
+                   << CommentStream.str() << SymbolDetailsStream.str();
+          } else {
+            outs() << InstPrintStream.str() << CommentStream.str()
+                   << SymbolDetailsStream.str();
----------------
You can reduce some of the duplication here by adding the comment and symbol details outside this if:

```
          if (PrintTargetAddress) {
            auto splitInstStream = InstPrintStream.str().rsplit('\t');
            outs() << splitInstStream.first << '\t' << Twine::utohexstr(Target);
          } else {
            outs() << InstPrintStream.str();
          }
          outs() << CommentStream.str() << SymbolDetailsStream.str();
```



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1500
+          }
+          TargetSymbolDetails.clear();
+        } else {
----------------
What's the point of this line? It's about to go out of scope.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1506
+        Comments.clear();
+        InstPrinterBuffer.clear();
         outs() << "\n";
----------------
Same comment as above. `InstPrinterBuffer` is not going to be used again, so why clear it?

Note that the `Comments` string and stream are constructed outside the loop, so need to be cleared in order to be reused. You may wish to do the same thing with the other streams for performance reasons (though I don't know for sure).


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

https://reviews.llvm.org/D71453