[PATCH] D70720: [llvm-objdump] Display locations of variables alongside disassembly

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard marked 24 inline comments as done.
ostannard added inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:558
+  unsigned Precedence;
+  SmallString<20> String;
+
----------------
aprantl wrote:
> 20 seems short. What's expected in here?
This is the rendered DWARF expression, which will typically be something like `R5` or `[SP+16]`.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:589
+    unsigned Opcode = Op.getCode();
+    if (Opcode == dwarf::DW_OP_piece) {
+      // DW_OP_piece - record piece of larger object.
----------------
aprantl wrote:
> Can you convert this into a switch()?
A few of these handle 32 opcodes (e.g. `DW_OP_reg0`..`DW_OP_reg31`), so converting it to a switch would involve expanding them out to 32 case statements each.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:609
+    } else if (Opcode >= dwarf::DW_OP_reg0 && Opcode <= dwarf::DW_OP_reg31) {
+      int DwarfRegNum = Opcode - dwarf::DW_OP_reg0;
+      int LLVMRegNum = *MRI->getLLVMRegNum(DwarfRegNum, false);
----------------
djtodoro wrote:
> I guess the register variables could be declared outside of the statement.
I don't think that would help, they have a different value in each case.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:796
+  // not seen any code which uses anything other than DW_OP_call_frame_cfa
+  // here, so might not be worth the extra complexity.
+  SmallString<20> FrameBase;
----------------
aprantl wrote:
> make this the doxygen comment
This comment applies to this block of code (up to the next comment), not the whole function.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:981
+      ActiveCols[ColIdx].LiveOut = LV.liveAtAddress(NextAddr);
+      LLVM_DEBUG(dbgs() << "pass 1, " << ThisAddr.Address << "-"
+                        << NextAddr.Address << ", " << LV.VarName << ", Col "
----------------
grimar wrote:
> Interesting to hear from others, I am not sure, do we allow `LLVM_DEBUG` in `llvm-objdump` code?
It's currently used in lli, verify-uselistorder, llvm-ifs, llvm-dwarfdump, bugpoint and llvm-jitlink, all in the tools directory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720