[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/include/llvm/DebugInfo/DIContext.h:32
+static const StringRef kDILineInfoBadString("<invalid>");
+static const StringRef kBadString("??");
+
----------------
mmpozulp wrote:
> 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.
> 
> 
Perhaps they could be static members of DILineInfo. To avoid the static constructor issue, they could also be `const char * const` variables, since they can't change.


================
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:
----------------
mmpozulp wrote:
> 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
> 
> ```
They'll only see this if standard output and standard error are printed to the same stream. I think you need something indicating the file in most cases, where possible.

Thinking about your previous version (reporting "invalid"), that sounds like something's wrong. Why isn't it able to print the object file name here? How could it be in an invalid state in that context? Sure, the debug info in the object file might be invalid, but that doesn't affect the file name of the object.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-no-debug-info.test:9
 # RUN: llvm-objdump --source %t.o | FileCheck %s --check-prefixes=CHECK,SOURCE
-# RUN: llvm-objdump --source %t2.o | FileCheck %s --implicit-check-not='main()'
+# RUN: llvm-objdump --source %t2.o 2>%t2.e | FileCheck %s --check-prefixes=CHECK --implicit-check-not='main()'
+# RUN: FileCheck %s --input-file %t2.e --check-prefixes=WARN
----------------
Nit: put a space after `>2`, here and elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62462