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.
-
REQUIRES doesn’t support the triple.
-
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
-
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. -
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). -
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
-
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).
-
Should it be
<triple>ortarget=<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.
D141007 [lit] Stop supporting triple substrings in UNSUPPORTED and XFAIL (llvm.org)