[lit] Add an option to print all features used in tests

[ldionne]

Summary

Lit test suites can tend to accumulate annotations that are not necessarily
relevant as time goes by, for example XFAILS on old compilers or platforms.
To help spot old annotations that can be cleaned up, it can be useful to
look at all features used inside a test suite.

This commit adds a new option '--show-features' that prints all the
features used in XFAIL, REQUIRES and UNSUPPORTED of all tests that
are discovered.

[ldionne]

As explained in the commit message, the main goal of this feature is to allow cleaning up Lit test suites that accumulate old markup. This allowed me to remove several unused features from the libc++ test suite, and also to find out some mistakes (e.g. some tests were requiring C++14 instead of c++14, and were never run). While this may seem trivial for a small test suite, it's actually very useful to have this programmatic tool for large test suites like libc++'s.

[yln]

I like this. Thanks for working on it!

Please check my assumptions, about features:
Available, but unused -> cruft
Not available, but used -> okay (e.g., test disabled because not right platform/architecture)

After adding all my comments above, I am assuming the workflow is as follows:
First run with --show-suites to see available features, then run --show-features to see used/requested features and cleanup the ones which are available but unused.

How about making this even easier and more discoverable by adding it directly to --show-suites to encourage people to clean things up?

Append "(unused)" marker to available, but unused feature to draw attention that they could be removed; or maybe even add a separate line for "unused" features.
Add another line for "unavailable" features.

Available features: darwin, win95 (unused)
Unavailable/requested features: linux

or

Available features: darwin
Unavailable features: linux
Unused features: win95

What do you think?

llvm/utils/lit/lit/Test.py

371

Is parsed a dict? If so, then I think we get a KeyError here; or the or [] part is redundant.

373

feature_keywords = ['UNSUPPORTED', ..]
and then do some mapping over that: keywords -> dict lookup -> chain -> tokenize -> chain

376

Prefer being dry over performance, remove if expr != '*'

llvm/utils/lit/lit/TestRunner.py

1462

KeyError or redundant or []?

llvm/utils/lit/lit/cl_arguments.py

159

Maybe name --show-used-features?

160

Show features used by selected tests; or use discovered_tests below

llvm/utils/lit/lit/main.py

78

If the main use case for this is identifying obsolete features that are defined, but never used then we could move this up and use discovered_tests. Or is it also useful to ask the question, which features do these selected/filtered tests use?

140

Import at top is better; remove

[ldionne]

Address review comments.

Also reuse BooleanExpression.isIdentifier to avoid code duplication.

[ldionne]

In D78589#1995591, @yln wrote:

I like this. Thanks for working on it!

Please check my assumptions, about features:
Available, but unused -> cruft
Not available, but used -> okay (e.g., test disabled because not right platform/architecture)

Yes, that's correct. There's also some features that are used in the test suite but never made available from the configuration, and those are cruft too. They are usually harder to spot because they can be created programmatically from the configuration, but those are the ones that are (usually) worth cleaning up.

After adding all my comments above, I am assuming the workflow is as follows:
First run with --show-suites to see available features, then run --show-features to see used/requested features and cleanup the ones which are available but unused.

How about making this even easier and more discoverable by adding it directly to --show-suites to encourage people to clean things up?

Append "(unused)" marker to available, but unused feature to draw attention that they could be removed; or maybe even add a separate line for "unused" features.
Add another line for "unavailable" features.

Available features: darwin, win95 (unused)
Unavailable/requested features: linux

or

Available features: darwin
Unavailable features: linux
Unused features: win95

What do you think?

I thought it made a lot of sense so I tried it out. I added a fake win95 feature somewhere in the libc++ test suite, made the Lit changes you suggested, and ran --show-suites:

-- Test Suites --
  libc++ - 6353 tests
    Source Root: [...]
    Exec Root  : [...]
    Available Features: apple-clang-11 apple-clang-11.0 apple-clang-11.0.0 -faligned-allocation -fsized-deallocation apple-clang availability=macosx availability=macosx10.15 c++2a darwin diagnose-if-support fcoroutines-ts fdelayed-template-parsing has-fblocks has-fobjc-arc libc++ libcpp-no-concepts locale.cs_CZ.ISO8859-2 locale.en_US.UTF-8 locale.fr_CA.ISO8859-1 locale.fr_FR.UTF-8 locale.ru_RU.UTF-8 locale.zh_CN.UTF-8 long_tests modules-support objective-c++ thread-safety
    Unavailable Features: -fmodules -fno-rtti LIBCXX-WINDOWS-FIXME apple-clang-10 apple-clang-10.0 apple-clang-10.0.0 apple-clang-9 apple-clang-9.0 apple-clang-9.1 asan availability=macosx10.10 availability=macosx10.11 availability=macosx10.12 availability=macosx10.13 availability=macosx10.14 availability=macosx10.9 c++03 c++11 c++14 c++17 c++98 c++filesystem-disabled clang clang-4 clang-4.0 clang-5 clang-5.0 clang-6 clang-6.0 clang-7 clang-7.0 clang-8 clang-8.0 clang-9 dylib-has-no-bad_any_cast dylib-has-no-bad_optional_access dylib-has-no-bad_variant_access dylib-has-no-shared_mutex gcc gcc-5 gcc-5.1 gcc-5.2 gcc-6 gcc-7 gcc-8 gcc-9 libatomic libcpp-has-no-global-filesystem-namespace libcpp-has-no-monotonic-clock libcpp-has-no-stdin libcpp-has-no-stdout libcpp-has-no-thread-unsafe-c-functions libcpp-has-no-threads libcpp-has-thread-api-external libcpp-has-thread-api-pthread libcpp-no-deduction-guides libcpp-no-exceptions libcpp-no-if-constexpr libcpp-no-structured-bindings libcpp-no-vcruntime libcxx_gdb libstdc++ linux linux-gnu msan msvc netbsd newlib sanitizer-new-delete suse-linux-enterprise-server-11 system-windows template-cost-testing ubsan verify-support win95 windows with_system_cxx_lib=macosx with_system_cxx_lib=macosx10.10 with_system_cxx_lib=macosx10.11 with_system_cxx_lib=macosx10.12 with_system_cxx_lib=macosx10.13 with_system_cxx_lib=macosx10.14 with_system_cxx_lib=macosx10.15 with_system_cxx_lib=macosx10.9
    Unused Features: apple-clang-11 apple-clang-11.0 apple-clang-11.0.0 availability=macosx
    Available Substitutions: [...]

Apologies for the large output, however I think this is the key to knowing whether it's worth doing this or not. My experience so far has been that cleaning up features is a fairly manual task -- one has to look at features individually and determine what to do with them. That's true both for features that are mentioned in the test suite (that we might want to clean up) and those set by the configuration (that we might want to clean up too).

Looking at the above output, I make the following observations:

1. There's a lot of output because there's a lot of features (both set and used)
2. The fake win95 feature is difficult to spot amongst all the Unavailable Features
3. The Unused Features are actually not really that useful. Even though apple-clang-11 is set but not used by the suite right now, it could be used in the future. It's just set programmatically and it's not a crufty feature.

For these reasons, I believe this output is actually more complicated than a simple output of all the features used by the test suite, and that's not super helpful. So my preference would be to stick with a command-line option that does exactly one thing, i.e. output the features used by the test suite. One can then use that to guide a manual investigation of what to do with them.

However, if you feel strongly that the above output is useful, my changes are stashed so I can do it without too much trouble.

llvm/utils/lit/lit/Test.py

371

It's a dict, but the parser's values are None if they haven't parsed anything. So the keys could be e.g. {'UNSUPPORTED:' : None}

376

It doesn't work when expr == *, that's why I added it :-). The problem is that '*' is not a valid value for a BooleanExpression, however '*' is still used in XFAIL: because it has a special meaning there (see https://github.com/llvm/llvm-project/blob/master/llvm/utils/lit/tests/Inputs/shtest-format/requires-star.txt and https://github.com/llvm/llvm-project/blob/master/llvm/utils/lit/lit/Test.py#L285).

llvm/utils/lit/lit/TestRunner.py

1377

I'm open to better name suggestions.

1462

'RUN:' is "special" in that we know it's always going to be a (maybe empty) list. I'll add or [] for consistency though, cause I agree it's non-trivial to know that (and it's arguably an implementation detail).

llvm/utils/lit/lit/cl_arguments.py

160

Done. However, I do think it would make more sense for all of them to use the filtered_tests instead, don't you think? The argument is that it's simply more general: no filter is equivalent to discovered_tests, but it also allows filtering if one wants.

Should I create a follow-up patch?

llvm/utils/lit/lit/main.py

78

Moved above to discovered_tests.

[yln]

Thanks for being thorough and explaining your reasoning so well. I now agree that "Unavailable Features" introduces too much noise to simply add it to the existing option without the user asking for it.

Two more quick questions:
We present features as a sum of all used features, however they are specific to configs/test suites. Could we group them by test suite? Maybe a good way to do this would be to extend the print_discovered function. This way we can also combine output from the "show and exit" options.

Currently we have "Available Features" in --show-suites, which can be either used or unused. When the user also asks for --show-used-features then maybe it would still be helpful to somehow mark the unused ones without introducing too much visual noise (e.g., with an asterisk + footnote). This would help with "de-crufting" in at least one direction.

llvm/utils/lit/lit/Test.py

376

Got it! Thanks for explaining.

llvm/utils/lit/lit/cl_arguments.py

160

That makes sense. Feel free to send a patch.

[ldionne]

Sorry for the delay in replying to this -- I got pulled into other things.

In D78589#2002960, @yln wrote:

Thanks for being thorough and explaining your reasoning so well. I now agree that "Unavailable Features" introduces too much noise to simply add it to the existing option without the user asking for it.

Two more quick questions:

1. We present features as a sum of all used features, however they are specific to configs/test suites. Could we group them by test suite? Maybe a good way to do this would be to extend the print_discovered function. This way we can also combine output from the "show and exit" options.

I actually see the features as being whatever features are used in the test files discovered by Lit -- not as a per test-suite thing. I actually don't think it makes a lot of sense to run Lit with multiple test suites at once (for example --param are shared across all test suites which is weird), but that's a different story.

2. Currently we have "Available Features" in --show-suites, which can be either used or unused. When the user also asks for --show-used-features then maybe it would still be helpful to somehow mark the unused ones without introducing too much visual noise (e.g., with an asterisk + footnote). This would help with "de-crufting" in at least one direction.

When we use --show-used-features, "Available Features" are not printed. Are you saying when one uses both --show-suites and --show-used-features? IMO, that's adding complexity for fairly little benefit -- I doubt anyone would even notice that feature. I like thinking that Lit can be used as a swiss army knife for extracting information about a test suite (or a bunch of tests), and then combined with a bit of Bash scripting or whatever to obtain the desired information.

Unless you feel strongly about points above, I'd like to move forward with this to avoid rotting of this patch.

[ldionne]

Rebase onto master

[yln]

Okay, thanks!

llvm/utils/lit/lit/Test.py

+import itertools
 import os
 from xml.sax.saxutils import quoteattr
 from json import JSONEncoder
 
 from lit.BooleanExpression import BooleanExpression
 
 # Test result codes.
 
def addMetric(self, name, value):
             raise TypeError("unexpected metric value: %r" % (value,))
         self.metrics[name] = value
 
     def addMicroResult(self, name, microResult):
         """
         addMicroResult(microResult)
 
         Attach a micro-test result to the test result, with the given name and
-        result.  It is an error to attempt to attach a micro-test with the 
+        result.  It is an error to attempt to attach a micro-test with the
         same name multiple times.
 
         Each micro-test result must be an instance of the Result class.
         """
         if name in self.microResults:
             raise ValueError("Result already includes microResult for %r" % (
                    name,))
         if not isinstance(microResult, Result):
def getUnsupportedFeatures(self):
         triple = getattr(self.suite.config, 'target_triple', "")
 
         try:
             return [item for item in self.unsupported
                     if BooleanExpression.evaluate(item, features, triple)]
         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
+        REQUIRES annotations for this test.
+        """
+        import lit.TestRunner
+        parsed = lit.TestRunner._parseKeywords(self.getSourcePath(), require_script=False)
+        unsupported = parsed['UNSUPPORTED:'] or []
+        requires = parsed['REQUIRES:'] or []
+        xfails = parsed['XFAIL:'] or []
+        tokens = itertools.chain.from_iterable(
+            BooleanExpression.tokenize(expr) for expr in
+                itertools.chain(unsupported, requires, xfails) if expr != '*'
+        )
+        SPECIALS = {'*', '&&', '||', '!', '(', ')', BooleanExpression.END}
+        tokens = set(tokens) - SPECIALS
+        return tokens
+
     def isEarlyTest(self):
         """
         isEarlyTest() -> bool
 
         Check whether this test should be executed early in a particular run.
         This can be used for test suites with long running tests to maximize
         parallelism or where it is desirable to surface their failures early.
         """
         return self.suite.config.is_early
 
     def writeJUnitXML(self, fil):
         """Write the test's report xml representation to a file handle."""
         test_name = quoteattr(self.path_in_suite[-1])
         test_path = self.path_in_suite[:-1]
         safe_test_path = [x.replace(".","_") for x in test_path]
         safe_name = self.suite.name.replace(".","-")
 
         if safe_test_path:
-            class_name = safe_name + "." + "/".join(safe_test_path) 
+            class_name = safe_name + "." + "/".join(safe_test_path)
         else:
             class_name = safe_name + "." + safe_name
         class_name = quoteattr(class_name)
         testcase_template = '<testcase classname={class_name} name={test_name} time="{time:.2f}"'
         elapsed_time = self.result.elapsed if self.result.elapsed is not None else 0.0
         testcase_xml = testcase_template.format(class_name=class_name, test_name=test_name, time=elapsed_time)
         fil.write(testcase_xml)
         if self.isFailure():

llvm/utils/lit/lit/TestRunner.py

def _handleBooleanExpr(line_number, line, output):
         output.extend(parts)
         # Evaluate each expression to verify syntax.
         # We don't want any results, just the raised ValueError.
         for s in output:
             if s != '*' and not s.endswith('\\'):
                 BooleanExpression.evaluate(s, [])
         return output
 
-def parseIntegratedTestScript(test, additional_parsers=[],
-                              require_script=True):
-    """parseIntegratedTestScript - Scan an LLVM/Clang style integrated test
-    script and extract the lines to 'RUN' as well as 'XFAIL', 'REQUIRES',
-    'UNSUPPORTED' and 'ALLOW_RETRIES' information.
+
+def _parseKeywords(sourcepath, additional_parsers=[],
+    """_parseKeywords
 
-    If additional parsers are specified then the test is also scanned for the
-    keywords they specify and all matches are passed to the custom parser.
+    Scan an LLVM/Clang style integrated test script and extract all the lines
+    pertaining to a special parser. This includes 'RUN', 'XFAIL', 'REQUIRES',
+    'UNSUPPORTED' and 'ALLOW_RETRIES', as well as other specified custom
+    parsers.
 
-    If 'require_script' is False an empty script
-    may be returned. This can be used for test formats where the actual script
-    is optional or ignored.
+    Returns a dictionary mapping each custom parser to its value after
+    parsing the test.
     """
-
     # Install the built-in keyword parsers.
     script = []
     builtin_parsers = [
-        IntegratedTestKeywordParser('RUN:', ParserKind.COMMAND,
-                                    initial_value=script),
-        IntegratedTestKeywordParser('XFAIL:', ParserKind.BOOLEAN_EXPR,
-                                    initial_value=test.xfails),
-        IntegratedTestKeywordParser('REQUIRES:', ParserKind.BOOLEAN_EXPR,
-                                    initial_value=test.requires),
-        IntegratedTestKeywordParser('UNSUPPORTED:', ParserKind.BOOLEAN_EXPR,
-                                    initial_value=test.unsupported),
+        IntegratedTestKeywordParser('RUN:', ParserKind.COMMAND, initial_value=script),
+        IntegratedTestKeywordParser('XFAIL:', ParserKind.BOOLEAN_EXPR),
+        IntegratedTestKeywordParser('REQUIRES:', ParserKind.BOOLEAN_EXPR),
+        IntegratedTestKeywordParser('UNSUPPORTED:', ParserKind.BOOLEAN_EXPR),
         IntegratedTestKeywordParser('ALLOW_RETRIES:', ParserKind.INTEGER),
         IntegratedTestKeywordParser('END.', ParserKind.TAG)
     ]
     keyword_parsers = {p.keyword: p for p in builtin_parsers}
 
     # Install user-defined additional parsers.
     for parser in additional_parsers:
         if not isinstance(parser, IntegratedTestKeywordParser):
             raise ValueError('additional parser must be an instance of '
                              'IntegratedTestKeywordParser')
         if parser.keyword in keyword_parsers:
             raise ValueError("Parser for keyword '%s' already exists"
                              % parser.keyword)
         keyword_parsers[parser.keyword] = parser
 
     # Collect the test lines from the script.
-    sourcepath = test.getSourcePath()
     for line_number, command_type, ln in \
             parseIntegratedTestScriptCommands(sourcepath,
                                               keyword_parsers.keys()):
         parser = keyword_parsers[command_type]
         parser.parseLine(line_number, ln)
         if command_type == 'END.' and parser.getValue() is True:
             break
 
def _parseKeywords(sourcepath, additional_parsers=[],
     for key in keyword_parsers:
         kp = keyword_parsers[key]
         if kp.kind != ParserKind.BOOLEAN_EXPR:
             continue
         value = kp.getValue()
         if value and value[-1][-1] == '\\':
             raise ValueError("Test has unterminated %s lines (with '\\')" % key)
 
+    # Make sure there's at most one ALLOW_RETRIES: line
+    allowed_retries = keyword_parsers['ALLOW_RETRIES:'].getValue()
+    if allowed_retries and len(allowed_retries) > 1:
+        return lit.Test.Result(Test.UNRESOLVED,
+                               "Test has more than one ALLOW_RETRIES lines")
+
+    return {p.keyword: p.getValue() for p in keyword_parsers.values()}
+
+
+def parseIntegratedTestScript(test, additional_parsers=[],
+                              require_script=True):
+    """parseIntegratedTestScript - Scan an LLVM/Clang style integrated test
+    script and extract the lines to 'RUN' as well as 'XFAIL', 'REQUIRES',
+    'UNSUPPORTED' and 'ALLOW_RETRIES' information into the given test.
+
+    If additional parsers are specified then the test is also scanned for the
+    keywords they specify and all matches are passed to the custom parser.
+
+    If 'require_script' is False an empty script
+    may be returned. This can be used for test formats where the actual script
+    is optional or ignored.
+    """
+    # Parse the test sources and extract test properties
+    parsed = _parseKeywords(test.getSourcePath(), additional_parsers, require_script)
+    script = parsed['RUN:']
+    test.xfails = parsed['XFAIL:'] or []
+    test.requires = parsed['REQUIRES:'] or []
+    test.unsupported = parsed['UNSUPPORTED:'] or []
+    if parsed['ALLOW_RETRIES:']:
+        test.allowed_retries = parsed['ALLOW_RETRIES:'][0]
+
     # Enforce REQUIRES:
     missing_required_features = test.getMissingRequiredFeatures()
     if missing_required_features:
         msg = ', '.join(missing_required_features)
         return lit.Test.Result(Test.UNSUPPORTED,
                                "Test requires the following unavailable "
                                "features: %s" % msg)
 
     # Enforce UNSUPPORTED:
     unsupported_features = test.getUnsupportedFeatures()
     if unsupported_features:
         msg = ', '.join(unsupported_features)
         return lit.Test.Result(
             Test.UNSUPPORTED,
             "Test does not support the following features "
             "and/or targets: %s" % msg)
 
-    # Handle ALLOW_RETRIES:
-    allowed_retries = keyword_parsers['ALLOW_RETRIES:'].getValue()
-    if allowed_retries:
-        if len(allowed_retries) > 1:
-            return lit.Test.Result(Test.UNRESOLVED,
-                                   "Test has more than one ALLOW_RETRIES lines")
-        test.allowed_retries = allowed_retries[0]
-
     # Enforce limit_to_features.
     if not test.isWithinFeatureLimits():
         msg = ', '.join(test.config.limit_to_features)
         return lit.Test.Result(Test.UNSUPPORTED,
                                "Test does not require any of the features "
                                "specified in limit_to_features: %s" % msg)
 
     return script

llvm/utils/lit/lit/cl_arguments.py

debug_group.add_argument("--debug",
             help="Enable debugging (for 'lit' development)",
             action="store_true")
     debug_group.add_argument("--show-suites",
             help="Show discovered test suites and exit",
             action="store_true")
     debug_group.add_argument("--show-tests",
             help="Show all discovered tests and exit",
             action="store_true")
+    debug_group.add_argument("--show-features",
+            help="Show all features used in the test suite (in XFAIL, UNSUPPORTED and REQUIRES) and exit",
+            action="store_true")
 
     # LIT is special: environment variables override command line arguments.
     env_args = shlex.split(os.environ.get("LIT_OPTS", ""))
     args = sys.argv[1:] + env_args
     opts = parser.parse_args(args)
 
     # Validate command line options
     if opts.echoAllCommands:

llvm/utils/lit/lit/main.py

 """
 lit - LLVM Integrated Tester.
 
 See lit.pod for more information.
 """
 
+import itertools
 import os
 import platform
 import sys
 import time
 
 import lit.cl_arguments
 import lit.discovery
 import lit.display
if not filtered_tests:
             sys.stderr.write("Suppressing error because '--allow-empty-runs' "
                              'was specified.\n')
             sys.exit(0)
         else:
             sys.stderr.write("Use '--allow-empty-runs' to suppress this "
                              'error.\n')
             sys.exit(2)
 
+    if opts.show_features:
+        features = set(itertools.chain.from_iterable(t.getUsedFeatures() for t in filtered_tests))
+        print(' '.join(sorted(features)))
+        sys.exit(0)
+
     determine_order(filtered_tests, opts.order)
 
     if opts.shard:
         (run, shards) = opts.shard
         filtered_tests = filter_by_shard(filtered_tests, run, shards, lit_config)
         if not filtered_tests:
             sys.stderr.write('warning: shard does not contain any tests.  '
                              'Consider decreasing the number of shards.\n')

 
 
 def print_discovered(tests, show_suites, show_tests):
     # Suite names are not necessarily unique.  Include object identity in sort
     # key to avoid mixing tests of different suites.
     tests.sort(key=lambda t: (t.suite.name, t.suite, t.path_in_suite))
 
     if show_suites:
         import itertools
         tests_by_suite = itertools.groupby(tests, lambda t: t.suite)
         print('-- Test Suites --')
         for suite, suite_iter in tests_by_suite:
             test_count = sum(1 for _ in suite_iter)
             print('  %s - %d tests' % (suite.name, test_count))
             print('    Source Root: %s' % suite.source_root)
             print('    Exec Root  : %s' % suite.exec_root)
             features = ' '.join(sorted(suite.config.available_features))

llvm/utils/lit/tests/Inputs/show-features/lit.cfg

+import lit.formats
+config.name = 'show-features'
+config.suffixes = ['.txt']
+config.test_format = lit.formats.ShTest()
+config.test_source_root = None
+config.test_exec_root = None

llvm/utils/lit/tests/Inputs/show-features/mixed.txt

+
+// REQUIRES: my-require-feature-2 || my-require-feature-3
+// UNSUPPORTED: my-unsupported-feature-2, my-unsupported-feature-3
+// XFAIL: my-xfail-feature-2, my-xfail-feature-3

llvm/utils/lit/tests/Inputs/show-features/requires.txt

+
+// REQUIRES: my-require-feature-1

llvm/utils/lit/tests/Inputs/show-features/unsupported.txt

+
+// UNSUPPORTED: my-unsupported-feature-1

llvm/utils/lit/tests/Inputs/show-features/xfail.txt

+
+// XFAIL: my-xfail-feature-1

llvm/utils/lit/tests/show-features.py

+# Check that --show-features works correctly.
+#
+# RUN: %{lit} %{inputs}/show-features --show-features | FileCheck %s
+# CHECK: my-require-feature-1 my-require-feature-2 my-require-feature-3
+# CHECK: my-unsupported-feature-1 my-unsupported-feature-2 my-unsupported-feature-3
+# CHECK: my-xfail-feature-1 my-xfail-feature-2 my-xfail-feature-3