[lit] Stop supporting triple substrings in UNSUPPORTED and XFAIL

[probinson]

Summary

AFAICT all in-tree lit tests have been converted to use target=...
and so there is no longer any need for triples being special.
Some project config files still define their own features based on
the triple, but those are normal feature words (although now are
redundant with target= checks).

https://discourse.llvm.org/t/rfc-lits-requires-and-triples/66041

[probinson]

Ping

[probinson]

Ping. Add @yln

[yln]

Thanks so much for this effort and seeing it through! I think this materially improves LIT feature handling and reduces confusion.

I had just one question: what kind of testing did we do / how did we discover all uses of the triple substring matches?
To reduce churn and the probability that this gets reverted, can we add a short explanation to the commit message that explains how patterns like UNSUPPORTED: arm64 should be changed and maybe a link to the commit that did this for an existing example.

Thanks so much again!

[ldionne]

I agree with @yln , thanks a lot for pushing this through.

One thing to keep in mind here is that folks who rebase patches on top of this patch may get bitten by this issue for a couple weeks, so it may be worth grepping for suspicious patches from time to time, or turning this into an explicit error instead (just for a few weeks).

[probinson]

In D141007#4059777, @yln wrote:

I had just one question: what kind of testing did we do / how did we discover all uses of the triple substring matches?

I wrote a patch to lit that would complain about undefined features. This was quite a bit of work as it mean changing config.available_features from a list to a dict, and then every place that used to do something like

if some_condition:
  config.available_features.add('name')

became instead

config.available_features['name'] = bool(some_condition)

and then BooleanExpression would complain if it found a (non-regex) feature name that wasn't in the dict. Some of these were simply typos, which I fixed; some were made-up words that meant "false"; and the rest (some hundreds, I don't remember the exact count) were triple substrings. I converted these to use target= over the course of about 50-60 individual patches.

I plan to run that again, now that everything _should_ be converted, to catch the last few, and then commit this patch.

The wonderful thing is, when this patch goes in, there can be no silent problems. Any remaining "mistakes" will actually cause test failures and be noticed. Anything that was XFAIL: substring will be treated as XFAIL: false and the test will fail. Anything that was UNSUPPORTED: substring will be treated as UNSUPPORTED: false and the test will run (and presumably fail).

To reduce churn and the probability that this gets reverted, can we add a short explanation to the commit message that explains how patterns like UNSUPPORTED: arm64 should be changed and maybe a link to the commit that did this for an existing example.

I can do that.

[probinson]

In D141007#4059797, @ldionne wrote:

One thing to keep in mind here is that folks who rebase patches on top of this patch may get bitten by this issue for a couple weeks, so it may be worth grepping for suspicious patches from time to time, or turning this into an explicit error instead (just for a few weeks).

As I mentioned in the previous comment, any remaining (or future) mistakes will cause test problems and should be sorted out pretty much immediately.

I will post a reply to the Announcements version of description, so anyone following Discourse should see it.

[probinson]

Examples added to the commit log.
Entry made in release notes.
I will keep an eye out for bot fail-mail related to this change.

Thanks!

llvm/docs/ReleaseNotes.rst

 ---------------------
 
 * Many Sanitizers (asan, fuzzer, lsan, safestack, scudo, tsan, ubsan) have
   support for Linux LoongArch 64-bit variant. Some of them may be rudimentary.
 
 Other Changes
 -------------
 
+* lit no longer supports using substrings of the default target triple as
+  feature names in ``UNSUPPORTED:`` and ``XFAIL:`` directives. These have been
+  replaced by the ``target=<triple>`` feature, and tests can use regex
+  matching to achieve the same effect. For example, ``UNSUPPORTED: arm``
+  would now be ``UNSUPPORTED: target=arm{{.*}}`` and ``XFAIL: windows``
+  would now be ``XFAIL: target={{.*}}-windows{{.*}}``.
+
 External Open Source Projects Using LLVM 15
 ===========================================
 
 * A project...
 
 Additional Information
 ======================
 

llvm/utils/lit/lit/BooleanExpression.py

class BooleanExpression:
     #   identifier   :: [-+=._a-zA-Z0-9]+
     #   braced_regex :: '{{' python_regex '}}'
 
     # Evaluates `string` as a boolean expression.
     # Returns True or False. Throws a ValueError on syntax error.
     #
     # Variables in `variables` are true.
     # Regexes that match any variable in `variables` are true.
-    # Substrings of `triple` are true.
     # 'true' is true.
     # All other identifiers are false.
     @staticmethod
-    def evaluate(string, variables, triple=""):
+    def evaluate(string, variables):
         try:
-            parser = BooleanExpression(string, set(variables), triple)
+            parser = BooleanExpression(string, set(variables))
             return parser.parseAll()
         except ValueError as e:
             raise ValueError(str(e) + ('\nin expression: %r' % string))
 
     #####
 
-    def __init__(self, string, variables, triple=""):
+    def __init__(self, string, variables):
         self.tokens = BooleanExpression.tokenize(string)
         self.variables = variables
         self.variables.add('true')
-        self.triple = triple
         self.value = None
         self.token = None
 
     # Singleton end-of-expression marker.
     END = object()
 
     # Tokenization pattern.
     Pattern = re.compile(r'\A\s*([()]|&&|\|\||!|(?:[-+=._a-zA-Z0-9]+|\{\{.+?\}\})+)\s*(.*)\Z')
def parseMATCH(self):
         regex = ''
         for part in filter(None, re.split(r'(\{\{.+?\}\})', self.token)):
             if part.startswith('{{'):
                 assert part.endswith('}}')
                 regex += '(?:{})'.format(part[2:-2])
             else:
                 regex += re.escape(part)
         regex = re.compile(regex)
-        self.value = self.token in self.triple or any(regex.fullmatch(var) for var in self.variables)
+        self.value = any(regex.fullmatch(var) for var in self.variables)
         self.token = next(self.tokens)
 
     def parseNOT(self):
         if self.accept('!'):
             self.parseNOT()
             self.value = not self.value
         elif self.accept('('):
             self.parseOR()
def test_variables(self):
 
         self.assertFalse(BooleanExpression.evaluate('false', variables))
         self.assertFalse(BooleanExpression.evaluate('True', variables))
         self.assertFalse(BooleanExpression.evaluate('true-ish', variables))
         self.assertFalse(BooleanExpression.evaluate('not_true', variables))
         self.assertFalse(BooleanExpression.evaluate('tru', variables))
         self.assertFalse(BooleanExpression.evaluate('{{its-true.+}}', variables))
 
-    def test_triple(self):
-        triple = 'arch-vendor-os'
-        self.assertTrue(BooleanExpression.evaluate('arch-', {}, triple))
-        self.assertTrue(BooleanExpression.evaluate('ar', {}, triple))
-        self.assertTrue(BooleanExpression.evaluate('ch-vend', {}, triple))
-        self.assertTrue(BooleanExpression.evaluate('-vendor-', {}, triple))
-        self.assertTrue(BooleanExpression.evaluate('-os', {}, triple))
-        self.assertFalse(BooleanExpression.evaluate('arch-os', {}, triple))
-
-        # When matching against the triple, a regex is treated as an identifier and checked
-        # for a literal match. This preserves existing behavior before regexes were introduced.
-        self.assertFalse(BooleanExpression.evaluate('arch-{{vendor}}-os', {}, triple))
-        self.assertTrue(BooleanExpression.evaluate('arch-{{vendor}}-os', {}, 'arch-{{vendor}}-os'))
-
     def test_matching(self):
         expr1 = 'linux && (target={{aarch64-.+}} || target={{x86_64-.+}})'
         self.assertTrue(BooleanExpression.evaluate(expr1, {'linux', 'target=x86_64-unknown-linux-gnu'}))
         self.assertFalse(BooleanExpression.evaluate(expr1, {'linux', 'target=i386-unknown-linux-gnu'}))
 
         expr2 = 'use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12}} && !no-exceptions'
         self.assertTrue(BooleanExpression.evaluate(expr2, {'use_system_cxx_lib', 'target=arm64-apple-macosx10.12'}))
         self.assertFalse(BooleanExpression.evaluate(expr2, {'use_system_cxx_lib', 'target=arm64-apple-macosx10.12', 'no-exceptions'}))

llvm/utils/lit/lit/Test.py

class Test:
     def __init__(self, suite, path_in_suite, config, file_path = None, gtest_json_file = None):
         self.suite = suite
         self.path_in_suite = path_in_suite
         self.config = config
         self.file_path = file_path
         self.gtest_json_file = gtest_json_file
 
         # A list of conditions under which this test is expected to fail.
-        # Each condition is a boolean expression of features and target
-        # triple parts. These can optionally be provided by test format
-        # handlers, and will be honored when the test result is supplied.
+        # Each condition is a boolean expression of features, or '*'.
+        # These can optionally be provided by test format handlers,
+        # and will be honored when the test result is supplied.
         self.xfails = []
 
         # If true, ignore all items in self.xfails.
         self.xfail_not = False
 
         # A list of conditions that must be satisfied before running the test.
         # Each condition is a boolean expression of features. All of them
         # must be True for the test to run.
-        # FIXME should target triple parts count here too?
         self.requires = []
 
         # A list of conditions that prevent execution of the test.
-        # Each condition is a boolean expression of features and target
-        # triple parts. All of them must be False for the test to run.
+        # Each condition is a boolean expression of features. All of them
+        # must be False for the test to run.
         self.unsupported = []
 
         # An optional number of retries allowed before the test finally succeeds.
         # The test is run at most once plus the number of retries specified here.
         self.allowed_retries = getattr(config, 'test_retry_attempts', 0)
 
         # The test result, once complete.
         self.result = None
def isExpectedToFail(self):
         executed.
         Throws ValueError if an XFAIL line has a syntax error.
         """
 
         if self.xfail_not:
           return False
 
         features = self.config.available_features
-        triple = getattr(self.suite.config, 'target_triple', "")
 
-        # Check if any of the xfails match an available feature or the target.
+        # Check if any of the xfails match an available feature.
         for item in self.xfails:
             # If this is the wildcard, it always fails.
             if item == '*':
                 return True
 
-            # If this is a True expression of features and target triple parts,
-            # it fails.
+            # If this is a True expression of features, it fails.
             try:
-                if BooleanExpression.evaluate(item, features, triple):
+                if BooleanExpression.evaluate(item, features):
                     return True
             except ValueError as e:
                 raise ValueError('Error in XFAIL list:\n%s' % str(e))
 
         return False
 
     def isWithinFeatureLimits(self):
         """
def getMissingRequiredFeatures(self):
         features = self.config.available_features
         return self.getMissingRequiredFeaturesFromList(features)
 
     def getUnsupportedFeatures(self):
         """
         getUnsupportedFeatures() -> list of strings
 
         Returns a list of features from UNSUPPORTED that are present
-        in the test configuration's features or target triple.
+        in the test configuration's features.
         Throws ValueError if an UNSUPPORTED line has a syntax error.
         """
 
         features = self.config.available_features
-        triple = getattr(self.suite.config, 'target_triple', "")
 
         try:
             return [item for item in self.unsupported
-                    if BooleanExpression.evaluate(item, features, triple)]
+                    if BooleanExpression.evaluate(item, features)]
         except ValueError as e:
             raise ValueError('Error in UNSUPPORTED list:\n%s' % str(e))
 
     def getUsedFeatures(self):
         """
         getUsedFeatures() -> list of strings
 
         Returns a list of all features appearing in XFAIL, UNSUPPORTED and