[PATCH] D62462: [llvm-objdump] Add warning messages if disassembly + source for problematic inputs

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-invalid-source.test:11
 # RUN: llvm-objdump --source %t.o | FileCheck %s --check-prefixes=CHECK,GOOD
-# RUN: llvm-objdump --source %t2.o | FileCheck %s --implicit-check-not="int *b = &a;"
+# RUN: llvm-objdump --source %t2.o 2>&1 | FileCheck %s --check-prefixes=CHECK,WARN --implicit-check-not="int *b = &a;"
 
----------------
I'm concerned that there might be an interleaving issue here, since stderr is normally unbuffered. This could make the test flaky, since the output ordering is important. Perhaps it would be wise to redirect stderr to a file, and check the file contents in a separate FileCheck run (using --input-file)?


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-invalid-source.test:15
 # CHECK-NEXT: ; int main() {
+# WARN:       warning: debug info line number 9999 exceeds the number of lines in
 # GOOD:       ;   int *b = &a;
----------------
I'd add a check for the file name in the message here. This can be done by the following:

```
# RUN: ... | FileCheck %s ... -DFILE=%t2.o
...
# WARN: ... in [[FILE]]
```

where FILE is an arbitrary variable name.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:526
   std::unordered_map<std::string, std::vector<StringRef>> LineCache;
+  // Keep track of missing sources
+  StringSet<> MissingSources;
----------------
These new comments (and the two above) are missing trailing full stops.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:591
 
-  if ((LineInfo.FileName == "<invalid>") || LineInfo.Line == 0 ||
+
+  if (LineInfo.FileName == "<invalid>") {
----------------
Delete this extra blank line.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:592
+
+  if (LineInfo.FileName == "<invalid>") {
+    if(!WarnedNoDebugInfo) {
----------------
I see this was there before, but `"<invalid>"` seems like a magic string. Is there a constant it is stored in to avoid having `"<invalid>"` explicitly mentioned in multiple places?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:594
+    if(!WarnedNoDebugInfo) {
+      warn(Twine("failed to parse debug info which may be missing"));
+      WarnedNoDebugInfo = true;
----------------
Include the file name in this error message somewhere.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62462