[PATCH] D76178: [lit] Recursively apply substitutions

[Julian Lettner via Phabricator via llvm-commits]

yln added a comment.

My feedback (also sent via email):

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 are:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76178/new/

https://reviews.llvm.org/D76178