[CommandLine] Error message for incorrect PositionalEatArgs usage

[loladiro]

Summary

bugpoint has several options specified as PositionalEatArgs to pass
options through to the underlying tool, e.g. -tool-args. The -help
message suggests the usage is: -tool-args=<string>. However, this is
misleading, because that's not how these arguments work. Rather than taking
a value, the option consumes all positional arguments until the next
recognized option (or all arguments if -- is specified at some point).
To make this slightly clearer, instead print the help as:

-tool-args <string>...                            - <tool arguments>...

Additionally, add an error if the user attempts to use a PositionalEatArgs
argument with a value, instead of silently ignoring it. Example:

./bin/bugpoint -tool-args=-mpcu=skylake-avx512
bugpoint: for the -tool-args option: This argument does not take a value.
    Instead, it consumes any positional arguments until the next recognized option.

[aprantl]

I suppose we could add a test for the erroneous case?

[loladiro]

Add test

[aprantl]

Great, thanks!

include/llvm/Support/CommandLine.h

 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <climits>
 #include <cstddef>
 #include <functional>
 #include <initializer_list>
 #include <string>
 #include <type_traits>
 #include <vector>
public:
   virtual void getExtraOptionNames(SmallVectorImpl<StringRef> &) {}
 
   // addOccurrence - Wrapper around handleOccurrence that enforces Flags.
   //
   virtual bool addOccurrence(unsigned pos, StringRef ArgName, StringRef Value,
                              bool MultiArg = false);
 
   // Prints option name followed by message.  Always returns true.
-  bool error(const Twine &Message, StringRef ArgName = StringRef());
+  bool error(const Twine &Message, StringRef ArgName = StringRef(), raw_ostream &Errs = llvm::errs());
+  bool error(const Twine &Message, raw_ostream &Errs) {
+    return error(Message, StringRef(), Errs);
+  }
 
   inline int getNumOccurrences() const { return NumOccurrences; }
   inline void reset() { NumOccurrences = 0; }
 };
 
 //===----------------------------------------------------------------------===//
 // Command line option modifiers that can be used to modify the behavior of
 // command line option parsers...

lib/Support/CommandLine.cpp

if (!Handler) {
              I != E; ++I)
           (*I)->addOccurrence(i, "", StringRef(argv[i]));
       }
       continue;
     }
 
     // If this is a named positional argument, just remember that it is the
     // active one...
-    if (Handler->getFormattingFlag() == cl::Positional)
+    if (Handler->getFormattingFlag() == cl::Positional) {
+      if ((Handler->getMiscFlags() & PositionalEatsArgs) && !Value.empty()) {
+        Handler->error("This argument does not take a value.\n"
+                       "\tInstead, it consumes any positional arguments until "
+                       "the next recognized option.", *Errs);
+        ErrorParsing = true;
+      }
       ActivePositionalArg = Handler;
+    }
     else
       ErrorParsing |= ProvideOption(Handler, ArgName, Value, argc, argv, i);
   }
 
   // Check and handle positional arguments now...
   if (NumPositionalRequired > PositionalVals.size()) {
       *Errs << ProgramName
              << ": Not enough positional command line arguments specified!\n"
bool CommandLineParser::ParseCommandLineOptions(int argc,
   }
   return true;
 }
 
 //===----------------------------------------------------------------------===//
 // Option Base class implementation
 //
 
-bool Option::error(const Twine &Message, StringRef ArgName) {
+bool Option::error(const Twine &Message, StringRef ArgName, raw_ostream &Errs) {
   if (!ArgName.data())
     ArgName = ArgStr;
   if (ArgName.empty())
-    errs() << HelpStr; // Be nice for positional arguments
+    Errs << HelpStr; // Be nice for positional arguments
   else
-    errs() << GlobalParser->ProgramName << ": for the -" << ArgName;
+    Errs << GlobalParser->ProgramName << ": for the -" << ArgName;
 
-  errs() << " option: " << Message << "\n";
+  Errs << " option: " << Message << "\n";
   return true;
 }
 
 bool Option::addOccurrence(unsigned pos, StringRef ArgName, StringRef Value,
                            bool MultiArg) {
   if (!MultiArg)
     NumOccurrences++; // Increment the number of times we have been seen
 

 
 // basic_parser implementation
 //
 
 // Return the width of the option tag for printing...
 size_t basic_parser_impl::getOptionWidth(const Option &O) const {
   size_t Len = O.ArgStr.size();
   auto ValName = getValueName();
-  if (!ValName.empty())
-    Len += getValueStr(O, ValName).size() + 3;
+  if (!ValName.empty()) {
+    size_t FormattingLen = 3;
+    if (O.getMiscFlags() & PositionalEatsArgs)
+      FormattingLen = 6;
+    Len += getValueStr(O, ValName).size() + FormattingLen;
+  }
 
   return Len + 6;
 }
 
 // printOptionInfo - Print out information about this option.  The
 // to-be-maintained width is specified.
 //
 void basic_parser_impl::printOptionInfo(const Option &O,
                                         size_t GlobalWidth) const {
   outs() << "  -" << O.ArgStr;
 
   auto ValName = getValueName();
-  if (!ValName.empty())
+  if (!ValName.empty()) {
+    if (O.getMiscFlags() & PositionalEatsArgs) {
+      outs() << " <" << getValueStr(O, ValName) << ">...";
+    } else {
       outs() << "=<" << getValueStr(O, ValName) << '>';
+    }
+  }
 
   Option::printHelpStr(O.HelpStr, GlobalWidth, getOptionWidth(O));
 }
 
 void basic_parser_impl::printOptionName(const Option &O,
                                         size_t GlobalWidth) const {
   outs() << "  -" << O.ArgStr;
   outs().indent(GlobalWidth - O.ArgStr.size());

unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, ReadConfigFile) {
   EXPECT_STREQ(Argv[2], "-option_3=abcd");
   EXPECT_STREQ(Argv[3], "-option_4=cdef");
 
   llvm::sys::fs::remove(TestCfg2);
   llvm::sys::fs::remove(TestCfg);
   llvm::sys::fs::remove(TestDir);
 }
 
+TEST(CommandLineTest, PositionalEatArgsError) {
+  static cl::list<std::string> PosEatArgs("positional-eat-args", cl::Positional,
+    cl::desc("<arguments>..."), cl::ZeroOrMore,
+    cl::PositionalEatsArgs);
+
+  const char *args[] = {"prog", "-positional-eat-args=XXXX"};
+  const char *args2[] = {"prog", "-positional-eat-args=XXXX", "-foo"};
+  const char *args3[] = {"prog", "-positional-eat-args", "-foo"};
+
+  std::string Errs;
+  raw_string_ostream OS(Errs);
+  EXPECT_FALSE(cl::ParseCommandLineOptions(2, args, StringRef(), &OS)); OS.flush();
+  EXPECT_FALSE(Errs.empty()); Errs.clear();
+  EXPECT_FALSE(cl::ParseCommandLineOptions(3, args2, StringRef(), &OS)); OS.flush();
+  EXPECT_FALSE(Errs.empty()); Errs.clear();
+  EXPECT_TRUE(cl::ParseCommandLineOptions(3, args3, StringRef(), &OS)); OS.flush();
+  EXPECT_TRUE(Errs.empty());
+}
+
 }  // anonymous namespace