[FileCheck] Implement -v and -vv for tracing matches
ClosedPublic

Authored by jdenny on May 19 2018, 4:57 PM.

Details

Summary

-v prints all directive pattern matches.

-vv additionally prints info that might be noise to users but that can
be helpful to FileCheck developers.

To maximize code reuse and to make diagnostics more consistent, this
patch also adjusts and extends some of the existing diagnostics.
CHECK-NOT failures now report variables uses. Many more diagnostics
now report the check prefix and kind of directive.

Diff Detail

Repository
rL LLVM

The code changes look pretty tidy. Let's sort out the -vv description and then we can call it good.

docs/CommandGuide/FileCheck.rst
107 ↗(On Diff #147689)

I might not get so specific with the list of what gets printed; I'd prefer a broader statement of intent, maybe with specifics as examples. Something like:

Print information helpful in diagnosing internal FileCheck issues; for example, discarded ...

utils/FileCheck/FileCheck.cpp
98 ↗(On Diff #147689)

Again, getting too specific may become a maintenance issue. The general motivation appears to be along the lines of "report things that FileCheck does or does not match, but which normally would not result in any output."

jdenny added inline comments.May 22 2018, 2:10 PM
utils/FileCheck/FileCheck.cpp
98 ↗(On Diff #147689)

Sure. Two questions:

  1. If I move each comment to the relevant VerboseVerbose check, that should eliminate the maintenance burden. That would allow us to remember the rationale behind how I divided things between -v and -vv. Then again, the rationale is now captured in phabricator, so I can just toss the comments if you prefer. What do you think?
  1. Is my understanding of the EOF pattern correct? Specifically, is there any reason why the EOF pattern is needed after a DAG group? Is that just an implementation convenience because DAGs and NOTs are stored together?

Thanks.

probinson added inline comments.May 23 2018, 8:15 AM
utils/FileCheck/FileCheck.cpp
98 ↗(On Diff #147689)
  1. Sounds great.
  1. DAG/NOT ranges are delimited by the surrounding match points; the EOF pattern is an implementation convenience to create an artificial match point at EOF, when the last directive in the file is DAG/NOT. It just simplifies handling the ranges.
jdenny updated this revision to Diff 148239.May 23 2018, 10:11 AM
jdenny marked 4 inline comments as done.

If this doesn't accomplish the changes you had in mind, please let me know.

I've also fixed a test case that was missing a newline at the end of the file, and that required adjusting some CHECK directives because of the EOF match.

This revision is now accepted and ready to land.May 23 2018, 10:45 AM

LGTM

Thanks. Of course, this patch depends on the non-overlapping DAG patches, so I'll commit later.

jdenny set the repository for this revision to rL LLVM.May 24 2018, 12:32 PM
jdenny updated this revision to Diff 153850.Jul 2 2018, 8:02 PM

Rebased.

jdenny updated this revision to Diff 154302.Jul 5 2018, 1:44 PM

This updates -v to make more sense in the case of the new CHECK-EMPTY directive. Without this update, -v reported the match for CHECK-EMPTY as occurring at the preceding newline. That's confusing, especially if the preceding line was also empty because it then seemed like the expected empty line was the preceding line instead. Because CHECK-EMPTY is conceptually a CHECK-NEXT for an empty line, -v now reports the match as occurring after the preceding newline, just as it does for CHECK-NEXT. That is, as for CHECK-NEXT, the match range starts at the empty line, and there is a constraint that there is one preceding newline in the search range.

Still LGTM. And of course still waiting on the other reviews, which I'm doing next.

This revision was automatically updated to reflect the committed changes.

@dblaikie @probinson @jdenny In practice it seems hard to modify a command line arg for FileCheck, since it requires changing all tests.
I recently did a similar option in https://reviews.llvm.org/D49328, but used an environment variable instead.
Independently, lit has -v and -vv options which are also hard to set through the ninja invocation.

Should we try to standardize on an environment variable, e.g. DEBUG=1/2/3/4? Ideally, it could be done automatically when the user does ninja -v, but AFAIK it does not set any environment variable which could be set from LIT.

@dblaikie @probinson @jdenny In practice it seems hard to modify a command line arg for FileCheck, since it requires changing all tests.
I recently did a similar option in https://reviews.llvm.org/D49328, but used an environment variable instead.
Independently, lit has -v and -vv options which are also hard to set through the ninja invocation.

Should we try to standardize on an environment variable, e.g. DEBUG=1/2/3/4? Ideally, it could be done automatically when the user does ninja -v, but AFAIK it does not set any environment variable which could be set from LIT.

A simple scheme that would give full control is LIT_FLAGS and FILECHECK_FLAGS.

I've not studied how lit and FileCheck handle conflicting flags, but some thought should be given to whether precedence is given to environment variables (whatever scheme we choose for those) or flags actually specified on the command line.

@jdenny The precedence should always be given to command line flags, since that's what was specified explicitly vs. lingering in the environment.
The problem is most developers don't invoke LIT directly, they invoke a ninja target, e.g. "ninja check-clang".
In such a pattern there's no way to pass LIT flags through.

@jdenny The precedence should always be given to command line flags, since that's what was specified explicitly vs. lingering in the environment.

Normally I'd agree, unless a use case we're trying to address is overriding what was specified on the command line. In that case, a _FORCE suffix and a warning might be best.

The problem is most developers don't invoke LIT directly, they invoke a ninja target, e.g. "ninja check-clang".
In such a pattern there's no way to pass LIT flags through.

If I run

$ LIT_FILTER=dtls_test ninja check-msan

lit runs only

MemorySanitizer-X86_64 :: dtls_test.c
MemorySanitizer-lld-X86_64 :: dtls_test.c

LIT_FLAGS would work too, right?

I'm on linux. Is there a problem on other platforms?

lit runs only

Yeah, that's why I have originally created the LIT_FILTER variable.

LIT_FLAGS would work too, right?

Does this variable exist? I've just grepped through LIT source and have not seen it.

I'd agree, unless a use case we're trying to address is overriding what was specified on the command line

I think it should be other way around: "normal" command line invocation should not have any extra flags, then environment variable (e.g. BOT config) modifies that, and then if the test authors no semantics better (e.g. it operates over HUUUUGE inputs) they can override the environment variable with flags.

lit runs only

Yeah, that's why I have originally created the LIT_FILTER variable.

LIT_FLAGS would work too, right?

Does this variable exist? I've just grepped through LIT source and have not seen it.

We're miscommunicating. I'm proposing LIT_FLAGS now, and I thought you were arguing it wouldn't be seen by lit because it was invoked via ninja, so I mentioned LIT_FILTER as an example where an environment variable passes through just fine. I didn't realize you were the author of LIT_FILTER.

To clarify, I'm proposing that LIT_FLAGS and FILECHECK_FLAGS environment variables would be a general way to pass flags to lit and FileCheck so that we don't have to keep creating new environment variables.

I'd agree, unless a use case we're trying to address is overriding what was specified on the command line

I think it should be other way around: "normal" command line invocation should not have any extra flags, then environment variable (e.g. BOT config) modifies that, and then if the test authors no semantics better (e.g. it operates over HUUUUGE inputs) they can override the environment variable with flags.

I agree that's probably best. If we find other use cases, we can handle those later.

@jdenny Right, I see now!
So my point was that instead of having many different "better-debug" flags, maybe it's better to have one, and just call it "VERBOSE", or similar.
Passing custom flags to LIT seems like a rather unlikely scenario.

@jdenny Right, I see now!
So my point was that instead of having many different "better-debug" flags, maybe it's better to have one, and just call it "VERBOSE", or similar.

Ah. The desire to have some way at all to pass various flags through ninja to lit and FileCheck resonated more with me, so I responded to that part only. But I agree that succinctness might be helpful too, and it sounds like you want to achieve that through numbered verbosity levels.

An environment variable should probably have a LIT_ or FILECHECK_ prefix, right? If you don't want to have to specify both, perhaps LIT_VERBOSE would tell lit to also set FILECHECK_VERBOSE.

At what verbosity levels would each of FileCheck's -v, -vv, and --dump-input-on-failure (any other flags) be? I suppose the order is -v, --dump-input-on-failure, and then -vv. If that makes sense, then why not rename the latter two to -vv and -vvv? The only trouble is if someone wants -vvv except he finds the large --dump-input-on-failure very annoying.

Passing custom flags to LIT seems like a rather unlikely scenario.

I'm not sure what you mean by custom, but there's already LIT_FILTER/--filter (and other test selection flags) and -v/-vv (and other output format flags).

@jdenny I'm not against exposing LIT_FLAGS.
What I was saying is that in my understanding, most of those scenarios with custom flags would be covered by "a developer is debugging a test and needs more information".
So I think that instead of exposing many variables and custom flags it would be great if we could have, e.g.

  • Default: same as now
  • LIT_DEBUG=1: FileCheck is passed -v, LIT is passed -vv
  • LIT_DEBUG=2: FileCheck is passed FILECHECK_VERBOSE (dumps all input on failure), plus same as above

Maybe:

Default: same as now
LIT_DEBUG=1: LIT is passed -vv
LIT_DEBUG=2: FileCheck is passed -v
LIT_DEBUG=3: FileCheck is passed FILECHECK_VERBOSE (dumps all input on failure), plus same as above

That is, there might be cases where you want to know (1) what command ran and failed but you don't need to see (2) every single match for a long sequence of FileCheck directives. The failing match might be sufficient. What do you think? In general, it's not clear to me how annoying unnecessary verbosity might become here.

Would buildbots be encouraged to set LIT_DEBUG at one of these levels? What about ninja check targets?

LIT_FLAGS and FILECHECK_FLAGS could still offer a way to fine-tune debugging and other flags when running lit via ninja. It seems logical for LIT_DEBUG to be applied first (as a coarse-grained setting), and then *_FLAGS would fine-tune it. That is, if flags are processed left-to-right, then LIT_DEBUG prepends to *_FLAGS. What do you think?

I see you dropped FileCheck -vv from your LIT_DEBUG list. Given that FileCheck -vv is probably rarely useful when debugging tests (as opposed to debugging FileCheck itself), and given that FILECHECK_FLAGS could be used to add it, I think I agree with that decision.

LIT_DEBUG=2: FileCheck is passed -v

In order to support this, can you post a new patch updating this one to have a default of verbose to be set via an environment variable? (FILECHECK_VERBOSE?)

That is, if flags are processed left-to-right, then LIT_DEBUG prepends to *_FLAGS. What do you think?

I am not sure what do you mean, I think supporting LIT_FLAGS is a separate feature for a separate use case.

LIT_DEBUG=2: FileCheck is passed -v

In order to support this, can you post a new patch updating this one to have a default of verbose to be set via an environment variable? (FILECHECK_VERBOSE?)

I think FILECHECK_FLAGS (and potentially LIT_FLAGS) is better than implementing separate environment variables, like FILECHECK_VERBOSE, for every FileCheck (or lit) command-line flag someone might want to control via the environment. I think it's obvious why that would be easier to implement and to remember, and I haven't heard an argument for why one environment variable per flag is better.

I'm fine with LIT_DEBUG as a more convenient way to specify a common set of debug-related flags. However, when LIT_DEBUG is set, lit could pass flags to FileCheck via FILECHECK_FLAGS.

That is, if flags are processed left-to-right, then LIT_DEBUG prepends to *_FLAGS. What do you think?

I am not sure what do you mean, I think supporting LIT_FLAGS is a separate feature for a separate use case.

Yes, but the relevance is as follows. When LIT_DEBUG is set, lit needs some way to pass flags to FileCheck. I suggest FILECHECK_FLAGS as a clean way to do that. If we're going to have FILECHECK_FLAGS, then we should have LIT_FLAGS for consistency, and both will help in use cases where LIT_DEBUG doesn't suffice.

And we must consider what happens when LIT_DEBUG's setting conflicts with other environment variables. That's the motivation for the text about prepending that you quoted above. These kinds of conflicts will happen regardless of whether we take the LIT_FLAGS/FILECHECK_FLAGS approach or the one-environment-variable-per-flag approach. But the above text is assuming the former approach.