[llvm] [lit] Are all RUN lines skipped in windows cmd? (PR #65242)
via llvm-commits
llvm-commits at lists.llvm.org
Sat Sep 9 18:04:41 PDT 2023
llvmbot wrote:
@llvm/pr-subscribers-testing-tools
<details>
<summary>Changes</summary>
Key issue
---------
When lit is configured to use windows `cmd` as an external shell, it appears that all RUN lines are effectively commented out. The problematic change landed in July 26, 2023, so it's surprising it hasn't come up yet. I'm afraid I don't have a windows setup, so I could use some help to verify what's happening.
Details
-------
In the case that lit is configured to use an external shell, D154280 (landed in 64d19542e78a on July 26, 2023) causes
`%dbg(RUN: at line N)` to be expanded in RUN lines early and in a manner that is specific to sh-like shells. As a result, later code in lit that expands it in a shell-specific manner is useless.
That sh-like expansion uses the `:` command as follows:
```
: 'RUN: at line N'; original-commands
```
In sh-like shells, the `:` command simply discards its arguments. However, in windows `cmd`, `:` indicates a goto label. That appears to effectively comment out the rest of the line, including the original commands from the RUN line.
I am not aware of any complaints about this change. Did I miss them? Are all tests still passing and so no one noticed? Lit's own test suite has some tests that normally fail if RUN lines don't execute. Is no one running lit's test suite with windows `cmd` as a lit external shell? Or is no one using windows `cmd` as a lit external shell at all anymore?
Another issue
-------------
D154280 doesn't implement `--per-test-coverage` for lit's internal shell.
Fix
---
This patch fixes the above problems by implementing `--per-test-coverage` before selecting the shell (internal or external) and by leaving `%dbg(RUN: at line N)` unexpanded. Thus, it is expanded later in a shell-specific manner, as before D154280.
I would like to understand whether windows `cmd` as a lit external shell is worthwhile to support anymore.
--
Full diff: https://github.com/llvm/llvm-project/pull/65242.diff
8 Files Affected:
- (modified) llvm/utils/lit/lit/TestRunner.py (+37-22)
- (modified) llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/lit.cfg (+4-1)
- (modified) llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py (+3-2)
- (modified) llvm/utils/lit/tests/Inputs/per-test-coverage/lit.cfg (+4-2)
- (modified) llvm/utils/lit/tests/Inputs/per-test-coverage/per-test-coverage.py (+3-2)
- (modified) llvm/utils/lit/tests/allow-retries.py (+12)
- (modified) llvm/utils/lit/tests/per-test-coverage-by-lit-cfg.py (+22-4)
- (modified) llvm/utils/lit/tests/per-test-coverage.py (+23-4)
<pre>
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 88755297c8e791a..e9e051086f3531a 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -57,6 +57,14 @@ def __init__(self, command, message):
kPdbgRegex = "%dbg\\(([^)'\"]*)\\)(.*)"
+def buildPdbgCommand(msg, cmd):
+ res = f"%dbg({msg}) {cmd}"
+ assert re.match(
+ kPdbgRegex, res
+ ), f"kPdbgRegex expected to match actual %dbg usage: {res}"
+ return res
+
+
class ShellEnvironment(object):
"""Mutable shell environment containing things like CWD and env vars.
@@ -1067,25 +1075,10 @@ def executeScriptInternal(test, litConfig, tmpBase, commands, cwd):
def executeScript(test, litConfig, tmpBase, commands, cwd):
bashPath = litConfig.getBashPath()
isWin32CMDEXE = litConfig.isWindows and not bashPath
- coverage_index = 0 # Counter for coverage file index
script = tmpBase + ".script"
if isWin32CMDEXE:
script += ".bat"
- # Set unique LLVM_PROFILE_FILE for each run command
- for j, ln in enumerate(commands):
- match = re.match(kPdbgRegex, ln)
- if match:
- command = match.group(2)
- commands[j] = match.expand(": '\\1'; \\2" if command else ": '\\1'")
- if litConfig.per_test_coverage:
- # Extract the test case name from the test object
- test_case_name = test.path_in_suite[-1]
- test_case_name = test_case_name.rsplit(".", 1)[0] # Remove the file extension
- llvm_profile_file = f"{test_case_name}{coverage_index}.profraw"
- commands[j] = f"export LLVM_PROFILE_FILE={llvm_profile_file} && {commands[j]}"
- coverage_index += 1
-
# Write script file
mode = "w"
open_kwargs = {}
@@ -1837,13 +1830,7 @@ def _handleCommand(cls, line_number, line, output, keyword):
if not output or not output[-1].add_continuation(line_number, keyword, line):
if output is None:
output = []
- pdbg = "%dbg({keyword} at line {line_number})".format(
- keyword=keyword, line_number=line_number
- )
- assert re.match(
- kPdbgRegex + "$", pdbg
- ), "kPdbgRegex expected to match actual %dbg usage"
- line = "{pdbg} {real_command}".format(pdbg=pdbg, real_command=line)
+ line = buildPdbgCommand(f"{keyword} at line {line_number}", line)
output.append(CommandDirective(line_number, line_number, keyword, line))
return output
@@ -2048,6 +2035,27 @@ def parseIntegratedTestScript(test, additional_parsers=[], require_script=True):
def _runShTest(test, litConfig, useExternalSh, script, tmpBase):
def runOnce(execdir):
+ # Set unique LLVM_PROFILE_FILE for each run command
+ if litConfig.per_test_coverage:
+ # Extract the test case name from the test object, and remove the
+ # file extension.
+ test_case_name = test.path_in_suite[-1]
+ test_case_name = test_case_name.rsplit(".", 1)[0]
+ coverage_index = 0 # Counter for coverage file index
+ for i, ln in enumerate(script):
+ match = re.match(kPdbgRegex, ln)
+ if match:
+ dbg = match.group(1)
+ command = match.group(2)
+ else:
+ command = ln
+ profile = f"{test_case_name}{coverage_index}.profraw"
+ coverage_index += 1
+ command = f"export LLVM_PROFILE_FILE={profile}; {command}"
+ if match:
+ command = buildPdbgCommand(dbg, command)
+ script[i] = command
+
if useExternalSh:
res = executeScript(test, litConfig, tmpBase, script, execdir)
else:
@@ -2071,7 +2079,14 @@ def runOnce(execdir):
# Re-run failed tests up to test.allowed_retries times.
execdir = os.path.dirname(test.getExecPath())
attempts = test.allowed_retries + 1
+ scriptInit = script
for i in range(attempts):
+ # runOnce modifies script, but applying the modifications again to the
+ # result can corrupt script, so we restore the original upon a retry.
+ # A cleaner solution would be for runOnce to encapsulate operating on a
+ # copy of script, but we actually want it to modify the original script
+ # so we can print the modified version under "Script:" below.
+ script = scriptInit[:]
res = runOnce(execdir)
if isinstance(res, lit.Test.Result):
return res
diff --git a/llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/lit.cfg b/llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/lit.cfg
index 186c81ebd172831..b83d61eec595b5d 100644
--- a/llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/lit.cfg
+++ b/llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/lit.cfg
@@ -3,6 +3,9 @@ import os
config.name = "per-test-coverage-by-lit-cfg"
config.suffixes = [".py"]
-config.test_format = lit.formats.ShTest(execute_external=True)
+config.test_format = lit.formats.ShTest(
+ execute_external=eval(lit_config.params.get("execute_external")),
+ preamble_commands=["%{python} %s | FileCheck -DINDEX=0 %s"]
+)
lit_config.per_test_coverage = True
config.substitutions.append(("%{python}", '"%s"' % (sys.executable)))
diff --git a/llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py b/llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py
index 0abe00bdedb1f28..a8790f74f178c91 100644
--- a/llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py
+++ b/llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py
@@ -1,5 +1,6 @@
# Check that the environment variable is set correctly
-# RUN: %{python} %s | FileCheck %s
+# RUN: %{python} %s | FileCheck -DINDEX=1 %s
+# RUN: %{python} %s | FileCheck -DINDEX=2 %s
# Python script to read the environment variable
# and print its value
@@ -8,4 +9,4 @@
llvm_profile_file = os.environ.get('LLVM_PROFILE_FILE')
print(llvm_profile_file)
-# CHECK: per-test-coverage-by-lit-cfg0.profraw
+# CHECK: per-test-coverage-by-lit-cfg[[INDEX]].profraw
diff --git a/llvm/utils/lit/tests/Inputs/per-test-coverage/lit.cfg b/llvm/utils/lit/tests/Inputs/per-test-coverage/lit.cfg
index 6e7f4c0e3bc97d7..9ffca93def73fdb 100644
--- a/llvm/utils/lit/tests/Inputs/per-test-coverage/lit.cfg
+++ b/llvm/utils/lit/tests/Inputs/per-test-coverage/lit.cfg
@@ -3,6 +3,8 @@ import os
config.name = "per-test-coverage"
config.suffixes = [".py"]
-config.test_format = lit.formats.ShTest(execute_external=True)
+config.test_format = lit.formats.ShTest(
+ execute_external=eval(lit_config.params.get("execute_external")),
+ preamble_commands=["%{python} %s | FileCheck -DINDEX=0 %s"]
+)
config.substitutions.append(("%{python}", '"%s"' % (sys.executable)))
-
diff --git a/llvm/utils/lit/tests/Inputs/per-test-coverage/per-test-coverage.py b/llvm/utils/lit/tests/Inputs/per-test-coverage/per-test-coverage.py
index 65e4aa69bff079f..59e806939a23acb 100644
--- a/llvm/utils/lit/tests/Inputs/per-test-coverage/per-test-coverage.py
+++ b/llvm/utils/lit/tests/Inputs/per-test-coverage/per-test-coverage.py
@@ -1,5 +1,6 @@
# Check that the environment variable is set correctly
-# RUN: %{python} %s | FileCheck %s
+# RUN: %{python} %s | FileCheck -DINDEX=1 %s
+# RUN: %{python} %s | FileCheck -DINDEX=2 %s
# Python script to read the environment variable
# and print its value
@@ -8,4 +9,4 @@
llvm_profile_file = os.environ.get('LLVM_PROFILE_FILE')
print(llvm_profile_file)
-# CHECK: per-test-coverage0.profraw
+# CHECK: per-test-coverage[[INDEX]].profraw
diff --git a/llvm/utils/lit/tests/allow-retries.py b/llvm/utils/lit/tests/allow-retries.py
index b8abe0ba4fee169..bf6c041933887a3 100644
--- a/llvm/utils/lit/tests/allow-retries.py
+++ b/llvm/utils/lit/tests/allow-retries.py
@@ -39,3 +39,15 @@
# RUN: rm -f %t.counter
# RUN: %{lit} %{inputs}/test_retry_attempts/test.py -Dcounter=%t.counter -Dpython=%{python} | FileCheck --check-prefix=CHECK-TEST6 %s
# CHECK-TEST6: Passed With Retry: 1
+
+# This test checks that --per-test-coverage doesn't accumulate inserted
+# LLVM_PROFILE_FILE= commands across retries.
+#
+# RUN: rm -f %t.counter
+# RUN: %{lit} -a %{inputs}/test_retry_attempts/test.py --per-test-coverage\
+# RUN: -Dcounter=%t.counter -Dpython=%{python} | \
+# RUN: FileCheck --check-prefix=CHECK-TEST7 %s
+# CHECK-TEST7: Command Output (stdout):
+# CHECK-TEST7: LLVM_PROFILE_FILE=
+# CHECK-TEST7-NOT: LLVM_PROFILE_FILE=
+# CHECK-TEST7: Passed With Retry: 1
diff --git a/llvm/utils/lit/tests/per-test-coverage-by-lit-cfg.py b/llvm/utils/lit/tests/per-test-coverage-by-lit-cfg.py
index 1cc3927dc89041c..189c1cebd623b4c 100644
--- a/llvm/utils/lit/tests/per-test-coverage-by-lit-cfg.py
+++ b/llvm/utils/lit/tests/per-test-coverage-by-lit-cfg.py
@@ -1,6 +1,24 @@
# Test if lit_config.per_test_coverage in lit.cfg sets individual test case coverage.
-# RUN: %{lit} -a -v %{inputs}/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py \
-# RUN: | FileCheck -match-full-lines %s
-#
-# CHECK: PASS: per-test-coverage-by-lit-cfg :: per-test-coverage-by-lit-cfg.py ({{[^)]*}})
+# RUN: %{lit} -a -vv -Dexecute_external=False \
+# RUN: %{inputs}/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py | \
+# RUN: FileCheck -DOUT=stdout %s
+
+# RUN: %{lit} -a -vv -Dexecute_external=True \
+# RUN: %{inputs}/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py | \
+# RUN: FileCheck -DOUT=stderr %s
+
+# CHECK: {{^}}PASS: per-test-coverage-by-lit-cfg :: per-test-coverage-by-lit-cfg.py ({{[^)]*}})
+# CHECK: Command Output ([[OUT]]):
+# CHECK-NEXT: --
+# CHECK: export
+# CHECK: LLVM_PROFILE_FILE=per-test-coverage-by-lit-cfg0.profraw
+# CHECK: per-test-coverage-by-lit-cfg.py
+# CHECK: {{RUN}}: at line 2
+# CHECK: export
+# CHECK: LLVM_PROFILE_FILE=per-test-coverage-by-lit-cfg1.profraw
+# CHECK: per-test-coverage-by-lit-cfg.py
+# CHECK: {{RUN}}: at line 3
+# CHECK: export
+# CHECK: LLVM_PROFILE_FILE=per-test-coverage-by-lit-cfg2.profraw
+# CHECK: per-test-coverage-by-lit-cfg.py
diff --git a/llvm/utils/lit/tests/per-test-coverage.py b/llvm/utils/lit/tests/per-test-coverage.py
index 428712e47925760..53c6ae99c594ffd 100644
--- a/llvm/utils/lit/tests/per-test-coverage.py
+++ b/llvm/utils/lit/tests/per-test-coverage.py
@@ -1,6 +1,25 @@
# Test LLVM_PROFILE_FILE is set when --per-test-coverage is passed to command line.
-# RUN: %{lit} -a -v --per-test-coverage %{inputs}/per-test-coverage/per-test-coverage.py \
-# RUN: | FileCheck -match-full-lines %s
-#
-# CHECK: PASS: per-test-coverage :: per-test-coverage.py ({{[^)]*}})
+# RUN: %{lit} -a -vv --per-test-coverage -Dexecute_external=False \
+# RUN: %{inputs}/per-test-coverage/per-test-coverage.py | \
+# RUN: FileCheck -DOUT=stdout %s
+
+# RUN: %{lit} -a -vv --per-test-coverage -Dexecute_external=True \
+# RUN: %{inputs}/per-test-coverage/per-test-coverage.py | \
+# RUN: FileCheck -DOUT=stderr %s
+
+# CHECK: {{^}}PASS: per-test-coverage :: per-test-coverage.py ({{[^)]*}})
+t-cfg.py ({{[^)]*}})
+# CHECK: Command Output ([[OUT]]):
+# CHECK-NEXT: --
+# CHECK: export
+# CHECK: LLVM_PROFILE_FILE=per-test-coverage0.profraw
+# CHECK: per-test-coverage.py
+# CHECK: {{RUN}}: at line 2
+# CHECK: export
+# CHECK: LLVM_PROFILE_FILE=per-test-coverage1.profraw
+# CHECK: per-test-coverage.py
+# CHECK: {{RUN}}: at line 3
+# CHECK: export
+# CHECK: LLVM_PROFILE_FILE=per-test-coverage2.profraw
+# CHECK: per-test-coverage.py
</pre>
</details>
https://github.com/llvm/llvm-project/pull/65242
More information about the llvm-commits
mailing list