[RFC] lit's REQUIRES and triples

Background

Lit has three optional directives that relate to test selection and status: REQUIRES, UNSUPPORTED, and XFAIL. Each of these takes a comma-separated list of boolean expressions. Values in these expressions are typically “feature keywords” defined by lit and the appropriate configuration files such as lit.cfg.py. Values can also be regular expressions that are matched against those keywords.

For UNSUPPORTED and XFAIL only, values can also be the default target triple used for the build, or any substring of that triple (note: not a regular expression).

REQUIRES disables the test (it will not be run) if any expression is false. In other words, REQUIRES requires all expressions to be true.

UNSUPPORTED disables the test if any expression is true.

XFAIL doesn’t disable the test, but causes lit to expect the test to fail, if any expression is true. “XFAIL: *” is a special notation for failing always.

Inconsistencies

There are two obvious inconsistencies in how these expressions are handled.

  1. REQUIRES doesn’t support the triple.

  2. Triple matching uses substrings rather than regular expressions.

The second one is really a matter of history; triple-matching was added before regular expression support was added, while substring matching on the triple was easy to do and filled the need. Nobody seems to be entirely clear on why REQUIRES never got triple support.

About a year ago, D107162 attempted to fix the first inconsistency, but had to be reverted because one bot insisted on failing for no apparent reason (at least, I was never able to figure it out).

Subsequently, @ldionne pointed out the second inconsistency, and that some subprojects (libcxx, libcxxabi, libunwind) use a different tactic: add a feature keyword target=<triple> and then use regular expression matching on that keyword, instead of substring matching on a semi-magical triple keyword. For example, where an llvm test might say UNSUPPORTED: darwin a libcxx test would say UNSUPPORTED: target={{.*}}darwin{{.*}}
This is admittedly a little more verbose; but, it helps avoid collisions between triple components and other keywords, and makes it more obvious that the condition is about the target rather than the host.

Consequences of the inconsistencies

I’ve identified three consequences, one of which is pretty bad (although fortunately rare) and the other two cause unnecessary cruft and complication.

Incorrectly specified tests

Because REQUIRES doesn’t support the triple, but the other directives do, on occasion people will write a test that puts a triple substring into the REQUIRES directive. The consequence is that the test is Unsupported everywhere; the test is never run. In the course of attempting to land D107162 I discovered three tests that had made this mistake; see the review for details.

A test that never runs (a) doesn’t provide the expected coverage; it is an example of a Rotten Green Test; and (b) will likely bit-rot, so that when it is discovered and re-enabled it is likely to need additional changes to run properly.

Weirdly specified tests

Given that there aren’t actually that many tests that fell into the “REQUIRES doesn’t support triples” trap, what do people do instead?

One workaround is to replace something like REQUIRES: darwin with UNSUPPORTED: !darwin which does work, although it reads very unnaturally, due to the double negative. I’ve had to make this suggestion on a variety of code reviews.

Unnecessary cruft in config files

Another workaround is to add code to your lit config file that adds the keyword that you really think should have been there in the first place. So you might see things like

if re.match(r'^.*-apple-darwin.*', config.target_triple):
    config.available_features.add("darwin")

and then you can go ahead and use REQUIRES: darwin just like you could have if lit were consistent about triples.

Planned work

  1. Add the triple as a feature keyword in base lit. TBD whether this would be the bare triple, or target=<triple> a-la libcxx and friends. See Questions at the end.

  2. Convert tests that use triple-substring matching to use regular-expression matching, in preparation for removing the triple-substring matching; the goal is for this to be NFC, with all tests running (or not) in the same situations that they did with substring matching. I’ve surveyed all subprojects with a test\lit* file, and the list of tests where this is required seems to be tractable (under a hundred, and mostly in clang). Some subprojects add plenty of their own triple-based keywords and any work there can be deferred (see Future work).

  3. Remove the triple-substring matching from UNSUPPORTED and XFAIL. Again the goal is NFC.

Future work

(I’m unlikely to do any of this myself.)

Go through projects that are adding triple-based keywords and see about replacing them with regular expressions. compiler-rt does this rather a lot; it adds <arch>-target-arch and <arch>-<os> feature keywords. The ubsan subtree adds arch=<arch> which is of course redundant with <arch>-target-arch and of course both would be superseded by <arch>-{{.*}} or `target=-{{.*}} after the above work is done.

In some cases this replacement is probably not desirable; some config files (including compiler-rt’s) do things like add a ppc keyword for any of the powerpc triple spellings, and that kind of shortcut is likely worth keeping. Arguably lit itself should define that kind of shortcut but that’s a separate task that I’m not going to pursue.

Questions

  1. Is this worth doing? The discussion in D107162 converged on Yes, which included comments from @delcypher, @jdenny-ornl, and Phab user yln (I don’t know their Github equivalent tag).

  2. Should it be <triple> or target=<triple> ? As mentioned above, three subprojects (that I know of) already use the latter, and this would be just a matter of hoisting that up into lit itself. On the other hand, a moderate number of tests already use a full triple, and they would not require modification if we went with the former.

4 Likes

Thank you so much for bringing this up!

Yes, IMO it’s totally worth doing. It’s not too hard, and it will increase the consistency of all our tests, which is great. The potential for rotten green tests, in particular, is IMO rather concerning and we should strive to make Lit as difficult to use incorrectly as possible.

I’m obviously biased, but given that several projects and platforms have a need for running tests on a different machine than the build host, IMO it makes sense to be explicit about what the triple means. Indeed, in libc++ we also have a feature like buildhost=<something>, and we use it to determine the capabilities of the build host itself (e.g. does it have clang-format?).

I think that using target=<FOO> would also make this transition easier because whenever you’d see an old <triple> feature, you’d know right away that it can be converted to the new target=<FOO> Lit feature. But whichever way we end up going, I think this is worth doing and libc++/libc++abi/libunwind will follow.

+1 for target=<triple> which makes the intention clearer.

“target” looks good, because for a cross-compiler it would be good to be explicit when something refers to the target and when the host.

@ldionne I’m not seeing where libcxx actually sets the target=<triple> feature, can you point me in the right direction?

FTR, this project is turning up other interesting tidbits. I’ve found a few misspelled keywords, and also that some tests are using REQUIRES: disabled to turn themselves off–more efficient than XFAIL: * although the latter has advantages too (automatically reports fixes as XPASS). I’ve worked up a patch that will report not-defined-anywhere keywords, which might be its own useful thing to have in lit.

Here: llvm-project/params.py at main · llvm/llvm-project · GitHub

Oh yeah, I’m sure you’ll find a lot of interesting stuff if you start investigating. I remember finding a good number of surprises when I investigated libc++'s own tests when I introduced the new test format a few years ago.

FTR: While pursuing this project, I wanted to proactively identify the places that were currently using triple-substrings so I’d have some idea what tests needed to be updated to use regexes.

I came up with a patch that, instead of maintaining a set() of keywords, uses a dict() mapping keywords to True/False. Instead a lit.config.py file saying if foo: available_features.add('bar') it does available_features['bar'] = bool(foo). Then, when evaluating an expression, lit can know whether the keyword can ever be enabled, and throw an exception when it can’t. This is the patch that has let me put in my recent patches to fix typos and whatnot in a variety of lit tests.

Upstreaming this would need some prerequisite changes to how keywords are used. Like the soon-to-be-live target=<triple> feature, other runtime-parameter kinds of features would want to start using a keyword=value format. For example:

  1. <arch>-registered-target would become registered-target=<arch>
  2. system-<os> would become system=<os> or more likely host=<os>
  3. a few other similar cases would be renamed

Then my bad-keyword-detection patch could trim off everything after the ‘=’ and poof we’d have typo-detection in all our REQUIRES/UNSUPPORTED/XFAIL clauses. The trimming is necessary because lit can’t a-priori know what all the possible values are; but the ‘=’ signifier seems like a good choice for bounding what we can detect.

This would be a long overhaul, of course, but typos in test files mean the tests tend to rot (I filed two issues due to that just this past week or so). Rotting tests aren’t good.

2 Likes

Patch to describe target=<triple> in the testing documentation is available: D139869

At some point I will post a note in the Announcements topic about this. It may take a while for people to get in the habit of using the new scheme.

I believe I have found and updated all the lit tests that depended on the triple-substring feature, and I’ve committed D139869 as mentioned in the previous comment. There is one remaining review for an update to some Polly tests, D139728, however that update corrects misuse of triple components in the REQUIRES clause, and therefore those tests never ran. I don’t view that as a blocker.

The remaining steps are to post something in the Announcements category, and actually remove the triple-substring feature from lit. That removal will catch any places that I missed during my passes over the project’s lit tests :slight_smile:

Maybe worth putting something in the Release Notes as well? This change will affect downstream tests that depend on the feature, as well as non-LLVM users of lit (I’m told there may be some).

I’m inclined to do the Announcements post today or tomorrow, but put off the final removal of the lit feature until the new year. I’m on holiday starting the 23rd myself, so I won’t be around to deal with any fallout that doesn’t get reported right away.

For completeness, the announcement is at New way to check target triple in lit tests - Announcements - LLVM Discussion Forums

I’ve posted the patch to remove triple-substring checking from lit as :gear: D141007 [lit] Stop supporting triple substrings in UNSUPPORTED and XFAIL (llvm.org)