[llvm-objdump] Allow short options to be grouped

[ormris]

Summary

https://bugs.llvm.org/show_bug.cgi?id=31679

[rupprecht]

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

110 ↗

(On Diff #185795)

This conflicts with -d and -f when grouped args are allowed

200 ↗

(On Diff #185795)

I'm very much interested in this being supported (i.e. I've seen objdump -sj.foo as mentioned in the bug), but according to http://llvm.org/docs/CommandLine.html#grouping, grouped args can't take values.

[ormris]

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

110 ↗

(On Diff #185795)

Is renaming this option possible? It doesn't seem to fit with the naming scheme...

200 ↗

(On Diff #185795)

True. I'll remove this for now.

[rupprecht]

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

110 ↗

(On Diff #185795)

Yep, it should be fine to rename it to "disassemble-functions" or something equally reasonable. You'll have to update test/tools/llvm-objdump/X86/disasm-specific-funcs.test.

[ormris]

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

110 ↗

(On Diff #185795)

Sounds good. I'll add that to the patch.

[ormris]

• Don't allow options that take arguments to be grouped.
• Rename df to disassemble-functions.

[rupprecht]

Thanks!

[ormris]

Thanks for the review! I'll go ahead and commit this.

[ormris]

Reopening to fix bot failures.

[ormris]

• Limit test to options that are more broadly supported

[rupprecht]

I think you can still add tests for the other options, e.g. test/tools/llvm-objdump/X86/macho-disassembly-g-dsym.test tests -d -g, so you could add another run statement to that test with -dg and use cmp to verify it produces the same thing

llvm/test/tools/llvm-objdump/option-grouping.test

1 ↗

(On Diff #186893)

This should also verify it produces the same output as -a -f -x -z

[ormris]

Good point. I'll take a look.

[ormris]

• Add grouped cases to tests that use more than one short switch.
• Compare output with ungrouped variant directly in option grouping test.

[rupprecht]

Just a couple comments, but feel free to commit once those are fixed. Thanks!

llvm/test/tools/llvm-objdump/AArch64/macho-fat-arm-disasm.test

2 ↗

(On Diff #187029)

These should use cmp to verify the files are identical, instead of just checking they match the same patterns

llvm/test/tools/llvm-objdump/option-grouping.test

1 ↗

(On Diff #187029)

Instead of tee, the standard way of doing this is to pass the file via FileCheck --input-file, e.g.
RUN: llvm-objdump -afxz ... > %t0
RUN: llvm-objdump -a -f -x -z ... > %t1
RUN: cmp %t0 %t1
RUN: FileCheck %s --input-file %t0

[ormris]

Sounds good. Thanks again for the review!

llvm/test/tools/llvm-objdump/AArch64/macho-fat-arm-disasm.test

2 ↗

(On Diff #187029)

OK. Will fix.

llvm/test/tools/llvm-objdump/option-grouping.test

1 ↗

(On Diff #187029)

Good to know. Will fix.

[jhenderson]

Before relanding this @ormris, it's probably worth getting @rafauler to confirm he's okay with the option name change, as he was the original one to add it. I've added him as a reviewer. Personally, I think the justification is good, but if it is going to cause horrendous pain, perhaps we should consider alternatives?

[rafauler]

That’s fine by me.

[ormris]

Sounds good. Relanding.

llvm/trunk/test/tools/llvm-objdump/X86/disasm-specific-funcs.test

 // RUN: yaml2obj -o %t.out %p/Inputs/simple-executable-x86_64.yaml
-// RUN: llvm-objdump -d %t.out -df=main | FileCheck %s
+// RUN: llvm-objdump -d %t.out -disassemble-functions=main | FileCheck %s
 
 // CHECK:              Disassembly of section .anothertext:
 // CHECK-NEXT:         main:
 // CHECK-NEXT:         10:       55      pushq   %rbp
 // CHECK-NEXT:         11:       48 89 e5        movq    %rsp, %rbp
 // CHECK-NEXT:         14:       48 83 ec 20     subq    $32, %rsp
 // CHECK-NEXT:         18:       48 8d 04 25 a8 00 00 00         leaq    168, %rax
 // CHECK-NEXT:         20:       c7 45 fc 00 00 00 00    movl    $0, -4(%rbp)

llvm/trunk/test/tools/llvm-objdump/option-grouping.test

+// RUN: llvm-objdump -DRafdghprstux %p/Inputs/private-headers-x86_64.elf 2>&1 | FileCheck %s
+// CHECK-NOT: Unknown command line argument

llvm/trunk/tools/llvm-objdump/MachODump.cpp

 #include <xar/xar.h>
 }
 #endif
 
 using namespace llvm;
 using namespace object;
 
 static cl::opt<bool>
-    UseDbg("g",
+    UseDbg("g", cl::Grouping,
            cl::desc("Print line information from debug info if available"));
 
 static cl::opt<std::string> DSYMFile("dsym",
                                      cl::desc("Use .dSYM file for debug info"));
 
 static cl::opt<bool> FullLeadingAddr("full-leading-addr",
                                      cl::desc("Print full leading address"));
 

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

cl::opt<unsigned long long> AdjustVMA(
     "adjust-vma",
     cl::desc("Increase the displayed address by the specified offset"),
     cl::value_desc("offset"), cl::init(0));
 
 cl::opt<bool>
     llvm::AllHeaders("all-headers",
                      cl::desc("Display all available header information"));
 static cl::alias AllHeadersShort("x", cl::desc("Alias for --all-headers"),
-                                 cl::NotHidden, cl::aliasopt(AllHeaders));
+                                 cl::NotHidden, cl::Grouping,
+                                 cl::aliasopt(AllHeaders));
 
 static cl::list<std::string>
 InputFilenames(cl::Positional, cl::desc("<input object files>"),cl::ZeroOrMore);
 
 cl::opt<bool>
 llvm::Disassemble("disassemble",
   cl::desc("Display assembler mnemonics for the machine instructions"));
 static cl::alias Disassembled("d", cl::desc("Alias for --disassemble"),
-                              cl::NotHidden, cl::aliasopt(Disassemble));
+                              cl::NotHidden, cl::Grouping,
+                              cl::aliasopt(Disassemble));
 
 cl::opt<bool>
 llvm::DisassembleAll("disassemble-all",
   cl::desc("Display assembler mnemonics for the machine instructions"));
 static cl::alias DisassembleAlld("D", cl::desc("Alias for --disassemble-all"),
-                                 cl::NotHidden, cl::aliasopt(DisassembleAll));
+                                 cl::NotHidden, cl::Grouping,
+                                 cl::aliasopt(DisassembleAll));
 
 cl::opt<bool> llvm::Demangle("demangle", cl::desc("Demangle symbols names"),
                              cl::init(false));
 
 static cl::alias DemangleShort("C", cl::desc("Alias for --demangle"),
-                               cl::NotHidden, cl::aliasopt(llvm::Demangle));
+                               cl::NotHidden, cl::Grouping,
+                               cl::aliasopt(llvm::Demangle));
 
 static cl::list<std::string>
-DisassembleFunctions("df",
+DisassembleFunctions("disassemble-functions",
                      cl::CommaSeparated,
                      cl::desc("List of functions to disassemble"));
 static StringSet<> DisasmFuncsSet;
 
 cl::opt<bool>
 llvm::Relocations("reloc",
                   cl::desc("Display the relocation entries in the file"));
 static cl::alias RelocationsShort("r", cl::desc("Alias for --reloc"),
-                                  cl::NotHidden,
+                                  cl::NotHidden, cl::Grouping,
                                   cl::aliasopt(llvm::Relocations));
 
 cl::opt<bool>
 llvm::DynamicRelocations("dynamic-reloc",
   cl::desc("Display the dynamic relocation entries in the file"));
 static cl::alias DynamicRelocationsd("R", cl::desc("Alias for --dynamic-reloc"),
-                                     cl::NotHidden,
+                                     cl::NotHidden, cl::Grouping,
                                      cl::aliasopt(DynamicRelocations));
 
 cl::opt<bool>
     llvm::SectionContents("full-contents",
                           cl::desc("Display the content of each section"));
 static cl::alias SectionContentsShort("s",
                                       cl::desc("Alias for --full-contents"),
-                                      cl::NotHidden,
+                                      cl::NotHidden, cl::Grouping,
                                       cl::aliasopt(SectionContents));
 
 cl::opt<bool> llvm::SymbolTable("syms", cl::desc("Display the symbol table"));
 static cl::alias SymbolTableShort("t", cl::desc("Alias for --syms"),
-                                  cl::NotHidden,
+                                  cl::NotHidden, cl::Grouping,
                                   cl::aliasopt(llvm::SymbolTable));
 
 cl::opt<bool>
 llvm::ExportsTrie("exports-trie", cl::desc("Display mach-o exported symbols"));
 
 cl::opt<bool>
 llvm::Rebase("rebase", cl::desc("Display mach-o rebasing info"));
 
 cl::opt<bool>
 llvm::Bind("bind", cl::desc("Display mach-o binding info"));
 
 cl::opt<bool>
 llvm::LazyBind("lazy-bind", cl::desc("Display mach-o lazy binding info"));
 
 cl::opt<bool>
 llvm::WeakBind("weak-bind", cl::desc("Display mach-o weak binding info"));
 
 cl::opt<bool>
 llvm::RawClangAST("raw-clang-ast",
     cl::desc("Dump the raw binary contents of the clang AST section"));
 
 static cl::opt<bool>
 MachOOpt("macho", cl::desc("Use MachO specific object file parser"));
 static cl::alias MachOm("m", cl::desc("Alias for --macho"), cl::NotHidden,
-                        cl::aliasopt(MachOOpt));
+                        cl::Grouping, cl::aliasopt(MachOOpt));
 
 cl::opt<std::string>
 llvm::TripleName("triple", cl::desc("Target triple to disassemble for, "
                                     "see -version for available targets"));
 
 cl::opt<std::string>
 llvm::MCPU("mcpu",
      cl::desc("Target a specific cpu type (-mcpu=help for details)"),
      cl::value_desc("cpu-name"),
      cl::init(""));
 
 cl::opt<std::string>
 llvm::ArchName("arch-name", cl::desc("Target arch to disassemble for, "
                                 "see -version for available targets"));
 
 cl::opt<bool>
 llvm::SectionHeaders("section-headers", cl::desc("Display summaries of the "
                                                  "headers for each section."));
 static cl::alias SectionHeadersShort("headers",
                                      cl::desc("Alias for --section-headers"),
                                      cl::NotHidden,
                                      cl::aliasopt(SectionHeaders));
 static cl::alias SectionHeadersShorter("h",
                                        cl::desc("Alias for --section-headers"),
-                                       cl::NotHidden,
+                                       cl::NotHidden, cl::Grouping,
                                        cl::aliasopt(SectionHeaders));
 
 static cl::opt<bool>
     ShowLMA("show-lma",
             cl::desc("Display LMA column when dumping ELF section headers"));
 
 cl::list<std::string>
 llvm::FilterSections("section", cl::desc("Operate on the specified sections only. "
llvm::NoShowRawInsn("no-show-raw-insn", cl::desc("When disassembling "
                                                  "the instruction bytes."));
 cl::opt<bool>
 llvm::NoLeadingAddr("no-leading-addr", cl::desc("Print no leading address"));
 
 cl::opt<bool>
 llvm::UnwindInfo("unwind-info", cl::desc("Display unwind information"));
 
 static cl::alias UnwindInfoShort("u", cl::desc("Alias for --unwind-info"),
-                                 cl::NotHidden, cl::aliasopt(UnwindInfo));
+                                 cl::NotHidden, cl::Grouping,
+                                 cl::aliasopt(UnwindInfo));
 
 cl::opt<bool>
 llvm::PrivateHeaders("private-headers",
                      cl::desc("Display format specific file headers"));
 
 cl::opt<bool>
 llvm::FirstPrivateHeader("private-header",
                          cl::desc("Display only the first format specific file "
                                   "header"));
 
 static cl::alias PrivateHeadersShort("p",
                                      cl::desc("Alias for --private-headers"),
-                                     cl::NotHidden,
+                                     cl::NotHidden, cl::Grouping,
                                      cl::aliasopt(PrivateHeaders));
 
 cl::opt<bool> llvm::FileHeaders(
     "file-headers",
     cl::desc("Display the contents of the overall file header"));
 
 static cl::alias FileHeadersShort("f", cl::desc("Alias for --file-headers"),
-                                  cl::NotHidden, cl::aliasopt(FileHeaders));
+                                  cl::NotHidden, cl::Grouping,
+                                  cl::aliasopt(FileHeaders));
 
 cl::opt<bool>
     llvm::ArchiveHeaders("archive-headers",
                          cl::desc("Display archive header information"));
 
 cl::alias ArchiveHeadersShort("a", cl::desc("Alias for --archive-headers"),
-                              cl::NotHidden, cl::aliasopt(ArchiveHeaders));
+                              cl::NotHidden, cl::Grouping,
+                              cl::aliasopt(ArchiveHeaders));
 
 cl::opt<bool>
     llvm::PrintImmHex("print-imm-hex",
                       cl::desc("Use hex format for immediate values"));
 
 cl::opt<bool> PrintFaultMaps("fault-map-section",
                              cl::desc("Display contents of faultmap section"));
 
 cl::opt<DIDumpType> llvm::DwarfDumpType(
     "dwarf", cl::init(DIDT_Null), cl::desc("Dump of dwarf debug sections:"),
     cl::values(clEnumValN(DIDT_DebugFrame, "frames", ".debug_frame")));
 
 cl::opt<bool> PrintSource(
     "source",
     cl::desc(
         "Display source inlined with disassembly. Implies disassemble object"));
 
 cl::alias PrintSourceShort("S", cl::desc("Alias for -source"), cl::NotHidden,
-                           cl::aliasopt(PrintSource));
+                           cl::Grouping, cl::aliasopt(PrintSource));
 
 cl::opt<bool> PrintLines("line-numbers",
                          cl::desc("Display source line numbers with "
                                   "disassembly. Implies disassemble object"));
 
 cl::alias PrintLinesShort("l", cl::desc("Alias for -line-numbers"),
-                          cl::NotHidden, cl::aliasopt(PrintLines));
+                          cl::NotHidden, cl::Grouping,
+                          cl::aliasopt(PrintLines));
 
 cl::opt<unsigned long long>
     StartAddress("start-address", cl::desc("Disassemble beginning at address"),
                  cl::value_desc("address"), cl::init(0));
 cl::opt<unsigned long long>
     StopAddress("stop-address",
                 cl::desc("Stop disassembly at address"),
                 cl::value_desc("address"), cl::init(UINT64_MAX));
 
 cl::opt<bool> DisassembleZeroes(
                 "disassemble-zeroes",
                 cl::desc("Do not skip blocks of zeroes when disassembling"));
 cl::alias DisassembleZeroesShort("z",
                                  cl::desc("Alias for --disassemble-zeroes"),
-                                 cl::NotHidden,
+                                 cl::NotHidden, cl::Grouping,
                                  cl::aliasopt(DisassembleZeroes));
 
 static StringRef ToolName;
 
 typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t>> SectionSymbolsTy;
 
 SectionFilter llvm::ToolSectionFilter(llvm::object::ObjectFile const &O) {
   return SectionFilter(