[OptTable] Improve error message output for grouped short options

As seen in https://bugs.llvm.org/show_bug.cgi?id=48880 the current
implementation for parsing grouped short options can return unclear
error messages. This change fixes the example given in the ticket in
which a flag is incorrectly given an argument. Also when parsing a
group we now keep reading past the first incorrect option and output
errors for all incorrect options in the group.

Differential Revision: https://reviews.llvm.org/D108770

llvm/lib/Option/OptTable.cpp

@@ -376,14 +376,23 @@ Arg *OptTable::parseOneArgGrouped(InputArgList &Args, unsigned &Index) const {
   if (Fallback) {
     Option Opt(Fallback, this);
     if (Arg *A = Opt.accept(Args, Str.substr(0, 2), true, Index)) {
-      if (Str.size() == 2)
+      // Check that the last option isn't a flag wrongly given an argument.
-        ++Index;
+      if (Str[2] == '=')
-      else
+        return new Arg(getOption(TheUnknownOptionID), Str, Index++, CStr);
+
       Args.replaceArgString(Index, Twine('-') + Str.substr(2));
       return A;
     }
   }
 
+  // In the case of an incorrect short option extract the character and move to
+  // the next one.
+  if (Str[1] != '-') {
+    CStr = Args.MakeArgString(Str.substr(0, 2));
+    Args.replaceArgString(Index, Twine('-') + Str.substr(2));
+    return new Arg(getOption(TheUnknownOptionID), CStr, Index, CStr);
+  }
+
   return new Arg(getOption(TheUnknownOptionID), Str, Index++, CStr);
 }
 

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

@@ -2,31 +2,31 @@
 # RUN: llvm-objcopy --help | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
 # RUN: not llvm-objcopy 2>&1 | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
 # RUN: not llvm-objcopy -- 2>&1 | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
-# 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-SHORT %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-LONG %s
 # RUN: not llvm-objcopy --strip-debug 2>&1 | FileCheck %s --check-prefix=NO-INPUT-FILES
 
 # RUN: llvm-strip -h | FileCheck --check-prefix=STRIP-USAGE %s --match-full-lines
 # RUN: llvm-strip --help | FileCheck --check-prefix=STRIP-USAGE %s --match-full-lines
 # RUN: not llvm-strip 2>&1 | FileCheck --check-prefix=STRIP-USAGE %s --match-full-lines
 # RUN: not llvm-strip -- 2>&1 | FileCheck --check-prefix=STRIP-USAGE %s --match-full-lines
-# 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-SHORT %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-LONG %s
 # RUN: not llvm-strip --strip-debug 2>&1 | FileCheck %s --check-prefix=NO-INPUT-FILES
 
 # RUN: llvm-install-name-tool -h | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s --match-full-lines
 # RUN: llvm-install-name-tool --help | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s --match-full-lines
 # RUN: not llvm-install-name-tool 2>&1 | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s --match-full-lines
-# RUN: not llvm-install-name-tool -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
+# RUN: not llvm-install-name-tool -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG-LONG %s
-# RUN: not llvm-install-name-tool --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
+# RUN: not llvm-install-name-tool --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG-LONG %s
 # RUN: not llvm-install-name-tool -add_rpath @executable 2>&1 | FileCheck %s --check-prefix=NO-INPUT-FILES
 # RUN: not llvm-install-name-tool -add_rpath @executable f1 f2 2>&1 | FileCheck %s --check-prefix=MULTIPLE-INPUT-FILES
 
 # RUN: llvm-bitcode-strip -h | FileCheck --check-prefix=BITCODE-STRIP-USAGE %s --match-full-lines
 # RUN: llvm-bitcode-strip --help | FileCheck --check-prefix=BITCODE-STRIP-USAGE %s --match-full-lines
 # RUN: not llvm-bitcode-strip 2>&1 | FileCheck --check-prefix=BITCODE-STRIP-USAGE %s --match-full-lines
-# RUN: not llvm-bitcode-strip -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
+# RUN: not llvm-bitcode-strip -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG-LONG %s
-# RUN: not llvm-bitcode-strip --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
+# RUN: not llvm-bitcode-strip --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG-LONG %s
 # RUN: not llvm-bitcode-strip f1 f2 2>&1 | FileCheck %s --check-prefix=MULTIPLE-INPUT-FILES
 
 # OBJCOPY-USAGE:  USAGE: llvm-objcopy [options] input [output]
@@ -41,6 +41,7 @@
 # BITCODE-STRIP-USAGE: USAGE: llvm-bitcode-strip [options] input
 # BITCODE-STRIP-USAGE: Pass @FILE as argument to read options from FILE.
 
-# UNKNOWN-ARG:    unknown argument '{{-+}}abcabc'
+# UNKNOWN-ARG-SHORT: unknown argument '-a'
+# UNKNOWN-ARG-LONG: unknown argument '{{-+}}abcabc'
 # NO-INPUT-FILES: no input file specified
 # MULTIPLE-INPUT-FILES: expects a single input file

llvm/unittests/Option/OptionParsingTest.cpp

@@ -376,3 +376,29 @@ TEST(Option, UnknownOptions) {
     EXPECT_EQ("--long", Unknown[1]);
   }
 }
+
+TEST(Option, FlagsWithoutValues) {
+  TestOptTable T;
+  T.setGroupedShortOptions(true);
+  unsigned MAI, MAC;
+  const char *Args[] = {"-A=1", "-A="};
+  InputArgList AL = T.ParseArgs(Args, MAI, MAC);
+  const std::vector<std::string> Unknown = AL.getAllArgValues(OPT_UNKNOWN);
+  ASSERT_EQ((size_t)2, Unknown.size());
+  EXPECT_EQ("-A=1", Unknown[0]);
+  EXPECT_EQ("-A=", Unknown[1]);
+}
+
+TEST(Option, UnknownGroupedShortOptions) {
+  TestOptTable T;
+  T.setGroupedShortOptions(true);
+  unsigned MAI, MAC;
+  const char *Args[] = {"-AuzK", "-AuzK"};
+  InputArgList AL = T.ParseArgs(Args, MAI, MAC);
+  const std::vector<std::string> Unknown = AL.getAllArgValues(OPT_UNKNOWN);
+  ASSERT_EQ((size_t)4, Unknown.size());
+  EXPECT_EQ("-u", Unknown[0]);
+  EXPECT_EQ("-z", Unknown[1]);
+  EXPECT_EQ("-u", Unknown[2]);
+  EXPECT_EQ("-z", Unknown[3]);
+}