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

Wed Apr 6 00:08:41 PDT 2022

jhenderson added a comment.

I think your test cases need extending with multiple source names per input, and also what happens if you specify multiple input files (I believe llvm-dwarfdump handles that, but I might be mistaken).

This new option still needs adding to the CommandGuide documentation at llvm/docs/CommandGuide/llvm-dwarfdump.rst.

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:1-3
+# RUN: yaml2obj --docnum=1 %s -o - \
+# RUN: | llvm-dwarfdump --show-sources - \
+# RUN: | FileCheck --check-prefix=CU-NAME-CHECK %s
----------------
Up to you, but I have a personal preference for this formatting, as it indicates on each line that there is a continuation involved, starting with a new command.

Also, I personally prefer it if tests create objects on disk rather than passing them via stdin. This is because it's easier to directly inspect the binary if there's a problem with the test. Otherwise, you have to (temporarily) modify the test to force it to dump it.

Same comments apply elsewhere.

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:5
+
+# CU-NAME-CHECK: name.c
+
----------------
You could simplify this an similar patterns by dropping "CHECK" from the prefix name.

This will also match "foobarname.csnfdssfnfjds" which probably isn't the intent. I think as you're testing a new dumping option, you should add the following options to the FileCheck command (also applies below):
```
--match-full-lines
--implicit-check-not={{.}}
```
The former effectively wraps the check pattern with `{{^}}` and `{{$}}`. If there's any whitespace involved, you can also add `--strict-whitespace` although I don't think there is here? The second option ensures that only the checked output is emitted and nothing else at all. This ensures there's no output before or after the checked pattern on different lines.

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:31
+# RUN: | llvm-dwarfdump --show-sources - \
+# RUN: | FileCheck --allow-empty --check-prefix=CU-COMP-DIR-CHECK %s
+
----------------
If you are expecting no output, I'd use `count 0` instead of FileCheck here as it's stricter. Should this case print a warning though?

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:92
+
+# CU-COMP-DIR-ABS-NAME-CHECK: /abs/name.c
+
----------------
This is a good example of something that will pass spuriously without `--match-full-lines`, since `comp/dir/abs/name.c` will be successfully matched by it.

================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:482-483
+  llvm::Optional<uint64_t> LastIndex = LT.getLastValidFileIndex();
+  for (uint64_t I = LT.hasFileAtIndex(0) ? 0 : 1,
+                E = LastIndex ? *LastIndex + 1 : 0;
+       I != E; ++I) {
----------------
I'm not sure I see testing that exercises both sides of each of the two ternaries in this loop.

================
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));
----------------
I don't believe that there's a test case for the case where an absolute path hasn't been produced?

================
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
----------------
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

================
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());
----------------
This last part of the sentence is garbled.

================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:508
+    // relative.
+    const auto *LT = DICtx.getLineTableForUnit(CU.get());
+    StringRef CompDir = CU->getCompilationDir();
----------------
Too much auto.

================
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();
----------------
The statement isn't correct though: filenames are included in the DWARF line table v4 and earlier... (although not explicitly the compilation directory).

================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:528-529
+  // includes line information for non CU sections (e.g., macros), as well as
+  // handling if the line information is present, but CUs aren't (allowed in
+  // DWARF v5).
+  DWARFDataExtractor LineData(DICtx.getDWARFObj(),
----------------
I'm not convinced that the bit in parentheses is correct, based on the earlier conversation in this review. I don't think it's particularly useful information either.

================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:536
+    const auto RecoverableErrorHandler = [&](Error Err) {
+      Result = false;
+      WithColor::defaultErrorHandler(std::move(Err));
----------------
You need a test case to show that this is set by parsing failures in the line table.

================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:552
+
+  for (const auto &Name : Sources)
+    OS << Name << "\n";
----------------
Too much auto and unnecessary "const &" (`StringRef` is designed to be trivially copyable).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87656