[lit] Fix tool substitution when paths or arguments contain spaces

[zturner]

Summary

If you try to substitute a path or argument with spaces in it, this would lead to an invalid substitution. This should fix it.

[stella.stamenova]

I tested this with your other change and the tests ran successfully. Some of the test code does use single quotes on Windows and double quotes on Linux, though, so it may be worth checking for both regardless of platform.

[rnk]

llvm/utils/lit/lit/llvm/subst.py

134–139

This is in LLVM, so let's just assume we're dealing with the lit internal shell or bash. In that case, we can use ' everywhere and the escaping is easy:

return "\'" + x.replace(r"'", r"'\''") + "\'"

Then you don't need a big comment explaining why we only care about spaces etc.

[zturner]

So unfortunately I think this isn't going to work, and I don't think there's a good way to fix it. This is just one example, but it illustrates the general problem that comes up in several other places as well.

Somewhere, for clang, we have this:

ToolSubst('%clang_analyze_cc1', command='%clang_cc1', extra_args=['-analyze', '%analyze']+additional_flags),
ToolSubst('%clang_cc1', command=self.config.clang, extra_args=['-cc1', '-internal-isystem', builtin_include_dir, '-nostdsysteminc']+additional_flags),

The order here matters. Since we apply substitutions in forward list order, if we see %clang_analyze_cc1 in a test file, we apply the first substitution, and since now everything is quoted, it is replaced with this:

'%clang_cc1' '-analyze' '%analyze`

Now we apply substitutions again, and we end up with this:

''path/to/clang' '-cc1' '-internal-system' 'some/dir' '-nostdsysteminc''

So now this entire string is quoted twice. By the time it goes to actually run the line, you have something like this:

"d:\src\llvmbuild\cl\debug\x64\bin\clang.EXE -cc1 -internal-isystem d:\src\llvmbuild\cl\debug\x64\lib\clang\8.0.0\include -nostdsysteminc" "-analyze" "-analyzer-constraints=range" "-triple" "x86_64-unknown-linux-gnu" "-analyzer-checker=core" "-verify" "D:\src\llvm-mono\clang\test\Analysis\z3\apsint.c"

Note here that d:\src\llvmbuild\cl\debug\x64\bin\clang.EXE -cc1 -internal-isystem d:\src\llvmbuild\cl\debug\x64\lib\clang\8.0.0\include -nostdsysteminc will be treated as a single executable name, which obviously is incorrect.

I don't really think there's a good way to handle this. We could do something like force the user to write:

ToolSubst('%clang_analyze_cc1', command=NestedSubstitution('%clang_cc1'), extra_args=['-analyze', NestedSubstitution('%analyze')]+additional_flags)

but that seems to just be adding more complexity. So I'll probably just fix this at the call site.

llvm/utils/lit/lit/llvm/subst.py

 import os
 import re
+import sys
 
 import lit.util
 
 expr = re.compile(r"^(\\)?((\| )?)\W+b(\S+)\\b\W*$")
 wordifier = re.compile(r"(\W*)(\b[^\b]+\b)")
 
 
 class FindTool(object):
def resolve(self, config, search_dirs):
         tool_name = tool_match.group(4)
 
         if isinstance(self.command, FindTool):
             command_str = self.command.resolve(config, search_dirs)
         else:
             command_str = str(self.command)
 
         if command_str:
+            args = [command_str]
             if self.extra_args:
-                command_str = ' '.join([command_str] + self.extra_args)
+                args = args + self.extra_args
+
+            # This isn't intended to be perfect, as doing so is more trouble
+            # than it's worth.  We just want to catch strings with unquoted
+            # spaces, since that is the use case that comes up the most
+            # frequently.  However, as the user might explicitly quote them,
+            # we do try to avoid quoting already quoted strings.
+            def quote(x):
+                if ' ' not in x:
+                    return x
+                quote_ch = '"' if sys.platform == 'win32' else "'"
+                if x[0] != quote_ch:
+                    x = quote_ch + x
+                if x[-1] != quote_ch:
+                    x = x + quote_ch
+                return x
+            command_str = ' '.join(quote(x) for x in args)
         else:
             if self.unresolved == 'warn':
                 # Warn, but still provide a substitution.
                 config.lit_config.note(
                     'Did not find ' + tool_name + ' in %s' % search_dirs)
                 command_str = os.path.join(
                     config.config.llvm_tools_dir, tool_name)
             elif self.unresolved == 'fatal':