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

Wed Apr 8 14:54:06 PDT 2020

I'm glad you agree that we should have fewer configuration options. :)

I would've expected %clang vs. %clang_asan to work out because of the \b
regex matching that we do, but applying longest patterns first in
asciibetical order would work fine too.

Thanks for sorting this out, it sounds like you have a good plan.

On Wed, Apr 8, 2020 at 2:41 PM Julian Lettner <julian.lettner at apple.com>
wrote:

> 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> 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/fa1721fc/attachment-0001.html>