[PATCH] D87656: [llvm-dwarfdump] --show-sources option to show all sources

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added a comment.

Apologies for the long delay; this change slipped my mind for a bit.



================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:491-493
+    if (sys::path::is_absolute(Path, sys::path::Style::posix) ||
+        sys::path::is_absolute(Path, sys::path::Style::windows))
+      Sources.push_back(std::move(Path));
----------------
jhenderson wrote:
> I don't believe that there's a test case for the case where an absolute path hasn't been produced?
Removed the check here; this was to avoid mixing relative and absolute paths from the CU and line table. But, now that we either get all names from the line table or just the name from the CU, this is no longer an issue.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:503
+
+  for (const auto &CU : DICtx.compile_units()) {
+    // Extract paths from the line table for this CU. This allows combining the
----------------
jhenderson wrote:
> Don't use `auto` unless the type is obvious from the immediate context (e.g. it's already specified on the line due to a case): https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
This appears to be one of the cases mentioned as an exception: where the underlying type is abstracted away.

I did a quick scan though usages of DICtx.compile_units(); all but two use (const auto &CU).
The two exceptions use (const std::unique_ptr<DwarfUnit> &CU), but given the context, it seems like the correct type should be DWARFUnitVector::UnitVector::const_reference.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:506-507
+    // compilation directory with the line information, in case both the include
+    // directory and file names in the line table are relative lien table are
+    // relative.
+    const auto *LT = DICtx.getLineTableForUnit(CU.get());
----------------
jhenderson wrote:
> This last part of the sentence is garbled.
Fixed; sorry about that.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:515
+    // itself. This information isn't included in the line table in DWARF v4 and
+    // ealier.
+    const char *Name = CU->getUnitDIE().getShortName();
----------------
jhenderson wrote:
> The statement isn't correct though: filenames are included in the DWARF line table v4 and earlier... (although not explicitly the compilation directory).
You're right; this simplifies things quite a bit. I've changed this to only add the name if the linetable is missing; otherwise we can get everything from the linetable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87656