[FileCheck] Given multiple -dump-input, prefer most verbose

[jdenny]

Summary

Problem: FILECHECK_OPTS was implemented so that a test runner, such
as a bot, can specify FileCheck debugging options, such as
-dump-input=fail. However, some existing test suites have FileCheck
calls that already specify -dump-input=fail or -dump-input=always.
Without this patch, such tests fail under such a test runner because
FileCheck doesn't accept multiple occurrences of -dump-input.

Solution: This patch permits multiple occurrences of -dump-input by
assigning precedence to its values in the following descending order:
help, always, fail, and never. That is, any occurrence of
help always obtains help, and otherwise the behavior is similar to
-v vs. -vv in that the option specifying the greatest verbosity
has precedence.

Rationale: My justification for the new behavior is as follows. I
have not experienced use cases where, either as a test runner or as a
test author, I want to limit the permitted debugging verbosity
(except as a test author in FileCheck's or lit's test suites where the
FileCheck debugging output itself is under test, but the solution
there is env FILECHECK_OPTS=, and I imagine we should use the same
solution anywhere else this need might occur). Of course, as either a
test runner or test author, it is useful to increase debugging
verbosity.

[probinson]

I know we've had previous differences of opinion on how many combinations to test, and I'll bring it up again. I think we really need:

• only one case where the two options are the same; that demonstrates a repeated option is not an error.
• minimum cases showing the higher one gets picked: never/fail, fail/always, always/help. By transitivity we conclude the others are fine.
• only one case for the other order, showing it's not first (or last) always wins.
• only two cases of environment/command, showing it's not one or the always wins.

So a total of 7 cases, rather than the 16 you have here.

The code is fine, the tests are fine, I think we just don't need to spend as much resource on the test here.

[jdenny]

@probinson : I followed your testing strategy, which I agree is better. Thanks for the quick review!

[probinson]

LGTM

llvm/test/FileCheck/dump-input-enable.txt

 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck -input-file %t.err -color %t.check -dump-input=help \
 ; RUN: | FileCheck %s -check-prefix=HELP
 
 ; No positional arg.
 ; RUN: %ProtectFileCheckOutput FileCheck -dump-input=help \
 ; RUN: | FileCheck %s -check-prefix=HELP
 
-HELP-NOT: {{.}}
-HELP: The following description was requested by -dump-input=help
-HELP: try{{.*}}-color
-HELP-NOT: {{.}}
-
 ;--------------------------------------------------
 ; Check -dump-input=never.
 ;
 ; Include the case without -v, which isn't covered elsewhere.
 ;--------------------------------------------------
 
+; FileCheck success, no -v => no dump, no trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
 ; RUN:           -match-full-lines -dump-input=never 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -allow-empty \
 ; RUN:             -check-prefixes=NOTRACE,NODUMP
 
+; FileCheck fail, no -v => no dump, no trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -input-file %t.err %t.check -check-prefix=CHECK \
 ; RUN:               -match-full-lines -dump-input=never 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -check-prefixes=NOTRACE,ERR,NODUMP
 
+; FileCheck success, -v => no dump, trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
 ; RUN:           -match-full-lines -dump-input=never -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -check-prefixes=TRACE,NODUMP
 
+; FileCheck fail, -v => no dump, trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -input-file %t.err %t.check -check-prefix=CHECK \
 ; RUN:               -match-full-lines -dump-input=never -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -check-prefixes=TRACE,ERR,NODUMP
 
 ;--------------------------------------------------
 ; Check no -dump-input, which defaults to never.
 ;--------------------------------------------------
 
+; FileCheck success, -v => no dump, trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
 ; RUN:           -match-full-lines -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -check-prefixes=TRACE,NODUMP
 
+; FileCheck fail, -v => no dump, trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -input-file %t.err %t.check -check-prefix=CHECK \
 ; RUN:               -match-full-lines -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -check-prefixes=TRACE,ERR,NODUMP
 
 ;--------------------------------------------------
 ; Check -dump-input=fail.
 ;
 ; Include the case without -v, which isn't covered elsewhere.
 ;--------------------------------------------------
 
+; FileCheck success, no -v => no dump, no trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
 ; RUN:           -match-full-lines -dump-input=fail 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -allow-empty \
 ; RUN:             -check-prefixes=NOTRACE,NODUMP
 
+; FileCheck fail, no -v => dump, no trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -input-file %t.err %t.check -check-prefix=CHECK \
 ; RUN:               -match-full-lines -dump-input=fail 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -check-prefixes=NOTRACE,ERR,DUMP-ERR
 
+; FileCheck success, -v => no dump, no trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
 ; RUN:           -match-full-lines -dump-input=fail -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -allow-empty \
 ; RUN:             -check-prefixes=NOTRACE,NODUMP
 
+; FileCheck fail, -v => dump, no trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -input-file %t.err %t.check -check-prefix=CHECK \
 ; RUN:               -match-full-lines -dump-input=fail -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines \
 ; RUN:                -check-prefixes=NOTRACE,ERR,DUMP-ERR,DUMP-ERR-V
 
 ;--------------------------------------------------
 ; Check -dump-input-on-failure.
 ;--------------------------------------------------
 
+; Command-line option.
+
+; FileCheck success, -v => no dump, no trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
 ; RUN:           -match-full-lines -dump-input-on-failure -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -allow-empty \
 ; RUN:             -check-prefixes=NOTRACE,NODUMP
 
+; FileCheck fail, -v => dump, no trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -input-file %t.err %t.check -check-prefix=CHECK \
 ; RUN:               -match-full-lines -dump-input-on-failure -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines \
 ; RUN:                -check-prefixes=NOTRACE,ERR,DUMP-ERR,DUMP-ERR-V
 
+; FILECHECK_DUMP_INPUT_ON_FAILURE=1.
+
+; FileCheck success, -v => no dump, no trace.
 ; RUN: %ProtectFileCheckOutput FILECHECK_DUMP_INPUT_ON_FAILURE=1 \
 ; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
 ; RUN:           -match-full-lines -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -allow-empty \
 ; RUN:                -check-prefixes=NOTRACE,NODUMP
 
+; FileCheck fail, -v => dump, no trace.
 ; RUN: %ProtectFileCheckOutput FILECHECK_DUMP_INPUT_ON_FAILURE=1 \
 ; RUN: not FileCheck -input-file %t.err %t.check -check-prefix=CHECK \
 ; RUN:               -match-full-lines -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines \
 ; RUN:                -check-prefixes=NOTRACE,ERR,DUMP-ERR,DUMP-ERR-V
 
 ;--------------------------------------------------
 ; Check -dump-input=always.
 ;--------------------------------------------------
 
+; FileCheck success, -v => dump, no trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
 ; RUN:           -match-full-lines -dump-input=always -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines -check-prefixes=NOTRACE,DUMP-OK
 
+; FileCheck fail, -v => dump, no trace.
 ; RUN: %ProtectFileCheckOutput \
 ; RUN: not FileCheck -input-file %t.err %t.check -check-prefix=CHECK \
 ; RUN:               -match-full-lines -dump-input=always -v 2>&1 \
 ; RUN: | FileCheck %s -match-full-lines \
 ; RUN:                -check-prefixes=NOTRACE,ERR,DUMP-ERR,DUMP-ERR-V
 
+;--------------------------------------------------
+; Check multiple -dump-input options.
+;
+; This ocurrs most commonly when a test author specifies -dump-input on a
+; specific FileCheck call while a test runner specifies -dump-input in
+; FILECHECK_OPTS, but check the behavior generally.
+;
+; "help" has precedence, and then the most verbose value wins.  The most
+; common combinations involve "fail" and "always", so test those the most.
+;--------------------------------------------------
+
+;- - - - - - - - - - - - - - - - - - - - - - - - -
+; Check duplicate.
+;- - - - - - - - - - - - - - - - - - - - - - - - -
+
+; fail, fail => fail (FileCheck fail => dump)
+; RUN: %ProtectFileCheckOutput \
+; RUN: not FileCheck -input-file %t.err %t.check -check-prefix=CHECK \
+; RUN:               -match-full-lines -dump-input=fail -dump-input=fail -v \
+; RUN:               2>&1 \
+; RUN: | FileCheck %s -match-full-lines \
+; RUN:                -check-prefixes=NOTRACE,ERR,DUMP-ERR,DUMP-ERR-V
+
+;- - - - - - - - - - - - - - - - - - - - - - - - -
+; Check precedence.
+;- - - - - - - - - - - - - - - - - - - - - - - - -
+
+; help, always => help
+; RUN: %ProtectFileCheckOutput \
+; RUN: FileCheck -input-file %t.err -color %t.check \
+; RUN:           -dump-input=help -dump-input=always \
+; RUN: | FileCheck %s -check-prefix=HELP
+
+; always, fail => always (FileCheck success => dump)
+; RUN: %ProtectFileCheckOutput \
+; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
+; RUN:           -match-full-lines -dump-input=always -dump-input=fail \
+; RUN:           -v 2>&1 \
+; RUN: | FileCheck %s -match-full-lines -check-prefixes=NOTRACE,DUMP-OK
+
+; fail, never => fail (FileCheck fail => dump)
+; RUN: %ProtectFileCheckOutput \
+; RUN: not FileCheck -input-file %t.err %t.check -check-prefix=CHECK \
+; RUN:               -match-full-lines -dump-input=fail -dump-input=never -v \
+; RUN:               2>&1 \
+; RUN: | FileCheck %s -match-full-lines \
+; RUN:                -check-prefixes=NOTRACE,ERR,DUMP-ERR,DUMP-ERR-V
+
+;- - - - - - - - - - - - - - - - - - - - - - - - -
+; Check that order doesn't matter.
+;- - - - - - - - - - - - - - - - - - - - - - - - -
+
+; fail, always => always (FileCheck success => dump)
+; RUN: %ProtectFileCheckOutput \
+; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
+; RUN:           -match-full-lines -dump-input=fail -dump-input=always \
+; RUN:           -v 2>&1 \
+; RUN: | FileCheck %s -match-full-lines -check-prefixes=NOTRACE,DUMP-OK
+
+;- - - - - - - - - - - - - - - - - - - - - - - - -
+; Check that FILECHECK_OPTS isn't handled differently.
+;- - - - - - - - - - - - - - - - - - - - - - - - -
+
+; always, fail => always (FileCheck success => dump)
+; RUN: %ProtectFileCheckOutput FILECHECK_OPTS=-dump-input=always \
+; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
+; RUN:           -match-full-lines -dump-input=fail -v 2>&1 \
+; RUN: | FileCheck %s -match-full-lines -check-prefixes=NOTRACE,DUMP-OK
+
+; fail, always => always (FileCheck success => dump)
+; RUN: %ProtectFileCheckOutput FILECHECK_OPTS=-dump-input=fail \
+; RUN: FileCheck -input-file %t.good %t.check -check-prefix=CHECK \
+; RUN:           -match-full-lines -dump-input=always -v 2>&1 \
+; RUN: | FileCheck %s -match-full-lines -check-prefixes=NOTRACE,DUMP-OK
+
 ; END.
 
 ;--------------------------------------------------
-; Check the output for all cases that actually process directives.
+; Check the output.
 ;--------------------------------------------------
 
+; HELP-NOT: {{.}}
+; HELP: The following description was requested by -dump-input=help
+; HELP: try{{.*}}-color
+; HELP-NOT: {{.}}
+
 ; Trace is sometimes suppressed.
 ; TRACE:       {{.*}}remark:{{.*}}
 ; NOTRACE-NOT: remark:
 
 ; Error diagnostics are never suppressed.
 ; ERR: {{.*}}error:{{.*}}
 
 ; NODUMP-NOT: <<<<<<

llvm/utils/FileCheck/FileCheck.cpp

 static cl::opt<bool> DumpInputOnFailure(
     "dump-input-on-failure",
     cl::init(std::getenv(DumpInputEnv) && *std::getenv(DumpInputEnv)),
     cl::desc("Dump original input to stderr before failing.\n"
              "The value can be also controlled using\n"
              "FILECHECK_DUMP_INPUT_ON_FAILURE environment variable.\n"
              "This option is deprecated in favor of -dump-input=fail.\n"));
 
+// The order of DumpInputValue members affects their precedence, as documented
+// for -dump-input below.
 enum DumpInputValue {
   DumpInputDefault,
-  DumpInputHelp,
   DumpInputNever,
   DumpInputFail,
-  DumpInputAlways
+  DumpInputAlways,
+  DumpInputHelp
 };
 
-static cl::opt<DumpInputValue> DumpInput(
-    "dump-input", cl::init(DumpInputDefault),
+static cl::list<DumpInputValue> DumpInputs(
+    "dump-input",
     cl::desc("Dump input to stderr, adding annotations representing\n"
-             " currently enabled diagnostics\n"),
+             "currently enabled diagnostics.  When there are multiple\n"
+             "occurrences of this option, the <value> that appears earliest\n"
+             "in the list below has precedence.\n"),
     cl::value_desc("mode"),
     cl::values(clEnumValN(DumpInputHelp, "help",
                           "Explain dump format and quit"),
-               clEnumValN(DumpInputNever, "never", "Never dump input"),
+               clEnumValN(DumpInputAlways, "always", "Always dump input"),
                clEnumValN(DumpInputFail, "fail", "Dump input on failure"),
-               clEnumValN(DumpInputAlways, "always", "Always dump input")));
+               clEnumValN(DumpInputNever, "never", "Never dump input")));
 
 typedef cl::list<std::string>::const_iterator prefix_iterator;
 
 
 
 
 
 

 int main(int argc, char **argv) {
   // Enable use of ANSI color codes because FileCheck is using them to
   // highlight text.
   llvm::sys::Process::UseANSIEscapeCodes(true);
 
   InitLLVM X(argc, argv);
   cl::ParseCommandLineOptions(argc, argv, /*Overview*/ "", /*Errs*/ nullptr,
                               "FILECHECK_OPTS");
+  DumpInputValue DumpInput =
+      DumpInputs.empty()
+          ? DumpInputDefault
+          : *std::max_element(DumpInputs.begin(), DumpInputs.end());
   if (DumpInput == DumpInputHelp) {
     DumpInputAnnotationHelp(outs());
     return 0;
   }
   if (CheckFilename.empty()) {
     errs() << "<check-file> not specified\n";
     return 2;
   }