[FileCheck] Provide an option for FileCheck to dump original input to stderr on failure

[george.karpenkov]

Summary

The option can be either set using environment variable (e.g. env FILECHECK_DUMP_INPUT_ON_FAILURE=1 ninja check-fuzzer) or with a FileCheck flag.

This can be extremely useful for debugging, cf. https://groups.google.com/forum/#!topic/llvm-dev/kLrzg8OM_h8 for discussion.

[dblaikie]

llvm/utils/FileCheck/FileCheck.cpp

1514 ↗

(On Diff #155527)

Is this some debugging that should be removed?

1516 ↗

(On Diff #155527)

Perhaps some clear markers should be used before/after the output so it can be easily cut out by tools (like buildbot) or eyeballed by users?

llvm/utils/lit/lit/TestingConfig.py

29 ↗

(On Diff #155527)

I expect this probably shouldn't be the default - maybe on buildbots, but probably not for the average user. And if it should be the default, maybe FileCheck itself should have it as the default, rather than lit here?

Probably worth an llvm-dev discussion if the default is going to change in FileCheck or in lit. (I know there was a discussion about adding the feature - but changing the default behavior is a bit more involved)

[thopre]

llvm/utils/FileCheck/FileCheck.cpp

103 ↗

(On Diff #155527)

Spurious change?

1515 ↗

(On Diff #155527)

Out is a boolean, why not just "if (out && ...)"?

[probinson]

+llvm-commits

[george.karpenkov]

llvm/utils/FileCheck/FileCheck.cpp

1515 ↗

(On Diff #155527)

It's an exit code. Saying if (out) seems to have wrong connotations.
Renamed and change type.

1516 ↗

(On Diff #155527)

Seems like a great idea!

llvm/utils/lit/lit/TestingConfig.py

29 ↗

(On Diff #155527)

This code is not setting the default.
It just tells LIT to propagate the environment variable.
Otherwise, it will not get to FileCheck.

[dblaikie]

Test coverage (in test/FileCheck) would be great!

[george.karpenkov]

@dblaikie Good point!

[lattner]

Just a drive by, but I'm hugely +1 on this change, thank you for tackling it!

[george.karpenkov]

@dblaikie Any remaining concerns? Can this be merged?

[dblaikie]

Looks good to me (maybe add a negative test case to ensure the full text is not dumped when neither the environment variable or command line parameter is used - I imagine most/many FileCheck test cases might be silently passing even if this behavior was enabled by default?). Also maybe the command line argument version of the test should specifically unset the environment variable - to make sure the test is testing what's intended even in cases where someone is setting the env variable on by default (some users may do this, or buildbots)? (worth checking that all the tests pass even if you turn this option on by default - maybe some FileCheck self-tests may fail when this unexpected output turns up?)

[george.karpenkov]

@lattner Thanks!

[george.karpenkov]

@dblaikie done&done. Using =false or =0 is sufficient to get the behavior you describe.

[dblaikie]

In D49328#1170336, @george.karpenkov wrote:

@dblaikie done&done. Using =false or =0 is sufficient to get the behavior you describe.

Sounds good. Please go ahead and commit this if you have commit access - otherwise let me know & I'll commit it for you.

Have you tested whether 'check-llvm' passes if a user is enabling this feature through the environment variable? (ie: they don't cause any accidental failures in FileCheck tests not expecting this output?) & also that enabling this feature via the environment variable does work - passing down through the build system, lit, etc (ie: inject a deliberate failure somewhere in LLVM & see if the failure prints out all the input)?

[george.karpenkov]

Have you tested whether 'check-llvm' passes if a user is enabling this feature through the environment variable

Not on check-llvm, but yes.

also that enabling this feature via the environment variable does work - passing down through the build system, lit

Yes, hence the change in "lit"

[george.karpenkov]

@dblaikie One thing where we could get unexpected behavior is when the environment variable is enabled, but the test does "not FileCheck",
thereby reversing the expected output.
However, I have only seen that done in test/MC/Mips, not in Clang at all, and arguably it's a bad design pattern with double negation,
where using -NOT directives was a way to go.

[dblaikie]

In D49328#1170354, @george.karpenkov wrote:

@dblaikie One thing where we could get unexpected behavior is when the environment variable is enabled, but the test does "not FileCheck",
thereby reversing the expected output.
However, I have only seen that done in test/MC/Mips, not in Clang at all, and arguably it's a bad design pattern with double negation,
where using -NOT directives was a way to go.

Yep, only FileCheck tests themselves should use 'not'. If you could check that these other tests pass with the feature enabled and fix any tests that aren't compatible with this feature, that'd be great!

[george.karpenkov]

I'm not going to change the MIPS tests :P Overall the behavior would be the same unless FILECHECK_DUMP_INPUT_ON_FAILURE is enabled, and even then, the "bad" tests would only generate extra output on the command line.

[dblaikie]

So long as the tests don't fail with the feature enabled, that's probably
good enough.

llvm/trunk/docs/CommandGuide/FileCheck.rst

.. option:: --implicit-check-not check-pattern
   checks. The option allows writing stricter tests without stuffing them with
   ``CHECK-NOT``\ s.
 
   For example, "``--implicit-check-not warning:``" can be useful when testing
   diagnostic messages from tools that don't have an option similar to ``clang
   -verify``. With this option FileCheck will verify that input does not contain
   warnings not covered by any ``CHECK:`` patterns.
 
+.. option:: --dump-input-on-failure
+
+  When the check fails, dump all of the original input.
+
 .. option:: --enable-var-scope
 
   Enables scope for regex variables.
 
   Variables with names that start with ``$`` are considered global and
   remain set throughout the file.
 
   All other variables get undefined after each encountered ``CHECK-LABEL``.

llvm/trunk/test/FileCheck/verbose_mode.txt

+; RUN: not FileCheck -input-file %s %s --check-prefix=CHECK1 --match-full-lines --dump-input-on-failure 2>&1 | FileCheck %s --check-prefix=CHECKERROR --match-full-lines
+; RUN: env FILECHECK_DUMP_INPUT_ON_FAILURE=1 not FileCheck -input-file %s %s --check-prefix=CHECK1 --match-full-lines 2>&1 |  FileCheck %s --check-prefix=CHECKERROR --match-full-lines
+; RUN: env FILECHECK_DUMP_INPUT_ON_FAILURE=1 not FileCheck -input-file %s %s --check-prefix=CHECK1 --match-full-lines --dump-input-on-failure=0 2>&1 |  FileCheck %s --check-prefix=CHECKERRORNOVERBOSE --match-full-lines
+
+hello
+world
+
+; CHECK1: ciao
+; CHECK1-NEXT: world
+
+; CHECKERROR: Full input was:
+; CHECKERROR-NEXT: <<<<<<
+; CHECKERROR: hello
+; CHECKERROR: world
+; CHECKERROR: >>>>>>
+
+; CHECKERRORNOVERBOSE-NOT: <<<<<<

llvm/trunk/utils/FileCheck/FileCheck.cpp

 
 static cl::opt<bool> Verbose("v", cl::init(false),
                              cl::desc("Print directive pattern matches.\n"));
 
 static cl::opt<bool> VerboseVerbose(
     "vv", cl::init(false),
     cl::desc("Print information helpful in diagnosing internal FileCheck\n"
              "issues.  Implies -v.\n"));
+static const char * DumpInputEnv = "FILECHECK_DUMP_INPUT_ON_FAILURE";
+
+static cl::opt<bool> DumpInputOnFailure(
+    "dump-input-on-failure", cl::init(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"));
 
 typedef cl::list<std::string>::const_iterator prefix_iterator;
 
 //===----------------------------------------------------------------------===//
 // Pattern Handling Code.
 //===----------------------------------------------------------------------===//
 
 namespace Check {
int main(int argc, char **argv) {
 
   SmallString<4096> InputFileBuffer;
   StringRef InputFileText = CanonicalizeFile(InputFile, InputFileBuffer);
 
   SM.AddNewSourceBuffer(MemoryBuffer::getMemBuffer(
                             InputFileText, InputFile.getBufferIdentifier()),
                         SMLoc());
 
-  return CheckInput(SM, InputFileText, CheckStrings) ? EXIT_SUCCESS : 1;
+  int ExitCode = CheckInput(SM, InputFileText, CheckStrings) ? EXIT_SUCCESS : 1;
+  if (ExitCode == 1 && DumpInputOnFailure)
+    errs() << "Full input was:\n<<<<<<\n" << InputFileText << "\n>>>>>>\n";
+
+  return ExitCode;
 }

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

def fromdefaults(litConfig):
                                      [os.environ.get('PATH','')]),
             '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']
+                     'TEMPDIR', 'AVRLIT_BOARD', 'AVRLIT_PORT',
+                     'FILECHECK_DUMP_INPUT_ON_FAILURE']
         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':