This is an archive of the discontinued LLVM Phabricator instance.

[lit] Improve the ToolSubst class a bit
ClosedPublic

Authored by zturner on Oct 4 2017, 3:36 PM.

Details

Summary

In a separate patch, I'm working on re-working clang and lld's lit config's to call a common use_clang and use_lld functions, respectively. Making this work and be generic enough that it also supports debuginfo-test's use case requires a little bit more power in how we specify a tool substitution. For example, lld doesn't prefix its tool substitutions with %, but clang does. And debuginfo-tests shouldn't fail if lld can't be found, but obviously lld should.

This patch adds the necessary bits to make this possible.

It also renames ToolFilter to ToolSubst, and moves it to its own file, to avoid a cyclic dependency between __init__.py and the other files in the same module.

Diff Detail

Event Timeline

zturner created this revision.Oct 4 2017, 3:36 PM
zturner planned changes to this revision.Oct 4 2017, 4:23 PM

There are some issues with the introducer concept here, so I'm going to rework this a little.

zturner updated this revision to Diff 117887.Oct 5 2017, 2:15 PM

This got a little bit more complicated. There were a lot of latent issues and inconsistencies in the way we added tool substitutions, but I think these are all gone now and everything is consistent.

The main change here is the improvements to the ToolSubst class to customize various parts of the substitution algorithm. The idea being twofold:

  1. Take this logic away from the person providing the name of the tool, so that everybody doesn't do it a little bit differently.
  2. Provide a way to specify more complex subsitution handling behavior, such as what happens when a tool can't be found, or how to find a tool (Sometimes people would look in the os PATH, sometimes in a specific set of directories, sometimes in the configuration's PATH.

For an example of #1, there was the ability to specify before and after characters that should fail a match, so that for example if a substitution is llc, then you won't match /llc when it is used as part of a path, or llc.exe when it used as a filename. But not all tools followed these guidelines, and it was done (unnecessarily) on a case-by-case basis without people understanding when/why to do it. And it didn't even handle everything, like a \ in front of a path (it always assume / paths). Now, you have to go out of your way to avoid the default behavior, which is almost always correct.

For #2, the motivation for this is that I want to provide a standardized set of substitutions for a tool. So that for example, debuginfo-tests can say use_clang() and lld can say use_clang() and get the same substitutions that lld and clang themselves use. But if lld fails to find its tools, then that's a fatal error, whereas if debuginfo-tests can't find lld tools, that's expected in certain scenarios. So the ability to customize this is needed.

As an aside, this patch exposed a bug in an existing test where clang-cc1 was being used instead of %clang_cc1, so that is fixed here.

rnk edited edge metadata.Oct 5 2017, 2:52 PM

This CL contains a lot of whitespace, formatting, and style changes. Can you commit those separately?

clang/test/lit.cfg.py
229–230

These new lines seem unnecessary, the comment is associated with this def.

llvm/test/lit.cfg.py
60–61

ditto

llvm/utils/lit/lit/util.py
1 ↗(On Diff #117887)

This all looks like style/formatting changes. Can you commit that separately?

zturner updated this revision to Diff 117907.Oct 5 2017, 3:00 PM

Removed formatting changes. Technically, I did the formatting changes first (using pyformat), and then rebased the original patch on top of the formatting changes and ran pyformat again.

rnk accepted this revision.Oct 5 2017, 3:49 PM

lgtm

llvm/test/lit.cfg.py
154

I don't think we need this FIXME, it's explained above where we compute lli_args.

This revision is now accepted and ready to land.Oct 5 2017, 3:49 PM
zturner added inline comments.Oct 5 2017, 4:26 PM
llvm/test/lit.cfg.py
154

It's not explained why some tests rely on the default args and some tests rely on a different set of args. Is that even correct? Reading the comment makes it sound like lli should be used for everything. Most people are probably just going to write lli without thinking about it, because that's how most other tool substitutions work, but it's nto clear from the comment if that's always the right thing to do, and the person writing the test may not even know to check. If anything, the "fix" suggested by the FIXME could be along the lines of "rename %lli to something that more clearly differentiates it from lli.

This revision was automatically updated to reflect the committed changes.