[libcxx-commits] [libcxx] cd7f975 - [libc++] NFC: Simplify substitutions by using lit recursive substitutions

Wed Apr 8 14:41:44 PDT 2020

Hi All,

After reading through this email thread and the discussion on the initial revision again, I have developed a bit of an opinion of my own.

TL;DR:
Reid already said:
> Fewer settings or modes of operation is better.
I agree with Reid; ideally this shouldn't have a configuration option.

Long version:
We are now defining two different modes of execution, which I think could be a source of confusion.

If I read it right, then the main concerns w.r.t. this feature were:
1. Possibility of non-termination (currently mitigated via recursion limit)
2. Performance
3. Complicates debugging

I think all of these can be addressed/mitigated:
1. Non-termination can be solved in a more principled way (by sorting substitutions or another similar algorithm).

2. I don't think performance will be an issue, but let's measure to make sure.  Louis already timed a complete run for libc++.  In addition, we can measure `env LIT_OPTS=--no-execute ninja check-all`.  `--no-execute` skips executing the actual tests, so we should get a good sense of lit overhead.

3. Debugging is a valid concern I think, especially because it doesn't have a purely technical solution.  However, IMO it has been given too much weight.  Let me try to explain my thoughts: the person doing the debugging is either aware of recursive expansion of substitutions or they aren't.  If you don't know what to look for, i.e., you don't know that your are in the "recursive expansion" mode, there is not much we can help with.  This can always happen and is even more insidious exactly because we have two different modes.  For experts, we can provide an ergonomic debugging helper, e.g., `print(config.expand('%clang'))`.

On the other hand, the configuration option adds complexity:
We had a long discussion where it should reside (I do think TestingConfig is the right place) and the infrastructure for the mode is almost as much code as the feature itself.  Not making this a configuration option sidesteps these issues.
Also, complexity tends to accumulate over time. A good "bad example" from the past: individual test timeouts, which sounds like a small, innocent feature.  We now have `maxIndividualTestTimeIsSupported` which has a special implementation for AIX! (https://reviews.llvm.org/D64251)

Since (1, 2) have a purely technical solution, and (3) the "debuggability" argument is messy IMO (introducing separate modes has it's own downsides), I vote for not making this configurable at all.

I talked to Louis.  He proposed the following plan and is willing to the work:
1. Implement longest-match matching of substitutions (this is a useful improvement in itself)
2. Address non-termination in a more principled way (by sorting substitutions or another similar algorithm)
3. Measure performance
4. Provide ergonomic helper to print expanded substitutions to aid debugging
5. Remove option in the config, turn on by default

My opinion on related issues:

Prefix matching
E.g, `%clang_asan` expanding for `%clang`, two solutions:
1) Use unique identifier, e.g., `%{clang}` or `%clang%`
2) Implement longest match

I prefer 2), because again it's a technical solution, while 2) is more convention-based (human) and would require a lot of changes to widely adopt.

Ordering of substitutions
I think the order of substitutions should not matter.  IMO, this is just another source of confusion.
Ideally substitutions would be a dictionary (unique keys) instead of a list of tuples.

Let me know what you think!

> On Apr 1, 2020, at 7:01 AM, Louis Dionne <ldionne at apple.com> wrote:
> 
> 
> 
>> On Apr 1, 2020, at 01:35, Dan Liew <dliew at apple.com <mailto:dliew at apple.com>> wrote:
>> 
>>> 
>>> 
>>> Thanks for your reply. So your opinion is that my proposed (2) is the right way forward. I still (weakly) think that we made the right design choice because whether to recursively expand or not seems like a property or the test suite as a whole, and not really something that you'd want to customize on a per-directory basis. It's kind of like the timeout time on tests or whether to be verbose.
>>> 
>> 
>> But our closet proxy for "test suite as a whole" are the TestingConfig objects (they are per test suite directory). Putting the property in LitConfig was never the right design (again sorry I didn't spot this) because that is global to all test suites which is wrong. Given that a test suite can customise its substitutions on a per-directory basis it makes sense that it would be technically possible to configure the substitution recursive expansion on a per-directory basis too. I'm not saying doing that would be a good idea but allowing this is at least consistent with what we already allow users of llvm-lit to do.
>> 
>> It's not like the test timeout or the verbosity. Both of those are properties global to the lit invocation and thus global across all test suite. In both cases it's this way **because** these properties can be set by a command line flag that is global over the lit invocation. When I implemented per-test-timeout, the fact I added it to the LitConfig object rather than TestingConfig was a design mistake that I didn't realise I had made until we got into this discussion.
>> 
>> 
>>> It seems to me like the root cause really is that we can modify the LitConfig globally from subdirectories. Consider for example the timeout time -- imagine what would happen if libc++ set the timeout time to e.g. 2 minutes because <reasons>. Then, imagine that the Clang config sets the timeout time to 10 seconds, because it doesn't make sense for Clang tests to take longer than that. In the current state of things, whichever lit.cfg file is processed last will win, which seems brittle. I believe we should either not share the global LitConfig instance (by copying it when we process subdirectories), or make it read-only after construction to make it clear that it shouldn't be modified. And if it was made read-only, then I agree it would make sense to move `recursiveExpansionLimit` to TestingConfig instead.
>>> 
>>> What do you think?
>> 
>> As I said above. When I implemented lit's per test timeout I made a design mistake putting it in the LitConfig object. It should be in TestingConfig. I think it would be extremely risky to move to a model where LitConfig objects get copied because there might be existing lit configs that rely on this behaviour.
> 
> Okay, I'll move it to the TestingConfig.
> 
> Louis

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20200408/00217fab/attachment-0001.html>