Page MenuHomePhabricator

[lit] Protect full test suite from FILECHECK_OPTS
ClosedPublic

Authored by jdenny on Jul 23 2019, 9:59 AM.

Details

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).

Diff Detail

Repository
rL LLVM

Event Timeline

jdenny created this revision.Jul 23 2019, 9:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 9:59 AM

This patch was extracted from D65121.

This revision is now accepted and ready to land.Jul 24 2019, 8:20 AM
This revision was automatically updated to reflect the committed changes.

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

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.)

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 ....

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?

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.

HERE: useExternalSh: False

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.

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.

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.

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 added a comment.Aug 1 2019, 7:53 AM

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.

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.

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 added a comment.Aug 1 2019, 9:32 AM

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. :-)