[PATCH] Lit to use the same rules for XFAIL and REQUIRES, to allow target-architecture-specific tests

[Alp Toker]

On 08/11/2013 14:25, Artyom Skrobov wrote:
> Hello,
>
> At present, the REQUIRES: clause matches only config.available_features, but
> XFAIL: also matches target triple substrings. This makes it easy to XFAIL a
> test on a certain platform; but there's no easy way to do the opposite and
> mark a test as only expected to pass on a certain platform -- short of
> specifying all other known platforms on the XFAIL line.
>
> Our proposed patch makes both clauses share the same rules, both for
> consistency in Lit behaviour, and for ease of setting up
> target-architecture-specific tests.
>
> For example, the XFAIL lines in test/ExecutionEngine/MCJIT/ (see
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130930/190084.
> html) make it very non-obvious which platform(s), if any, the tests should
> *not* fail on. With the proposed change to Lit, the applicability of those
> tests could be made clearer.

Hello Artyom,

I have strong reservations about adding this capability to REQUIRES
globally.

There's already a tendency amongst some contributors to avoid thinking
about portability using XFAIL, but at least with XFAIL we have a chance
to detect those even if it takes a few months or years for somebody to
notice. With REQUIRES, those tests won't even get run.

I can see why your feature would be useful specifically for JIT-related
tests that have a legitimate reason to check the platform triple, but
for everything else in LLVM and clang it's certain to get abused.

There are just too many tests switched off today with REQUIRES because
the author was too lazy to make their feature portable, or because at a
later point someone broke the test and found it easier to disable it
than fix the problem.

There's another problem with the XFAIL scheme that makes it a bad idea
to carry forward. The substring-based triple matching has been causing
trouble:

+        # If this is a part of the target triple, it is available.
+        if feature in self.suite.config.target_triple:
+            return True

"REQUIRES: win" would be suppressed on all platforms other than Windows
and Darwin, but unlike "XFAIL: win", nobody would ever realize the test
was silently suppressed. This is a dangerous failure mode.

So, I agree a solution is needed for the JIT tests, but this isn't it.

As a short/mid-term solution, how about instead handling this with logic
in the lit.cfg under ExecutionEngine where you have access to
config.target_triple?

PS. I'd like to remove the target_triple feature from XFAIL as well at
some point. It has almost no legitimate users.

Alp.

-- 
http://www.nuanti.com
the browser experts