[llvm-objdump] Fix the Assertion failure when providing invalid --debug-vars or --dwarf values

[gbreynoo]

Summary

As seen in https://bugs.llvm.org/show_bug.cgi?id=52213 llvm-objdump asserts if either the --debug-vars or the --dwarf options are provided with invalid values. As suggested, this fix adds use of a default value to these options and errors when given bad input.

[jhenderson]

llvm/test/tools/llvm-objdump/X86/debug-vars.yaml

1 ↗

(On Diff #381094)

Copy/paste error.

There's already testing in ARM/debug-vars-dwarf4.yaml for the valid values for --debug-vars, so I'd just simplify by putting the error check in there.

llvm/test/tools/llvm-objdump/X86/eh_frame.yaml

1 ↗

(On Diff #381094)

I'd suggest calling this test dwarf_frames.yaml, since that's the switch under test.

[jhenderson]

llvm/test/tools/llvm-objdump/X86/debug-vars.yaml

1 ↗

(On Diff #381094)

To clarify further: I think the test there is just a generic test, so the fact that it's in ARM doesn't mean we need another test case for X86, since the functionality is generic.

llvm/test/tools/llvm-objdump/X86/eh_frame.yaml

1 ↗

(On Diff #381094)

Actually, having looked at what testing there already is, I'd consider naming this dwarf_invalid.yaml, and putting it outside the X86 directory: this test contains no X86-specific functionality, so it doesn't need to be there.

17 ↗

(On Diff #381094)

I believe you can drop the Machine line, since you're not trying to do anything that needs a target (but you might not be able to).

[gbreynoo]

Updated after James' comments and fixed the format issue.

[jhenderson]

llvm/test/tools/llvm-objdump/ELF/ARM/debug-vars-dwarf4.s

40

I don't think this is going to do what you think it will.

I think you're trying to feed the llvm-mc output into llvm-objdump (the - means read input from stdin), but you aren't piping any llvm-mc output into llvm-objdump.

llvm/test/tools/llvm-objdump/dwarf_invalid.yaml

2

This comment is out-of-date, given the contents and test name.

There is already testing of --dwarf=frames output for llvm-objdump. You only need to check the invalid value case.

[gbreynoo]

Fixed comment and added input to llvm-objdump call as suggested by James.

[gbreynoo]

I have also removed the successful test case from dwarf_invalid.yaml.

llvm/test/tools/llvm-objdump/ELF/ARM/debug-vars-dwarf4.s

40

As the commandline parsing gave the error before the validity of the input was checked I didn't think we needed this line. To avoid confusion I have added the input.

[jhenderson]

LGTM, with nit.

llvm/test/tools/llvm-objdump/ELF/ARM/debug-vars-dwarf4.s

40

We want to ensure that there are no other errors being emitted too, otherwise, we don't know that this error actually prevents continuation of the process, hence we need a valid input.

llvm/test/tools/llvm-objdump/dwarf_invalid.yaml

2

Nit: add missing trailing full stop.

[gbreynoo]

Fixed James' Nit and reduced the multiple identical calls to llvm-mc

[jhenderson]

LGTM again.

[jhenderson]

llvm/test/tools/llvm-objdump/ELF/ARM/debug-vars-dwarf4.s

14

Please indent this and all similar lines in this file. The logic is that by indenting it, you show that this is a continuation of the previous RUN line.

llvm/test/tools/llvm-objdump/ELF/ARM/debug-vars-dwarf4.s

 ## Check that the --debug-vars option works for simple register locations, when
 ## using DWARF4 debug info, with multiple functions in one section. Check that
 ## the live-range lines are rendered correctly when using the --no-show-raw-insn,
 ## --line-numbers and --source options. These do not affect the DWARF parsing
 ## used by --debug-vars, but do add extra lines or columns to the output, so we
 ## test to make sure the live ranges are still displayed correctly.
 
 ## Generated with this compile command, with the source code in Inputs/debug.c:
 ## clang --target=arm--none-eabi -march=armv7-a -c debug.c -O1 -gdwarf-4 -S -o -
 
-# RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj | \
-# RUN:     llvm-objdump - -d --debug-vars | \
-# RUN:     FileCheck %s --check-prefix=RAW --strict-whitespace
+# RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj -o %t.o
+
+# RUN: llvm-objdump %t.o -d --debug-vars | \
+# RUN: FileCheck %s --check-prefix=RAW --strict-whitespace
 
 ## Check that passing the default value for --debug-vars-indent (52) makes no
 ## change to the output.
-# RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj | \
-# RUN:     llvm-objdump - -d --debug-vars --debug-vars-indent=52 | \
-# RUN:     FileCheck %s --check-prefix=RAW --strict-whitespace
+# RUN: llvm-objdump %t.o -d --debug-vars --debug-vars-indent=52 | \
+# RUN: FileCheck %s --check-prefix=RAW --strict-whitespace
 
-# RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj | \
-# RUN:     llvm-objdump - -d --debug-vars --debug-vars-indent=30 | \
-# RUN:     FileCheck %s --check-prefix=INDENT --strict-whitespace
+# RUN: llvm-objdump %t.o -d --debug-vars --debug-vars-indent=30 | \
+# RUN: FileCheck %s --check-prefix=INDENT --strict-whitespace
 
-# RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj | \
-# RUN:     llvm-objdump - -d --debug-vars --no-show-raw-insn | \
-# RUN:     FileCheck %s --check-prefix=NO-RAW --strict-whitespace
+# RUN: llvm-objdump %t.o -d --debug-vars --no-show-raw-insn | \
+# RUN: FileCheck %s --check-prefix=NO-RAW --strict-whitespace
 
-# RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj | \
-# RUN:     llvm-objdump - -d --debug-vars --no-show-raw-insn --line-numbers | \
-# RUN:     FileCheck %s --check-prefix=LINE-NUMS --strict-whitespace
+# RUN: llvm-objdump %t.o -d --debug-vars --no-show-raw-insn --line-numbers | \
+# RUN: FileCheck %s --check-prefix=LINE-NUMS --strict-whitespace
 
 # RUN: mkdir -p %t/a
 # RUN: cp %p/Inputs/debug.c %t/a/debug.c
 # RUN: sed -e "s,SRC_COMPDIR,%/t/a,g" %s > %t.s
 # RUN: llvm-mc -triple armv8a--none-eabi < %t.s -filetype=obj | \
-# RUN:     llvm-objdump - -d --debug-vars --no-show-raw-insn --source | \
-# RUN:     FileCheck %s --check-prefix=SOURCE --strict-whitespace
+# RUN: llvm-objdump - -d --debug-vars --no-show-raw-insn --source | \
+# RUN: FileCheck %s --check-prefix=SOURCE --strict-whitespace
 
 ## An optional argument to the --debug-vars= option can be used to switch
 ## between unicode and ascii output (with unicode being the default).
-# RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj | \
-# RUN:     llvm-objdump - -d --debug-vars=unicode | \
-# RUN:     FileCheck %s --check-prefix=RAW --strict-whitespace
-# RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj | \
-# RUN:     llvm-objdump - -d --debug-vars=ascii | \
-# RUN:     FileCheck %s --check-prefix=ASCII --strict-whitespace
+# RUN: llvm-objdump %t.o -d --debug-vars=unicode | \
+# RUN: FileCheck %s --check-prefix=RAW --strict-whitespace
+# RUN: llvm-objdump %t.o -d --debug-vars=ascii | \
+# RUN: FileCheck %s --check-prefix=ASCII --strict-whitespace
+# RUN: not llvm-objdump %t.o -d --debug-vars=bad_value 2>&1 | \
+# RUN: FileCheck %s --check-prefix=ERROR
 
 ## Note that llvm-objdump emits tab characters in the disassembly, assuming an
 ## 8-byte tab stop, so these might not look aligned in a text editor.
 
 # RAW: 00000000 <foo>:
 # RAW-NEXT:                                                                             ┠─ a = R0 
 # RAW-NEXT:                                                                             ┃ ┠─ b = R1 
 # RAW-NEXT:                                                                             ┃ ┃ ┠─ c = R2 

 # ASCII-NEXT:        4: 02 00 80 e0   add r0, r0, r2                                  ^ | | v   
 # ASCII-NEXT:        8: 1e ff 2f e1   bx  lr                                          v v v     
 # ASCII-EMPTY:
 # ASCII-NEXT: 0000000c <bar>:
 # ASCII-NEXT:                                                                             |- a = R0 
 # ASCII-NEXT:        c: 01 00 80 e2   add r0, r0, #1                                  |         
 # ASCII-NEXT:       10: 1e ff 2f e1   bx  lr                                          v         
 
+# ERROR: error: 'bad_value' is not a valid value for '--debug-vars='
+
   .text
   .syntax unified
   .eabi_attribute 67, "2.09"
   .eabi_attribute 6, 10
   .eabi_attribute 7, 65
   .eabi_attribute 8, 1
   .eabi_attribute 9, 2
   .fpu  neon

llvm/test/tools/llvm-objdump/dwarf_invalid.yaml

+## Test invalid use of the --dwarf option.
+
+# RUN: yaml2obj %s -o %t
+# RUN: not llvm-objdump --dwarf=bad_value %t 2>&1 | FileCheck %s --check-prefix=ERROR
+
+# ERROR: error: 'bad_value' is not a valid value for '--dwarf='
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL

llvm/tools/llvm-objdump/llvm-objdump.h

 class MachOObjectFile;
 class MachOUniversalBinary;
 class RelocationRef;
 struct VersionEntry;
 } // namespace object
 
 namespace objdump {
 
-enum DebugVarsFormat {
-  DVDisabled,
-  DVUnicode,
-  DVASCII,
-};
+enum DebugVarsFormat { DVDisabled, DVUnicode, DVASCII, DVInvalid };
 
 extern bool ArchiveHeaders;
 extern int DbgIndent;
 extern DebugVarsFormat DbgVariables;
 extern bool Demangle;
 extern bool Disassemble;
 extern bool DisassembleAll;
 extern DIDumpType DwarfDumpType;

llvm/tools/llvm-objdump/llvm-objdump.cpp

if (const opt::Arg *A = InputArgs.getLastArg(ID)) {
     if (!llvm::to_integer(V, Value, 0)) {
       reportCmdLineError(A->getSpelling() +
                          ": expected a non-negative integer, but got '" + V +
                          "'");
     }
   }
 }
 
+static void invalidArgValue(const opt::Arg *A) {
+  reportCmdLineError("'" + StringRef(A->getValue()) +
+                     "' is not a valid value for '" + A->getSpelling() + "'");
+}
+
 static std::vector<std::string>
 commaSeparatedValues(const llvm::opt::InputArgList &InputArgs, int ID) {
   std::vector<std::string> Values;
   for (StringRef Value : InputArgs.getAllArgValues(ID)) {
     llvm::SmallVector<StringRef, 2> SplitValues;
     llvm::SplitString(Value, SplitValues, ",");
     for (StringRef SplitValue : SplitValues)
       Values.push_back(SplitValue.str());
static void parseObjdumpOptions(const llvm::opt::InputArgList &InputArgs) {
   Demangle = InputArgs.hasArg(OBJDUMP_demangle);
   Disassemble = InputArgs.hasArg(OBJDUMP_disassemble);
   DisassembleAll = InputArgs.hasArg(OBJDUMP_disassemble_all);
   SymbolDescription = InputArgs.hasArg(OBJDUMP_symbol_description);
   DisassembleSymbols =
       commaSeparatedValues(InputArgs, OBJDUMP_disassemble_symbols_EQ);
   DisassembleZeroes = InputArgs.hasArg(OBJDUMP_disassemble_zeroes);
   if (const opt::Arg *A = InputArgs.getLastArg(OBJDUMP_dwarf_EQ)) {
-    DwarfDumpType =
-        StringSwitch<DIDumpType>(A->getValue()).Case("frames", DIDT_DebugFrame);
+    DwarfDumpType = StringSwitch<DIDumpType>(A->getValue())
+                        .Case("frames", DIDT_DebugFrame)
+                        .Default(DIDT_Null);
+    if (DwarfDumpType == DIDT_Null)
+      invalidArgValue(A);
   }
   DynamicRelocations = InputArgs.hasArg(OBJDUMP_dynamic_reloc);
   FaultMapSection = InputArgs.hasArg(OBJDUMP_fault_map_section);
   FileHeaders = InputArgs.hasArg(OBJDUMP_file_headers);
   SectionContents = InputArgs.hasArg(OBJDUMP_full_contents);
   PrintLines = InputArgs.hasArg(OBJDUMP_line_numbers);
   InputFilenames = InputArgs.getAllArgValues(OBJDUMP_INPUT);
   MachOOpt = InputArgs.hasArg(OBJDUMP_macho);
static void parseObjdumpOptions(const llvm::opt::InputArgList &InputArgs) {
   TripleName = InputArgs.getLastArgValue(OBJDUMP_triple_EQ).str();
   UnwindInfo = InputArgs.hasArg(OBJDUMP_unwind_info);
   Wide = InputArgs.hasArg(OBJDUMP_wide);
   Prefix = InputArgs.getLastArgValue(OBJDUMP_prefix).str();
   parseIntArg(InputArgs, OBJDUMP_prefix_strip, PrefixStrip);
   if (const opt::Arg *A = InputArgs.getLastArg(OBJDUMP_debug_vars_EQ)) {
     DbgVariables = StringSwitch<DebugVarsFormat>(A->getValue())
                        .Case("ascii", DVASCII)
-                       .Case("unicode", DVUnicode);
+                       .Case("unicode", DVUnicode)
+                       .Default(DVInvalid);
+    if (DbgVariables == DVInvalid)
+      invalidArgValue(A);
   }
   parseIntArg(InputArgs, OBJDUMP_debug_vars_indent_EQ, DbgIndent);
 
   parseMachOOptions(InputArgs);
 
   // Parse -M (--disassembler-options) and deprecated
   // --x86-asm-syntax={att,intel}.
   //