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

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

ostannard added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:130
+  Distance to indent the source-level variable display, relative to the start
+  of the disassembly. Defaults to 50 characters.
+
----------------
jhenderson wrote:
> I just tried this out locally, and on an already-wide window, a fairly basic expression wrapped with the default of 50 characters. Any particular reason you've used 50?
I picked that to be wide enough to avoid colliding with the disassembled instructions, except in cases where they include long symbol names. However, maybe it is a bit too high. I'll reduce it to 40, which still avoids collisions with a few samples of (ARM and AArch64) code I've tried, and avoids wrapping on an 80-column terminal with the `--no-show-raw-insn` option. I'd recommend using this with `--no-show-raw-insn`, especially on narrow terminals, because the disassembly alone ends at around column 65-70 without it, not leaving enough room for the variable display.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h:121
 
+    iterator skip_bytes(uint32_t Add) {
+      return iterator(Expr, Op.EndOffset + Add);
----------------
jhenderson wrote:
> `skip_bytes` -> `skipBytes` to match LLVM coding standards.
> 
> Also, use `uint64_t` instead of `uint32_t`, sine that's what the type of `Op.EndOffset` is.
I've removed this function, it's not needed until a later patch.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:405
+bool DWARFExpression::prettyPrint(raw_ostream &OS, const MCRegisterInfo *MRI) {
+  return prettyPrintDWARFExpr(OS, begin(), end(), MRI);
+}
----------------
jhenderson wrote:
> Any particular reason you haven't just inlined the whole function here?
The outlined version will be used by DW_OP_fbreg and DW_OP_entry_value, which need to print a sub-range of the expression. I could inline it here, but would have to revert that soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720