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

[Louis Dionne via Phabricator via llvm-commits]

ldionne added a comment.

In D78589#1995591 <https://reviews.llvm.org/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.



================
Comment at: llvm/utils/lit/lit/Test.py:371
+        parsed = lit.TestRunner._parseKeywords(self.getSourcePath(), require_script=False)
+        unsupported = parsed['UNSUPPORTED:'] or []
+        requires = parsed['REQUIRES:'] or []
----------------
yln wrote:
> Is parsed a dict? If so, then I think we get a KeyError here; or the `or []` part is redundant.
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}`


================
Comment at: llvm/utils/lit/lit/Test.py:376
+            BooleanExpression.tokenize(expr) for expr in
+                itertools.chain(unsupported, requires, xfails) if expr != '*'
+        )
----------------
yln wrote:
> Prefer being dry over performance, remove `if expr != '*'`
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).


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1377
 
-    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.
+def _parseKeywords(sourcepath, additional_parsers=[],
+                   require_script=True):
----------------
I'm open to better name suggestions.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1462
+    parsed = _parseKeywords(test.getSourcePath(), additional_parsers, require_script)
+    script = parsed['RUN:']
+    test.xfails = parsed['XFAIL:'] or []
----------------
yln wrote:
> KeyError or redundant `or []`?
`'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).


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:160
+    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")
----------------
yln wrote:
> Show features used by selected tests; or use discovered_tests below
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?


================
Comment at: llvm/utils/lit/lit/main.py:78
+        print(' '.join(sorted(features)))
+        sys.exit(0)
+
----------------
yln wrote:
> 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?
Moved above to `discovered_tests`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78589/new/

https://reviews.llvm.org/D78589