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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a subscriber: Higuoxing.
jhenderson added a comment.
This revision now requires changes to proceed.

Please add the new option to the Command Guide documentation.



================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.s:8
+
+        .text
+        .file   "basic.c"
----------------
You can probably dramatically simplify this code by changing to use yaml2obj. I believe that ELF yaml2obj DWARF support is sufficiently powerful now to achieve this. @Higuoxing may be able to provide more information on this, as he did the work recently there. See also llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml for an example input.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:455
+  for (const auto &CU : DICtx.compile_units()) {
+    const auto *LT = DICtx.getLineTableForUnit(CU.get());
+    for (uint32_t I = 1; I <= LT->Prologue.FileNames.size(); ++I) {
----------------
I think we need testing for multiple CUs. The current test only checks a single one. This might go against the yaml2obj usage suggested above though (@Higuoxing, is there support for multiple tables in .debug_line yet?).


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:456
+    const auto *LT = DICtx.getLineTableForUnit(CU.get());
+    for (uint32_t I = 1; I <= LT->Prologue.FileNames.size(); ++I) {
+      std::string FullPath;
----------------
Not that it likely is going to matter in any practical situation, but this should probably be `uint64_t` technically - the FileNames are set via LEB128 values (see e.g. DW_LNS_set_file) and thus technically have no upper bound in size from the file format. I won't fight too hard for this if you don't want to though.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:467
+  Sources.erase(std::unique(Sources.begin(), Sources.end()), Sources.end());
+  for (const auto &Name : Sources)
+    OS << Name << "\n";
----------------



================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:711
       Success &= handleFile(Object, collectObjectSectionSizes, OutputFile.os());
+  } else if (ShowSources) {
+    for (auto Object : Objects)
----------------
Not related to this patch, or even something you should do yourself. More idle musing - as llvm-dwarfdump starts gaining moreof these options, it feels like it should be able to do multiple at once (e.g. allow `llvm-dwarfdump --show-sources --show-section-sizes`).


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

https://reviews.llvm.org/D87656