[FileCheck] Parse command-line options from FILECHECK_OPTS

[jdenny]

Summary

This feature makes it easy to tune FileCheck diagnostic output when
running the test suite via ninja, a bot, or an IDE. For example:

$ FILECHECK_OPTS='-color -v -dump-input-on-failure' \
  LIT_FILTER='OpenMP/for_codegen.cpp' ninja check-clang \
  | less -R

[jdenny]

Ping.

[jdenny]

Ping. Rebased.

This is a small patch to FileCheck that I think could be beneficial throughout LLVM. Does it need an RFC?

[probinson]

llvm/docs/CommandGuide/FileCheck.rst

28 ↗

(On Diff #172659)

This currently implies the command line is done first, which it isn't.
"Options are parsed from the env var and then from the command line" or something along those lines.

llvm/utils/FileCheck/FileCheck.cpp

117 ↗

(On Diff #172659)

For arguments such as "" and nullptr, generally the style is to put the parameter name in a comment so the reader knows what is being omitted. So in this case something like:

..., /*Overview*/ "", /*Errs*/ nullptr, "FILECHECK_OPTS");

[jdenny]

llvm/docs/CommandGuide/FileCheck.rst

28 ↗

(On Diff #172659)

Good point. I'll just reverse that. Thanks.

Either way, as you hinted, the order is only implied here. I'm intentionally avoiding being explicit because I know of no FileCheck option where the order of multiple occurrences should matter. The only option I know of for which order does matter is -D: all definitions of a variable after the first are ignored. My guess is that behavior is unintentional and multiply defined variables should be errors instead. I just wrote a quick patch to make them errors, and I see no new test fails from check-all. I'm happy to submit that patch... or to be advised that I need to rethink this issue.

On the other hand, for the (internal) documentation of ParseCommandLineOptions, I was explicit about the order because ParseCommandLineOptions might one day be reused by some other program where this issue needs to be considered.

llvm/utils/FileCheck/FileCheck.cpp

117 ↗

(On Diff #172659)

Thanks. Just out of curiosity, is this style documented somewhere? I'll make the change regardless.

[jdenny]

Address reviewer comments:

• Add comments for empty-string and nullptr arguments.
• Don't imply command-line options are parsed before FILECHECK_OPTS.

[probinson]

LGTM.
A patch to report an error on multiple -D of the same variable is probably a good thing, particularly since it doesn't work properly.

llvm/utils/FileCheck/FileCheck.cpp

117 ↗

(On Diff #172659)

Interesting, I thought it was but now that I look, I don't see it in the coding standard. I do see this used in the project and I like it, so at some point I'll probably put up a patch to add it to the docs.

[jdenny]

In D53517#1289308, @probinson wrote:

LGTM.
A patch to report an error on multiple -D of the same variable is probably a good thing, particularly since it doesn't work properly.

Thanks. I'll polish that up and submit it eventually.

llvm/trunk/docs/CommandGuide/FileCheck.rst

 
 The ``match-filename`` file specifies the file that contains the patterns to
 match.  The file to verify is read from standard input unless the
 :option:`--input-file` option is used.
 
 OPTIONS
 -------
 
+Options are parsed from the environment variable ``FILECHECK_OPTS``
+and from the command line.
+
 .. option:: -help
 
  Print a summary of command line options.
 
 .. option:: --check-prefix prefix
 
  FileCheck searches the contents of ``match-filename`` for patterns to
  match.  By default, these patterns are prefixed with "``CHECK:``".

llvm/trunk/include/llvm/Support/CommandLine.h

 namespace cl {
 
 //===----------------------------------------------------------------------===//
 // ParseCommandLineOptions - Command line option processing entry point.
 //
 // Returns true on success. Otherwise, this will print the error message to
 // stderr and exit if \p Errs is not set (nullptr by default), or print the
 // error message to \p Errs and return false if \p Errs is provided.
+//
+// If EnvVar is not nullptr, command-line options are also parsed from the
+// environment variable named by EnvVar.  Precedence is given to occurrences
+// from argv.  This precedence is currently implemented by parsing argv after
+// the environment variable, so it is only implemented correctly for options
+// that give precedence to later occurrences.  If your program supports options
+// that give precedence to earlier occurrences, you will need to extend this
+// function to support it correctly.
 bool ParseCommandLineOptions(int argc, const char *const *argv,
                              StringRef Overview = "",
-                             raw_ostream *Errs = nullptr);
+                             raw_ostream *Errs = nullptr,
+                             const char *EnvVar = nullptr);
 
 //===----------------------------------------------------------------------===//
 // ParseEnvironmentOptions - Environment variable option processing alternate
 //                           entry point.
 //
 void ParseEnvironmentOptions(const char *progName, const char *envvar,
                              const char *Overview = "");
 

llvm/trunk/lib/Support/CommandLine.cpp

void cl::ParseEnvironmentOptions(const char *progName, const char *envVar,
   // Parse the value of the environment variable into a "command line"
   // and hand it off to ParseCommandLineOptions().
   TokenizeGNUCommandLine(*envValue, Saver, newArgv);
   int newArgc = static_cast<int>(newArgv.size());
   ParseCommandLineOptions(newArgc, &newArgv[0], StringRef(Overview));
 }
 
 bool cl::ParseCommandLineOptions(int argc, const char *const *argv,
-                                 StringRef Overview, raw_ostream *Errs) {
-  return GlobalParser->ParseCommandLineOptions(argc, argv, Overview,
+                                 StringRef Overview, raw_ostream *Errs,
+                                 const char *EnvVar) {
+  SmallVector<const char *, 20> NewArgv;
+  BumpPtrAllocator A;
+  StringSaver Saver(A);
+  NewArgv.push_back(argv[0]);
+
+  // Parse options from environment variable.
+  if (EnvVar) {
+    if (llvm::Optional<std::string> EnvValue =
+            sys::Process::GetEnv(StringRef(EnvVar)))
+      TokenizeGNUCommandLine(*EnvValue, Saver, NewArgv);
+  }
+
+  // Append options from command line.
+  for (int I = 1; I < argc; ++I)
+    NewArgv.push_back(argv[I]);
+  int NewArgc = static_cast<int>(NewArgv.size());
+
+  // Parse all options.
+  return GlobalParser->ParseCommandLineOptions(NewArgc, &NewArgv[0], Overview,
                                                Errs);
 }
 
 void CommandLineParser::ResetAllOptionOccurrences() {
   // So that we can parse different command lines multiple times in succession
   // we reset all option values to look like they have never been seen before.
   for (auto SC : RegisteredSubCommands) {
     for (auto &O : SC->OptionsMap)

llvm/trunk/test/FileCheck/envvar-opts.txt

+; Create a case that produces a simple diagnostic.
+; RUN: echo foo > %t.in
+; CHECK: foo
+; CHECK: bar
+
+; RUN: FILECHECK_OPTS= \
+; RUN: not FileCheck %s -input-file %t.in 2>&1 \
+; RUN: | FileCheck -check-prefix QUIET %s
+
+; RUN: FILECHECK_OPTS=-v \
+; RUN: not FileCheck %s -input-file %t.in 2>&1 \
+; RUN: | FileCheck -check-prefix VERB %s
+
+; QUIET-NOT: remark: {{CHECK}}: expected string found in input
+; VERB:      remark: {{CHECK}}: expected string found in input

llvm/trunk/utils/FileCheck/FileCheck.cpp

 }
 
 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);
+  cl::ParseCommandLineOptions(argc, argv, /*Overview*/ "", /*Errs*/ nullptr,
+                              "FILECHECK_OPTS");
 
   FileCheckRequest Req;
   for (auto Prefix : CheckPrefixes)
     Req.CheckPrefixes.push_back(Prefix);
 
   for (auto CheckNot : ImplicitCheckNot)
     Req.ImplicitCheckNot.push_back(CheckNot);
 

llvm/trunk/utils/lit/lit/TestingConfig.py

def fromdefaults(litConfig):
             'LLVM_DISABLE_CRASH_REPORT' : '1',
             }
 
         pass_vars = ['LIBRARY_PATH', 'LD_LIBRARY_PATH', 'SYSTEMROOT', 'TERM',
                      'CLANG', 'LD_PRELOAD', 'ASAN_OPTIONS', 'UBSAN_OPTIONS',
                      'LSAN_OPTIONS', 'ADB', 'ANDROID_SERIAL',
                      'SANITIZER_IGNORE_CVE_2016_2143', 'TMPDIR', 'TMP', 'TEMP',
                      'TEMPDIR', 'AVRLIT_BOARD', 'AVRLIT_PORT',
-                     'FILECHECK_DUMP_INPUT_ON_FAILURE']
+                     'FILECHECK_DUMP_INPUT_ON_FAILURE', 'FILECHECK_OPTS']
         for var in pass_vars:
             val = os.environ.get(var, '')
             # Check for empty string as some variables such as LD_PRELOAD cannot be empty
             # ('') for OS's such as OpenBSD.
             if val:
                 environment[var] = val
 
         if sys.platform == 'win32':