OptTable: stop parsing options after "--" alone.

[t.p.northover]

Summary

Conventionally, -- on its own tells a command to stop parsing further options and treat all future arguments as positional inputs. Used to act on pesky files starting with -, and possibly other things.

I wasn't entirely sure it should apply to all tools, but it seems like a reasonable default position to take. And the -- would have been a hard error (unknown option) for all existing backends anyway so I don't think it can break anyone.

[jhenderson]

I feel like this is a non-trivial change that requires greater visibility and buy-in from the community. I personally have no issues with it, but I think it's worth raising on the LLVM Discourse channel.

llvm/test/tools/llvm-nm/option--.test

2

I don't think a test in tools/llvm-nm is the right place for this sort of behaviour, as it isn't code that is specific to llvm-nm. Can it be tested using the gtest unit test system? If not, then the test still would belong in llvm/test/Option or something like that.

[MaskRay]

This behavior makes sense to me. This is llvm/lib/SupportCommandLine.cpp and GNU getopt_long's behavior, used by a lot of utilities, well beyond GNU. (The convention might be by others?)

POSIX considers -- special as well.

argv[optind]   points to the string "--"

getopt() shall return -1 after incrementing optind.

clang/include/clang/Driver/Options.td currently defines def _DASH_DASH : Option<["--"], "", KIND_REMAINING_ARGS>, and this change will change the behavior and cause some interesting interop issues (I don't investigate closely) with D98824.

% gcc -- -E a.c
gcc: error: unrecognized command-line option ‘--’
% clang -- -E a.c
clang: error: no such file or directory: '-E'
% as -- a.c
waiting for stdin as if `--` is spelled as `-`. I think this is gas's bug.

llvm/test/tools/llvm-nm/option--.test

2

The test can be added to llvm/unittests/Option/OptionParsingTest.cpp

3

The message is OS dependent. We can use %errc_ENOENT (see llvm/docs/TestingGuide.rst).

[t.p.northover]

Sorry about this, but I hadn't realised Clang itself used this library until you mentioned the behaviour there. And that led me to test failures with clang-linker-wrapper (already does something similar, but really weirdly) and clang-scan-deps. And it turns out this faciliy already pretty much exists but is setup in the option table.

I'd kind of prefer opt out personally (it's a niche use-case, but you'd probably want it more than not when it comes up). But being able to control it is definitely better than my original patch so I rewrote it to only affect llvm-nm after all.

[t.p.northover]

Switch to existing solution for this. I think that makes the llvm-nm test probably the right place again so I've left it.

[t.p.northover]

Test was leaving stray files in the source dir.

[t.p.northover]

Ping.

[MaskRay]

The patch looks good to me. It's a shame that we have to explicitly teach a tool to use OPT_DASH_DASH instead of making this a built-in feature of LLVMOption and making the feature opt-out (-- is an overwhelming convention and usually a tool needs arguments to not obey that).
That said, accepting -- in llvm-nm is still a good change.

@jhenderson usually has strong opinions on option handling and should sign off on this patch.

llvm/test/tools/llvm-nm/option--.test

2

See llvm/docs/TestingGuide.rst:692 %errc_ENOENT

5

to prevent issues when %t already exists as a file or exists as a directory with existing files potentially intervening with this test execution.

[MaskRay]

I think def DASH_DASH and if (opt::Arg *A = Args.getLastArgNoClaim(OPT_DASH_DASH)) will probably be non-ergonomic for most tools. I created D152286 [Option] Support special argument "--".

[jhenderson]

Sorry for the silence: I've been only doing minimal levels of reviewing the past couple of weeks, and this week isn't shaping up to be much better for various reasons. This patch looks good, aside from the hard-coded error message in the check (see @MaskRay's inline comment) which is causing a test failure on Windows., but a more general solution would be better if possible, so I suggest this is parked until D152286 has landed or otherwise.

llvm/test/tools/llvm-nm/option--.test

+; RUN: not llvm-nm -- -weird-filename-that-doesnt-exist 2>&1 | FileCheck %s --check-prefix=CHECK-NOTOPT
+; CHECK-NOTOPT: error: -weird-filename-that-doesnt-exist: No such file or directory
+
+; RUN: llvm-as < %s > %p/-.bc
+; RUN: llvm-nm -- %p/-.bc | FileCheck %s
+
+; CHECK: foo
+define void @foo() {
+  ret void
+}

llvm/tools/llvm-nm/Opts.td

 def : F<"r", "Alias for --reverse-sort">, Alias<reverse_sort>;
 def : F<"S", "Alias for --print-size">, Alias<print_size>;
 def : JoinedOrSeparate<["-"], "t">, HelpText<"Alias for --radix">, Alias<radix_EQ>, MetaVarName<"<radix>">;
 def : F<"u", "Alias for --undefined-only">, Alias<undefined_only>;
 def : F<"U", "Alias for --defined-only">, Alias<defined_only>;
 def : F<"v", "Alias for --numeric-sort">, Alias<numeric_sort>;
 def : F<"V", "Alias for --version">, Alias<version>;
 def : F<"W", "Alias for --no-weak">, Alias<no_weak>;
+
+def DASH_DASH : Option<["--"], "", KIND_REMAINING_ARGS>;

llvm/tools/llvm-nm/llvm-nm.cpp

for (opt::Arg *A : Args.filtered(OPT_s, OPT_INPUT)) {
     } else {
       InputFilenames.push_back(A->getValue());
     }
   }
   if (!SegSect.empty() && SegSect.size() != 2)
     error("bad number of arguments (must be two arguments)",
           "for the -s option");
 
+  // Everything after `--` on its own should be considered an input file.
+  if (opt::Arg *A = Args.getLastArgNoClaim(OPT_DASH_DASH))
+    InputFilenames.insert(InputFilenames.end(), A->getValues().begin(),
+                          A->getValues().end());
+
   if (InputFilenames.empty())
     InputFilenames.push_back("a.out");
   if (InputFilenames.size() > 1)
     MultipleFiles = true;
 
   if (NoDyldInfo && (AddDyldInfo || DyldInfoOnly))
     error("--no-dyldinfo can't be used with --add-dyldinfo or --dyldinfo-only");