Rebased onto the current D154987. See its summary for how to apply it successfully first.

Rebased onto PR #66408, which includes recent changes from main. Within TestRunner.py, this rebase required moving runShScript and PythonDirectiveLitAPI into runOnce to make the per-run --per-test-coverage file index visible to lit.run(cmd) calls. Extended tests to cover that case.

Rebased onto the most recent version of D154984.

Rebased onto today's main. This brings in PR #65242, which included some code to accommodate the Script: section. The current patch eliminates the Script: section, so this update cleans up that code. Otherwise, the current patch hasn't changed much.

In D154984#4628289, @awarzynski wrote:

I'm not sure if this has been discussed already

Yes, it has.

In D156954#4627613, @mstorsjo wrote:

I doubt that's related

In D156954#4626071, @dyung wrote:

Getting close, the last failure just seems to be due to the path separators '/' vs. '\' being explicitly checked for.

In D156954#4625928, @dyung wrote:

@jdenny I am the owner of the llvm-clang-x86_64-sie-win bot, let me know if you need any help to fix the failures from your change.

@awarzynski Thanks again for reviewing.

Replace <lit test> with the actual test file name when compiling PYTHON lines. This info helps the python interpreter produce more informative diagnostics, including not only the test file name but also sometimes full PYTHON lines quoted from the test file. After a future patch, I expect it to also help when setting breakpoints while using a python debugger on lit test files.

In D154984#4617772, @MaskRay wrote:

Consider linking to https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839 for this patch and D156954.

I saw only one bit of feedback from the RFC that affects this patch. That is, theoretically, a windows user might wish to adjust the shell comment sequence that lit's internal shell uses in its execution trace. This feature might help with copying multiple command lines at once from that trace and pasting them into any of the various shells that people use in windows. However, people seemed fine to wait to do that in a separate patch if an actual user who cares about that use case speaks up.

The RFC has been up for a week with no significant activity for several days. The response was very positive. Thanks for all the feedback so far in this review, which led to D156954 and thus helped to make the RFC a success.

In D158164#4598113, @jhenderson wrote:

In D158164#4595420, @strimo378 wrote:

--match-full-lines is require to check that nothing trailing is missing. Current approach is to add in 100 lines {{$}}

Okay, I think I see your point. You are simply looking to find a way to apply that regex pattern to every line, much like --match-full-lines adds an implicit {{^}} and {{$}} around each line. Is that correct? And you don't want to use --match-full-lines because of the noisy prefixes (hence I'm not sure @MaskRay's suggestion will work).

I'm not strongly opposed to the new options.

The requested RFC for D154984 and D156954: https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839

The requested RFC for D154984 and D156954: https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839

Rebase onto recent D156954 changes to address another libcxx pre-commit CI failure.

D154987's pre-commit CI revealed another issue with libcxx. This update tries to fix that. See changes to the patch summary for details.

In D154987#4568886, @jhenderson wrote:

In D154987#4539229, @awarzynski wrote:

In D154987#4531662, @jhenderson wrote:

I think it would be worth outlining in this patch why simply calling an external python script (which could actually be embedded within the file) is insufficient for the motivating case.

+1

I just extended TestingGuide.rst. Let me know if it's not what you are looking for.

Thanks. In my case at least, I was just referring to enhancing the commit description or even just posting a review comment with the explanation. Enhancing the documentation works for me though.

Rebase onto recent D156954 changes to address libcxx pre-commit CI failure.

D154987's pre-commit CI discovered that libcxx's custom lit test format doesn't always use %dbg(...) on commands. Chances are that there are other test suites in the same boat. Eliminate the associated assertion and todo in this patch.

Rebase onto D156954 and update to its execution trace style.

In D156954#4564171, @awarzynski wrote:

Thank you so much! This fully addresses the concerns that I raised in https://reviews.llvm.org/D154984.

Improve test coverage for new behavior. Fix issues that revealed:

• Drive-by fix that came up in testing: internal not cmd sometimes produced exit status of True. Fixed that to be 1.
• Instead of # command line, add # preamble command line before command lines from preamble_commands.
• When expanding %dbg(...), don't modify the original script. Otherwise, upon every retry (due to ALLOW_RETRIES), we keep prepending the generic # command line to it as if there was no %dbg(...).
• Assert that %dbg(...) is never missing. ninja check-lit doesn't fail this assert. CI testing will hopefully answer this question for all other uses.
• If @echo has any unexpected results, instead of failing a generic assertion and losing the specific diagnostic, enable the normal execution trace as if it's echo.

This patch will eventually need to be updated for the changes in D156954, but I haven't gotten there yet.

Thanks for the additional comments from everyone.

Rebased. Extended docs with comparison to using %python, as requested.

In D156954#4558080, @MaskRay wrote:

Thank you for the nice summary! I just read the internal implementation the other day and glanced at the change. This change is reasonable and the test coverage looks great.

Rebased.

Rebased.

In D154984#4540226, @awarzynski wrote:

Yes, that should work.

In D154984#4537086, @jdenny wrote:

What if we put the output in shell comments so it has no effect when pasted into a terminal?

In D154984#4536987, @MaskRay wrote:

If the internal shell can collect Command Output (stdout): and stderr and print them after all RUN commands are dumped, we can remove the prompts.

In D154987#4531915, @sammccall wrote:

I'm expecting we'll end up with some way of writing assertions directly within python, rather than just calling out to shell commands that verify themselves.
(If this isn't added deliberately, then I'd expect it'll turn up anyway via raise or lit.run("false") or something)

In D154987#4531624, @Endill wrote:

In D154987#4529228, @sammccall wrote:

Could the tests you're targeting be written as actual python files, with an appropriate library of operations? (Some of this library could of course be shared with the shell test implementation).

I think it would accurate to say that the idea this patch implements was born out of use case I have for C++ defect report test suite.

In D154987#4529228, @sammccall wrote:

I have to offer a dissenting voice here...

In D154984#4508883, @MaskRay wrote:

2. Extend lit's internal shell to print full shell command lines before executing them. Reconfigure tests suites that need this functionality to use lit's internal shell.

Does this mean to print a pipe as one single command instead of two?

@MaskRay and @awarzynski: Thanks for your comments. You both have pointed out that the -vv-style command trace breaks apart commands joined by | (and other command separators) so it's hard to copy-and-paste command lines to a terminal.

In D154987#4501813, @Endill wrote:

Thank you for coming up with the idea, and then implementing it!
Unfortunately, I can't review your implementation from lit standpoint, but it looks solid judging by documentation.

Addressed reviewer comment: make import example clearer.

Other than the comment I just requested, LGTM. Thanks for the cleanup.

In D154130#4502036, @MrTrillian wrote:

In D154130#4487292, @jdenny wrote:

3. Extend lit's own test suite to cover it.

I submitted an update with your suggestions #1 and #2 but this #3 is much more difficult because of the nature of %{t:real}.

@ldionne: It looks like this patch has revealed an existing bug in libcxx/test/libcxx/selftest/sh.cpp/empty.sh.cpp, but I'm not sure how best to handle it. Can you help?

In D154984#4493934, @jdenny wrote:

2. They take a long time to run. I don't know how to address that in the -vv trace. Indeed it seems you would need something like the "Script:" section.

In D154984#4493743, @awarzynski wrote:

If a RUN line fails, you don't see the remaining RUN lines that didn't execute. Are you saying you would like to see those too?

I meant something slightly different. Some RUN get very complicated - especially when adding {RE}DEFINE to the mix and e.g. emulators to run tests on. I would like to be able to simply check that LIT correctly interprets my RUN lines without running the tests (which can take some time to run and generate a lot of output). I think that that would be very useful.

In D154987#4491993, @arichardson wrote:

Are you also planning to adjust the update_tests.py scripts to support this syntax?

In D154984#4493485, @Endill wrote:

LGTM, but I'd wait for more approvals

In D154984#4491339, @awarzynski wrote:

This is a very nice clean-up, thanks!

I've only skimmed through so far and just wanted to post a quick question about this:

In D154594#4489052, @dstuttard wrote:

REQUIRES etc are global - whereas substitutions are not - and can be changed as the script runs. What version of the substitution should apply, I guess it could be the context of the actual REQUIRES/UNSUPPORTED (this would require more significant changes to the file though as these would require tracking in the same way as RUN and DEFINE/REDEFINE etc).

3. Extend lit's own test suite to cover it.

If the %> feature is going to remain (depending on, for example, the answer to @tahonermann's question about modulemap), please:

In D149685#4479109, @cseo wrote:

OK, thanks! I'll take a look then.

In D149685#4471532, @cseo wrote:

I think there's a corner case you may have missed. I'm still seeing this on aarch64:

/llvm-project/openmp/libomptarget/test/mapping/target_derefence_array_pointrs.cpp:28:12: error: CHECK: expected string not found in input
 // CHECK: 3
           ^
<stdin>:1:103: note: scanning from here
Libomptarget message: explicit extension not allowed: host address specified is 0x0000ffffd26ff7c0 (12 bytes), but device allocation maps to host at 0x0000ffffd26ff7c0 (8 bytes)
                                                                                                      ^
<stdin>:1:104: note: possible intended match here
Libomptarget message: explicit extension not allowed: host address specified is 0x0000ffffd26ff7c0 (12 bytes), but device allocation maps to host at 0x0000ffffd26ff7c0 (8 bytes)

Your patch fixes most failures, except this one. Fails with and without LTO enabled. Could you please check?

In D150856#4472330, @ldionne wrote:

Thanks for the review.