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