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

[Eric Christopher via Phabricator via llvm-commits]

echristo added a comment.

Some nits and style/comment/readability requests. Generally pretty happy with how it looks though. And thanks James for the numerous rounds of reviews already! :)



================
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.
----------------
jhenderson wrote:
> 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.
FWIW I'd have probably gone with --debug-vars=<encoding> ?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h:144
+  /// printed.
+  bool prettyPrint(raw_ostream &OS, const MCRegisterInfo *RegInfo);
+
----------------
printCompact maybe?


================
Comment at: llvm/include/llvm/Support/FormattedStream.h:110
+  unsigned getColumn() {
+    ComputePosition(getBufferStart(), GetNumBytesInBuffer());
+    return Position.first;
----------------
Comment what ComputePosition is doing here?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:356
+
+static bool prettyPrintDWARFExpr(raw_ostream &OS, DWARFExpression::iterator I,
+                                 const DWARFExpression::iterator E,
----------------
If you wouldn't mind commenting each of the cases with what's going on in them it'd be appreciated.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:596-598
+    bool LiveIn;
+    bool LiveOut;
+    bool MustDrawLabel;
----------------
Since this is going to get constructed with subsequent boolean arguments can we move them to enums/something? int, true, false, true isn't real obvious what it's constructing :)


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:713
+
+  /// Update to match the state of the instruction between ThisAddr and
+  /// NextAddr. In the common case, any live range active at ThisAddr is
----------------
Not a huge fan of boolean arguments. Is there a different factoring here that'll help?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720