Differential D47114
[FileCheck] Implement -v and -vv for tracing matches Authored by jdenny on May 19 2018, 4:57 PM.
Details -v prints all directive pattern matches. -vv additionally prints info that might be noise to users but that can To maximize code reuse and to make diagnostics more consistent, this
Diff Detail Event TimelineComment Actions The code changes look pretty tidy. Let's sort out the -vv description and then we can call it good.
Comment Actions 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. Comment Actions Thanks. Of course, this patch depends on the non-overlapping DAG patches, so I'll commit later. Comment Actions 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. Comment Actions @dblaikie @probinson @jdenny In practice it seems hard to modify a command line arg for FileCheck, since it requires changing all tests. 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. Comment Actions 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. Comment Actions @jdenny The precedence should always be given to command line flags, since that's what was specified explicitly vs. lingering in the environment. Comment Actions 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.
If I run $ LIT_FILTER=dtls_test ninja check-msanlit 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? Comment Actions
Yeah, that's why I have originally created the LIT_FILTER variable.
Does this variable exist? I've just grepped through LIT source and have not seen it.
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. Comment Actions 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 agree that's probably best. If we find other use cases, we can handle those later. Comment Actions @jdenny Right, I see now! Comment Actions 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.
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). Comment Actions @jdenny I'm not against exposing LIT_FLAGS.
Comment Actions 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. Comment Actions
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 am not sure what do you mean, I think supporting LIT_FLAGS is a separate feature for a separate use case. Comment Actions 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.
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. | ||||||||||||||||||||||||
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 ...