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

[Sergio Perez Gonzalez via Phabricator via llvm-commits]

sperezglz 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); }
----------------
jhenderson wrote:
> Nit: missing trailing full stop.
Sure , I'll add it.


================
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 "
----------------
jhenderson wrote:
> "tgt" is not an obvious abbreviation for "target" to me. I'd recommend just "print-target-addr" or "print-target-address".
Yes that makes sense.


================
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 ":  ";
----------------
jhenderson wrote:
> This change seems unnecessary for the rest of the change?
This was necessary because this function is used in the Hexagon prettyPrinter and it was printing directly to "outs()" by default while I'm sending everything to a temporary buffer so I had to add a parameter to make it flexible, this came out on a couple of Hexagon tests failing, but with this change those work fine.


================
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)
----------------
jhenderson wrote:
> 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.
Yes the variable name was a typo I'll change it and add explicit type to the variable.

About the approach, in my opinion the main advantage is that the core of changes is centralized to one file and does not involve changing every target Inst Printer, the hard requirement is that there is a "\t" between instruction and immediate.

I'll fix the stuff pointed out by the comments and leave it for discussion and reference in case other suggestions come up.




================
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();
----------------
jhenderson wrote:
> 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();
> ```
> 
You are right , I'll clean it up.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1506
+        Comments.clear();
+        InstPrinterBuffer.clear();
         outs() << "\n";
----------------
jhenderson wrote:
> 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).
Yeah sorry about those two, I tried to keep it safe by clearing but lost track of the scope where they live.



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

https://reviews.llvm.org/D71453