[Option] Support special argument "--"

[MaskRay]

Summary

Many command line option implementations, including getopt_long and our
Support/CommandLine.cpp, support -- as an end-of-option indicator. All
the subsequent arguments are then treated as positional arguments.

D1387 added KIND_REMAINING_ARGS and 76ff1d915c9c42823a3f2b08ff936cf7a48933c5 dropped special handling of --.
Users need to add def DASH_DASH : Option<["--"], "", KIND_REMAINING_ARGS>; and
append OPT_DASH_DASH to the OPT_INPUT list., which is not ergonomic.

Restore this feature under an option and modify llvm-strings to utilize the
feature as an example. In the future, we probably should enable this feature by
default and exclude some tools that handle DASH_DASH differently (clang,
clang-scan-deps, etc. I suspect that many are workarounds for LLVMOption not
supporting -- as a built-in feature).

[MaskRay]

remove untested DashDash from Opts.td. It would make -- without DashDashParsing mode an option.

[MaskRay]

(CC @hans Just noticed that you have D1387 which allows us to remove the hard-coded handling of "--".
I don't find DASH_DASH in modern lld, but Clang does have a special case.)

[serge-sans-paille]

Big fan of this change. I added a few potential minor suggestions.
We may want to also modify the generated help to suggest the special meaning of --?

llvm/lib/Option/OptTable.cpp

475

Probably pedantic, but the other part of the code use `std::make_unique<Arg>(...) instead which at least convey the ownership very clearly.

llvm/unittests/Option/OptionParsingTest.cpp

418

Would be great to have a test without any trailing argument after the -- too.

[MaskRay]

improve test

[MaskRay]

llvm/lib/Option/OptTable.cpp

475

The primary code path has Args.append(A.release());. I think it's better to use new than make_unique + release.

[serge-sans-paille]

llvm/lib/Option/OptTable.cpp

475

Missed that. Thanks o/

[hans]

In D152286#4400088, @MaskRay wrote:

(CC @hans Just noticed that you have D1387 which allows us to remove the hard-coded handling of "--".
I don't find DASH_DASH in modern lld, but Clang does have a special case.)

Thanks! The motivation for introducing KIND_REMAINING_ARGS was to support the /link flag. I think at the time I figured it was more elegant that -- could now be implemented with the regular option parsing machinery rather than hard-coding, but as you say since most option parsing libraries support --, maybe it makes sense to put the hard-coded behavior back.

[jhenderson]

Couple of nits from me. I've got no objections to this change, but am a bit rusty on the code, so won't approve myself.

llvm/include/llvm/Option/OptTable.h

144

llvm/lib/Option/OptTable.cpp

471–472

Either "marks all trailing..." or "arguments positional" (but not both).

[MaskRay]

improve comments

llvm/include/llvm/Option/OptTable.h

struct Info {
     const char *Values;
   };
 
 private:
   /// The option information table.
   ArrayRef<Info> OptionInfos;
   bool IgnoreCase;
   bool GroupedShortOptions = false;
+  bool DashDashParsing = false;
   const char *EnvVar = nullptr;
 
   unsigned InputOptionID = 0;
   unsigned UnknownOptionID = 0;
 
 protected:
   /// The index of the first option which can be parsed (i.e., is not a
   /// special option like 'input' or 'unknown', and is not an option group).
public:
   }
 
   /// Specify the environment variable where initial options should be read.
   void setInitialOptionsFromEnvironment(const char *E) { EnvVar = E; }
 
   /// Support grouped short options. e.g. -ab represents -a -b.
   void setGroupedShortOptions(bool Value) { GroupedShortOptions = Value; }
 
+  /// Set whether "--" stops option parsing and treats all subsequent arguments
+  /// as positional. E.g. -- -a -b gives two positional inputs.
+  void setDashDashParsing(bool Value) { DashDashParsing = Value; }
+
   /// Find possible value for given flags. This is used for shell
   /// autocompletion.
   ///
   /// \param [in] Option - Key flag like "-stdlib=" when "-stdlib=l"
   /// was passed to clang.
   ///
   /// \param [in] Arg - Value which we want to autocomplete like "l"
   /// when "-stdlib=l" was passed to clang.

llvm/lib/Option/OptTable.cpp

while (Index < End) {
     }
     // Ignore empty arguments (other things may still take them as arguments).
     StringRef Str = Args.getArgString(Index);
     if (Str == "") {
       ++Index;
       continue;
     }
 
+    // In DashDashParsing mode, the first "--" stops option scanning and treats
+    // all subsequent arguments as positional.
+    if (DashDashParsing && Str == "--") {
+      while (++Index < End) {
+        Args.append(new Arg(getOption(InputOptionID), Str, Index,
+                            Args.getArgString(Index)));
+      }
+      break;
+    }
+
     unsigned Prev = Index;
     std::unique_ptr<Arg> A = GroupedShortOptions
                  ? parseOneArgGrouped(Args, Index)
                  : ParseOneArg(Args, Index, FlagsToInclude, FlagsToExclude);
     assert((Index > Prev || GroupedShortOptions) &&
            "Parser failed to consume argument.");
 
     // Check for missing argument error.

llvm/test/tools/llvm-strings/dash-filename.test

+## Show that -- stops option scanning.
+
+RUN: rm -rf %t && mkdir %t && cd %t
+RUN: echo abcd > -a
+RUN: llvm-strings -f -- -a | FileCheck %s
+CHECK: -a: abcd

llvm/tools/llvm-strings/llvm-strings.cpp

 #include "Opts.inc"
 #undef OPTION
 };
 
 class StringsOptTable : public opt::GenericOptTable {
 public:
   StringsOptTable() : GenericOptTable(InfoTable) {
     setGroupedShortOptions(true);
+    setDashDashParsing(true);
   }
 };
 } // namespace
 
 static StringRef ToolName;
 
 static cl::list<std::string> InputFileNames(cl::Positional,
                                             cl::desc("<input object files>"));

llvm/unittests/Option/OptionParsingTest.cpp

TYPED_TEST(OptTableTest, ParseGroupedShortOptions) {
 
   // Short options followed by a long option. We probably should disallow this.
   const char *Args3[] = {"-AIblorp"};
   InputArgList AL3 = T.ParseArgs(Args3, MAI, MAC);
   EXPECT_TRUE(AL3.hasArg(OPT_A));
   EXPECT_TRUE(AL3.hasArg(OPT_Blorp));
 }
 
+TYPED_TEST(OptTableTest, ParseDashDash) {
+  TypeParam T;
+  T.setDashDashParsing(true);
+  unsigned MAI, MAC;
+
+  const char *Args1[] = {"-A", "--"};
+  InputArgList AL = T.ParseArgs(Args1, MAI, MAC);
+  EXPECT_TRUE(AL.hasArg(OPT_A));
+  EXPECT_EQ(size_t(0), AL.getAllArgValues(OPT_INPUT).size());
+  EXPECT_EQ(size_t(0), AL.getAllArgValues(OPT_UNKNOWN).size());
+
+  const char *Args2[] = {"-A", "--", "-A", "--", "-B"};
+  AL = T.ParseArgs(Args2, MAI, MAC);
+  EXPECT_TRUE(AL.hasArg(OPT_A));
+  EXPECT_FALSE(AL.hasArg(OPT_B));
+  const std::vector<std::string> Input = AL.getAllArgValues(OPT_INPUT);
+  ASSERT_EQ(size_t(3), Input.size());
+  EXPECT_EQ("-A", Input[0]);
+  EXPECT_EQ("--", Input[1]);
+  EXPECT_EQ("-B", Input[2]);
+  EXPECT_EQ(size_t(0), AL.getAllArgValues(OPT_UNKNOWN).size());
+
+  T.setDashDashParsing(false);
+  AL = T.ParseArgs(Args2, MAI, MAC);
+  EXPECT_TRUE(AL.hasArg(OPT_A));
+  EXPECT_TRUE(AL.hasArg(OPT_B));
+  EXPECT_EQ(size_t(0), AL.getAllArgValues(OPT_INPUT).size());
+  const std::vector<std::string> Unknown = AL.getAllArgValues(OPT_UNKNOWN);
+  ASSERT_EQ(size_t(2), Unknown.size());
+  EXPECT_EQ("--", Unknown[0]);
+  EXPECT_EQ("--", Unknown[1]);
+}
+
 TYPED_TEST(OptTableTest, UnknownOptions) {
   TypeParam T;
   unsigned MAI, MAC;
   const char *Args[] = {"-u", "--long", "0"};
   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);

llvm/unittests/Option/Opts.td

 def Ermgh : Joined<["--"], "ermgh">, HelpText<"The ermgh option">, MetaVarName<"ERMGH">, Flags<[OptFlag1]>;
 def Fjormp : Flag<["--"], "fjormp">, HelpText<"The fjormp option">, Flags<[OptFlag1]>;
 
 def Glorrmp_eq : Flag<["--"], "glorrmp=">;
 
 def Blurmpq : Flag<["--"], "blurmp">;
 def Blurmpq_eq : Flag<["--"], "blurmp=">;
 
-def DashDash : Option<["--"], "", KIND_REMAINING_ARGS>;
-
 class XOpts<string base> : KeyPathAndMacro<"X->", base> {}
 
 def marshalled_flag_d : Flag<["-"], "marshalled-flag-d">,
   MarshallingInfoFlag<XOpts<"MarshalledFlagD">>;
 def marshalled_flag_c : Flag<["-"], "marshalled-flag-c">,
   MarshallingInfoFlag<XOpts<"MarshalledFlagC">>,
   ImpliedByAnyOf<[marshalled_flag_d.KeyPath], "true">;
 def marshalled_flag_b : Flag<["-"], "marshalled-flag-b">,
   MarshallingInfoFlag<XOpts<"MarshalledFlagB">>,
   ImpliedByAnyOf<[marshalled_flag_d.KeyPath], "true">;
 def marshalled_flag_a : Flag<["-"], "marshalled-flag-a">,
   MarshallingInfoFlag<XOpts<"MarshalledFlagA">>,
   ImpliedByAnyOf<[marshalled_flag_c.KeyPath, marshalled_flag_b.KeyPath]>;