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

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

ostannard marked 30 inline comments as done.
ostannard added a comment.

Sorry about that, I'd been marking comments as "done" as I went, but forgot to actually click "submit". I also accidentally included D70756 <https://reviews.llvm.org/D70756> in the patch.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1106
+    } else {
+      consumeError(Loc.takeError());
+    }
----------------
jhenderson wrote:
> A comment explaining why we just throw away the error here woul be good.
This change is from D70756, included in this patch by mistake.


================
Comment at: llvm/lib/Support/FormattedStream.cpp:40
+    else
+      MultiByte = false;
+
----------------
aprantl wrote:
> Is a FormattedStream always UTF-8 or does this need to be guarded somehow?
I think we always use UTF-8 (or 7-bit ASCII) internally, so no need to guard this.


================
Comment at: llvm/test/tools/llvm-objdump/ARM/debug-vars-dwarf4-sections.s:57
+.Lfunc_begin0:
+	.file	1 "/work" "llvm/src/llvm/test/tools/llvm-objdump/ARM/Inputs/debug.c"
+	.loc	1 1 0
----------------
jhenderson wrote:
> Is this string important to the test? If it is, I don't think it'll work on others' machines.
> 
> You may need to use sed to replace the input string at test time. There are examples in the source-interleave tests in the X86 folder of llvm-objdump.
I'm using sed to replace this string in debug-vars-dwarf4.s, where I'm testing the interaction with source and line number interleaving. I didn't think it would be worth testing that in every file, because the interactions are all in the formatting of the output, which is the same in each file.


================
Comment at: llvm/test/tools/llvm-symbolizer/frame-loclistx.s:1
+// Test dwarf-5 DW_AT_location [DW_FORM_loclistx].
+// REQUIRES: aarch64-registered-target
----------------
jhenderson wrote:
> This test seems unrelated?
Another part of D70756 I included in the patch by mistake.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:686-688
+        // FIXME: getLocations seems to get the section index wrong for
+        // objects built with -ffunction-sections, for now we just fix it up
+        // here.
----------------
jhenderson wrote:
> File a bug for this FIXME against the getLocations method.
This appears to have been fixed since I wrote this, so I've removed the workaround, and no ticket is needed.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:729-730
+  unsigned MoveToFirstVarColumn(formatted_raw_ostream &OS) {
+    unsigned FirstUnprintedColumn =
+        std::max((int)(OS.getColumn() - getIndentLevel() + 1) / 2, 0);
+    if ((getIndentLevel() + FirstUnprintedColumn * 2) > OS.getColumn())
----------------
jhenderson wrote:
> What's the `/ 2` and `* 2` below about?
I'll add a comment to explain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720