[lit] Don't expand escapes until all substitutions have been applied

[broadwaylamb]

Summary

Otherwise, if a Lit script contains escaped substitutions (like %%p in this test), they are unescaped during recursive application of substitutions, and the results are unexpected.

We solve it using the fact that double percent signs are first replaced with #_MARKER_#, and only after all the other substitutions have been applied, #_MARKER_# is replaced with a single percent sign. The only change is that instead of replacing #_MARKER_# at each recursion step, we replace it once after the last recursion step.

[ldionne]

llvm/utils/lit/lit/TestRunner.py

1173

I was working on a similar change locally, and the approach I took instead was this:

1. Remove both ('%%', '#_MARKER_#') and ('#_MARKER_#', '%') from the default substitutions, which is done above in getDefaultSubstitutions.
2. Define the following functions in applySubstitutions:

def escape(ln):
    return _caching_re_compile('%%').sub('#_MARKER_#', ln)

def unescape(ln):
    return _caching_re_compile('#_MARKER_#').sub('%', ln)

3. Apply those functions around the actual substitution: return list(map(lambda ln: unescape(process(escape(ln))), script)).

This approach seems simpler to me and I think it's a bit more explicit in what we're trying to achieve. It also has the added benefit that we don't clutter the list of substitutions with those substitutions that are implementation details of the escape mechanism only.

WDYT?

llvm/utils/lit/tests/shtest-recursive-substitution.py

23–27

Can you add a new test case instead of piggy-backing on the existing ones? It seems cleaner to me to test one correctness aspect for each test case.

[broadwaylamb]

Address the review comments

[ldionne]

llvm/utils/lit/lit/TestRunner.py

1203

I guess list() is now redundant.

[broadwaylamb]

Replace %% with #_MARKER_# before each substitution.

[broadwaylamb]

llvm/utils/lit/lit/TestRunner.py

1203

Oh, for some reason I thought list comprehensions are lazy. One moment.

[broadwaylamb]

Remove redundant conversion of a list comprehension to list.

[ldionne]

Please make sure you run a few test suites. Ideally at least:

check-clang
check-lld
check-llvm
check-cxx
check-cxxabi
check-unwind

[broadwaylamb]

I've verified that it works with the test suites that you've mentioned, and also compiler-rt tests. I'm going to commit it now.

llvm/utils/lit/lit/TestRunner.py

def getDefaultSubstitutions(test, tmpDir, tmpBase, normalize_slashes=False):
 
     # Normalize slashes, if requested.
     if normalize_slashes:
         sourcepath = sourcepath.replace('\\', '/')
         sourcedir = sourcedir.replace('\\', '/')
         tmpDir = tmpDir.replace('\\', '/')
         tmpBase = tmpBase.replace('\\', '/')
 
-    # We use #_MARKER_# to hide %% while we do the other substitutions.
     substitutions = []
-    substitutions.extend([('%%', '#_MARKER_#')])
     substitutions.extend(test.config.substitutions)
     tmpName = tmpBase + '.tmp'
     baseName = os.path.basename(tmpBase)
     substitutions.extend([('%s', sourcepath),
                           ('%S', sourcedir),
                           ('%p', sourcedir),
                           ('%{pathsep}', os.pathsep),
                           ('%t', tmpName),
                           ('%basename_t', baseName),
-                          ('%T', tmpDir),
-                          ('#_MARKER_#', '%')])
+                          ('%T', tmpDir)])
 
     # "%/[STpst]" should be normalized.
     substitutions.extend([
             ('%/s', sourcepath.replace('\\', '/')),
             ('%/S', sourcedir.replace('\\', '/')),
             ('%/p', sourcedir.replace('\\', '/')),
             ('%/t', tmpBase.replace('\\', '/') + '.tmp'),
             ('%/T', tmpDir.replace('\\', '/')),
def applySubstitutions(script, substitutions, recursion_limit=None):
     substitution b in line ln.
 
     If a substitution expands into another substitution, it is expanded
     recursively until the line has no more expandable substitutions. If
     the line can still can be substituted after being substituted
     `recursion_limit` times, it is an error. If the `recursion_limit` is
     `None` (the default), no recursive substitution is performed at all.
     """
+
+    # We use #_MARKER_# to hide %% while we do the other substitutions.
+    def escape(ln):
+        return _caching_re_compile('%%').sub('#_MARKER_#', ln)
+
+    def unescape(ln):
+        return _caching_re_compile('#_MARKER_#').sub('%', ln)
+
     def processLine(ln):
         # Apply substitutions
         for a,b in substitutions:
             if kIsWindows:
                 b = b.replace("\\","\\\\")
             # re.compile() has a built-in LRU cache with 512 entries. In some
             # test suites lit ends up thrashing that cache, which made e.g.
             # check-llvm run 50% slower.  Use an explicit, unbounded cache
             # to prevent that from happening.  Since lit is fairly
             # short-lived, since the set of substitutions is fairly small, and
             # since thrashing has such bad consequences, not bounding the cache
             # seems reasonable.
-            ln = _caching_re_compile(a).sub(str(b), ln)
+            ln = _caching_re_compile(a).sub(str(b), escape(ln))
 
         # Strip the trailing newline and any extra whitespace.
         return ln.strip()
 
     def processLineToFixedPoint(ln):
         assert isinstance(recursion_limit, int) and recursion_limit >= 0
         origLine = ln
         steps = 0
         processed = processLine(ln)
         while processed != ln and steps < recursion_limit:
             ln = processed
             processed = processLine(ln)
             steps += 1
 
         if processed != ln:
             raise ValueError("Recursive substitution of '%s' did not complete "
                              "in the provided recursion limit (%s)" % \
                              (origLine, recursion_limit))
 
         return processed
 
-    # Note Python 3 map() gives an iterator rather than a list so explicitly
-    # convert to list before returning.
     process = processLine if recursion_limit is None else processLineToFixedPoint
-    return list(map(process, script))
+    
+    return [unescape(process(ln)) for ln in script]
 
 
 class ParserKind(object):
     """
     An enumeration representing the style of an integrated test keyword or
     command.
 
     TAG: A keyword taking no value. Ex 'END.'

llvm/utils/lit/tests/Inputs/shtest-recursive-substitution/escaping/lit.cfg

+import lit.formats
+config.name = 'escaping'
+config.suffixes = ['.py']
+config.test_format = lit.formats.ShTest()
+config.test_source_root = None
+config.test_exec_root = None
+
+config.substitutions = [("%rec1", "%%s"), ("%rec2", "%rec1")]
+
+config.recursiveExpansionLimit = 5

llvm/utils/lit/tests/Inputs/shtest-recursive-substitution/escaping/test.py

+# RUN: echo %rec2 %%s %%%%s

llvm/utils/lit/tests/shtest-recursive-substitution.py

 
 # RUN: not %{lit} -j 1 %{inputs}/shtest-recursive-substitution/not-an-integer --show-all 2>&1 | FileCheck --check-prefix=CHECK-TEST4 %s
 # CHECK-TEST4: recursiveExpansionLimit must be either None or an integer
 
 # RUN: not %{lit} -j 1 %{inputs}/shtest-recursive-substitution/negative-integer --show-all 2>&1 | FileCheck --check-prefix=CHECK-TEST5 %s
 # CHECK-TEST5: recursiveExpansionLimit must be a non-negative integer
 
 # RUN: %{lit} -j 1 %{inputs}/shtest-recursive-substitution/set-to-none --show-all | FileCheck --check-prefix=CHECK-TEST6 %s
 # CHECK-TEST6: PASS: set-to-none :: test.py
+
+# RUN: %{lit} -j 1 %{inputs}/shtest-recursive-substitution/escaping --show-all | FileCheck --check-prefix=CHECK-TEST7 %s
+# CHECK-TEST7: PASS: escaping :: test.py
+# CHECK-TEST7: $ "echo" "%s" "%s" "%%s"