[lldb][test] Remove self references from decorators

[rupprecht]

This is partial step toward removing the vendored unittest2 dep in favor of the unittest library in standard python. One of the large differences is when xfail decorators are evaluated. With the unittest2 vendored dep, this can happen at the moment of calling the test case, and with LLDB's decorator wrappers, we are passed the test class in the decorator arg. With the unittest framework, this is determined much earlier; we cannot decide when the test is about to start that we need to xfail.

Fortunately, almost none of these checks require any state that can't be determined statically. For this patch, I moved the impl for all the checks to lldbplatformutil and pointed the decorators to that, removing as many self (i.e. test class object) references as possible. I left wrappers within TestBase that forward to lldbplatformutil for convenience, but we should probably remove those later.

The remaining check that can't be moved statically is the check for the debug info type (e.g. to xfail only for dwarf). Fixing that requires a different approach, so I will postpone that to the next patch.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jordan Rupprecht (rupprecht)

Changes

---

Patch is 22.83 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/72416.diff

3 Files Affected:

• (modified) lldb/packages/Python/lldbsuite/test/decorators.py (+53-34)
• (modified) lldb/packages/Python/lldbsuite/test/lldbplatformutil.py (+145-1)
• (modified) lldb/packages/Python/lldbsuite/test/lldbtest.py (+9-94)

diff --git a/lldb/packages/Python/lldbsuite/test/decorators.py b/lldb/packages/Python/lldbsuite/test/decorators.py
index b8fea1e02e864de..14328f3c54a8d1f 100644
--- a/lldb/packages/Python/lldbsuite/test/decorators.py
+++ b/lldb/packages/Python/lldbsuite/test/decorators.py
@@ -197,15 +197,17 @@ def _decorateTest(
 ):
     def fn(self):
         skip_for_os = _match_decorator_property(
-            lldbplatform.translate(oslist), self.getPlatform()
+            lldbplatform.translate(oslist), lldbplatformutil.getPlatform()
         )
         skip_for_hostos = _match_decorator_property(
             lldbplatform.translate(hostoslist), lldbplatformutil.getHostPlatform()
         )
         skip_for_compiler = _match_decorator_property(
-            compiler, self.getCompiler()
-        ) and self.expectedCompilerVersion(compiler_version)
-        skip_for_arch = _match_decorator_property(archs, self.getArchitecture())
+            compiler, lldbplatformutil.getCompiler()
+        ) and lldbplatformutil.expectedCompilerVersion(compiler_version)
+        skip_for_arch = _match_decorator_property(
+            archs, lldbplatformutil.getArchitecture()
+        )
         skip_for_debug_info = _match_decorator_property(debug_info, self.getDebugInfo())
         skip_for_triple = _match_decorator_property(
             triple, lldb.selected_platform.GetTriple()
@@ -236,7 +238,7 @@ def fn(self):
         )
         skip_for_dwarf_version = (dwarf_version is None) or (
             _check_expected_version(
-                dwarf_version[0], dwarf_version[1], self.getDwarfVersion()
+                dwarf_version[0], dwarf_version[1], lldbplatformutil.getDwarfVersion()
             )
         )
         skip_for_setting = (setting is None) or (setting in configuration.settings)
@@ -375,7 +377,9 @@ def skipIf(
 def _skip_for_android(reason, api_levels, archs):
     def impl(obj):
         result = lldbplatformutil.match_android_device(
-            obj.getArchitecture(), valid_archs=archs, valid_api_levels=api_levels
+            lldbplatformutil.getArchitecture(),
+            valid_archs=archs,
+            valid_api_levels=api_levels,
         )
         return reason if result else None
 
@@ -537,7 +541,10 @@ def wrapper(*args, **kwargs):
 
 def expectedFlakeyOS(oslist, bugnumber=None, compilers=None):
     def fn(self):
-        return self.getPlatform() in oslist and self.expectedCompiler(compilers)
+        return (
+            lldbplatformutil.getPlatform() in oslist
+            and lldbplatformutil.expectedCompiler(compilers)
+        )
 
     return expectedFlakey(fn, bugnumber)
 
@@ -618,9 +625,11 @@ def are_sb_headers_missing():
 def skipIfRosetta(bugnumber):
     """Skip a test when running the testsuite on macOS under the Rosetta translation layer."""
 
-    def is_running_rosetta(self):
+    def is_running_rosetta():
         if lldbplatformutil.getPlatform() in ["darwin", "macosx"]:
-            if (platform.uname()[5] == "arm") and (self.getArchitecture() == "x86_64"):
+            if (platform.uname()[5] == "arm") and (
+                lldbplatformutil.getArchitecture() == "x86_64"
+            ):
                 return "skipped under Rosetta"
         return None
 
@@ -694,7 +703,7 @@ def skipIfWindows(func):
 def skipIfWindowsAndNonEnglish(func):
     """Decorate the item to skip tests that should be skipped on non-English locales on Windows."""
 
-    def is_Windows_NonEnglish(self):
+    def is_Windows_NonEnglish():
         if sys.platform != "win32":
             return None
         kernel = ctypes.windll.kernel32
@@ -724,11 +733,15 @@ def skipUnlessTargetAndroid(func):
 def skipIfHostIncompatibleWithRemote(func):
     """Decorate the item to skip tests if binaries built on this host are incompatible."""
 
-    def is_host_incompatible_with_remote(self):
-        host_arch = self.getLldbArchitecture()
+    def is_host_incompatible_with_remote():
+        host_arch = lldbplatformutil.getLldbArchitecture()
         host_platform = lldbplatformutil.getHostPlatform()
-        target_arch = self.getArchitecture()
-        target_platform = "darwin" if self.platformIsDarwin() else self.getPlatform()
+        target_arch = lldbplatformutil.getArchitecture()
+        target_platform = (
+            "darwin"
+            if lldbplatformutil.platformIsDarwin()
+            else lldbplatformutil.getPlatform()
+        )
         if (
             not (target_arch == "x86_64" and host_arch == "i386")
             and host_arch != target_arch
@@ -771,8 +784,8 @@ def skipUnlessPlatform(oslist):
 def skipUnlessArch(arch):
     """Decorate the item to skip tests unless running on the specified architecture."""
 
-    def arch_doesnt_match(self):
-        target_arch = self.getArchitecture()
+    def arch_doesnt_match():
+        target_arch = lldbplatformutil.getArchitecture()
         if arch != target_arch:
             return "Test only runs on " + arch + ", but target arch is " + target_arch
         return None
@@ -797,8 +810,8 @@ def skipIfTargetAndroid(bugnumber=None, api_levels=None, archs=None):
 def skipUnlessAppleSilicon(func):
     """Decorate the item to skip tests unless running on Apple Silicon."""
 
-    def not_apple_silicon(test):
-        if platform.system() != "Darwin" or test.getArchitecture() not in [
+    def not_apple_silicon():
+        if platform.system() != "Darwin" or lldbplatformutil.getArchitecture() not in [
             "arm64",
             "arm64e",
         ]:
@@ -811,10 +824,10 @@ def not_apple_silicon(test):
 def skipUnlessSupportedTypeAttribute(attr):
     """Decorate the item to skip test unless Clang supports type __attribute__(attr)."""
 
-    def compiler_doesnt_support_struct_attribute(self):
-        compiler_path = self.getCompiler()
+    def compiler_doesnt_support_struct_attribute():
+        compiler_path = lldbplatformutil.getCompiler()
         f = tempfile.NamedTemporaryFile()
-        cmd = [self.getCompiler(), "-x", "c++", "-c", "-o", f.name, "-"]
+        cmd = [lldbplatformutil.getCompiler(), "-x", "c++", "-c", "-o", f.name, "-"]
         p = subprocess.Popen(
             cmd,
             stdin=subprocess.PIPE,
@@ -833,8 +846,8 @@ def compiler_doesnt_support_struct_attribute(self):
 def skipUnlessHasCallSiteInfo(func):
     """Decorate the function to skip testing unless call site info from clang is available."""
 
-    def is_compiler_clang_with_call_site_info(self):
-        compiler_path = self.getCompiler()
+    def is_compiler_clang_with_call_site_info():
+        compiler_path = lldbplatformutil.getCompiler()
         compiler = os.path.basename(compiler_path)
         if not compiler.startswith("clang"):
             return "Test requires clang as compiler"
@@ -861,18 +874,21 @@ def is_compiler_clang_with_call_site_info(self):
 def skipUnlessThreadSanitizer(func):
     """Decorate the item to skip test unless Clang -fsanitize=thread is supported."""
 
-    def is_compiler_clang_with_thread_sanitizer(self):
+    def is_compiler_clang_with_thread_sanitizer():
         if is_running_under_asan():
             return "Thread sanitizer tests are disabled when runing under ASAN"
 
-        compiler_path = self.getCompiler()
+        compiler_path = lldbplatformutil.getCompiler()
         compiler = os.path.basename(compiler_path)
         if not compiler.startswith("clang"):
             return "Test requires clang as compiler"
         if lldbplatformutil.getPlatform() == "windows":
             return "TSAN tests not compatible with 'windows'"
         # rdar://28659145 - TSAN tests don't look like they're supported on i386
-        if self.getArchitecture() == "i386" and platform.system() == "Darwin":
+        if (
+            lldbplatformutil.getArchitecture() == "i386"
+            and platform.system() == "Darwin"
+        ):
             return "TSAN tests not compatible with i386 targets"
         if not _compiler_supports(compiler_path, "-fsanitize=thread"):
             return "Compiler cannot compile with -fsanitize=thread"
@@ -884,7 +900,7 @@ def is_compiler_clang_with_thread_sanitizer(self):
 def skipUnlessUndefinedBehaviorSanitizer(func):
     """Decorate the item to skip test unless -fsanitize=undefined is supported."""
 
-    def is_compiler_clang_with_ubsan(self):
+    def is_compiler_clang_with_ubsan():
         if is_running_under_asan():
             return (
                 "Undefined behavior sanitizer tests are disabled when runing under ASAN"
@@ -895,7 +911,7 @@ def is_compiler_clang_with_ubsan(self):
 
         # Try to compile with ubsan turned on.
         if not _compiler_supports(
-            self.getCompiler(),
+            lldbplatformutil.getCompiler(),
             "-fsanitize=undefined",
             "int main() { int x = 0; return x / x; }",
             outputf,
@@ -910,7 +926,10 @@ def is_compiler_clang_with_ubsan(self):
 
         # Find the ubsan dylib.
         # FIXME: This check should go away once compiler-rt gains support for __ubsan_on_report.
-        cmd = "%s -fsanitize=undefined -x c - -o - -### 2>&1" % self.getCompiler()
+        cmd = (
+            "%s -fsanitize=undefined -x c - -o - -### 2>&1"
+            % lldbplatformutil.getCompiler()
+        )
         with os.popen(cmd) as cc_output:
             driver_jobs = cc_output.read()
             m = re.search(r'"([^"]+libclang_rt.ubsan_osx_dynamic.dylib)"', driver_jobs)
@@ -942,7 +961,7 @@ def is_running_under_asan():
 def skipUnlessAddressSanitizer(func):
     """Decorate the item to skip test unless Clang -fsanitize=thread is supported."""
 
-    def is_compiler_with_address_sanitizer(self):
+    def is_compiler_with_address_sanitizer():
         # Also don't run tests that use address sanitizer inside an
         # address-sanitized LLDB. The tests don't support that
         # configuration.
@@ -951,7 +970,7 @@ def is_compiler_with_address_sanitizer(self):
 
         if lldbplatformutil.getPlatform() == "windows":
             return "ASAN tests not compatible with 'windows'"
-        if not _compiler_supports(self.getCompiler(), "-fsanitize=address"):
+        if not _compiler_supports(lldbplatformutil.getCompiler(), "-fsanitize=address"):
             return "Compiler cannot compile with -fsanitize=address"
         return None
 
@@ -966,8 +985,8 @@ def skipIfAsan(func):
 def skipUnlessAArch64MTELinuxCompiler(func):
     """Decorate the item to skip test unless MTE is supported by the test compiler."""
 
-    def is_toolchain_with_mte(self):
-        compiler_path = self.getCompiler()
+    def is_toolchain_with_mte():
+        compiler_path = lldbplatformutil.getCompiler()
         compiler = os.path.basename(compiler_path)
         f = tempfile.NamedTemporaryFile()
         if lldbplatformutil.getPlatform() == "windows":
@@ -1053,7 +1072,7 @@ def skipIfLLVMTargetMissing(target):
 
 # Call sysctl on darwin to see if a specified hardware feature is available on this machine.
 def skipUnlessFeature(feature):
-    def is_feature_enabled(self):
+    def is_feature_enabled():
         if platform.system() == "Darwin":
             try:
                 DEVNULL = open(os.devnull, "w")
diff --git a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
index 2ecfe527e0f251e..4b51366cf284ec3 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -7,12 +7,15 @@
 import subprocess
 import sys
 import os
+from distutils.version import LooseVersion
 from urllib.parse import urlparse
 
 # LLDB modules
-from . import configuration
 import lldb
+from . import configuration
+from . import lldbtest_config
 import lldbsuite.test.lldbplatform as lldbplatform
+from lldbsuite.test.builders import get_builder
 
 
 def check_first_register_readable(test_case):
@@ -184,3 +187,144 @@ def hasChattyStderr(test_case):
     ):
         return True  # The dynamic linker on the device will complain about unknown DT entries
     return False
+
+
+def builder_module():
+    return get_builder(sys.platform)
+
+
+def getArchitecture():
+    """Returns the architecture in effect the test suite is running with."""
+    module = builder_module()
+    arch = module.getArchitecture()
+    if arch == "amd64":
+        arch = "x86_64"
+    if arch in ["armv7l", "armv8l"]:
+        arch = "arm"
+    return arch
+
+
+lldbArchitecture = None
+
+
+def getLldbArchitecture():
+    """Returns the architecture of the lldb binary."""
+    global lldbArchitecture
+    if not lldbArchitecture:
+        # These two target settings prevent lldb from doing setup that does
+        # nothing but slow down the end goal of printing the architecture.
+        command = [
+            lldbtest_config.lldbExec,
+            "-x",
+            "-b",
+            "-o",
+            "settings set target.preload-symbols false",
+            "-o",
+            "settings set target.load-script-from-symbol-file false",
+            "-o",
+            "file " + lldbtest_config.lldbExec,
+        ]
+
+        output = subprocess.check_output(command)
+        str = output.decode()
+
+        for line in str.splitlines():
+            m = re.search(r"Current executable set to '.*' \((.*)\)\.", line)
+            if m:
+                lldbArchitecture = m.group(1)
+                break
+
+    return lldbArchitecture
+
+
+def getCompiler():
+    """Returns the compiler in effect the test suite is running with."""
+    module = builder_module()
+    return module.getCompiler()
+
+
+def getCompilerBinary():
+    """Returns the compiler binary the test suite is running with."""
+    return getCompiler().split()[0]
+
+
+def getCompilerVersion():
+    """Returns a string that represents the compiler version.
+    Supports: llvm, clang.
+    """
+    compiler = getCompilerBinary()
+    version_output = subprocess.check_output([compiler, "--version"], errors="replace")
+    m = re.search("version ([0-9.]+)", version_output)
+    if m:
+        return m.group(1)
+    return "unknown"
+
+
+def getDwarfVersion():
+    """Returns the dwarf version generated by clang or '0'."""
+    if configuration.dwarf_version:
+        return str(configuration.dwarf_version)
+    if "clang" in getCompiler():
+        try:
+            triple = builder_module().getTriple(getArchitecture())
+            target = ["-target", triple] if triple else []
+            driver_output = subprocess.check_output(
+                [getCompiler()] + target + "-g -c -x c - -o - -###".split(),
+                stderr=subprocess.STDOUT,
+            )
+            driver_output = driver_output.decode("utf-8")
+            for line in driver_output.split(os.linesep):
+                m = re.search("dwarf-version=([0-9])", line)
+                if m:
+                    return m.group(1)
+        except subprocess.CalledProcessError:
+            pass
+    return "0"
+
+
+def expectedCompilerVersion(compiler_version):
+    """Returns True iff compiler_version[1] matches the current compiler version.
+    Use compiler_version[0] to specify the operator used to determine if a match has occurred.
+    Any operator other than the following defaults to an equality test:
+        '>', '>=', "=>", '<', '<=', '=<', '!=', "!" or 'not'
+
+    If the current compiler version cannot be determined, we assume it is close to the top
+    of trunk, so any less-than or equal-to comparisons will return False, and any
+    greater-than or not-equal-to comparisons will return True.
+    """
+    if compiler_version is None:
+        return True
+    operator = str(compiler_version[0])
+    version = compiler_version[1]
+
+    if version is None:
+        return True
+
+    test_compiler_version = getCompilerVersion()
+    if test_compiler_version == "unknown":
+        # Assume the compiler version is at or near the top of trunk.
+        return operator in [">", ">=", "!", "!=", "not"]
+
+    if operator == ">":
+        return LooseVersion(test_compiler_version) > LooseVersion(version)
+    if operator == ">=" or operator == "=>":
+        return LooseVersion(test_compiler_version) >= LooseVersion(version)
+    if operator == "<":
+        return LooseVersion(test_compiler_version) < LooseVersion(version)
+    if operator == "<=" or operator == "=<":
+        return LooseVersion(test_compiler_version) <= LooseVersion(version)
+    if operator == "!=" or operator == "!" or operator == "not":
+        return str(version) not in str(test_compiler_version)
+    return str(version) in str(test_compiler_version)
+
+
+def expectedCompiler(compilers):
+    """Returns True iff any element of compilers is a sub-string of the current compiler."""
+    if compilers is None:
+        return True
+
+    for compiler in compilers:
+        if compiler in getCompiler():
+            return True
+
+    return False
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 19ba0e8c133edcf..90941a7cc3270ce 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -29,7 +29,6 @@
 
 # System modules
 import abc
-from distutils.version import LooseVersion
 from functools import wraps
 import gc
 import glob
@@ -58,7 +57,6 @@
 from lldbsuite.support import encoded_file
 from lldbsuite.support import funcutils
 from lldbsuite.support import seven
-from lldbsuite.test.builders import get_builder
 from lldbsuite.test_event import build_exception
 
 # See also dotest.parseOptionsAndInitTestdirs(), where the environment variables
@@ -516,7 +514,7 @@ def getsource_if_available(obj):
 
 
 def builder_module():
-    return get_builder(sys.platform)
+    return lldbplatformutil.builder_module()
 
 
 class Base(unittest2.TestCase):
@@ -1310,82 +1308,29 @@ def isAArch64Windows(self):
 
     def getArchitecture(self):
         """Returns the architecture in effect the test suite is running with."""
-        module = builder_module()
-        arch = module.getArchitecture()
-        if arch == "amd64":
-            arch = "x86_64"
-        if arch in ["armv7l", "armv8l"]:
-            arch = "arm"
-        return arch
+        return lldbplatformutil.getArchitecture()
 
     def getLldbArchitecture(self):
         """Returns the architecture of the lldb binary."""
-        if not hasattr(self, "lldbArchitecture"):
-            # These two target settings prevent lldb from doing setup that does
-            # nothing but slow down the end goal of printing the architecture.
-            command = [
-                lldbtest_config.lldbExec,
-                "-x",
-                "-b",
-                "-o",
-                "settings set target.preload-symbols false",
-                "-o",
-                "settings set target.load-script-from-symbol-file false",
-                "-o",
-                "file " + lldbtest_config.lldbExec,
-            ]
-
-            output = check_output(command)
-            str = output.decode()
-
-            for line in str.splitlines():
-                m = re.search(r"Current executable set to '.*' \((.*)\)\.", line)
-                if m:
-                    self.lldbArchitecture = m.group(1)
-                    break
-
-        return self.lldbArchitecture
+        return lldbplatformutil.getLldbArchitecture()
 
     def getCompiler(self):
         """Returns the compiler in effect the test suite is running with."""
-        module = builder_module()
-        return module.getCompiler()
+        return lldbplatformutil.getCompiler()
 
     def getCompilerBinary(self):
         """Returns the compiler binary the test suite is running with."""
-        return self.getCompiler().split()[0]
+        return lldbplatformutil.getCompilerBinary()
 
     def getCompilerVersion(self):
         """Returns a string that represents the compiler version.
         Supports: llvm, clang.
         """
-        compiler = self.getCompilerBinary()
-        version_output = check_output([compiler, "--version"], errors="replace")
-        m = re.search("version ([0-9.]+)", version_output)
-        if m:
-            return m.group(1)
-        return "unknown"
+        return lldbplatformutil.getCompilerVersion()
 
     def getDwarfVersion(self):
         """Returns the dwarf version generated by clang or '0'."""
-        if configuration.dwarf_version:
-            return str(configuration.dwarf_versio...
[truncated]

[JDevlieghere]

👏 Thanks for doing this!

[medismailben]

Nice! LGTM with some nits.

[bulbazord]

Beautiful, thank you for working on this.
After this change, what's left to get us using the python-provided unittest?

[rupprecht]

Beautiful, thank you for working on this. After this change, what's left to get us using the python-provided unittest?

The giant lambda inside _decorateTest has one remaining self reference, which is self.getDebugInfo() -- i.e. the debug info specific variant created by the metaclass factory. My current approach is basically to annotate the test in a similar way as we do for the @no_debug_info_test, with some state that indicates which debug info kinds should be skipped/xfailed, and then when the metaclass creates each variant, it can create them as being wrapped w/ @skip or @expectedFailure depending on the result of that.

After that, not a lot. Both @JDevlieghere and I have made an attempt at this in the past, and so we've already done a lot of cleanup to make things work with either library.