From 71d7fed3bc2ad6c22729d446526a59fcfd99bd03 Mon Sep 17 00:00:00 2001 From: gbreynoo Date: Tue, 31 Aug 2021 16:41:08 +0100 Subject: [PATCH] [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 | 17 ++++++++++---- .../test/tools/llvm-objcopy/tool-help-message.test | 19 ++++++++-------- llvm/unittests/Option/OptionParsingTest.cpp | 26 ++++++++++++++++++++++ 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/llvm/lib/Option/OptTable.cpp b/llvm/lib/Option/OptTable.cpp index a994a42..2770f4d 100644 --- a/llvm/lib/Option/OptTable.cpp +++ b/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) - ++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); + + 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); } diff --git a/llvm/test/tools/llvm-objcopy/tool-help-message.test b/llvm/test/tools/llvm-objcopy/tool-help-message.test index c51221b..7e72acb 100644 --- a/llvm/test/tools/llvm-objcopy/tool-help-message.test +++ b/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 %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] @@ -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 diff --git a/llvm/unittests/Option/OptionParsingTest.cpp b/llvm/unittests/Option/OptionParsingTest.cpp index f0299d2..520801d 100644 --- a/llvm/unittests/Option/OptionParsingTest.cpp +++ b/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 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 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]); +} -- 2.7.4