[lit] Fix internal env calling other internal commands

[jdenny]

Summary

Without this patch, when using lit's internal shell, if env on a lit
RUN line calls cd, diff, or any of the other in-process shell
builtins that lit implements, lit accidentally searches for the latter
as an external executable.

This patch puts such builtins in a map so that boilerplate for them
need only be implemented once. This patch moves that handling after
processing of env so that env calling such a builtin can be
detected. Finally, because such calls appears to be useless, this
patch takes the safe approach of diagnosing them rather than
supporting them.

[rnk]

lgtm

I suppose you could add a test for composing env with in process builtins, but I think that's already in the next patch.

[jdenny]

In D66506#1638493, @rnk wrote:

lgtm

I suppose you could add a test for composing env with in process builtins, but I think that's already in the next patch.

Good point. I originally wrote this patch after the next one, and I forgot about that change when I reversed them. That also means this patch isn't NFC.

I'll clean that up before pushing.

Thanks!

[jdenny]

As discussed, moved relevant tests from D65697 to this patch, and rewrote summary to describe the non-NFC parts of this change.

llvm/utils/lit/lit/TestRunner.py

for arg_idx, arg in enumerate(args[1:]):
         key, eq, val = arg.partition('=')
         # Stop if there was no equals.
         if eq == '':
             arg_idx_next = arg_idx + 1
             break
         env.env[key] = val
     return args[arg_idx_next:]
 
+def executeBuiltinCd(cmd, shenv):
+    """executeBuiltinCd - Change the current directory."""
+    if len(cmd.args) != 2:
+        raise InternalShellError("'cd' supports only one argument")
+    newdir = cmd.args[1]
+    # Update the cwd in the parent environment.
+    if os.path.isabs(newdir):
+        shenv.cwd = newdir
+    else:
+        shenv.cwd = os.path.realpath(os.path.join(shenv.cwd, newdir))
+    # The cd builtin always succeeds. If the directory does not exist, the
+    # following Popen calls will fail instead.
+    return ShellCommandResult(cmd, "", "", 0, False)
+
+def executeBuiltinExport(cmd, shenv):
+    """executeBuiltinExport - Set an environment variable."""
+    if len(cmd.args) != 2:
+        raise InternalShellError("'export' supports only one argument")
+    updateEnv(shenv, cmd.args)
+    return ShellCommandResult(cmd, "", "", 0, False)
+
 def executeBuiltinEcho(cmd, shenv):
     """Interpret a redirected echo command"""
     opened_files = []
     stdin, stdout, stderr = processRedirects(cmd, subprocess.PIPE, shenv,
                                              opened_files)
     if stdin != subprocess.PIPE or stderr != subprocess.PIPE:
         raise InternalShellError(
                 cmd, "stdin and stderr redirects not supported for echo")
if args:
             stdout.write(encode(' '))
         stdout.write(encode(maybeUnescape(args[-1])))
     if write_newline:
         stdout.write(encode('\n'))
 
     for (name, mode, f, path) in opened_files:
         f.close()
 
-    if not is_redirected:
-        return stdout.getvalue()
-    return ""
+    output = "" if is_redirected else stdout.getvalue()
+    return ShellCommandResult(cmd, output, "", 0, False)
 
 def executeBuiltinMkdir(cmd, cmd_shenv):
     """executeBuiltinMkdir - Create new directories."""
     args = expand_glob_expressions(cmd.args, cmd_shenv.cwd)[1:]
     try:
         opts, args = getopt.gnu_getopt(args, 'p')
     except getopt.GetoptError as err:
         raise InternalShellError(cmd, "Unsupported: 'mkdir':  %s" % str(err))
for path in args:
                     os.chmod(path,
                              stat.S_IMODE(os.stat(path).st_mode) | stat.S_IWRITE)
                 os.remove(path)
         except OSError as err:
             stderr.write("Error: 'rm' command failed, %s" % str(err))
             exitCode = 1
     return ShellCommandResult(cmd, "", stderr.getvalue(), exitCode, False)
 
+def executeBuiltinColon(cmd, cmd_shenv):
+    """executeBuiltinColon - Discard arguments and exit with status 0."""
+    return ShellCommandResult(cmd, "", "", 0, False)
+
 def processRedirects(cmd, stdin_source, cmd_shenv, opened_files):
     """Return the standard fds for cmd after applying redirects
 
     Returns the three standard file descriptors for the new child process.  Each
     fd may be an open, writable file object or a sentinel value from the
     subprocess module.
     """
 
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)
 
-    # Handle shell builtins first.
-    if cmd.commands[0].args[0] == 'cd':
-        if len(cmd.commands) != 1:
-            raise ValueError("'cd' cannot be part of a pipeline")
-        if len(cmd.commands[0].args) != 2:
-            raise ValueError("'cd' supports only one argument")
-        newdir = cmd.commands[0].args[1]
-        # Update the cwd in the parent environment.
-        if os.path.isabs(newdir):
-            shenv.cwd = newdir
-        else:
-            shenv.cwd = os.path.realpath(os.path.join(shenv.cwd, newdir))
-        # The cd builtin always succeeds. If the directory does not exist, the
-        # following Popen calls will fail instead.
-        return 0
-
-    # 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.
-    # FIXME: Standardize on the builtin echo implementation. We can use a
-    # temporary file to sidestep blocking pipe write issues.
-    if cmd.commands[0].args[0] == 'echo' and len(cmd.commands) == 1:
-        output = executeBuiltinEcho(cmd.commands[0], shenv)
-        results.append(ShellCommandResult(cmd.commands[0], output, "", 0,
-                                          False))
-        return 0
-
-    if cmd.commands[0].args[0] == 'export':
-        if len(cmd.commands) != 1:
-            raise ValueError("'export' cannot be part of a pipeline")
-        if len(cmd.commands[0].args) != 2:
-            raise ValueError("'export' supports only one argument")
-        updateEnv(shenv, cmd.commands[0].args)
-        return 0
-
-    if cmd.commands[0].args[0] == 'mkdir':
-        if len(cmd.commands) != 1:
-            raise InternalShellError(cmd.commands[0], "Unsupported: 'mkdir' "
-                                     "cannot be part of a pipeline")
-        cmdResult = executeBuiltinMkdir(cmd.commands[0], shenv)
-        results.append(cmdResult)
-        return cmdResult.exitCode
-
-    if cmd.commands[0].args[0] == 'rm':
-        if len(cmd.commands) != 1:
-            raise InternalShellError(cmd.commands[0], "Unsupported: 'rm' "
-                                     "cannot be part of a pipeline")
-        cmdResult = executeBuiltinRm(cmd.commands[0], shenv)
-        results.append(cmdResult)
-        return cmdResult.exitCode
-
-    if cmd.commands[0].args[0] == ':':
-        if len(cmd.commands) != 1:
-            raise InternalShellError(cmd.commands[0], "Unsupported: ':' "
-                                     "cannot be part of a pipeline")
-        results.append(ShellCommandResult(cmd.commands[0], '', '', 0, False))
-        return 0;
-
     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,
+                       'echo': executeBuiltinEcho,
+                       'mkdir': executeBuiltinMkdir,
+                       'rm': executeBuiltinRm,
+                       ':': executeBuiltinColon}
     # To avoid deadlock, we use a single stderr stream for piped
     # output. This is null until we have seen some output using
     # stderr.
     for i,j in enumerate(cmd.commands):
         # Reference the global environment by default.
         cmd_shenv = shenv
         args = list(j.args)
         if j.args[0] == 'env':
             # Create a copy of the global environment and modify it for this one
             # command. There might be multiple envs in a pipeline:
             #   env FOO=1 llc < %s | env BAR=2 llvm-mc | FileCheck %s
             cmd_shenv = ShellEnvironment(shenv.cwd, shenv.env)
             args = updateEnv(cmd_shenv, j.args)
             if not args:
                 raise InternalShellError(j,
                                          "Error: 'env' requires a subcommand")
 
+        # 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.
+        # FIXME: Standardize on the builtin echo implementation. We can use a
+        # temporary file to sidestep blocking pipe write issues.
+        inproc_builtin = inproc_builtins.get(args[0], None)
+        if inproc_builtin and (args[0] != 'echo' or len(cmd.commands) == 1):
+            # env calling an in-process builtin is useless, so we take the safe
+            # approach of complaining.
+            if not cmd_shenv is shenv:
+                raise InternalShellError(j, "Error: 'env' cannot call '{}'"
+                                            .format(args[0]))
+            if len(cmd.commands) != 1:
+                raise InternalShellError(j, "Unsupported: '{}' cannot be part"
+                                            " of a pipeline".format(args[0]))
+            result = inproc_builtin(j, cmd_shenv)
+            results.append(result)
+            return result.exitCode
+
         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
             stderrIsStdout = True

llvm/utils/lit/tests/Inputs/shtest-env/env-calls-cd.txt

+# RUN: env -u FOO BAR=3 cd foobar

llvm/utils/lit/tests/Inputs/shtest-env/env-calls-colon.txt

+# RUN: env -u FOO BAR=3 :

llvm/utils/lit/tests/Inputs/shtest-env/env-calls-echo.txt

+# RUN: env -u FOO BAR=3 echo hello world

llvm/utils/lit/tests/Inputs/shtest-env/env-calls-export.txt

+# RUN: env -u FOO BAR=3 export BAZ=3

llvm/utils/lit/tests/Inputs/shtest-env/env-calls-mkdir.txt

+# RUN: env -u FOO BAR=3 mkdir foobar

llvm/utils/lit/tests/Inputs/shtest-env/env-calls-rm.txt

+# RUN: env -u FOO BAR=3 rm foobar

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

 # Check the env command
 #
 # RUN: not %{lit} -j 1 -a -v %{inputs}/shtest-env \
 # RUN: | FileCheck -match-full-lines %s
 #
 # END.
 
 # Make sure env commands are included in printed commands.
 
-# CHECK: -- Testing: 7 tests{{.*}}
+# CHECK: -- Testing: 13 tests{{.*}}
 
 # CHECK: FAIL: shtest-env :: env-args-last-is-assign.txt ({{[^)]*}})
 # CHECK: Error: 'env' requires a subcommand
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-env :: env-args-last-is-u-arg.txt ({{[^)]*}})
 # CHECK: Error: 'env' requires a subcommand
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-env :: env-args-last-is-u.txt ({{[^)]*}})
 # CHECK: Error: 'env' requires a subcommand
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-env :: env-args-none.txt ({{[^)]*}})
 # CHECK: Error: 'env' requires a subcommand
 # CHECK: error: command failed with exit status: {{.*}}
 
+# CHECK: FAIL: shtest-env :: env-calls-cd.txt ({{[^)]*}})
+# CHECK: $ "env" "-u" "FOO" "BAR=3" "cd" "foobar"
+# CHECK: Error: 'env' cannot call 'cd'
+# CHECK: error: command failed with exit status: {{.*}}
+
+# CHECK: FAIL: shtest-env :: env-calls-colon.txt ({{[^)]*}})
+# CHECK: $ "env" "-u" "FOO" "BAR=3" ":"
+# CHECK: Error: 'env' cannot call ':'
+# CHECK: error: command failed with exit status: {{.*}}
+
+# CHECK: FAIL: shtest-env :: env-calls-echo.txt ({{[^)]*}})
+# CHECK: $ "env" "-u" "FOO" "BAR=3" "echo" "hello" "world"
+# CHECK: Error: 'env' cannot call 'echo'
+# CHECK: error: command failed with exit status: {{.*}}
+
+# CHECK: FAIL: shtest-env :: env-calls-export.txt ({{[^)]*}})
+# CHECK: $ "env" "-u" "FOO" "BAR=3" "export" "BAZ=3"
+# CHECK: Error: 'env' cannot call 'export'
+# CHECK: error: command failed with exit status: {{.*}}
+
+# CHECK: FAIL: shtest-env :: env-calls-mkdir.txt ({{[^)]*}})
+# CHECK: $ "env" "-u" "FOO" "BAR=3" "mkdir" "foobar"
+# CHECK: Error: 'env' cannot call 'mkdir'
+# CHECK: error: command failed with exit status: {{.*}}
+
+# CHECK: FAIL: shtest-env :: env-calls-rm.txt ({{[^)]*}})
+# CHECK: $ "env" "-u" "FOO" "BAR=3" "rm" "foobar"
+# CHECK: Error: 'env' cannot call 'rm'
+# CHECK: error: command failed with exit status: {{.*}}
+
 # CHECK: PASS: shtest-env :: env-u.txt ({{[^)]*}})
 # CHECK: $ "{{[^"]*}}" "print_environment.py"
 # CHECK: $ "env" "-u" "FOO" "{{[^"]*}}" "print_environment.py"
 # CHECK: $ "env" "-u" "FOO" "-u" "BAR" "{{[^"]*}}" "print_environment.py"
+# CHECK-NOT: ${{.*}}print_environment.py
 
 # CHECK: PASS: shtest-env :: env.txt ({{[^)]*}})
 # CHECK: $ "env" "A_FOO=999" "{{[^"]*}}" "print_environment.py"
 # CHECK: $ "env" "A_FOO=1" "B_BAR=2" "C_OOF=3" "{{[^"]*}}" "print_environment.py"
+# CHECK-NOT: ${{.*}}print_environment.py
 
 # CHECK: PASS: shtest-env :: mixed.txt ({{[^)]*}})
 # CHECK: $ "env" "A_FOO=999" "-u" "FOO" "{{[^"]*}}" "print_environment.py"
 # CHECK: $ "env" "A_FOO=1" "-u" "FOO" "B_BAR=2" "-u" "BAR" "C_OOF=3" "{{[^"]*}}" "print_environment.py"
+# CHECK-NOT: ${{.*}}print_environment.py
 
 # CHECK: Expected Passes : 3
-# CHECK: Unexpected Failures: 4
+# CHECK: Unexpected Failures: 10
 # CHECK-NOT: {{.}}