[llvm-objcopy] Accept --long-option but not -long-option

[MaskRay]

Summary

llvm-{objcopy,strip} (and many other LLVM binary utilities) accept
cl::opt style -long-option as well as many short options (e.g. -p -S
-x). People who use them as replacement of GNU binutils often use the
grouped option syntax (POSIX Utility Conventions), e.g. -Sx => -S -x,
-Wd => -W -d, -sj.text => -s -j.text

There is ambiguity if a long option starts with the character used by a
short option. Drop the support for -long-option to resolve the ambiguity.

This divergence from other utilities is accepted (other utilities
continue supporting -long-option).
https://lists.llvm.org/pipermail/llvm-dev/2019-April/131786.html

[jhenderson]

The behaviour change looks good, in the sense that it does what you set out to do. I don't have any real objections to it aside from the fact that it is different from how LLVM tools typically work, but I personally also find it odd the LLVM tools work with a single-dash! I'd like others to comment on this though before giving the thumbs up, particularly @jakehehrlich as the code owner.

[emaste]

LGTM. I much prefer only - short and -- long options, except for lld where unfortunately there's a precedent of accepting both.

[MaskRay]

except for lld where unfortunately there's a precedent of accepting both.

ld.lld -d -ont -b lame -lld -t -hats -v -ery -understandablec -ompatibilit -yissues

[MaskRay]

Raised a discussion on the mailing list http://lists.llvm.org/pipermail/llvm-dev/2019-April/131786.html

[jakehehrlich]

I feel like we didn't get a solid conclusion here but that the general consensus is that this change is more ideal. I think the argument that this is incompatible with merged short options and that that might have compatibility issues is a strong argument personally. The other not-quite-conclusion-but-leaning-that-way seems to be that we can't really make this same choice for all binaries. Do we feel like the conclusion of the llvm-dev post supports yet further fragmenting the tools? I don't know and I'm concerned about making that choice. We have to way tool consistency with GNU compatibility here and that's a hard choice. I'll add an email later to see if we can flesh this out more.

[MaskRay]

In D60439#1470580, @jakehehrlich wrote:

I feel like we didn't get a solid conclusion here but that the general consensus is that this change is more ideal. I think the argument that this is incompatible with merged short options and that that might have compatibility issues is a strong argument personally. The other not-quite-conclusion-but-leaning-that-way seems to be that we can't really make this same choice for all binaries. Do we feel like the conclusion of the llvm-dev post supports yet further fragmenting the tools? I don't know and I'm concerned about making that choice. We have to way tool consistency with GNU compatibility here and that's a hard choice. I'll add an email later to see if we can flesh this out more.

In the thread https://lists.llvm.org/pipermail/llvm-dev/2019-April/131786.html some people expressed that the single-dash -long-option seems unusual. Note, cl::Grouping is not used in utilities other than these binary utilities. Some people said the policy can be set per-command basis.

Can we fix llvm-objcopy now?

[jakehehrlich]

Eh I guess this is fine. I'm kind of 50/50 on this myself. I don't think you'll break any major team or code base I know about but I could be wrong. I don't really like fragmenting the standard used across these tools but grouped options increases compatibility so I think on balance I support this.

[jakehehrlich]

It would be helpful to convert all of the listed tools over as well for consistency

[MaskRay]

In D60439#1479473, @jakehehrlich wrote:

It would be helpful to convert all of the listed tools over as well for consistency

Thank you! I'll find some easier ones to convert. llvm-objcopy is the starter as it is the youngest :)

[MaskRay]

Rebase. Update description. Handle --allow-broken-links

test/tools/llvm-objcopy/ELF/help-message.test

-# RUN: llvm-objcopy -help | FileCheck --check-prefix=OBJCOPY-USAGE %s
+# RUN: llvm-objcopy -h | FileCheck --check-prefix=OBJCOPY-USAGE %s
 # RUN: llvm-objcopy --help | FileCheck --check-prefix=OBJCOPY-USAGE %s
 # RUN: not llvm-objcopy 2>&1 | FileCheck --check-prefix=OBJCOPY-USAGE %s
 # RUN: not llvm-objcopy -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-objcopy --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-objcopy --strip-debug 2>&1 | FileCheck %s --check-prefix=NO-INPUT-FILES
 
-# RUN: llvm-strip -help | FileCheck --check-prefix=STRIP-USAGE %s
+# RUN: llvm-strip -h | FileCheck --check-prefix=STRIP-USAGE %s
 # RUN: llvm-strip --help | FileCheck --check-prefix=STRIP-USAGE %s
 # RUN: not llvm-strip 2>&1 | FileCheck --check-prefix=STRIP-USAGE %s
 # RUN: not llvm-strip -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-strip --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-strip --strip-debug 2>&1 | FileCheck %s --check-prefix=NO-INPUT-FILES
 
 
 # OBJCOPY-USAGE:  USAGE: llvm-objcopy
 # STRIP-USAGE:    USAGE: llvm-strip
 # UNKNOWN-ARG:    unknown argument '{{-+}}abcabc'
 # NO-INPUT-FILES: No input file specified

test/tools/llvm-objcopy/ELF/objcopy-version.test

-# RUN: llvm-objcopy -version | FileCheck %s
 # RUN: llvm-objcopy --version | FileCheck %s
 # RUN: llvm-objcopy -V | FileCheck %s
 
 # CHECK: {{ version }}

test/tools/llvm-objcopy/ELF/strip-all.test

 
 # RUN: rm -f %t.a
 # RUN: llvm-ar crs %t.a %t
 # RUN: llvm-objcopy --strip-all %t.a %t.a
 # RUN: llvm-ar p %t.a > %t6
 # RUN: cmp %t2 %t6
 
 # RUN: cp %t %t7
-# RUN: llvm-strip -strip-all %t7
+# RUN: llvm-strip --strip-all %t7
 # RUN: cmp %t2 %t7
 
 # RUN: cp %t %t8
-# RUN: llvm-strip --strip-all %t8
+# RUN: llvm-objcopy -S %t8 %t8
 # RUN: cmp %t2 %t8
 
 # RUN: cp %t %t9
-# RUN: llvm-objcopy -S %t9 %t9
+# RUN: llvm-strip -s %t9
 # RUN: cmp %t2 %t9
 
-# RUN: cp %t %t10
-# RUN: llvm-strip -s %t10
-# RUN: cmp %t2 %t10
-
 # Verify that a non-existent symbol table (after first call to llvm-strip)
 # can be handled correctly.
-# RUN: cp %t %t11
-# RUN: llvm-strip --strip-all --keep-symbol=unavailable_symbol %t11
-# RUN: llvm-strip --strip-all --keep-symbol=unavailable_symbol %t11
-# RUN: cmp %t2 %t11
+# RUN: cp %t %t10
+# RUN: llvm-strip --strip-all --keep-symbol=unavailable_symbol %t10
+# RUN: llvm-strip --strip-all --keep-symbol=unavailable_symbol %t10
+# RUN: cmp %t2 %t10
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:

test/tools/llvm-objcopy/ELF/strip-version.test

-# RUN: llvm-strip -version | FileCheck %s
 # RUN: llvm-strip --version | FileCheck %s
 # RUN: llvm-strip -V | FileCheck %s
 
 # CHECK-DAG: {{ version }}
 # CHECK-DAG: GNU strip

tools/llvm-objcopy/ObjcopyOpts.td

 include "llvm/Option/OptParser.td"
 
 multiclass Eq<string name, string help> {
-  def NAME : Separate<["--", "-"], name>;
-  def NAME #_eq : Joined<["--", "-"], name #"=">,
+  def NAME : Separate<["--"], name>;
+  def NAME #_eq : Joined<["--"], name #"=">,
                   Alias<!cast<Separate>(NAME)>,
                   HelpText<help>;
 }
 
-def help : Flag<["-", "--"], "help">;
+def help : Flag<["--"], "help">;
+def h : Flag<["-"], "h">, Alias<help>;
 
 defm binary_architecture
     : Eq<"binary-architecture", "Used when transforming an architecture-less "
                                 "format (such as binary) to another format">;
 def B : JoinedOrSeparate<["-"], "B">, Alias<binary_architecture>;
 
 defm target : Eq<"target", "Format of the input and output file">,
               Values<"binary">;
 def F : JoinedOrSeparate<["-"], "F">, Alias<target>;
 
 defm input_target : Eq<"input-target", "Format of the input file">,
                     Values<"binary">;
 def I : JoinedOrSeparate<["-"], "I">, Alias<input_target>;
 
 defm output_target : Eq<"output-target", "Format of the output file">,
                      Values<"binary">;
 def O : JoinedOrSeparate<["-"], "O">, Alias<output_target>;
 
-def compress_debug_sections : Flag<["--", "-"], "compress-debug-sections">;
+def compress_debug_sections : Flag<["--"], "compress-debug-sections">;
 def compress_debug_sections_eq
-    : Joined<["--", "-"], "compress-debug-sections=">,
+    : Joined<["--"], "compress-debug-sections=">,
       MetaVarName<"[ zlib | zlib-gnu ]">,
       HelpText<"Compress DWARF debug sections using specified style. Supported "
                "styles: 'zlib-gnu' and 'zlib'">;
-def decompress_debug_sections : Flag<["-", "--"], "decompress-debug-sections">,
+def decompress_debug_sections : Flag<["--"], "decompress-debug-sections">,
                                 HelpText<"Decompress DWARF debug sections.">;
 defm split_dwo
     : Eq<"split-dwo", "Equivalent to extract-dwo on the input file to "
                       "<dwo-file>, then strip-dwo on the input file">,
       MetaVarName<"dwo-file">;
 
 def enable_deterministic_archives
-    : Flag<["-", "--"], "enable-deterministic-archives">,
+    : Flag<["--"], "enable-deterministic-archives">,
       HelpText<"Enable deterministic mode when copying archives (use zero for "
                "UIDs, GIDs, and timestamps).">;
 def D : Flag<["-"], "D">,
         Alias<enable_deterministic_archives>,
         HelpText<"Alias for --enable-deterministic-archives">;
 
 def disable_deterministic_archives
-    : Flag<["-", "--"], "disable-deterministic-archives">,
+    : Flag<["--"], "disable-deterministic-archives">,
       HelpText<"Disable deterministic mode when copying archives (use real "
                "values for UIDs, GIDs, and timestamps).">;
 def U : Flag<["-"], "U">,
         Alias<disable_deterministic_archives>,
         HelpText<"Alias for --disable-deterministic-archives">;
 
-def preserve_dates : Flag<["-", "--"], "preserve-dates">,
+def preserve_dates : Flag<["--"], "preserve-dates">,
                      HelpText<"Preserve access and modification timestamps">;
 def p : Flag<["-"], "p">, Alias<preserve_dates>;
 
 defm add_gnu_debuglink
     : Eq<"add-gnu-debuglink", "Add a .gnu_debuglink for <debug-file>">,
       MetaVarName<"debug-file">;
 
 defm remove_section : Eq<"remove-section", "Remove <section>">,

 
 defm set_section_flags
     : Eq<"set-section-flags",
          "Set section flags for a given section. Flags supported for GNU "
          "compatibility: alloc, load, noload, readonly, debug, code, data, "
          "rom, share, contents, merge, strings.">,
       MetaVarName<"section=flag1[,flag2,...]">;
 
-def strip_all : Flag<["-", "--"], "strip-all">,
+def strip_all : Flag<["--"], "strip-all">,
                 HelpText<"Remove non-allocated sections outside segments. "
                          ".gnu.warning* sections are not removed">;
 def S : Flag<["-"], "S">, Alias<strip_all>;
-def strip_all_gnu : Flag<["-", "--"], "strip-all-gnu">,
+def strip_all_gnu : Flag<["--"], "strip-all-gnu">,
                     HelpText<"Compatible with GNU objcopy's --strip-all">;
-def strip_debug : Flag<["-", "--"], "strip-debug">,
+def strip_debug : Flag<["--"], "strip-debug">,
                   HelpText<"Remove all debug information">;
 def g : Flag<["-"], "g">, Alias<strip_debug>,
         HelpText<"Alias for --strip-debug">;
-def strip_dwo : Flag<["-", "--"], "strip-dwo">,
+def strip_dwo : Flag<["--"], "strip-dwo">,
                 HelpText<"Remove all DWARF .dwo sections from file">;
 def strip_sections
-    : Flag<["-", "--"], "strip-sections">,
+    : Flag<["--"], "strip-sections">,
       HelpText<"Remove all section headers and all sections not in segments">;
 def strip_non_alloc
-    : Flag<["-", "--"], "strip-non-alloc">,
+    : Flag<["--"], "strip-non-alloc">,
       HelpText<"Remove all non-allocated sections outside segments">;
-def strip_unneeded : Flag<["-", "--"], "strip-unneeded">,
+def strip_unneeded : Flag<["--"], "strip-unneeded">,
                      HelpText<"Remove all symbols not needed by relocations">;
 defm strip_unneeded_symbol
     : Eq<"strip-unneeded-symbol",
          "Remove symbol <symbol> if it is not needed by relocations">,
       MetaVarName<"symbol">;
 defm strip_unneeded_symbols
     : Eq<"strip-unneeded-symbols",
          "Reads a list of symbols from <filename> and removes them "
          "if they are not needed by relocations">,
       MetaVarName<"filename">;
 
 def extract_dwo
-    : Flag<["-", "--"], "extract-dwo">,
+    : Flag<["--"], "extract-dwo">,
       HelpText<
           "Remove all sections that are not DWARF .dwo sections from file">;
 
 def localize_hidden
-    : Flag<["-", "--"], "localize-hidden">,
+    : Flag<["--"], "localize-hidden">,
       HelpText<
           "Mark all symbols that have hidden or internal visibility as local">;
 defm localize_symbol : Eq<"localize-symbol", "Mark <symbol> as local">,
                        MetaVarName<"symbol">;
 defm localize_symbols
     : Eq<"localize-symbols",
          "Reads a list of symbols from <filename> and marks them local.">,
       MetaVarName<"filename">;

 defm weaken_symbol : Eq<"weaken-symbol", "Mark <symbol> as weak">,
                      MetaVarName<"symbol">;
 defm weaken_symbols
     : Eq<"weaken-symbols",
          "Reads a list of symbols from <filename> and marks them weak.">,
       MetaVarName<"filename">;
 
 def W : JoinedOrSeparate<["-"], "W">, Alias<weaken_symbol>;
-def weaken : Flag<["-", "--"], "weaken">,
+def weaken : Flag<["--"], "weaken">,
              HelpText<"Mark all global symbols as weak">;
 
-def discard_locals : Flag<["-", "--"], "discard-locals">,
+def discard_locals : Flag<["--"], "discard-locals">,
                      HelpText<"Remove compiler-generated local symbols, (e.g. "
                               "symbols starting with .L)">;
 def X : Flag<["-"], "X">, Alias<discard_locals>;
 
 def discard_all
-    : Flag<["-", "--"], "discard-all">,
+    : Flag<["--"], "discard-all">,
       HelpText<"Remove all local symbols except file and section symbols">;
 def x : Flag<["-"], "x">, Alias<discard_all>;
 defm strip_symbol : Eq<"strip-symbol", "Remove symbol <symbol>">,
                     MetaVarName<"symbol">;
 defm strip_symbols
     : Eq<"strip-symbols",
          "Reads a list of symbols from <filename> and removes them.">,
       MetaVarName<"filename">;
 
 def N : JoinedOrSeparate<["-"], "N">, Alias<strip_symbol>;
 defm keep_symbol : Eq<"keep-symbol", "Do not remove symbol <symbol>">,
                    MetaVarName<"symbol">;
 def K : JoinedOrSeparate<["-"], "K">, Alias<keep_symbol>;
 
 defm keep_symbols
     : Eq<"keep-symbols",
          "Reads a list of symbols from <filename> and runs as if "
          "--keep-symbol=<symbol> is set for each one. <filename> "
          "contains one symbol per line and may contain comments beginning with "
          "'#'. Leading and trailing whitespace is stripped from each line. May "
          "be repeated to read symbols from many files.">,
       MetaVarName<"filename">;
 
 def only_keep_debug
-    : Flag<["-", "--"], "only-keep-debug">,
+    : Flag<["--"], "only-keep-debug">,
       HelpText<"Clear sections that would not be stripped by --strip-debug. "
                "Currently only implemented for COFF.">;
 
-def keep_file_symbols : Flag<["-", "--"], "keep-file-symbols">,
+def keep_file_symbols : Flag<["--"], "keep-file-symbols">,
                         HelpText<"Do not remove file symbols">;
 defm dump_section
     : Eq<"dump-section",
          "Dump contents of section named <section> into file <file>">,
       MetaVarName<"section=file">;
 defm prefix_symbols
     : Eq<"prefix-symbols", "Add <prefix> to the start of every symbol name">,
       MetaVarName<"prefix">;
 
-def version : Flag<["-", "--"], "version">,
+def version : Flag<["--"], "version">,
               HelpText<"Print the version and exit.">;
 def V : Flag<["-"], "V">, Alias<version>;
 defm build_id_link_dir
     : Eq<"build-id-link-dir", "Set directory for --build-id-link-input and "
                               "--build-id-link-output to <dir>">,
       MetaVarName<"dir">;
 defm build_id_link_input
     : Eq<"build-id-link-input", "Hard-link the input to <dir>/xx/xxx<suffix> "
                                 "name derived from hex build ID">,
       MetaVarName<"suffix">;
 defm build_id_link_output
     : Eq<"build-id-link-output", "Hard-link the output to <dir>/xx/xxx<suffix> "
                                  "name derived from hex build ID">,
       MetaVarName<"suffix">;
 
 def regex
-    : Flag<["-", "--"], "regex">,
+    : Flag<["--"], "regex">,
       HelpText<"Permit regular expressions in name comparison">;
 
 defm set_start : Eq<"set-start", "Set the start address to <addr>. Overrides "
                     "any previous --change-start or --adjust-start values.">,
                  MetaVarName<"addr">;
 defm change_start : Eq<"change-start", "Add <incr> to the start address. Can be "                        
                        "specified multiple times, all values will be applied "
                        "cumulatively.">,
                     MetaVarName<"incr">;
-def adjust_start : JoinedOrSeparate<["-", "--"], "adjust-start">,
+def adjust_start : JoinedOrSeparate<["--"], "adjust-start">,
                    Alias<change_start>;
 
 defm add_symbol
     : Eq<"add-symbol", "Add new symbol <name> to .symtab. Accepted flags: "
          "global, local, weak, default, hidden, file, section, object, "
          "function, indirect-function. Accepted but ignored for "
          "compatibility: debug, constructor, warning, indirect, synthetic, "
          "unique-object, before.">,
       MetaVarName<"name=[section:]value[,flags]">;

tools/llvm-objcopy/StripOpts.td

 include "llvm/Option/OptParser.td"
 
 multiclass Eq<string name, string help> {
-  def NAME : Separate<["--", "-"], name>;
-  def NAME #_eq : Joined<["--", "-"], name #"=">,
+  def NAME : Separate<["--"], name>;
+  def NAME #_eq : Joined<["--"], name #"=">,
                   Alias<!cast<Separate>(NAME)>,
                   HelpText<help>;
 }
 
-def help : Flag<["-", "--"], "help">;
+def help : Flag<["--"], "help">;
+def h : Flag<["-"], "h">, Alias<help>;
 
 def enable_deterministic_archives
-    : Flag<["-", "--"], "enable-deterministic-archives">,
+    : Flag<["--"], "enable-deterministic-archives">,
       HelpText<"Enable deterministic mode when stripping archives (use zero "
                "for UIDs, GIDs, and timestamps).">;
 def D : Flag<["-"], "D">,
         Alias<enable_deterministic_archives>,
         HelpText<"Alias for --enable-deterministic-archives">;
 
 def disable_deterministic_archives
-    : Flag<["-", "--"], "disable-deterministic-archives">,
+    : Flag<["--"], "disable-deterministic-archives">,
       HelpText<"Disable deterministic mode when stripping archives (use real "
                "values for UIDs, GIDs, and timestamps).">;
 def U : Flag<["-"], "U">,
         Alias<disable_deterministic_archives>,
         HelpText<"Alias for --disable-deterministic-archives">;
 
-defm output : Eq<"o", "Write output to <file>">, MetaVarName<"output">;
+def output : JoinedOrSeparate<["-"], "o">, HelpText<"Write output to <file>">;
 
-def preserve_dates : Flag<["-", "--"], "preserve-dates">,
+def preserve_dates : Flag<["--"], "preserve-dates">,
                      HelpText<"Preserve access and modification timestamps">;
 def p : Flag<["-"], "p">, Alias<preserve_dates>;
 
-def strip_all : Flag<["-", "--"], "strip-all">,
+def strip_all : Flag<["--"], "strip-all">,
                 HelpText<"Remove non-allocated sections outside segments. "
                          ".gnu.warning* sections are not removed">;
 def s : Flag<["-"], "s">, Alias<strip_all>;
 
-def strip_all_gnu : Flag<["-", "--"], "strip-all-gnu">,
+def strip_all_gnu : Flag<["--"], "strip-all-gnu">,
                     HelpText<"Compatible with GNU strip's --strip-all">;
-def strip_debug : Flag<["-", "--"], "strip-debug">,
+def strip_debug : Flag<["--"], "strip-debug">,
                   HelpText<"Remove debugging symbols only">;
 def d : Flag<["-"], "d">, Alias<strip_debug>;
 def g : Flag<["-"], "g">, Alias<strip_debug>;
 def S : Flag<["-"], "S">, Alias<strip_debug>;
-def strip_unneeded : Flag<["-", "--"], "strip-unneeded">,
+def strip_unneeded : Flag<["--"], "strip-unneeded">,
                      HelpText<"Remove all symbols not needed by relocations">;
 
 defm remove_section : Eq<"remove-section", "Remove <section>">,
                       MetaVarName<"section">;
 def R : JoinedOrSeparate<["-"], "R">, Alias<remove_section>;
 
 defm strip_symbol : Eq<"strip-symbol", "Strip <symbol>">,
                     MetaVarName<"symbol">;
 def N : JoinedOrSeparate<["-"], "N">, Alias<strip_symbol>;
 
 defm keep_section : Eq<"keep-section", "Keep <section>">,
                     MetaVarName<"section">;
 defm keep_symbol : Eq<"keep-symbol", "Do not remove symbol <symbol>">,
                    MetaVarName<"symbol">;
-def keep_file_symbols : Flag<["-", "--"], "keep-file-symbols">,
+def keep_file_symbols : Flag<["--"], "keep-file-symbols">,
                         HelpText<"Do not remove file symbols">;
 
 def K : JoinedOrSeparate<["-"], "K">, Alias<keep_symbol>;
 
 def only_keep_debug
-    : Flag<["-", "--"], "only-keep-debug">,
+    : Flag<["--"], "only-keep-debug">,
       HelpText<"Clear sections that would not be stripped by --strip-debug. "
                "Currently only implemented for COFF.">;
 
-def discard_locals : Flag<["-", "--"], "discard-locals">,
+def discard_locals : Flag<["--"], "discard-locals">,
                      HelpText<"Remove compiler-generated local symbols, (e.g. "
                               "symbols starting with .L)">;
 def X : Flag<["-"], "X">, Alias<discard_locals>;
 
 def discard_all
-    : Flag<["-", "--"], "discard-all">,
+    : Flag<["--"], "discard-all">,
       HelpText<"Remove all local symbols except file and section symbols">;
 def x : Flag<["-"], "x">, Alias<discard_all>;
 
 def regex
-    : Flag<["-", "--"], "regex">,
+    : Flag<["--"], "regex">,
       HelpText<"Permit regular expressions in name comparison">;
 
-def version : Flag<["-", "--"], "version">,
+def version : Flag<["--"], "version">,
               HelpText<"Print the version and exit.">;
 def V : Flag<["-"], "V">, Alias<version>;