[OptTable] Improve error message output for grouped short options

[gbreynoo]

Summary

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.

This change includes additional test cases added to llvm-symbolizer/unknown-argument.test and fixes up llvm-objcopy/tool-help-message.test due to the change in error output. Note that due to implementation of llvm-objcopy only the first incorrect argument is output.

[gbreynoo]

llvm/lib/Option/OptTable.cpp

379–383

I could not find a case in which we would enter if (Str.size() == 2) so I have removed it. I think either the option would have already been accepted or we want to create the an Arg as unknown option.

[MaskRay]

The main test should be moved from llvm-symbolizer to llvm/unittests/Option/OptionParsingTest.cpp

[MaskRay]

llvm/lib/Option/OptTable.cpp

379–383

This is ok

[jhenderson]

llvm/lib/Option/OptTable.cpp

379–383

389

[gbreynoo]

Fixed comments as suggested by Jhenderson and moved the llvm-symbolizer/unknown-argument.test changes to unit tests as suggested by MaskRay.

[MaskRay]

LGTM.

[jhenderson]

LGTM.

[kda]

This looks like it broke the sanitizer build blocks.
https://lab.llvm.org/buildbot/#/builders/5/builds/11210

A guide to reproducing sanitizer bots can be found here: https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild

relevant log snippet below:

FAIL: LLVM-Unit :: Option/./OptionTests/Option.FlagsWithoutValues (73786 of 78710)
******************** TEST 'LLVM-Unit :: Option/./OptionTests/Option.FlagsWithoutValues' FAILED ********************
Script:
--
/b/sanitizer-x86_64-linux-fast/build/llvm_build_asan/unittests/Option/./OptionTests --gtest_filter=Option.FlagsWithoutValues
--
Note: Google Test filter = Option.FlagsWithoutValues
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from Option
[ RUN      ] Option.FlagsWithoutValues
[       OK ] Option.FlagsWithoutValues (0 ms)
[----------] 1 test from Option (0 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[  PASSED  ] 1 test.
=================================================================
==28024==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 176 byte(s) in 2 object(s) allocated from:
    #0 0x37e7fd in operator new(unsigned long) /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cpp:95:3
    #1 0x3c88e6 in llvm::opt::Option::accept(llvm::opt::ArgList const&, llvm::StringRef, bool, unsigned int&) const /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Option/Option.cpp:236:32
    #2 0x3cd195 in llvm::opt::OptTable::parseOneArgGrouped(llvm::opt::InputArgList&, unsigned int&) const /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Option/OptTable.cpp:378:22
    #3 0x3cf4f2 in llvm::opt::OptTable::ParseArgs(llvm::ArrayRef<char const*>, unsigned int&, unsigned int&, unsigned int, unsigned int) const /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Option/OptTable.cpp:485:20
    #4 0x3a8ce0 in Option_FlagsWithoutValues_Test::TestBody() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/unittests/Option/OptionParsingTest.cpp:385:23
    #5 0x4d60db in HandleExceptionsInMethodIfSupported<testing::Test, void> /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #6 0x4d60db in testing::Test::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2508:5
    #7 0x4d890c in testing::TestInfo::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2684:11
    #8 0x4d9cc0 in testing::TestSuite::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2816:28
    #9 0x508c51 in testing::internal::UnitTestImpl::RunAllTests() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:5338:44
    #10 0x50710c in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
    #11 0x50710c in testing::UnitTest::Run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4925:10
    #12 0x4b92b0 in RUN_ALL_TESTS /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2473:46
    #13 0x4b92b0 in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:10
    #14 0x7fc2d313b09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
SUMMARY: AddressSanitizer: 176 byte(s) leaked in 2 allocation(s).
********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  LLVM-Unit :: Option/./OptionTests/Option.FlagsWithoutValues

[gbreynoo]

Thanks for the revert @kda, I've reapplied with a fix.

llvm/lib/Option/OptTable.cpp

for (; Start != End; ++Start) {
 
     // Otherwise, see if the argument is missing.
     if (Prev != Index)
       return nullptr;
   }
   if (Fallback) {
     Option Opt(Fallback, this);
     if (Arg *A = Opt.accept(Args, Str.substr(0, 2), true, Index)) {
-      if (Str.size() == 2)
-        ++Index;
-      else
-        Args.replaceArgString(Index, Twine('-') + Str.substr(2));
+      // Check that the last option isn't a flag wrongly given an argument.
+      if (Str[2] == '=')
+        return new Arg(getOption(TheUnknownOptionID), Str, Index++, CStr);
+
       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);
 }
 
 Arg *OptTable::ParseOneArg(const ArgList &Args, unsigned &Index,
                            unsigned FlagsToInclude,
                            unsigned FlagsToExclude) const {
   unsigned Prev = Index;
   const char *Str = Args.getArgString(Index);

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

 # RUN: llvm-objcopy -h | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
 # 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 %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-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 %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-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 %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-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 %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-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]
 # OBJCOPY-USAGE:  Pass @FILE as argument to read options from FILE.
 
 # STRIP-USAGE:    USAGE: llvm-strip [options] inputs...
 # STRIP-USAGE:    Pass @FILE as argument to read options from FILE.
 
 # INSTALL-NAME-TOOL-USAGE: USAGE: llvm-install-name-tool [options] input
 # INSTALL-NAME-TOOL-USAGE: Pass @FILE as argument to read options from FILE.
 
 # 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

for (int I = 0; I < 2; ++I) {
     T.setGroupedShortOptions(I != 0);
     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("-u", Unknown[0]);
     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]);
+}