[lit] Handle plain negations directly in the internal shell

[mstorsjo]

Summary

Keep running "not --crash" via the external "not" executable, but
for plain negations, and for cases that use the shell "!" operator,
just skip that argument and invert the return code.

The libcxx tests only use the shell operator "!" for negations,
never the "not" executable, because libcxx tests can be run without
having a fully built llvm tree available providing the "not"
executable.

This allows using the internal shell for libcxx tests.

[rnk]

lgtm

[mstorsjo]

FWIW, I'm looking into a test failure on one buildbot relating to this, https://lab.llvm.org/buildbot/#/builders/123/builds/3464/steps/8/logs/stdio, where clang-tidy/infrastructure/validate-check-names.cpp now fails.

[jdenny]

Please add tests before committing this again.

[mstorsjo]

@jdenny - Updated with some tests. The existing tests in the shtest-not suite seem to cover the uses of 'not' quite well - so just for the change of making 'not' evaluated builtin those existing tests should cover all aspects I think. But for additionally handling '!' as equivalent to 'not' (except for the --crash option), I added a couple simple tests in the shtest-not suite - I didn't choose to duplicate all the aspects of suite but only a few trivial ones.

To actually be able to reland this, we'd need to figure out a sensible way to handle the unrelated breakage that just surfaces when this is in, e.g. with D99330 or something else similar.

[jdenny]

In D98859#2652464, @mstorsjo wrote:

@jdenny - Updated with some tests. The existing tests in the shtest-not suite seem to cover the uses of 'not' quite well - so just for the change of making 'not' evaluated builtin those existing tests should cover all aspects I think.

But lit's test suite doesn't check that internal not is actually used instead of external not, right? Is that change more a side effect than a goal of this patch?

If it seems worthwhile to verify that internal not continues to be used, we do have llvm/utils/lit/tests/Inputs/fake-externals, which is a directory of fake external commands that always fail. It's in PATH while running lit's own test suite to prove that external commands aren't called accidentally. You could add a simple not script there. The only challenge I foresee is that it would need to defer to the external not if --crash is the first argument.

But for additionally handling '!' as equivalent to 'not' (except for the --crash option), I added a couple simple tests in the shtest-not suite - I didn't choose to duplicate all the aspects of suite but only a few trivial ones.

Thanks for adding those.

I agree you don't need to replicate all existing not tests for !, but I feel there are a few more basic cases that ought to be covered. Most importantly, your new tests would pass even if ! always blindly has exit status zero. not-calls-diff.txt (at least) checks that not produces a non-zero exit status when expected. Also, there's no test that ! properly complains about a missing subcommand. That's a separate code path from when not doesn't have a subcommand.

To actually be able to reland this, we'd need to figure out a sensible way to handle the unrelated breakage that just surfaces when this is in, e.g. with D99330 or something else similar.

Understood. If possible, I'd like to leave that review to people who work regularly in windows.

[mstorsjo]

In D98859#2653046, @jdenny wrote:

But lit's test suite doesn't check that internal not is actually used instead of external not, right? Is that change more a side effect than a goal of this patch?

The main target is to make '!' work for negations when using the internal shell, as there's no '!' executable. (Libcxx uses '!' as it can be tested without building llvm executables.) Technically it would be enough to only make '!' a builtin and keep 'not' external - but that might be a bigger mess implementation wise. (That would sidestep the whole windows arg quoting can of worms that I opened though.)

If it seems worthwhile to verify that internal not continues to be used, we do have llvm/utils/lit/tests/Inputs/fake-externals, which is a directory of fake external commands that always fail. It's in PATH while running lit's own test suite to prove that external commands aren't called accidentally. You could add a simple not script there.

Ah, good point, I can try to add that.

The only challenge I foresee is that it would need to defer to the external not if --crash is the first argument.

Hmm, maybe if one updates $PATH to remove the path containing $0 and then executes not again...

But for additionally handling '!' as equivalent to 'not' (except for the --crash option), I added a couple simple tests in the shtest-not suite - I didn't choose to duplicate all the aspects of suite but only a few trivial ones.

Thanks for adding those.

I agree you don't need to replicate all existing not tests for !, but I feel there are a few more basic cases that ought to be covered. Most importantly, your new tests would pass even if ! always blindly has exit status zero. not-calls-diff.txt (at least) checks that not produces a non-zero exit status when expected. Also, there's no test that ! properly complains about a missing subcommand. That's a separate code path from when not doesn't have a subcommand.

Thanks, those are good points. Will add more tests for those cases.

To actually be able to reland this, we'd need to figure out a sensible way to handle the unrelated breakage that just surfaces when this is in, e.g. with D99330 or something else similar.

Understood. If possible, I'd like to leave that review to people who work regularly in windows.

Yeah, definitely :-) It's quite a mess...

[mstorsjo]

Improved the testcases for '!'.

Adding a fake external for the 'not' executable is a bit tricky though... With the code as is, for a sequence of 'not not --crash <cmd>', it deduces that there's a '--crash' option in there, and chooses not to switch to the internal not for the outer invocation either.

To fix that case, I'd have to change the code to peel off as many 'not' invocations from the top until the first one with a '--crash' option. Not sure if that's worth the extra complexity for the implementation - what do you think?

[mstorsjo]

In D98859#2662417, @mstorsjo wrote:

Adding a fake external for the 'not' executable is a bit tricky though... With the code as is, for a sequence of 'not not --crash <cmd>', it deduces that there's a '--crash' option in there, and chooses not to switch to the internal not for the outer invocation either.

To fix that case, I'd have to change the code to peel off as many 'not' invocations from the top until the first one with a '--crash' option. Not sure if that's worth the extra complexity for the implementation - what do you think?

@jdenny - with this change to the functionality, I could add a fake-external not to to the tests, to make sure we always evaluate all the leading instances of not/! internally, up to the first not --crash:

diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index fbb8e93058a2..8e07d09e8bd6 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -708,16 +708,18 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
                                                 " subcommand")
             elif args[0] == 'not':
                 not_args.append(args.pop(0))
-                not_count += 1
                 if args and args[0] == '--crash':
                     not_args.append(args.pop(0))
                     not_crash = True
+                if not not_crash:
+                    not_count += 1
                 if not args:
                     raise InternalShellError(j, "Error: 'not' requires a"
                                                 " subcommand")
             elif args[0] == '!':
                 not_args.append(args.pop(0))
-                not_count += 1
+                if not not_crash:
+                    not_count += 1
                 if not args:
                     raise InternalShellError(j, "Error: '!' requires a"
                                                 " subcommand")
@@ -769,13 +771,10 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
         # blindly pass along the status it receives from any command it calls.
         
         # For plain negations, either 'not' without '--crash', or the shell
-        # operator '!', leave them out from the command to execute and
-        # invert the result code afterwards.
-        if not_crash:
-            args = not_args + args
-            not_count = 0                       
-        else:
-            not_args = []
+        # operator '!' (counted via not_count), leave them out from the
+        # command to execute and invert the result code afterwards.
+        args = not_args[not_count:] + args
+        not_args = []
        
         stdin, stdout, stderr = processRedirects(j, default_stdin, cmd_shenv,
                                                  opened_files)

[jdenny]

In D98859#2662417, @mstorsjo wrote:

Improved the testcases for '!'.

Thanks. LGTM except of course the windows problem you're addressing elsewhere.

In D98859#2663010, @mstorsjo wrote:

In D98859#2662417, @mstorsjo wrote:

Adding a fake external for the 'not' executable is a bit tricky though... With the code as is, for a sequence of 'not not --crash <cmd>', it deduces that there's a '--crash' option in there, and chooses not to switch to the internal not for the outer invocation either.

To fix that case, I'd have to change the code to peel off as many 'not' invocations from the top until the first one with a '--crash' option. Not sure if that's worth the extra complexity for the implementation - what do you think?

@jdenny - with this change to the functionality, I could add a fake-external not to to the tests, to make sure we always evaluate all the leading instances of not/! internally, up to the first not --crash:

With the change you proposed, what happens to not --crash not cmd? Won't the inner not be caught as a fake external? I suppose the not --crash would fail either way, but stderr would be different, and it would be more confusing to debug.

It seems to me this is getting too confusing. Sorry, my fake external suggestion is probably not worth the effort right now. Thanks for looking into it.

If you agree with that assessment, would you extend the fake-externals comments in llvm/utils/lit/tests/lit.cfg to mention why not isn't included? You might say: Don't do this for 'not' because lit uses the external 'not' throughout a RUN line that calls 'not --crash'.

Eventually, it would be great to see all this handling simplified. It seems one way to do that is to implement not --crash in python. But that's a different patch.

[mstorsjo]

In D98859#2669167, @jdenny wrote:

With the change you proposed, what happens to not --crash not cmd? Won't the inner not be caught as a fake external? I suppose the not --crash would fail either way, but stderr would be different, and it would be more confusing to debug.

No, that case would work. My fake not executable looks like this:

#!/usr/bin/env python
 
import fake_external
import os
import subprocess
import sys
 
 
if len(sys.argv) > 1 and sys.argv[1] == '--crash':
  # Call the real 'not' binary; remove the dirname of argv[0] from the PATH
  paths = os.environ['PATH'].split(os.pathsep)
  cur_dir = os.path.dirname(sys.argv[0])
  if cur_dir in paths:
    paths.remove(cur_dir)
  os.environ['PATH'] = os.pathsep.join(paths)
  # Call the other 'not', replace argv[0] with a 'not' without a potential
  # path, to let it search for it in the path.
  args = sys.argv
  args[0] = 'not'
  sys.exit(subprocess.call(args))
 
fake_external.execute(__file__)

So the fake-externals dir gets removed from the path when re-executing not --crash.

It seems to me this is getting too confusing. Sorry, my fake external suggestion is probably not worth the effort right now. Thanks for looking into it.

If you agree with that assessment, would you extend the fake-externals comments in llvm/utils/lit/tests/lit.cfg to mention why not isn't included? You might say: Don't do this for 'not' because lit uses the external 'not' throughout a RUN line that calls 'not --crash'.

Sure, that sounds good to me.

Eventually, it would be great to see all this handling simplified. It seems one way to do that is to implement not --crash in python. But that's a different patch.

Yup, that sounds like a more worthwhile future direction.

[jdenny]

Ah, I see. Even though it wouldn't add very much code, it sounds like we agree that the conceptual complexity isn't worthwhile given the direction we'd like to go eventually. Thanks again for looking into it.

[mstorsjo]

Added the comment about why we don't have a fake external 'not'; leaving this review in approved state until we figure out a way forward with either D99330 or D99406.

[mstorsjo]

FWIW, it should be possible to reapply this one now after D99938 landed, but I'll let the dust settle after that one first, before proceeding with this one.

[hubert.reinterpretcast]

llvm/utils/lit/lit/TestRunner.py

781

Hello, this logic is not actually sound: There are tests that are sensitive to the specific return code and not just whether the result is zero or not.

From http://lab.llvm.org:8014/#/builders/126/builds/273/steps/5/logs/stdio:

/scratch/powerllvm/powerllvm_env/aix-ppc64-ppc64le/clang-ppc64-aix-ppc64le/stage1/utils/lit/tests/shtest-output-printing.py:15:15: error: CHECK-NEXT: expected string not found in input
# CHECK-NEXT: Exit Code: 1
              ^
/scratch/powerllvm/powerllvm_env/aix-ppc64-ppc64le/clang-ppc64-aix-ppc64le/stage1/utils/lit/tests/Output/shtest-output-printing.py.tmp.out:9:3: note: scanning from here
--
  ^
/scratch/powerllvm/powerllvm_env/aix-ppc64-ppc64le/clang-ppc64-aix-ppc64le/stage1/utils/lit/tests/Output/shtest-output-printing.py.tmp.out:10:1: note: possible intended match here
Exit Code: 2
^

Input file: /scratch/powerllvm/powerllvm_env/aix-ppc64-ppc64le/clang-ppc64-aix-ppc64le/stage1/utils/lit/tests/Output/shtest-output-printing.py.tmp.out
Check file: /scratch/powerllvm/powerllvm_env/aix-ppc64-ppc64le/clang-ppc64-aix-ppc64le/stage1/utils/lit/tests/shtest-output-printing.py

-dump-input=help explains the following input dump.

Input was:
<<<<<<
           1: -- Testing: 1 tests, 1 workers --_
           2: FAIL: shtest-output-printing :: basic.txt (1 of 1)_
           3: ******************** TEST 'shtest-output-printing :: basic.txt' FAILED ********************_
           4: Script:_
           5: --_
           6: : 'RUN: at line 1'; true_
           7: : 'RUN: at line 2'; echo hi_
           8: : 'RUN: at line 3'; not not wc missing-file &> /scratch/powerllvm/powerllvm_env/aix-ppc64-ppc64le/clang-ppc64-aix-ppc64le/stage1/utils/lit/tests/Inputs/shtest-output-printing/Output/basic.txt.tmp.out_
           9: --_
next:15'0       X error: no match found
          10: Exit Code: 2_
next:15'0     ~~~~~~~~~~~~~
next:15'1     ?             possible intended match

@mstorsjo, please revert if a fix (with an appropriate test) is not forthcoming.

[mstorsjo]

llvm/utils/lit/lit/TestRunner.py

781

Thanks for pointing this out, and sorry for the breakage. I'll push a fix including a test for it momentarily.

[mstorsjo]

llvm/utils/lit/lit/TestRunner.py

781

@jdenny I pushed a fix for this in rGd0b03ec401e8465b88893a4c56aeb0c787a54ad9, I hope you don't mind - let me know if you'd want changes to it.

[hubert.reinterpretcast]

llvm/utils/lit/lit/TestRunner.py

781

Thanks @mstorsjo for the quick fix! I'm seeing the test pass on AIX now.

llvm/utils/lit/lit/TestRunner.py

if isinstance(cmd, ShUtil.Seq):
             if res == 0:
                 res = _executeShCmd(cmd.rhs, shenv, results, timeoutHelper)
             return res
 
         raise ValueError('Unknown shell command: %r' % cmd.op)
     assert isinstance(cmd, ShUtil.Pipeline)
 
     procs = []
+    negate_procs = []
     default_stdin = subprocess.PIPE
     stderrTempFiles = []
     opened_files = []
     named_temp_files = []
     builtin_commands = set(['cat', 'diff'])
     builtin_commands_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), "builtin_commands")
     inproc_builtins = {'cd': executeBuiltinCd,
                        'export': executeBuiltinExport,
for i,j in enumerate(cmd.commands):
                 not_args.append(args.pop(0))
                 not_count += 1
                 if args and args[0] == '--crash':
                     not_args.append(args.pop(0))
                     not_crash = True
                 if not args:
                     raise InternalShellError(j, "Error: 'not' requires a"
                                                 " subcommand")
+            elif args[0] == '!':
+                not_args.append(args.pop(0))
+                not_count += 1
+                if not args:
+                    raise InternalShellError(j, "Error: '!' requires a"
+                                                " subcommand")
             else:
                 break
 
         # Handle in-process builtins.
         #
         # Handle "echo" as a builtin if it is not part of a pipeline. This
         # greatly speeds up tests that construct input files by repeatedly
         # echo-appending to a file.
for i,j in enumerate(cmd.commands):
         # We had to search through the 'not' commands to find all the 'env'
         # commands and any other in-process builtin command.  We don't want to
         # reimplement 'not' and its '--crash' here, so just push all 'not'
         # commands back to be called as external commands.  Because this
         # approach effectively moves all 'env' commands up front, it relies on
         # the assumptions that (1) environment variables are not intended to be
         # relevant to 'not' commands and (2) the 'env' command should always
         # blindly pass along the status it receives from any command it calls.
+
+        # For plain negations, either 'not' without '--crash', or the shell
+        # operator '!', leave them out from the command to execute and
+        # invert the result code afterwards.
+        if not_crash:
             args = not_args + args
+            not_count = 0
+        else:
+            not_args = []
 
         stdin, stdout, stderr = processRedirects(j, default_stdin, cmd_shenv,
                                                  opened_files)
 
         # If stderr wants to come from stdout, but stdout isn't a pipe, then put
         # stderr on a pipe and treat it as stdout.
         if (stderr == subprocess.STDOUT and stdout != subprocess.PIPE):
             stderr = subprocess.PIPE
for i,j in enumerate(cmd.commands):
         try:
             procs.append(subprocess.Popen(args, cwd=cmd_shenv.cwd,
                                           executable = executable,
                                           stdin = stdin,
                                           stdout = stdout,
                                           stderr = stderr,
                                           env = cmd_shenv.env,
                                           close_fds = kUseCloseFDs))
+            negate_procs.append((not_count % 2) != 0)
             # Let the helper know about this process
             timeoutHelper.addProcess(procs[-1])
         except OSError as e:
             raise InternalShellError(j, 'Could not create process ({}) due to {}'.format(executable, e))
 
         # Immediately close stdin for any process taking stdin from us.
         if stdin == subprocess.PIPE:
             procs[-1].stdin.close()
for i,f in stderrTempFiles:
         f.close()
 
     exitCode = None
     for i,(out,err) in enumerate(procData):
         res = procs[i].wait()
         # Detect Ctrl-C in subprocess.
         if res == -signal.SIGINT:
             raise KeyboardInterrupt
+        if negate_procs[i]:
+            res = not res
 
         # Ensure the resulting output is always of string type.
         try:
             if out is None:
                 out = ''
             else:
                 out = to_string(out.decode('utf-8', errors='replace'))
         except:

llvm/utils/lit/tests/Inputs/shtest-not/exclamation-args-nested-none.txt

+# RUN: ! ! !

llvm/utils/lit/tests/Inputs/shtest-not/exclamation-args-none.txt

+# RUN: !

llvm/utils/lit/tests/Inputs/shtest-not/exclamation-calls-external.txt

+# Simple uses of '!'
+
+# RUN: ! %{python} fail.py
+# RUN: ! ! %{python} pass.py
+# RUN: ! ! ! %{python} fail.py
+# RUN: ! ! ! ! %{python} pass.py
+
+# pass.py succeeds but we expect failure
+# RUN: ! %{python} pass.py

llvm/utils/lit/tests/shtest-not.py

 # Check the not command
 
 # FIXME: this test depends on order of tests
 # RUN: rm -f %{inputs}/shtest-not/.lit_test_times.txt
 
 # RUN: not %{lit} -j 1 -a -v %{inputs}/shtest-not \
 # RUN: | FileCheck -match-full-lines %s
 #
 # END.
 
 # Make sure not and env commands are included in printed commands.
 
-# CHECK: -- Testing: 13 tests{{.*}}
+# CHECK: -- Testing: 16 tests{{.*}}
+
+# CHECK: FAIL: shtest-not :: exclamation-args-nested-none.txt {{.*}}
+# CHECK: $ "!" "!" "!"
+# CHECK: Error: '!' requires a subcommand
+# CHECK: error: command failed with exit status: {{.*}}
+
+# CHECK: FAIL: shtest-not :: exclamation-args-none.txt {{.*}}
+# CHECK: $ "!"
+# CHECK: Error: '!' requires a subcommand
+# CHECK: error: command failed with exit status: {{.*}}
+
+# CHECK: FAIL: shtest-not :: exclamation-calls-external.txt {{.*}}
+
+# CHECK: $ "!" "{{[^"]*}}" "fail.py"
+# CHECK: $ "!" "!" "{{[^"]*}}" "pass.py"
+# CHECK: $ "!" "!" "!" "{{[^"]*}}" "fail.py"
+# CHECK: $ "!" "!" "!" "!" "{{[^"]*}}" "pass.py"
+
+# CHECK: $ "!" "{{[^"]*}}" "pass.py"
+# CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-not :: not-args-last-is-crash.txt {{.*}}
 # CHECK: $ "not" "--crash"
 # CHECK: Error: 'not' requires a subcommand
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-not :: not-args-nested-none.txt {{.*}}
 # CHECK: $ "not" "not" "not"

 
 # CHECK: FAIL: shtest-not :: not-calls-rm.txt {{.*}}
 # CHECK: $ "not" "rm" {{.*}}
 # CHECK: $ "not" "--crash" "rm" "foobar"
 # CHECK: Error: 'not --crash' cannot call 'rm'
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: Passed:  1
-# CHECK: Failed: 12
+# CHECK: Failed: 15
 # CHECK-NOT: {{.}}