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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Is there a particular reason you've used ARM rather than x86 for these tests? A loose convention from my experience is that tests that require a target, but where the target doesn't actually matter, are written for X86.

More generally on the tests, do they need to be as big and complex as they are? Can they be reduced and still demonstrate the required behaviour?

I haven't reviewed big chunks of this patch yet, because it is so big. Anything you can do to divide it up would make the reviews much easier.



================
Comment at: llvm/lib/Support/FormattedStream.cpp:31-32
   for (const char *End = Ptr + Size; Ptr != End; ++Ptr) {
+    // If this is a multi-byte sequence, skip the extra bytes, and don't check
+    // for special whitespace characters.
+    if ((*Ptr & 0b11100000) == 0b11000000) {
----------------
What do you mean by multi-byte here?


================
Comment at: llvm/test/tools/llvm-objdump/ARM/debug-vars-dwarf4-sections.s:1
+// RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj | \
+// RUN:     llvm-objdump - -d -debug-vars -no-show-raw-insn | \
----------------
'#' is more commonly used in most of our (new) llvm binutils tests as the characters for RUN/CHECK lines, especially so in things involving assembly.
Recent convention is then for comments to use '##', to distinguish them from actual lit test lines.

Also, rather than including the source in this test and in the Inputs folder, please just reference the file in the Inputs folder in a comment. Finally, please put a top-level comment at the top of the test explaining what the test is trying to achieve.

Same comments apply to every test.


================
Comment at: llvm/test/tools/llvm-objdump/PowerPC/debug-vars.s:1
+// RUN: llvm-mc -triple powerpc64-unknown-linux < %s -filetype=obj | \
+// RUN:     llvm-objdump - -d -debug-vars -no-show-raw-insn | \
----------------
By my understanding, the behaviour under-test is not really tied to the target. As such, you don't need both PowerPC and ARM variants. One target suffices (also, see my out-of-line comment).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:73
 #include <cstring>
+#include <set>
 #include <system_error>
----------------
Do you need all the new headers? I see no reference to std::set here for a start.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:345
+static cl::opt<bool> DbgVariables("debug-vars", cl::init(false));
+static cl::opt<int> DbgIndent("debug-vars-indent", cl::init(50));
+
----------------
This option isn't documented.

Both options need help text.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:578
+
+static void PrettyPrintDWARFOps(raw_ostream &OS, DWARFExpression::iterator I,
+                                const DWARFExpression::iterator E,
----------------
This function is very long, and I don't feel confident reviewing it. Can some of the functionality be deferred to subsequent patches, e.g. supporting various opcodes etc?

The ideal would be for this patch to add the very basic functionality and add support for more opcodes etc over time.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1064
+        // with box drawing characters joining it to the live range line.
+        OS << (ActiveCols[ColIdx].LiveIn ? "┠─ " : "┌─ ");
+        WithColor(OS, raw_ostream::GREEN)
----------------
As noted on the mailing list, have you checked that this works with typical Windows command prompts (i.e. cmd and PowerShell)? Can plain ASCII serve your purpose just as easily?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1704
 
+
 static void disassembleObject(const Target *TheTarget, const ObjectFile *Obj,
----------------
Get rid of this extra blank line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720