[CommandLine] Remove `may only occur zero or one times!` error

[MaskRay]

Summary

Early adoption of new technologies or adjusting certain code generation/IR optimization thresholds
is often available through some cl::opt options (which have unstable surfaces).
Specifying such an option twice will lead to an error.

% clang -c a.c -mllvm -disable-binop-extract-shuffle -mllvm -disable-binop-extract-shuffle
clang (LLVM option parsing): for the --disable-binop-extract-shuffle option: may only occur zero or one times!
% clang -c a.c -mllvm -hwasan-instrument-reads=0 -mllvm -hwasan-instrument-reads=0
clang (LLVM option parsing): for the --hwasan-instrument-reads option: may only occur zero or one times!
% clang -c a.c -mllvm --scalar-evolution-max-arith-depth=32 -mllvm --scalar-evolution-max-arith-depth=16
clang (LLVM option parsing): for the --scalar-evolution-max-arith-depth option: may only occur zero or one times!

The option is specified twice, because there is sometimes a global setting and
a specific file or project may need to override (or duplicately specify) the
value.

The error is contrary to the common practice of getopt/getopt_long command line
utilities that let the last option win and the getLastArg behavior used by
Clang driver options. I have seen such errors for several times. I think the
error just makes users inconvenient, while providing very little value on
discouraging production usage of unstable surfaces (this goal is itself
controversial, because developers might not want to commit to a stable surface
too early, or there is just some subtle codegen toggle which is infeasible to
have a driver option). Therefore, I suggest we drop the diagnostic, at least
before the diagnostic gets sufficiently better support for the overridding needs.

Removing the error is a degraded error checking experience. I think this error
checking behavior, if desirable, should be enabled explicitly by tools. Users
preferring the behavior can figure out a way to do so.

[MaskRay]

add a unittest

[jhenderson]

I can't say I really care about the existing behaviour, so LGTM, but I'd like to give others a chance to look at it.

llvm/test/tools/llvm-libtool-darwin/deterministic-library.test

27–28

Why |&?

[echristo]

I'd probably prefer that we don't do this since we're looking primarily at options that shouldn't be used outside of development, e.g. clang has a driver that does accept these kinds of things as does lld. Open to arguments for and against though :)

[MaskRay]

In D120455#3343416, @echristo wrote:

I'd probably prefer that we don't do this since we're looking primarily at options that shouldn't be used outside of development, e.g. clang has a driver that does accept these kinds of things as does lld. Open to arguments for and against though :)

Keep a diagnostic to discourage -mllvm users? For 99.9% use cases the user doesn't specify a cl::opt option twice, removing the diagnostic does no discouragement to them.
In the case that a user wants a workaround, the error prevents them from doing so: adding a new driver option and cherry picking a commit into a release is significant hassle for many groups.

I am going to so I am probably against adding a new driver option just so that some cl::opt options can be overridden. Driver options have stronger stability guarantee. Many cl::opt optimization and code generation are considered unstable and the developer has various reasons that it is not mature enough to justify a driver option.

[rnk]

I'm in favor of this change, and the code looks good, but there is some disagreement, so we should get more consensus.

@dexonsmith, any thoughts on this change, or is there a better person who can give some perspective on the usability of LLVM options at Apple?

[echristo]

My biggest concern is llvm developing a gcc-esque "--params" style set of options which is what this would give. If we want these to be a support surface we should enable and test them as such. Just allowing it without that gives the entire ecosystem another set of support surface that we actually don't want people to use. Workarounds downstream should be handled by downstream as much as possible or via coordination with developers on a "hey, can we actually wait here" on options.

[xbolva00]

Just allowing it without that gives the entire ecosystem another set of support surface that we actually don't want people to use.

+1.

So maybe hide it under cmake flag?

[rnk]

I'm not familiar with gcc --param, but if this is it, then it doesn't seem so bad. Nobody is suggesting that this be a stable, supported interface where we make long term commitments for LLVM option stability.

In any case, -mllvm already exists, and cl::opt is used by other components (MLIR). This change by itself is reasonable, and it doesn't stop us from working to reduce the exposed support surface of clang in the future. Having a slightly bad user experience for mllvm options doesn't actually reduce the support surface area of clang. We may have to agree to disagree here. :)

[MaskRay]

|& -> |

Update justification

[MaskRay]

In D120455#3343588, @rnk wrote:

I'm not familiar with gcc --param, but if this is it, then it doesn't seem so bad. Nobody is suggesting that this be a stable, supported interface where we make long term commitments for LLVM option stability.

In any case, -mllvm already exists, and cl::opt is used by other components (MLIR). This change by itself is reasonable, and it doesn't stop us from working to reduce the exposed support surface of clang in the future. Having a slightly bad user experience for mllvm options doesn't actually reduce the support surface area of clang. We may have to agree to disagree here. :)

@echristo What do you think of my updated summary?

In D120455#3343539, @rnk wrote:

I'm in favor of this change, and the code looks good, but there is some disagreement, so we should get more consensus.

@dexonsmith, any thoughts on this change, or is there a better person who can give some perspective on the usability of LLVM options at Apple?

⬆️

[dexonsmith]

In D120455#3343539, @rnk wrote:

I'm in favor of this change, and the code looks good, but there is some disagreement, so we should get more consensus.

@dexonsmith, any thoughts on this change, or is there a better person who can give some perspective on the usability of LLVM options at Apple?

⬆️

Sorry I missed before!

I don't have a strong opinion; IMO -mllvm options shouldn't get used (except by compiler developers) since they're unstable, although I'm sure they get used a little anyway. IMO, if this makes it easier for the intended users (compiler developers) then there isn't a real downside to it.

@ab @t.p.northover @Gerolf , any thoughts from you?

[ab]

In D120455#3352886, @dexonsmith wrote:

I don't have a strong opinion; IMO -mllvm options shouldn't get used (except by compiler developers) since they're unstable, although I'm sure they get used a little anyway. IMO, if this makes it easier for the intended users (compiler developers) then there isn't a real downside to it.

@ab @t.p.northover @Gerolf , any thoughts from you?

Yeah, this seems reasonable, but no strong opinions either. I totally empathize with the concerns, and ideally we wouldn't have all this exposed surface, but I'm not sure this meaningfully changes that.

[MaskRay]

In D120455#3355507, @ab wrote:

In D120455#3352886, @dexonsmith wrote:

I don't have a strong opinion; IMO -mllvm options shouldn't get used (except by compiler developers) since they're unstable, although I'm sure they get used a little anyway. IMO, if this makes it easier for the intended users (compiler developers) then there isn't a real downside to it.

@ab @t.p.northover @Gerolf , any thoughts from you?

Yeah, this seems reasonable, but no strong opinions either. I totally empathize with the concerns, and ideally we wouldn't have all this exposed surface, but I'm not sure this meaningfully changes that.

Thanks for the feedback so far!

@echristo Do you still have concerns with this specific change after my updated summary?

[MaskRay]

echristo: I mean, yes, but I'm also happy to be outvoted here :)

Thanks for all of the discussion, it's really appreciated!

Thanks! I will push this in 2 days if nobody objects.

llvm/lib/Support/CommandLine.cpp

bool Option::error(const Twine &Message, StringRef ArgName, raw_ostream &Errs) {
   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
 
-  switch (getNumOccurrencesFlag()) {
-  case Optional:
-    if (NumOccurrences > 1)
-      return error("may only occur zero or one times!", ArgName);
-    break;
-  case Required:
-    if (NumOccurrences > 1)
-      return error("must occur exactly one time!", ArgName);
-    LLVM_FALLTHROUGH;
-  case OneOrMore:
-  case ZeroOrMore:
-  case ConsumeAfter:
-    break;
-  }
-
   return handleOccurrence(pos, ArgName, Value);
 }
 
 // getValueStr - Get the value description string, using "DefaultMsg" if nothing
 // has been specified yet.
 //
 static StringRef getValueStr(const Option &O, StringRef DefaultMsg) {
   if (O.ValueStr.empty())

llvm/test/tools/llvm-libtool-darwin/create-static-lib.test

 ## This test checks that a correct static library is created.
 
 # RUN: yaml2obj %S/Inputs/input1.yaml -o %t-input1.o
 # RUN: yaml2obj %S/Inputs/input2.yaml -o %t-input2.o
 # RUN: llvm-as %S/Inputs/x86_64-osx.ll -o %t-x86_64.bc
 
-# RUN: rm -rf %t.lib
+# RUN: rm -rf %t.lib %t2.lib
 # RUN: llvm-libtool-darwin -static -o %t.lib %t-input1.o %t-input2.o %t-x86_64.bc
+# RUN: llvm-libtool-darwin -static -o %t2.lib -o %t.lib %t-input1.o %t-input2.o %t-x86_64.bc
+# RUN: not ls %t2.lib
 
 ## Check that binaries are present:
 # RUN: llvm-ar t %t.lib | \
 # RUN:   FileCheck %s --check-prefix=CHECK-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
 
 # CHECK-NAMES:      [[PREFIX]]-input1.o
 # CHECK-NAMES-NEXT: [[PREFIX]]-input2.o
 # CHECK-NAMES-NEXT: [[PREFIX]]-x86_64.bc

llvm/test/tools/llvm-libtool-darwin/deterministic-library.test

 
 ## Test values are preserved (with -U):
 # RUN: llvm-libtool-darwin -static -o %t.lib -U %t-input1.o
 # RUN: env TZ=GMT llvm-ar tv %t.lib | FileCheck %s --check-prefix=CHECK-NONDETERMINISTIC
 
 # CHECK-NONDETERMINISTIC:  {{[[:space:]]1995[[:space:]]}}
 
 ## D Flag specified more than once:
-# RUN: not llvm-libtool-darwin -static -o %t.lib %t-input1.o -D -D 2>&1 | \
-# RUN:   FileCheck %s --check-prefix=CHECK-ERROR-D
-
-# CHECK-ERROR-D: for the -D option: may only occur zero or one times!
+# RUN: llvm-libtool-darwin -static -o %t.lib %t-input1.o -D -D 2>&1 | count 0
+# RUN: env TZ=GMT llvm-ar tv %t.lib | FileCheck %s --check-prefix=CHECK-DETERMINISTIC
 
 ## U Flag specified more than once:
-# RUN: not llvm-libtool-darwin -static -o %t.lib %t-input1.o -U -U 2>&1 | \
-# RUN:   FileCheck %s --check-prefix=CHECK-ERROR-U
-
-# CHECK-ERROR-U: for the -U option: may only occur zero or one times!
+# RUN: llvm-libtool-darwin -static -o %t.lib %t-input1.o -U -U 2>&1 | count 0
+# RUN: env TZ=GMT llvm-ar tv %t.lib | FileCheck %s --check-prefix=CHECK-NONDETERMINISTIC
 
 ## Both D and U flags specified:
 # RUN: not llvm-libtool-darwin -static -o %t.lib %t-input1.o -D -U 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=CHECK-ERROR-BOTH
 
 # CHECK-ERROR-BOTH: error: cannot specify both -D and -U flags

llvm/test/tools/llvm-libtool-darwin/filelist.test

 # CHECK-SYMBOLS:      Archive map
 # CHECK-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
 # CHECK-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
 # CHECK-SYMBOLS-NEXT: _x86_64 in [[PREFIX]]-x86_64.bc
 # CHECK-SYMBOLS-EMPTY:
 
 # RUN: rm -rf %t/dirname && mkdir -p %t/dirname
 # RUN: yaml2obj %S/Inputs/input1.yaml -o %t/dirname/%basename_t.tmp-input1.o
-# RUN: echo %basename_t.tmp-input1.o > %t.files.txt
+# RUN: echo %basename_t.tmp-input1.o > %t.files2.txt
 
 ## Passing in dirname:
-# RUN: llvm-libtool-darwin -static -o %t.lib -filelist %t.files.txt,%t/dirname
+# RUN: llvm-libtool-darwin -static -o %t.lib -filelist %t.files2.txt,%t/dirname
 # RUN: llvm-ar t %t.lib | \
 # RUN:   FileCheck %s --check-prefix=DIRNAME-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
 # RUN: llvm-nm --print-armap %t.lib | \
 # RUN:   FileCheck %s --check-prefix=DIRNAME-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
 
 # DIRNAME-NAMES:      [[PREFIX]]-input1.o
 
 # DIRNAME-SYMBOLS:      Archive map
 # DIRNAME-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
 # DIRNAME-SYMBOLS-EMPTY:
 
 ## Passing both -filelist option and object file as input:
-# RUN: llvm-libtool-darwin -static -o %t.lib -filelist %t.files.txt,%t/dirname %t-input2.o
+# RUN: llvm-libtool-darwin -static -o %t.lib -filelist %t.files2.txt,%t/dirname %t-input2.o
 # RUN: llvm-ar t %t.lib | \
 # RUN:   FileCheck %s --check-prefix=REVERSE-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
 # RUN: llvm-nm --print-armap %t.lib | \
 # RUN:   FileCheck %s --check-prefix=REVERSE-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
 
 # REVERSE-NAMES:      [[PREFIX]]-input2.o
 # REVERSE-NAMES-NEXT: [[PREFIX]]-input1.o
 

 ## Check that an error is thrown when the filelist contains a line with only spaces:
 # RUN: echo %t-input2.o > %t.space-line.txt
 # RUN: echo "     " >> %t.space-line.txt
 # RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.space-line.txt 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=FILE-ERROR -DFILE='     ' -DMSG=%errc_ENOENT --strict-whitespace
 
 ## Filelist option specified more than once:
 # RUN: touch %t.list1.txt and %t.list2.txt
-# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.list1.txt -filelist %t.list2.txt 2>&1 | \
-# RUN:   FileCheck %s --check-prefix=DUPLICATE-ERROR
-
-# DUPLICATE-ERROR: for the --filelist option: may only occur zero or one times!
+# RUN: llvm-libtool-darwin -static -o %t.lib -filelist %t.empty-list -filelist %t.files.txt 2>&1
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp

llvm/test/tools/llvm-libtool-darwin/invalid-input-output-args.test

 # NO-OUTPUT: for the -o option: must be specified
 
 ## Missing argument to -o:
 # RUN: not llvm-libtool-darwin -static %t.input -o 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=MISSING
 
 # MISSING: for the -o option: requires a value!
 
-## Passing in two output files:
-# RUN: not llvm-libtool-darwin -static %t.input -o %t.lib1 -o %t.lib2 2>&1 | \
-# RUN:   FileCheck %s --check-prefix=DOUBLE-OUTPUT
-
-# DOUBLE-OUTPUT: for the -o option: may only occur zero or one times!
-
 ## Input file not found:
 # RUN: not llvm-libtool-darwin -static -o %t.lib %t.missing 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=NO-FILE -DFILE=%t.missing -DMSG=%errc_ENOENT
 
 # NO-FILE: error: '[[FILE]]': [[MSG]]
 
 ## Input file is not an object file:
 # RUN: touch %t.invalid

llvm/unittests/Support/CommandLineTest.cpp

ASSERT_EQ(cl::NotHidden, TestOption3.getOptionHiddenFlag())
       << "Hid extra option that should be visable.";
 
   StringMap<cl::Option *> &Map =
       cl::getRegisteredOptions(*cl::TopLevelSubCommand);
   ASSERT_EQ(cl::NotHidden, Map["help"]->getOptionHiddenFlag())
       << "Hid default option that should be visable.";
 }
 
+TEST(CommandLineTest, SetMultiValues) {
+  StackOption<int> Option("option");
+  const char *args[] = {"prog", "-option=1", "-option=2"};
+  EXPECT_TRUE(cl::ParseCommandLineOptions(array_lengthof(args), args,
+                                          StringRef(), &llvm::nulls()));
+  EXPECT_EQ(Option, 2);
+}
+
 TEST(CommandLineTest, SetValueInSubcategories) {
   cl::ResetCommandLineParser();
 
   StackSubCommand SC1("sc1", "First subcommand");
   StackSubCommand SC2("sc2", "Second subcommand");
 
   StackOption<bool> TopLevelOpt("top-level", cl::init(false));
   StackOption<bool> SC1Opt("sc1", cl::sub(SC1), cl::init(false));

mlir/test/Pass/pipeline-options-parsing.mlir

 // RUN: not mlir-opt %s -pass-pipeline='builtin.module(test-module-pass{)' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_1 %s
 // RUN: not mlir-opt %s -pass-pipeline='builtin.module(test-module-pass{test-option=3})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_2 %s
 // RUN: not mlir-opt %s -pass-pipeline='builtin.module(builtin.func(test-options-pass{list=3}), test-module-pass{invalid-option=3})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_3 %s
 // RUN: not mlir-opt %s -pass-pipeline='test-options-pass{list=3 list=notaninteger}' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_4 %s
-// RUN: not mlir-opt %s -pass-pipeline='builtin.func(test-options-pass{list=1,2,3,4 list=5 string=value1 string=value2})' 2>&1 | FileCheck --check-prefix=CHECK_ERROR_5 %s
+// RUN: mlir-opt %s -pass-pipeline='builtin.func(test-options-pass{list=1,2,3,4 list=5 string=value1 string=value2})'
 // RUN: mlir-opt %s -verify-each=false -pass-pipeline='builtin.func(test-options-pass{string-list=a list=1,2,3,4 string-list=b,c list=5 string-list=d string=nested_pipeline{arg1=10 arg2=" {} " arg3=true}})' -test-dump-pipeline 2>&1 | FileCheck --check-prefix=CHECK_1 %s
 // RUN: mlir-opt %s -verify-each=false -test-options-pass-pipeline='list=1 string-list=a,b' -test-dump-pipeline 2>&1 | FileCheck --check-prefix=CHECK_2 %s
 // RUN: mlir-opt %s -verify-each=false -pass-pipeline='builtin.module(builtin.func(test-options-pass{list=3}), builtin.func(test-options-pass{list=1,2,3,4}))' -test-dump-pipeline 2>&1 | FileCheck --check-prefix=CHECK_3 %s
 
 // CHECK_ERROR_1: missing closing '}' while processing pass options
 // CHECK_ERROR_2: no such option test-option
 // CHECK_ERROR_3: no such option invalid-option
 // CHECK_ERROR_4: 'notaninteger' value invalid for integer argument
-// CHECK_ERROR_5: string option: may only occur zero or one times
 
 // CHECK_1: test-options-pass{list=1,2,3,4,5 string=nested_pipeline{arg1=10 arg2=" {} " arg3=true} string-list=a,b,c,d}
 // CHECK_2: test-options-pass{list=1 string= string-list=a,b}
 // CHECK_3: builtin.module(builtin.func(test-options-pass{list=3 string= }), builtin.func(test-options-pass{list=1,2,3,4 string= }))