In D47114#1189632, @george.karpenkov wrote:

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

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

In D47114#1170688, @george.karpenkov wrote:

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

Ping. Rebased.

In D47114#1170522, @george.karpenkov wrote:

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.

In D47114#1170460, @george.karpenkov wrote:

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

In D47114#1170378, @george.karpenkov wrote:

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

In D48986#1170073, @probinson wrote:

Sorry, lost track of this one. One comment suggestion and LGTM.

Ping. Rebased.

Removed config.test_exec_root, which is unique to each config by default.

In D49249#1161704, @morehouse wrote:

The other test configs don't define config.test_exec_root at all. Do we need it?

Rebased. There's 1 new test requiring -allow-deprecated-dag-overlap.

In D47106#1157397, @probinson wrote:

The mechanical changes to the tests are actually pretty easy to skim through, given the way Phabricator's web interface does change highlighting. The three you cited are all fine.

In D47106#1156353, @probinson wrote:

Given that you feel so very strongly about it, and that this is a tool that really has absolutely zero exposure outside our own test system, I'm willing to say that erring on the side of extra testing is acceptable here.

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.

In D47106#1110343, @jdenny wrote:

In D47106#1110240, @probinson wrote:

At that point, I think the behavior becomes tractable. The first DAG group finds what it finds, the NOT forces no reordering therefore the second DAG group's range starts at the endpoint of the first group's matches. After the second DAG group runs, the NOT's range is from the endpoint of the first group's matches to the earliest of the second group's matches.

How does that all sound?

The main question remaining for me is about the start of the second DAG group's match range and thus whether reordering detection happens.

In D47106#1150943, @probinson wrote:

Regarding the DAG-NOT-DAG reordering diagnostic: Rereading the comments, and the llvm-dev discussion, are we agreed that we can eliminate it? That is, the first DAG group defines some match range, the second DAG group defines a disjoint match range, and the NOT group looks at the text from the end of the first match range to the start of the second match range. No attempt to diagnose ordering problems.

Ping. Rebased.

Rebased.

Rebased.

Rebased. There are 5 new tests requiring -allow-deprecated-dag-overlap.

Rebased. The number of tests broken by this change in functionality continues to grow. Can we move forward?

%T should also be deprecated (or removed) below, where it's documented incorrectly:

Rebased. Ping.

Rebased. Two more tests are affected:

Rebased.

Rebased. Ping.

Would there be any harm in committing this patch as is? If we decide to remove all support for windows cmd.exe, that should be a separate patch anyway.

In D44598#1115251, @jdenny wrote:

In D44598#1115242, @delcypher wrote:

A high level question. Why are we even testing using cmd.exe with an external shell? IIRC that's not a use case LLVM uses for Windows. Instead
the internal shell is used on Windows. Is this (cmd.exe as an external shell) really something we want to support? I don't see anything desirable about supporting cmd.exe as an
external shell because that means it can be very easy to write non-portable shell tests.

I'm not a windows developer. I fixed that because others reverted my patch when it broke windows buildbots. Someone else will have to answer. Here's the bugzilla, in case that helps:

https://bugs.llvm.org/show_bug.cgi?id=37239

In D44598#1115242, @delcypher wrote:

In D44598#1115233, @jdenny wrote:

In D44598#1115229, @lebedev.ri wrote:

In D44598#1115221, @jdenny wrote:

Ping.

What seems to be the problem?
This was accepted before, reverted due to windows bot breakage,
https://reviews.llvm.org/D44598#1091098 suggests that breakage is fixed.
Just commit again?

There was enough of a code change that I thought the reviewer would want to take another look. Is that not the right approach?

A high level question. Why are we even testing using cmd.exe with an external shell? IIRC that's not a use case LLVM uses for Windows. Instead
the internal shell is used on Windows. Is this (cmd.exe as an external shell) really something we want to support? I don't see anything desirable about supporting cmd.exe as an
external shell because that means it can be very easy to write non-portable shell tests.

In D44598#1115229, @lebedev.ri wrote:

In D44598#1115221, @jdenny wrote:

Ping.

What seems to be the problem?
This was accepted before, reverted due to windows bot breakage,
https://reviews.llvm.org/D44598#1091098 suggests that breakage is fixed.
Just commit again?

Ping.

In D47106#1110240, @probinson wrote:

(Replying here instead of inline as the thread is getting kind of long.)

Sorry, I didn't realize FileCheck would try to detect and explicitly diagnose reordering DAG across NOT. That seems like more trouble than it's worth. NOT is documented to enforce ordering, and we can make it just do that, and if you mess up then your test fails by not matching rather than telling you something is in the wrong order. (Then you can use -v or -vv to work out what happened, if you need to.)

Once we define the DAGs as separated into distinct groups by a NOT, I think it's conceptually straightforward to say each DAG group has a "matching range" from the start of the earliest match to the end of the latest match. Two DAG groups can't have overlapping match ranges (just like you are making individual DAG matches not overlap).