Page MenuHomePhabricator

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

Authored by george.karpenkov on Jul 13 2018, 4:04 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

dblaikie added inline comments.Jul 16 2018, 6:15 PM
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 added a subscriber: thopre.Jul 17 2018, 2:29 AM
thopre added inline comments.
llvm/utils/FileCheck/FileCheck.cpp
103 ↗(On Diff #155527)

Spurious change?

1515 ↗(On Diff #155527)

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

george.karpenkov updated this revision to Diff 155933.
george.karpenkov marked an inline comment as done.
george.karpenkov marked an inline comment as done.
george.karpenkov added inline comments.
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.

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

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

@dblaikie Any remaining concerns? Can this be merged?

dblaikie accepted this revision.Jul 20 2018, 11:27 AM

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?)

This revision is now accepted and ready to land.Jul 20 2018, 11:27 AM

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

@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)?

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"

@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.

This revision was automatically updated to reflect the committed changes.

@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!

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.

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