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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-dwarfdump.rst:118-121
+.. option:: --show-sources
+
+            Print all source files mentioned in the debug information. Absolute
+            paths are given whenever possible.
----------------
Options should be in alphabetical order, so this needs moving above --statistics.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:7
+# CU-NAME: first.c
+# CU-NAME: second.c
+
----------------
Should this be `CU-NAME-NEXT:`?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:46-50
+# RUN: FileCheck --check-prefix=CU-COMP-DIR \
+# RUN:   --match-full-lines --implicit-check-not={{.}} --input-file %t.comp-dir.err %s
+
+# CU-COMP-DIR: warning: {{.*}}: missing name for compilation unit.
+# CU-COMP-DIR: warning: {{.*}}: missing name for compilation unit.
----------------
1) According to the LLVM style guide, errors and warnings shouldn't have a trailing full stop.
2) I'm assuming that the `{{.*}}` is the file name? If so, you can leverage the FileCheck `-D` option to check it explicitly:
```
# RUN: FileCheck -DFILE=%t.comp-dir.err ...
# CU-COMP-DIR: warning: [[FILE]]: ...
```


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:87-89
+# RUN: llvm-dwarfdump --show-sources %t.comp-dir-rel-name.o > %t.comp-dir-rel-name.dwarfdump
+# RUN: FileCheck --check-prefix=CU-COMP-DIR-REL-NAME \
+# RUN:   --match-full-lines --implicit-check-not={{.}} --input-file %t.comp-dir-rel-name.dwarfdump %s
----------------
For FileCheck commands, it's fine to pipe stdin directly to the command, rather than going via an intermediate file.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:134
+
+# RUN: yaml2obj --docnum=4 %s -o %t.comp-dir-abs-name-posix.o
+# RUN: llvm-dwarfdump --show-sources %t.comp-dir-abs-name-posix.o > %t.comp-dir-abs-name-posix.dwarfdump
----------------
You can leverage yaml2obj's -D option much in the same manner as the FileCheck one above to avoid the need for two (or more) near-identical blocks of YAML, e.g. to provide the file names in posix and windows formats. There may well be other cases within your tests that are similar.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:399-400
+# RUN: yaml2obj --docnum=9 %s -o %t.no-filenames.o
+# RUN: llvm-dwarfdump --show-sources %t.no-filenames.o > %t.no-filenames.dwarfdump
+# RUN: count 0 < %t.no-filenames.dwarfdump
+
----------------
Similar to the above comment about piping, pipe the output directly to `count` here.

In general, the rule I follow is: is the output a binary file or similar? If so, stick it in a file. Otherwise, pipe it.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:420
+# RUN: llvm-mc -triple x86_64-pc-linux %S/Inputs/debug_line_malformed.s -filetype=obj -o %t-malformed.o
+# RUN: not llvm-dwarfdump --show-sources %t-malformed.o
----------------
Check the error message.


================
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
----------------
jhenderson wrote:
> 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.
You missed the space indentation I included in my suggested edit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87656