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

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

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

> does it work with an ET_DYN/ET_DYN?

Not currently, I'll investigate and fix that as a separate patch.



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:562
+
+/// Stores a single expression representing the location of a source-level
+/// variable, along with the PC range for which that expression is valid.
----------------
MaskRay wrote:
> `// `
> 
> Internal classes should not use Doxygen comments.
@aprantl previously asked for these to be changed to doxygen. I don't see anything explicit about this in the coding standard, but apparently IDEs can make use of these.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:596-598
+    bool LiveIn;
+    bool LiveOut;
+    bool MustDrawLabel;
----------------
echristo wrote:
> 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 :)
This is only ever constructed with these all set to false, so  I can remove them from the constructor, and use the member names explicitly when modifying them.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:658
+  unsigned getIndentLevel() const {
+    return DbgIndent + (NoShowRawInsn ? 16 : 40);
+  }
----------------
MaskRay wrote:
> Ideally `unsigned TabStop = NoShowRawInsn ? 16 : 40` in `printInst` should be extracted as a function.
> 
> Is `--no-leading-addr` taken into account?
`--no-leading-addr` isn't taken into account, because it doesn't affect the column the instruction starts at.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:705
+
+  void AddCompileUnit(DWARFDie D) {
+    if (D.getTag() == dwarf::DW_TAG_subprogram)
----------------
MaskRay wrote:
> `addCompileUnit`
> 
> Looks like this function can be merged with `addFunction()`.
I don't see how, given that it calls `addFunction` in two places.


================
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
----------------
MaskRay wrote:
> echristo wrote:
> > Not a huge fan of boolean arguments. Is there a different factoring here that'll help?
> No need to use `/// `. These classes are internal.
It's not easy to factor out, because `CheckedVarIdxs` is defined in the first half then used in the second. However, I can rename it to better describe what it does.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1762
                              !DisassembleAll;
+      formatted_raw_ostream FOS(outs());
       while (Index < End) {
----------------
MaskRay wrote:
> Does this harm performance when --debug-vars is not used?
> 
> I once caused a regression (fixed by D64969).
If I've understood that discussion properly, the performance problem was caused by creating a formatted_raw_ostream for each instruction, because it flushes the underlying stream in the constructor. Here, i'm creating one per disassembled function, so the overhead will be much lower.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720