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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:132-135
+.. option:: --debug-vars-unicode=<true|false>
+
+  If true, use unicode characters when displaying source-level variables. If
+  false, use only ASCII characters. Defaults to true.
----------------
ostannard wrote:
> jhenderson wrote:
> > This isn't quite the interface I'd use. I'd actually recommend making ASCII the default, since it will work in all situations, rather than requiring Windows users to specify an extra switch. If there's a good reason not to, I'd change this to --debug-vars-ascii, so that it's the normal practice of "use a switch to enable a behaviour" i.e. `--debug-vars-unicode` is all that is needed (and there's no need to do the slightly weird syntax of `--debug-vars-unicode=false`).
> > 
> > A third option (possibly in conjunction with the third) would be to change the switch name to --debug-vars-style, and allow it to take an enum of ASCII/Unicode (or more specifically UTF-8 probably). However, I don't think it's likely that we'll ever need more than two modes, so this is probably overkill.
> > 
> > My personal preference would be the first of the option, as I am primarily Windows developer, as are all of the developers I work with/make tools for.
> I've tested this on windows 10, and the unicode output displays correctly (in both cmd.exe and powershell). Is there some other version or configuration of windows in which this won't work? If ASCII is only needed for older versions of windows, I'd like to keep the default as unicode, because I find it much more legible than the ASCII version. There's also the option of making ASCII the default for windows and unicode elsewhere, but I'd rather avoid that if possible.
> 
> I do like the idea of switching the spelling to `--debug-vars-ascii` though.
Thanks. Could you remove the "=<true|false>" bit from the description (you don't include it for --debug-vars after all...). Also replace "If true" with "If specified" and ". If false, use" with ", otherwise, use" and delete the "Defaults to false" sentence.


================
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.
+
----------------
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?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h:121
 
+    iterator skip_bytes(uint32_t Add) {
+      return iterator(Expr, Op.EndOffset + Add);
----------------
`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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:343-344
+/// A user-facing string representation of a DWARF expression. This might be an
+/// Address expression, in which case it will get an implicit dereference, or a
+/// Value expression, which does not.
+struct PrintedExpr {
----------------
Would "in which case it will be implicitly dereferenced" make more sense?

Also I'd probably just get rid of the "which does not" bit.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:405
+bool DWARFExpression::prettyPrint(raw_ostream &OS, const MCRegisterInfo *MRI) {
+  return prettyPrintDWARFExpr(OS, begin(), end(), MRI);
+}
----------------
Any particular reason you haven't just inlined the whole function here?


================
Comment at: llvm/test/tools/llvm-objdump/ARM/debug-vars-dwarf4.s:6
+## used by --debug-vars, but do add extra lines or columns to the output, so we
+## test to make sure the live ranges are still diaplayed correctly.
+
----------------
Typo: diaplayed -> displayed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720