D’oh, I meant < not >, thanks. I blame tiredness 
Having a format that is 95% of the way there for Windows users is definitely desirable - I’ve routinely in the past copy-pasted from the lit output, but have the same various difficulties you’ve highlighted before. It’s more common that I want to run only a single line than the full execution sequence though, so having a multi-line command sequence copy-paste option isn’t really needed for my use case at least.
In that case, my inclination is to keep the comment sequence fixed as # until someone says they want multi-command-line copying to windows cmd. If someone does, I think it would be straight-forward to implement something like --shell-comment=rem in a separate patch.
As long as implementation doesn’t prevent possible alternative output formatting for shell commands in the future, I think it’s fine just to go with the default. The, in the future, someone can just add --shell-syntax={pwsh,cmd,sh;...} option if there would be benefit to it.
What is there already works 99% of the time, and stuff like < is almost always redundant (I’d go as far as to say that it’s unwanted and may cause issues with other tooling, like generate_test_checks), since it’s often used like tool -params < file.ext, then just tool -params file.ext is fine/preferred.
Apologies if this is addressed and I missed it, or if it’s out of scope here: have we considered making it so we use the internal shell everywhere but allow individual tests to opt in to using a native shell? Basically instead of REQUIRES: shell disabling those tests, have them execute with a native shell if possible. I found this whole RFC very confusing since I’ve never seen the execution trace before until I read your message and tried using LIT_USE_INTERNAL_SHELL=1.
1 Like
I think lit’s internal shell would be a better default. However, others might disagree, and I think that discussion requires a different RFC.
The main problem I’m aware of is that switching to lit’s internal shell might break many existing tests that depend on external shells. I don’t know whether they’ve all already been identified with REQUIRES: shell, but my guess is they haven’t.
Maybe this RFC will raise awareness of lit’s internal shell and convince LLVM developers to reconfigure their test suites to use it. If enough do, changing lit’s default might become easy.
1 Like
< %s is often preferred because then the output automatically goes to stdout. Contrast with llc foo.ll where it will create a foo.s and thus require -o - as well, which is longer than <.
Also < %s means the input file has no name, which is helpful when writing FileCheck checks for strings that could unexpectedly appear as part of a file path.
(See my 2016 lightning talk https://www.youtube.com/watch?v=4rhW8knj0L8&pp=ygUNbGx2bSByb2JpbnNvbg%3D%3D)
3 Likes
< %sis often preferred because then the output automatically goes to stdout. Contrast withllc foo.llwhere it will create afoo.sand thus require-o -as well, which is longer than<.
Though -o - %s works a lot better when copy&pasting command-lines to lldb/gdb. but I can see why that may not be the main motivation for choosing…
3 Likes
I think it should be. I’ve had failures from not using < %s about twice ever. I have problems from it not being -o - every single day
I’ve had failures from not using < %s about twice ever
But they tend to be the kinds of issues that don’t show up immediately. The other related one that comes to mind is tests that break based on the branch name due to the Clang-produced IR’s metadata embedding it and breaking a CHECK-NOT. Both rarely show, but they do show. There are a couple of tests that do clang | not grep alloca, for example, which has the result you might expect when you have a feature branch to alter something involving alloca… (ask me how I know).
2 Likes
But this is a symptom of other sources of fragility, the file name is not the core issue. Both CHECK-NOT and using grep for tests is bad. grep should simply not be used (migrating all tests to FileCheck is another long term stalled migration), and CHECK-NOT should be used very sparingly (and should only be used in conjunction with other bounding checks. Most of the time if you’re using CHECK-NOT, you’re better off with CHECK-NEXT in the relevant area
1 Like
I don’t quite get why CHECK-NOT is not desirable? It seems very powerful to me to check for the absence of something, and checking for some exact patterns makes the test over-constrained and more fragile actually.
1 Like
It’s far too easy to have a typo or copy paste error in a -NOT check so you end up passing for the wrong reason. That test will also never be updated if the form of the thing you are checking is not there ever changes (e.g. check-not some opcode, and then later it starts getting mutated into a commuted form)
1 Like
Hi,
I’m a little puzzled here.
To me it seems like if you use “lit’s internal shell” the changes here are pretty nice in that the output you see looks better.
However, we must be a lot of users that don’t use that? I wasn’t even aware such a thing existed before reading this RFC.
And if you don’t use lit’s internal shell, to be blunt, these changes kind of just makes the normal daily workflow worse?
Before if I had a lit test fail, I would see e.g. a printout like
: 'RUN: at line 2'; /repo/uabelho/dev-main/llvm/build-all/bin/opt -S -passes=argpromotion < /repo/uabelho/dev-main/llvm/test/Transforms/ArgumentPromotion/opaque-ptr.ll | /repo/uabelho/dev-main/llvm/build-all/bin/FileCheck /repo/uabelho/dev-main/llvm/test/Transforms/ArgumentPromotion/opaque-ptr.ll
which I in many/most cases could just copy/paste to reproduce the failure.
Now instead I get
+ : 'RUN: at line 2'
+ /repo/uabelho/dev-main/llvm/build-all/bin/opt -S -passes=argpromotion
+ /repo/uabelho/dev-main/llvm/build-all/bin/FileCheck /repo/uabelho/dev-main/llvm/test/Transforms/ArgumentPromotion/opaque-ptr.ll
So when I run “ninja check-all” now and a lit test somewhere fails, the debugging experience for me is worse since I can no longer directly see a command line that I can copy/paste to reproduce the failure.
Now I either have to copy/paste a couple of different things from the printouts, or I have to rerun the specific testcase with
LIT_USE_INTERNAL_SHELL=1 llvm-lit [...]
and if that works, then I can copy/paste the command I wanted.
Sure it works but as I started this post, I’m a little puzzled not more people find this extra step in a normal workflow slightly annoying.
If lit’s internal shell was the default this would probably all just be good but unfortunately it isn’t.
2 Likes
It doesn’t seem feasible to me to provide a good user experience for every shell people use (various UNIX-like shells, cmd, powershell). As an example, in one of the discussions around this feature I remember that there’s no single comment syntax that is supported in every shell people care about.
lit internal shell provides a single target to optimize user experience for. I think making it usable in more contexts would be a better option to spend our scarce resources on.
2 Likes
@mikaelholmen, thanks for posting your concern here. Would you please try the following patch?
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 461cf63d6b96..4d1caad5369c 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -1153,8 +1153,14 @@ def executeScript(test, litConfig, tmpBase, commands, cwd):
for j, ln in enumerate(commands):
match = re.match(kPdbgRegex, ln)
if match:
+ dbg = match.group(1)
command = match.group(2)
- commands[j] = match.expand(": '\\1'; \\2" if command else ": '\\1'")
+ commands[j] = f"echo '{dbg}'"
+ if command:
+ commands[j] += f": {shlex.quote(command.lstrip())} >&2 " \
+ f"&& {command}"
+ else:
+ commands[j] += " has no command after substitutions"
if litConfig.per_test_coverage:
# Extract the test case name from the test object
test_case_name = test.path_in_suite[-1]
It occurs to me now that this might improve the external shell usability issue without sacrificing the original goals of D154984. If it is sufficient for people here, I’ll create a phabricator review (or github PR).
This patch doesn’t make the external shell experience as good as the internal shell experience, but I agree with @Endill that is not likely feasible. The above patch also doesn’t address windows, but we can probably do something similar there once people agree on the direction.
1 Like
It’s not clear to me that this is a good answer to a report of a regression. While of course we can’t expect changes to improve every situation uniformingly, the fact that something was working before is proof enough that it was “feasible to provide a good experience” and it degraded.
More importantly, if we had the lit shell as default and the test-suite that don’t support it opt-out explicitly (or better, per-test with REQUIRES: shell), there would be less focus on the external shell support. The “lit internal shell provides a single target to optimize user experience for” would be much more stronger in this case IMO.
1 Like
I just learned something that I had misunderstood and misstated earlier (sorry about that): lit’s internal shell is the default. Here’s the lit.formats.ShTest constructor prototype in llvm/utils/lit/lit/formats/shtest.py:
def __init__(
self, execute_external=False, extra_substitutions=[], preamble_commands=[]
)
However, many test suites set execute_external explicitly. So, I think we would have to change all those test suites.
This is true for lit, but within LLVM it is in general configured through:
class LLVMConfig(object):
def __init__(self, lit_config, config):
self.lit_config = lit_config
self.config = config
features = config.available_features
self.use_lit_shell = False
...
Where the default seems rather False, I would look into changing it in the config here ultimately?
1 Like