[libcxx-commits] [libcxx] [lit] Clean up internal shell parse errors with ScriptFatal (PR #68496)

Joel E. Denny via libcxx-commits libcxx-commits at lists.llvm.org
Sat Oct 7 13:03:40 PDT 2023


https://github.com/jdenny-ornl created https://github.com/llvm/llvm-project/pull/68496

Without this patch, the functions `executeScriptInternal` and thus `runOnce` in `llvm/utils/lit/lit/TestRunner.py` return either the tuple `(out, err, exitCode, timeoutInfo)` or a `lit.Test.Result` object.  They return the latter only when there's a lit internal shell parse error in a RUN line.  In my opinion, a more straight-forward way to handle exceptional cases like that is to use python exceptions.

For that purpose, this patch introduces `ScriptFatal`.  Thus, this patch changes `executeScriptInternal` to always either return the tuple or raise the `ScriptFatal` exception.  It updates `runOnce` and `libcxx/utils/libcxx/test/format.py` to catch the exception rather than check for the special return type.

This patch also changes `runOnce` to convert the exception to a `Test.UNRESOLVED` result instead of `TEST.FAIL`.  The former is the proper result for such a malformed test, for which a rerun (given an `ALLOW_RETRIES:`) serves no purpose.  There are at least two benefits from this change.  First, `_runShTest` no longer has to specially and cryptically handle this case to avoid unnecessary reruns.  Second, an `XFAIL:` directive no longer hides such a failure [as we saw previously](https://reviews.llvm.org/D154987#4501125).

To facilitate the `_runShTest` change, this patch inserts the internal shell parse error diagnostic into the format of the test's normal debug output rather than suppressing the latter entirely.  That change is also important for [D154987](https://reviews.llvm.org/D154987), which proposes to reuse `ScriptFatal` for python compile errors in PYTHON lines or in `config.prologue`.  In that case, the diagnostic might follow debugging output from the test's previous RUN or PYTHON lines, so suppressing the normal debug output would lose information.

>From 909a48a4d1dd04b1841d5e6953797001a560c432 Mon Sep 17 00:00:00 2001
From: "Joel E. Denny" <jdenny.ornl at gmail.com>
Date: Sat, 7 Oct 2023 15:56:28 -0400
Subject: [PATCH] [lit] Clean up internal shell parse errors with ScriptFatal

Without this patch, the functions `executeScriptInternal` and thus
`runOnce` in `llvm/utils/lit/lit/TestRunner.py` return either the
tuple `(out, err, exitCode, timeoutInfo)` or a `lit.Test.Result`
object.  They return the latter only when there's a lit internal shell
parse error in a RUN line.  In my opinion, a more straight-forward way
to handle exceptional cases like that is to use python exceptions.

For that purpose, this patch introduces `ScriptFatal`.  Thus, this
patch changes `executeScriptInternal` to always either return the
tuple or raise the `ScriptFatal` exception.  It updates `runOnce` and
`libcxx/utils/libcxx/test/format.py` to catch the exception rather
than check for the special return type.

This patch also changes `runOnce` to convert the exception to a
`Test.UNRESOLVED` result instead of `TEST.FAIL`.  The former is the
proper result for such a malformed test, for which a rerun (given an
`ALLOW_RETRIES:`) serves no purpose.  There are at least two benefits
from this change.  First, `_runShTest` no longer has to specially and
cryptically handle this case to avoid unnecessary reruns.  Second, an
`XFAIL:` directive no longer hides such a failure [as we saw
previously](https://reviews.llvm.org/D154987#4501125).

To facilitate the `_runShTest` change, this patch inserts the internal
shell parse error diagnostic into the format of the test's normal
debug output rather than suppressing the latter entirely.  That change
is also important for [D154987](https://reviews.llvm.org/D154987),
which proposes to reuse `ScriptFatal` for python compile errors in
PYTHON lines or in `config.prologue`.  In that case, the diagnostic
might follow debugging output from the test's previous RUN or PYTHON
lines, so suppressing the normal debug output would lose information.
---
 libcxx/utils/libcxx/test/format.py   | 11 ++++----
 llvm/utils/lit/lit/TestRunner.py     | 41 +++++++++++++++++++---------
 llvm/utils/lit/tests/shtest-shell.py | 18 ++++++++----
 3 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/libcxx/utils/libcxx/test/format.py b/libcxx/utils/libcxx/test/format.py
index c7c0bad681ddc8b..4106cd83db34b7d 100644
--- a/libcxx/utils/libcxx/test/format.py
+++ b/libcxx/utils/libcxx/test/format.py
@@ -45,11 +45,12 @@ def _executeScriptInternal(test, litConfig, commands):
 
     _, tmpBase = _getTempPaths(test)
     execDir = os.path.dirname(test.getExecPath())
-    res = lit.TestRunner.executeScriptInternal(
-        test, litConfig, tmpBase, parsedCommands, execDir, debug=False
-    )
-    if isinstance(res, lit.Test.Result):  # Handle failure to parse the Lit test
-        res = ("", res.output, 127, None)
+    try:
+        res = lit.TestRunner.executeScriptInternal(
+            test, litConfig, tmpBase, parsedCommands, execDir, debug=False
+        )
+    except lit.TestRunner.ScriptFatal as e:
+        res = ("", str(e), 127, None)
     (out, err, exitCode, timeoutInfo) = res
 
     return (out, err, exitCode, timeoutInfo, parsedCommands)
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index a6314e35c1a045c..590538f1e5e6039 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -34,6 +34,17 @@ def __init__(self, command, message):
         self.message = message
 
 
+class ScriptFatal(Exception):
+    """
+    A script had a fatal error such that there's no point in retrying.  The
+    message has not been emitted on stdout or stderr but is instead included in
+    this exception.
+    """
+
+    def __init__(self, message):
+        super().__init__(message)
+
+
 kIsWindows = platform.system() == "Windows"
 
 # Don't use close_fds on Windows.
@@ -1009,7 +1020,8 @@ def formatOutput(title, data, limit=None):
     return out
 
 
-# Normally returns out, err, exitCode, timeoutInfo.
+# Always either returns the tuple (out, err, exitCode, timeoutInfo) or raises a
+# ScriptFatal exception.
 #
 # If debug is True (the normal lit behavior), err is empty, and out contains an
 # execution trace, including stdout and stderr shown per command executed.
@@ -1043,9 +1055,9 @@ def executeScriptInternal(test, litConfig, tmpBase, commands, cwd, debug=True):
                 ShUtil.ShParser(ln, litConfig.isWindows, test.config.pipefail).parse()
             )
         except:
-            return lit.Test.Result(
-                Test.FAIL, f"shell parser error on {dbg}: {command.lstrip()}\n"
-            )
+            raise ScriptFatal(
+                f"shell parser error on {dbg}: {command.lstrip()}\n"
+            ) from None
 
     cmd = cmds[0]
     for c in cmds[1:]:
@@ -2130,7 +2142,9 @@ def parseIntegratedTestScript(test, additional_parsers=[], require_script=True):
     return script
 
 
+# Always returns a lit.Test.Result.
 def _runShTest(test, litConfig, useExternalSh, script, tmpBase):
+    # Always returns the tuple (out, err, exitCode, timeoutInfo).
     def runOnce(execdir):
         # script is modified below (for litConfig.per_test_coverage, and for
         # %dbg expansions).  runOnce can be called multiple times, but applying
@@ -2158,12 +2172,16 @@ def runOnce(execdir):
                     command = buildPdbgCommand(dbg, command)
                 scriptCopy[i] = command
 
-        if useExternalSh:
-            res = executeScript(test, litConfig, tmpBase, scriptCopy, execdir)
-        else:
-            res = executeScriptInternal(test, litConfig, tmpBase, scriptCopy, execdir)
-        if isinstance(res, lit.Test.Result):
-            return res
+        try:
+            if useExternalSh:
+                res = executeScript(test, litConfig, tmpBase, scriptCopy, execdir)
+            else:
+                res = executeScriptInternal(
+                    test, litConfig, tmpBase, scriptCopy, execdir
+                )
+        except ScriptFatal as e:
+            out = f"# " + "\n# ".join(str(e).splitlines()) + "\n"
+            return out, "", 1, None, Test.UNRESOLVED
 
         out, err, exitCode, timeoutInfo = res
         if exitCode == 0:
@@ -2183,9 +2201,6 @@ def runOnce(execdir):
     attempts = test.allowed_retries + 1
     for i in range(attempts):
         res = runOnce(execdir)
-        if isinstance(res, lit.Test.Result):
-            return res
-
         out, err, exitCode, timeoutInfo, status = res
         if status != Test.FAIL:
             break
diff --git a/llvm/utils/lit/tests/shtest-shell.py b/llvm/utils/lit/tests/shtest-shell.py
index a043582d6ae2b9a..86851194880620b 100644
--- a/llvm/utils/lit/tests/shtest-shell.py
+++ b/llvm/utils/lit/tests/shtest-shell.py
@@ -561,10 +561,17 @@
 
 # FIXME: The output here sucks.
 #
-# CHECK: FAIL: shtest-shell :: error-1.txt
-# CHECK: *** TEST 'shtest-shell :: error-1.txt' FAILED ***
-# CHECK: shell parser error on RUN: at line 3: echo "missing quote
-# CHECK: ***
+#       CHECK: UNRESOLVED: shtest-shell :: error-1.txt
+#  CHECK-NEXT: *** TEST 'shtest-shell :: error-1.txt' FAILED ***
+#  CHECK-NEXT: Exit Code: 1
+# CHECK-EMPTY:
+#  CHECK-NEXT: Command Output (stdout):
+#  CHECK-NEXT: --
+#  CHECK-NEXT: # shell parser error on RUN: at line 3: echo "missing quote
+# CHECK-EMPTY:
+#  CHECK-NEXT: --
+# CHECK-EMPTY:
+#  CHECK-NEXT: ***
 
 # CHECK: FAIL: shtest-shell :: error-2.txt
 # CHECK: *** TEST 'shtest-shell :: error-2.txt' FAILED ***
@@ -643,4 +650,5 @@
 #      CHECK: ***
 
 # CHECK: PASS: shtest-shell :: valid-shell.txt
-# CHECK: Failed Tests (39)
+# CHECK: Unresolved Tests (1)
+# CHECK: Failed Tests (38)



More information about the libcxx-commits mailing list