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

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

ostannard marked 5 inline comments as done.
ostannard 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.

I used ARM simply because that's what I'm familiar with, which I assume is the same reason most tests 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 don't think the size of the source/assembly can be reduced much, because these need quite a few live-ranges to test columns being re-used, and live-range lines being printer to the right of expressions. If you mean removing parts of the debug info which isn't used (e.g. line tables and types), then I'd rather keep the full compiler output, so that these are easier to update in future.



================
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) {
----------------
jhenderson wrote:
> What do you mean by multi-byte here?
UTF-8 characters which are represented by 2, 3 or 4 bytes. This is need for `getColumn` and `PadToColumn` to work correctly with the unicode line-drawing characters.


================
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 | \
----------------
jhenderson wrote:
> 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).
I added a PowerPC variant of this test because it uses the `DW_OP_regx` and `DW_OP_bregx` opcodes.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:578
+
+static void PrettyPrintDWARFOps(raw_ostream &OS, DWARFExpression::iterator I,
+                                const DWARFExpression::iterator E,
----------------
jhenderson wrote:
> 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.
Ok, I'll try to split this out into separate patches.


================
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)
----------------
jhenderson wrote:
> 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?
I've not tried this on windows yet, I'll do that today. I did experiment with some plain ascii versions, but none of them were as clear as the unicode version, so I don't think we should switch to ascii everywhere, even if we do need it for windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720