[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

Is this some debugging that should be removed?

1516

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

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

Spurious change?

1515

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

[probinson]

+llvm-commits

[george.karpenkov]

llvm/utils/FileCheck/FileCheck.cpp

1515

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

1516

Seems like a great idea!

llvm/utils/lit/lit/TestingConfig.py

29

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/utils/FileCheck/FileCheck.cpp

 
 static cl::opt<bool> AllowDeprecatedDagOverlap(
     "allow-deprecated-dag-overlap", cl::init(false),
     cl::desc("Enable overlapping among matches in a group of consecutive\n"
              "CHECK-DAG directives.  This option is deprecated and is only\n"
              "provided for convenience as old tests are migrated to the new\n"
              "non-overlapping CHECK-DAG implementation.\n"));
 
+static const char * DumpInputEnv = "FILECHECK_DUMP_INPUT_ON_FAILURE";
+
+static cl::opt<const char*> 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 {
 enum CheckType {
   CheckNone = 0,
   CheckPlain,
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;
+  bool Out = CheckInput(SM, InputFileText, CheckStrings) ? EXIT_SUCCESS : 1;
+  errs() << "Var value = " << std::getenv(DumpInputEnv);
+  if (Out == 1 && std::getenv(DumpInputEnv))
+    errs() << "Full input:\n" << InputFileText;
+
+  return Out;
 }

llvm/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':