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

[Mike Pozulp via Phabricator via llvm-commits]

mmpozulp marked 2 inline comments as done.
mmpozulp added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DIContext.h:32
+static const StringRef kDILineInfoBadString("<invalid>");
+static const StringRef kBadString("??");
+
----------------
jhenderson wrote:
> grimar wrote:
> > Variable names should start from the upper case (LLVM coding style).
> Just to add, the 'k' really isn't a useful part of the name. We don't use that style of variable naming.
> 
> Also, I don't think you need to define them in the header. You just need to add a declaration with `extern`.
Where should I put the definition? 

Also, I just realized that these use static constructors, which the [[ https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors | LLVM Coding Standards ]] prohibit. I could use the Scott Meyers singleton


```
static StringRef &DILineInfoBadString() {
  static StringRef S("<invalid>");
  return S;
}
static StringRef &Addr2LineBadString() {
  static StringRef S("??");
  return S;
}
```
but it forces references to change from `DILineInfoBadString` to `DILineInfoBadString()` and trips this compile warning for some TUs (e.g. lib/DebugInfo/DWARF/DWARFVerifier.cpp)
```
warning: ‘llvm::StringRef& llvm::Addr2LineBadString()’ defined but not used [-Wunused-function]
```
when I build with gcc 9.1.0. The warnings should go away when I move the definition out of the header.




================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-no-debug-info.test:12
 
+# WARN:        warning: failed to parse debug info from file <invalid>
 # CHECK:       0000000000000010 main:
----------------
jhenderson wrote:
> Hmm... Not sure about this one. I think we need a different message in that case, since '<invalid>' isn't actually a file.
I think `failed to parse debug info` is a bit better than `failed to parse debug info from file "<invalid>"`. Here's what the user sees

```
[mike at gannon build]$ ./bin/llvm-objdump --source test/tools/llvm-objdump/X86/Output/source-interleave-no-debug-info.test.tmp2.o

test/tools/llvm-objdump/X86/Output/source-interleave-no-debug-info.test.tmp2.o:	file format ELF64-x86-64


Disassembly of section .text:

0000000000000000 foo:
./bin/llvm-objdump: warning: failed to parse debug info
       0: 55                           	pushq	%rbp
       1: 48 89 e5                     	movq	%rsp, %rbp
       4: 8b 05 00 00 00 00            	movl	(%rip), %eax
       a: 5d                           	popq	%rbp
       b: c3                           	retq
       c: 90                           	nop
       d: 90                           	nop
       e: 90                           	nop
       f: 90                           	nop

0000000000000010 main:
      10: 55                           	pushq	%rbp
      11: 48 89 e5                     	movq	%rsp, %rbp
      14: 53                           	pushq	%rbx
      15: 48 83 ec 18                  	subq	$24, %rsp
      19: c7 45 f4 00 00 00 00         	movl	$0, -12(%rbp)
      20: 48 c7 45 e8 00 00 00 00      	movq	$0, -24(%rbp)
      28: 8b 1d 00 00 00 00            	movl	(%rip), %ebx
      2e: e8 cd ff ff ff               	callq	-51 <foo>
      33: 01 d8                        	addl	%ebx, %eax
      35: 48 83 c4 18                  	addq	$24, %rsp
      39: 5b                           	popq	%rbx
      3a: 5d                           	popq	%rbp
      3b: c3                           	retq

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62462