Page MenuHomePhabricator

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

Authored by broadwaylamb on Jul 15 2020, 11:21 AM.

Details

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.

Diff Detail

Event Timeline

broadwaylamb created this revision.Jul 15 2020, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2020, 11:21 AM
ldionne requested changes to this revision.Jul 15 2020, 11:33 AM
ldionne added inline comments.
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)
  1. 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.

This revision now requires changes to proceed.Jul 15 2020, 11:33 AM

Address the review comments

ldionne accepted this revision.Jul 15 2020, 11:54 AM
ldionne added inline comments.
llvm/utils/lit/lit/TestRunner.py
1203

I guess list() is now redundant.

This revision is now accepted and ready to land.Jul 15 2020, 11:54 AM

Replace %% with #_MARKER_# before each substitution.

broadwaylamb marked an inline comment as done.Jul 15 2020, 12:22 PM
broadwaylamb added inline comments.
llvm/utils/lit/lit/TestRunner.py
1203

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

Remove redundant conversion of a list comprehension to list.

ldionne accepted this revision.Jul 15 2020, 1:08 PM

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

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

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.

This revision was automatically updated to reflect the committed changes.