[lld][llvm-lit] Enabled lit internal shell for lld test suite

[connieyzhu]

This patch sets lit's internal shell to be the default shell when running lld tests. Tests that REQUIRES: shell are not currently supported by the internal shell, so those tests are skipped with this implementation.

Resolves #102700.

[llvmbot]

@llvm/pr-subscribers-lld

Author: Connie Zhu (connieyzhu)

Changes

This patch sets lit's internal shell to be the default shell when running lld tests. Tests that REQUIRES: shell are not currently supported by the internal shell, so those tests are skipped with this implementation.

Resolves #102700.

---

Full diff: https://github.com/llvm/llvm-project/pull/106651.diff

1 Files Affected:

• (modified) lld/test/lit.cfg.py (+12-1)

diff --git a/lld/test/lit.cfg.py b/lld/test/lit.cfg.py
index d309c2ad4ee284..fd9afd4fb65894 100644
--- a/lld/test/lit.cfg.py
+++ b/lld/test/lit.cfg.py
@@ -19,7 +19,18 @@
 # testFormat: The test format to use to interpret tests.
 #
 # For now we require '&&' between commands, until they get globally killed and the test runner updated.
-config.test_format = lit.formats.ShTest(not llvm_config.use_lit_shell)
+# We prefer the lit internal shell which provides a better user experience on failures
+# unless the user explicitly disables it with LIT_USE_INTERNAL_SHELL=0 env var.
+use_lit_shell = True
+lit_shell_env = os.environ.get("LIT_USE_INTERNAL_SHELL")
+if lit_shell_env:
+    use_lit_shell = lit.util.pythonize_bool(lit_shell_env)
+
+config.test_format = lit.formats.ShTest(execute_external=not use_lit_shell)
+
+# TODO: Tests that REQUIRES: shell are unsupported by the lit internal shell
+if use_lit_shell:
+    config.available_features.discard("shell")
 
 # suffixes: A list of file extensions to treat as test files.
 config.suffixes = [".ll", ".s", ".test", ".yaml", ".objtxt"]

[jh7370]

How many tests have REQUIRES: shell in the lld testsuite? I have real concerns that we'll end up with tests that are NEVER enabled, because nobody is going to explicitly specify to use the external shell.

[connieyzhu]

How many tests have REQUIRES: shell in the lld testsuite?

There are these tests:
./lld/test/wasm/lto/cache-warnings.ll
./lld/test/wasm/reproduce.s
./lld/test/ELF/arm-exidx-range.s
./lld/test/ELF/linkerscript/invalid.test
./lld/test/ELF/lto/cache-warnings.ll
./lld/test/ELF/lto/resolution-err.ll
./lld/test/ELF/lto/comdat-nodeduplicate.ll
./lld/test/ELF/file-access.s
./lld/test/ELF/color-diagnostics.test
./lld/test/MachO/link-search-at-loader-path-symlink.s
./lld/test/MachO/lto-cache-warnings.ll
./lld/test/MachO/framework.s
./lld/test/MachO/reproduce.s
./lld/test/MachO/implicit-and-allowable-clients.test
./lld/test/MachO/stabs.s
./lld/test/MachO/color-diagnostics.test
./lld/test/COFF/lto-cache-warnings.ll
./lld/test/COFF/linkrepro-res.test
./lld/test/COFF/linkrepro.test
./lld/test/COFF/color-diagnostics.test

I have real concerns that we'll end up with tests that are NEVER enabled, because nobody is going to explicitly specify to use the external shell.

We do have long-term plans to address this issue with REQUIRES: shell, but I'm not sure what can be done in the short-term so that we don't lose coverage.

[jh7370]

We do have long-term plans to address this issue with REQUIRES: shell, but I'm not sure what can be done in the short-term so that we don't lose coverage.

Don't enable the internal shell by default seems like a viable option to me...

Perhaps more helpfully, I think most (but probably not all, e.g. the color-diagnostics tests) of these tests can be modified to work with the internal shell without issue. In some cases, the only necessary modification may be removing the REQUIRES: shell. For example, I'm not sure why ./lld/test/wasm/reproduce.s uses it: the majority of the commands in there are available in the internal shell, and the remainder (i.e. tar) are part of the minimum required tools, I believe. LLD has some tests that use tar without any REQUIRES, and others that have the gnutar requirement (the latter is presumably redundant, given that the former tests exist and pass fine).

We shouldn't enable the internal shell by default until we've got a clear migration path forward for all of these tests, preferably with them modified up front before the default change.

[ilovepi]

I agree we should avoid loss of coverage when possible.

One thing that would be good is to get a buildbot configuration in CI that uses the internal shell (aside from Windows), so these test suites don’t regress during the migration period. I wonder if there is a good bot candidate we can use? Preferably one that has substantial overlap with other configurations. Since the internal shell is reportedly faster, maybe the presubmit bots?

Another option would be to give lit a 3rd mode to run in, where it runs only tests with REQUIRES: shell in the external shell, but all the other tests in the internal shell.

For now let’s create (or update?) a bug for the migration of the tests and note that this patch is blocked on that work.