From c0fa7bc7059e19c022148e2f04554225b25a4846 Mon Sep 17 00:00:00 2001 From: "Joel E. Denny" Date: Thu, 14 Sep 2023 10:04:23 -0400 Subject: [PATCH] [lit] Fix some issues from --per-test-coverage D154280 (landed in 64d19542e78a in July, 2023) implements `--per-test-coverage` (which can also be specified via `lit_config.per_test_coverage`). However, it has a few issues, which the current patch addresses: 1. D154280 implements `--per-test-coverage` only for the case that lit is configured to use an external shell. The current patch extends the implementation to lit's internal shell. 2. In the case that lit is configured to use an external shell, regardless of whether `--per-test-coverage` is actually specified, D154280 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 as there's nothing left to expand. The current patch cleans up the implementation to avoid useless code. 3. Because of issue 2, D154280 corrupts support for windows `cmd` as an external shell (effectively comments out all RUN lines with `:`). The current patch happens to fix that particular corruption by addressing issue 2. However, D122569 (landed in 1041a9642ba0 in April, 2022) had already broken support for windows `cmd` as an external shell (discards RUN lines when expanding `%dbg(RUN: at line N)`). The current patch does not attempt to fix that bug. For further details, see the PR discussion of the current patch. The current patch addresses the above issues by implementing `--per-test-coverage` before selecting the shell (internal or external) and by leaving `%dbg(RUN: at line N)` unexpanded there. Thus, it is expanded later in a shell-specific manner, as before D154280. This patch introduces `buildPdbgCommand` into lit's implementation to encapsulate the process of building (or rebuilding in the case of the `--per-test-coverage` implementation) a full `%dbg(RUN: at line N) cmd` line and asserting that the result matches `kPdbgRegex`. It also cleans up that and all other uses of `kPdbgRegex` to operate on the full line with `re.fullmatch` not `re.match`. This change better reflects the intention in every case, but it is expected to be NFC because `kPdbgRegex` ends in `.*` and thus avoids the difference between `re.fullmatch` and `re.match`. The only caveat is that `.*` does not match newlines, but RUN lines cannot contain newlines currently, so this caveat currently shouldn't matter in practice. The original `--per-test-coverage` implementation avoided accumulating `export LLVM_PROFILE_FILE={profile}` insertions across retries (due to `ALLOW_RETRIES`) by skipping the insertion if `%dbg(RUN: at line N)` was not present and thus had already been expanded. However, the current patch makes sure the insertions also happen for commands without `%dbg(RUN: at line N)`, such as preamble commands or some commands from other lit test formats. Thus, the current patch implements a different mechanism to avoid accumulating those insertions (see code comments). --- llvm/utils/lit/lit/TestRunner.py | 65 ++++++++++++------- .../per-test-coverage-by-lit-cfg/lit.cfg | 5 +- .../per-test-coverage-by-lit-cfg.py | 5 +- .../tests/Inputs/per-test-coverage/lit.cfg | 6 +- .../per-test-coverage/per-test-coverage.py | 5 +- llvm/utils/lit/tests/allow-retries.py | 12 ++++ .../lit/tests/per-test-coverage-by-lit-cfg.py | 26 ++++++-- llvm/utils/lit/tests/per-test-coverage.py | 26 ++++++-- 8 files changed, 110 insertions(+), 40 deletions(-) diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py index 88755297c8e79..b2c50f563696f 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.fullmatch( + 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. @@ -985,7 +993,7 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper): def executeScriptInternal(test, litConfig, tmpBase, commands, cwd): cmds = [] for i, ln in enumerate(commands): - match = re.match(kPdbgRegex, ln) + match = re.fullmatch(kPdbgRegex, ln) if match: command = match.group(2) ln = commands[i] = match.expand(": '\\1'; \\2" if command else ": '\\1'") @@ -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 = {} @@ -1096,7 +1089,7 @@ def executeScript(test, litConfig, tmpBase, commands, cwd): f = open(script, mode, **open_kwargs) if isWin32CMDEXE: for i, ln in enumerate(commands): - match = re.match(kPdbgRegex, ln) + match = re.fullmatch(kPdbgRegex, ln) if match: command = match.group(2) commands[i] = match.expand( @@ -1109,7 +1102,7 @@ def executeScript(test, litConfig, tmpBase, commands, cwd): f.write("\n@if %ERRORLEVEL% NEQ 0 EXIT\n".join(commands)) else: for i, ln in enumerate(commands): - match = re.match(kPdbgRegex, ln) + match = re.fullmatch(kPdbgRegex, ln) if match: command = match.group(2) commands[i] = match.expand(": '\\1'; \\2" if command else ": '\\1'") @@ -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.fullmatch(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 186c81ebd1728..b83d61eec595b 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 0abe00bdedb1f..a8790f74f178c 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 6e7f4c0e3bc97..9ffca93def73f 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 65e4aa69bff07..59e806939a23a 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 b8abe0ba4fee1..bf6c041933887 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 1cc3927dc8904..189c1cebd623b 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 428712e479257..cf5e82c44dc51 100644 --- a/llvm/utils/lit/tests/per-test-coverage.py +++ b/llvm/utils/lit/tests/per-test-coverage.py @@ -1,6 +1,24 @@ # 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 ({{[^)]*}}) +# 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