[lit] Protect full test suite from FILECHECK_OPTS

[jdenny]

Summary

lit's test suite calls lit multiple times for various sample test
suites. FILECHECK_OPTS is safe for FileCheck calls in lit's test
suite. It's not safe for FileCheck calls in the sample test suites,
whose output affects the results of lit's test suite.

Without this patch, only one such sample test suite is protected from
FILECHECK_OPTS, and I admit I haven't discovered other cases for
which I can produce false failures using FILECHECK_OPTS. However,
it's hard to predict the future, especially false passes. Thus, this
patch protects all existing and future sample test suites from
FILECHECK_OPTS (and the deprecated
FILECHECK_DUMP_INPUT_ON_FAILURE).

[jdenny]

This patch was extracted from D65121.

[probinson]

LGTM

[mgorny]

This change broke NetBSD buildbots, e.g.: http://lab.llvm.org:8011/builders/netbsd-amd64/builds/21152/steps/run%20unit%20tests/logs/FAIL%3A%20lit%3A%3A%20googletest-upstream-format.py

Please review https://reviews.llvm.org/D65334 and https://reviews.llvm.org/D65335 for a fix.

[stella.stamenova]

This actually breaks the lit tests on Windows when the path to Python has spaces. For example:

Command Output (stdout):
--
$ ":" "RUN: at line 3"
note: command had no output on stdout or stderr
$ "not" "env" "FILECHECK_OPTS=" "FILECHECK_DUMP_INPUT_ON_FAILURE=" "C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python36_64\python.exe" "E:/_work/18/s/llvm\utils\lit\lit.py" "-j" "1" "-v" "E:/_work/18/b/utils/lit/tests\Inputs/shtest-output-printing"
# command stderr:
C:\Program: can't open file 'Files': [Errno 2] No such file or directory

[jdenny]

In D65156#1606769, @stella.stamenova wrote:

This actually breaks the lit tests on Windows when the path to Python has spaces. For example:

Thanks. Is the outermost lit call here running lit's internal shell or an external shell? (The lit call I mean is the one running lit's test suite and complaining about that command line, which is a lit call within lit's test suite.)

[mgorny]

Hmm, I suspect the problem here mostly affects people running the test suite via CMake rule rather than directly, correct? So maybe it would be more portable to clean the env via ${CMAKE_PROGRAM} -E env ....

[probinson]

In D65156#1607252, @jdenny wrote:

Thanks. Is the outermost lit call here running lit's internal shell or an external shell? (The lit call I mean is the one running lit's test suite and complaining about that command line, which is a lit call within lit's test suite.)

How can I determine which one it's using?

[jdenny]

In D65156#1608250, @probinson wrote:

In D65156#1607252, @jdenny wrote:

Thanks. Is the outermost lit call here running lit's internal shell or an external shell? (The lit call I mean is the one running lit's test suite and complaining about that command line, which is a lit call within lit's test suite.)

How can I determine which one it's using?

This quick hack reveals it. I'm not aware of an existing option to print this info.

diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index ac627d51c1c1..61e20e75f4a0 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -1590,6 +1590,7 @@ def _runShTest(test, litConfig, useExternalSh, script, tmpBase):
     lit.util.mkdir_p(os.path.dirname(tmpBase))
 
     execdir = os.path.dirname(test.getExecPath())
+    print("HERE: useExternalSh: " + str(useExternalSh))
     if useExternalSh:
         res = executeScript(test, litConfig, tmpBase, script, execdir)
     else:

As an example of how to interpret the output, you might run this (somewhere lit's test suite is currently successful):

$ FILECHECK_OPTS='-color -dump-input=always -vv' LIT_OPTS='-a --filter=shtest-run-at-line' ninja check-lit |& less -R

For me on ubuntu, this reveals that useExternalSh is False for the outermost lit call. For inner lit calls, the FileCheck input dump shows it's True for external-shell/*.txt tests, and it's False for internal-shell/*.txt tests. Look for the keyword execute_external in lit config files to see what's setting this.

[stella.stamenova]

HERE: useExternalSh: False

[jdenny]

In D65156#1608636, @stella.stamenova wrote:

HERE: useExternalSh: False

Thanks. That's what it should be, so that doesn't explain the error.

After debugging more on my system, it seems that not in front of a %{lit} causes lit's internal shell to use an external env instead of the internal env. Fine for ubuntu. Not fine for windows. Will post more later.

[mgorny]

In D65156#1608751, @jdenny wrote:

After debugging more on my system, it seems that not in front of a %{lit} causes lit's internal shell to use an external env instead of the internal env. Fine for ubuntu. Not fine for windows. Will post more later.

Yeah, that makes sense. Since not is an external program, it obviously does not use the internal shell.

I would suggest moving the env calls into a separate substitution, and moving them always to the front.

[jdenny]

In D65156#1608789, @mgorny wrote:

In D65156#1608751, @jdenny wrote:

After debugging more on my system, it seems that not in front of a %{lit} causes lit's internal shell to use an external env instead of the internal env. Fine for ubuntu. Not fine for windows. Will post more later.

Yeah, that makes sense. Since not is an external program, it obviously does not use the internal shell.

I would suggest moving the env calls into a separate substitution, and moving them always to the front.

I'm thinking a more general fix is to implement not in lit's internal shell. not in front of any lit's internal shell commands is a problem.

[jdenny]

In D65156#1608810, @jdenny wrote:

I'm thinking a more general fix is to implement not in lit's internal shell. not in front of any lit's internal shell commands is a problem.

Below is a quick hack to see if this approach solves the recent problems. Can @stella.stamenova, @probinson, and @mgorny (who all seem to have experienced slightly different variations on this problem) please check that "ninja check-lit" succeeds on their bots? My expectation is that D65335 is not necessary with this change. This is not a complete implementation of not, so I'm not sure other test suites will pass. I'll work on that afterward.

diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index ac627d51c1c1..042734b1f8e2 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -859,6 +859,11 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
         results.append(ShellCommandResult(cmd.commands[0], '', '', 0, False))
         return 0;
 
+    if cmd.commands[0].args[0] == 'not':
+        cmd.commands[0].args.pop(0)
+        res = _executeShCmd(cmd, shenv, results, timeoutHelper)
+        return res == 0
+
     procs = []
     default_stdin = subprocess.PIPE
     stderrTempFiles = []
diff --git a/llvm/utils/lit/tests/shtest-output-printing.py b/llvm/utils/lit/tests/shtest-output-printing.py
index 80304258fd95..57f830fbe920 100644
--- a/llvm/utils/lit/tests/shtest-output-printing.py
+++ b/llvm/utils/lit/tests/shtest-output-printing.py
@@ -24,7 +24,7 @@
 # CHECK-NEXT: hi
 #
 # CHECK:      $ ":" "RUN: at line 3"
-# CHECK-NEXT: $ "not" "not" "wc" "missing-file"
+# CHECK-NEXT: $ "wc" "missing-file"
 # CHECK-NEXT: # redirected output from '{{.*(/|\\\\)}}basic.txt.tmp.out':
 # CHECK-NEXT: {{cannot open missing-file|missing-file.* No such file or directory}}
 # CHECK:      note: command had no output on stdout or stderr

[jdenny]

I've noticed there is also an existing problem with env calling env: the former is executed by lit's internal shell, and the latter is an external command. That happens in some lit tests due to this patch's %{lit} substitution, so my quick hack yesterday might not have fixed all test failures people experienced.

It seems this issue is not going to resolve quickly. For the sake of the windows bots, I'm planning to revert this patch and r367123 (D65335) for now. Let me know if you object.

I've started working on solutions, which seem to make lit's internal shell more robust anyway.

[probinson]

Thanks for tracking down the internal/external thing.
not as an internal command might work; note that it takes an optional --crash which does something to make crash detection more reliable.

[probinson]

Is it feasible to turn off the FILECHECK variables for the entire 'lit' suite including its sample sub-suites?
I think that's an acceptable limitation. It is always possible to go back to the old way of debugging tests by running commands individually.

[jdenny]

In D65156#1610325, @probinson wrote:

Is it feasible to turn off the FILECHECK variables for the entire 'lit' suite including its sample sub-suites?
I think that's an acceptable limitation. It is always possible to go back to the old way of debugging tests by running commands individually.

I think that's feasible, and we can always change our minds later, so no objections here.

I'll continue to pick away at my approach for now as I'm already pretty far into it, and I find it more useful... unless people think it's doomed. :-)

llvm/trunk/utils/lit/tests/lit.cfg

 llvm_config.with_environment('PYTHONPATH', lit_path, append_path=True)
 
 # Add llvm and lit tools directories if this config is being loaded indirectly.
 for attribute in ('llvm_tools_dir', 'lit_tools_dir'):
     directory = getattr(config, attribute, None)
     if directory:
         llvm_config.with_environment('PATH', directory, append_path=True)
 
+# This test suite calls %{lit} to test lit's behavior for the sample test
+# suites in %{inputs}.  This test suite's results are then determined in part
+# by %{lit}'s textual output, which includes the output of FileCheck calls
+# within %{inputs}'s test suites.  Thus, %{lit} clears environment variables
+# that can affect FileCheck's output.
 config.substitutions.append(('%{inputs}', os.path.join(
     config.test_source_root, 'Inputs')))
-config.substitutions.append(('%{lit}', "%%{python} %s" % (
-    os.path.join(lit_path, 'lit.py'),)))
+config.substitutions.append(('%{lit}',
+    "{env} %{{python}} {lit}".format(
+        env="env -u FILECHECK_OPTS -u FILECHECK_DUMP_INPUT_ON_FAILURE",
+        lit=os.path.join(lit_path, 'lit.py'))))
 config.substitutions.append(('%{python}', '"%s"' % (sys.executable)))
 
 # Enable coverage.py reporting, assuming the coverage module has been installed
 # and sitecustomize.py in the virtualenv has been modified appropriately.
 if lit_config.params.get('check-coverage', None):
     config.environment['COVERAGE_PROCESS_START'] = os.path.join(
         os.path.dirname(__file__), ".coveragerc")
 
 # Add a feature to detect if psutil is available
 supported, errormsg = lit_config.maxIndividualTestTimeIsSupported
 if supported:
     config.available_features.add("lit-max-individual-test-time")
 else:
     lit_config.warning('Setting a timeout per test not supported. ' + errormsg
                        + ' Some tests will be skipped and the --timeout'
                          ' command line argument will not work.')

llvm/trunk/utils/lit/tests/shtest-run-at-line.py

 # Check that -vv makes the line number of the failing RUN command clear.
 # (-v is actually sufficient in the case of the internal shell.)
 #
-# RUN: env -u FILECHECK_OPTS not %{lit} -j 1 -vv %{inputs}/shtest-run-at-line > %t.out
+# RUN: not %{lit} -j 1 -vv %{inputs}/shtest-run-at-line > %t.out
 # RUN: FileCheck --input-file %t.out %s
 #
 # END.
 
 
 # CHECK: Testing: 4 tests