[lldb][test] Support remote run of Shell tests #95986
Conversation
dzhidzhoev
requested review from
rupprecht,
JDevlieghere,
DavidSpickett,
labath and
jimingham
|
@llvm/pr-subscribers-lldb Author: Vladislav Dzhidzhoev (dzhidzhoev) Changes
It has been tested on Ubuntu AArch64 with libcxx enabled for tests. Full diff: https://github.com/llvm/llvm-project/pull/95986.diff 7 Files Affected:
diff --git a/lldb/test/API/lit.cfg.py b/lldb/test/API/lit.cfg.py
index 96520c7c82624..6a0a1b0a76675 100644
--- a/lldb/test/API/lit.cfg.py
+++ b/lldb/test/API/lit.cfg.py
@@ -303,6 +303,13 @@ def delete_module_cache(path):
# In particular, (1) is visited at the top of the file, since the script
# derives other information from it.
+if is_configured("lldb_platform_url"):
+ dotest_cmd += ["--platform-url", config.lldb_platform_url]
+if is_configured("lldb_platform_working_dir"):
+ dotest_cmd += ["--platform-working-dir", config.lldb_platform_working_dir]
+if is_configured("cmake_sysroot"):
+ dotest_cmd += ["--sysroot", config.cmake_sysroot]
+
if is_configured("dotest_user_args_str"):
dotest_cmd.extend(config.dotest_user_args_str.split(";"))
diff --git a/lldb/test/API/lit.site.cfg.py.in b/lldb/test/API/lit.site.cfg.py.in
index 8b2d09ae41cd2..ec388297e91eb 100644
--- a/lldb/test/API/lit.site.cfg.py.in
+++ b/lldb/test/API/lit.site.cfg.py.in
@@ -24,6 +24,9 @@ config.lua_executable = "@Lua_EXECUTABLE@"
config.lua_test_entry = "TestLuaAPI.py"
config.dotest_common_args_str = lit_config.substitute("@LLDB_TEST_COMMON_ARGS@")
config.dotest_user_args_str = lit_config.substitute("@LLDB_TEST_USER_ARGS@")
+config.lldb_platform_url = lit_config.substitute("@LLDB_PLATFORM_URL@")
+config.lldb_platform_working_dir = lit_config.substitute("@LLDB_PLATFORM_WORKING_DIR@")
+config.cmake_sysroot = lit_config.substitute("@LLDB_TEST_SYSROOT@" or "@DEFAULT_SYSROOT@")
config.lldb_enable_python = @LLDB_ENABLE_PYTHON@
config.dotest_lit_args_str = None
config.enabled_plugins = []
diff --git a/lldb/test/Shell/Settings/TestEchoCommands.test b/lldb/test/Shell/Settings/TestEchoCommands.test
index 234b9742bfa2a..ce78f91e1cbd4 100644
--- a/lldb/test/Shell/Settings/TestEchoCommands.test
+++ b/lldb/test/Shell/Settings/TestEchoCommands.test
@@ -2,6 +2,8 @@
# RUN: %lldb -x -b -o 'settings set interpreter.echo-comment-commands false' -s %S/Inputs/EchoCommandsTest.in | FileCheck %S/Inputs/EchoCommandsNoComments.out
# RUN: %lldb -x -b -o 'settings set interpreter.echo-commands false' -s %S/Inputs/EchoCommandsTest.in | FileCheck %S/Inputs/EchoCommandsNone.out
+XFAIL: remote{{.*}}
+
RUN: echo start >%t.file
RUN: %lldb -x -b --source-quietly -s %S/Inputs/EchoCommandsTest.in >>%t.file
RUN: echo done >>%t.file
diff --git a/lldb/test/Shell/Target/target-label.test b/lldb/test/Shell/Target/target-label.test
index 5ac430601e29a..7f4f31e09fa16 100644
--- a/lldb/test/Shell/Target/target-label.test
+++ b/lldb/test/Shell/Target/target-label.test
@@ -4,16 +4,16 @@
target create -l "ls" /bin/ls
target list
-# CHECK: * target #0 (ls): /bin/ls
+# CHECK: * target #0 (ls): [[LS_PATH:.*]]
script lldb.target.SetLabel("")
target list
-# CHECK: * target #0: /bin/ls
+# CHECK: * target #0: [[LS_PATH]]
target create -l "cat" /bin/cat
target list
-# CHECK: target #0: /bin/ls
-# CHECK-NEXT: * target #1 (cat): /bin/cat
+# CHECK: target #0: [[LS_PATH]]
+# CHECK-NEXT: * target #1 (cat): [[CAT_PATH:.*]]
target create -l "cat" /bin/cat
# CHECK: Cannot use label 'cat' since it's set in target #1.
@@ -22,12 +22,12 @@ target create -l 42 /bin/cat
# CHECK: error: Cannot use integer as target label.
target select 0
-# CHECK: * target #0: /bin/ls
-# CHECK-NEXT: target #1 (cat): /bin/cat
+# CHECK: * target #0: [[LS_PATH]]
+# CHECK-NEXT: target #1 (cat): [[CAT_PATH]]
target select cat
-# CHECK: target #0: /bin/ls
-# CHECK-NEXT: * target #1 (cat): /bin/cat
+# CHECK: target #0: [[LS_PATH]]
+# CHECK-NEXT: * target #1 (cat): [[CAT_PATH]]
script lldb.target.GetLabel()
# CHECK: 'cat'
@@ -36,5 +36,5 @@ script lldb.debugger.GetTargetAtIndex(0).SetLabel('Not cat')
# CHECK: success
target list
-# CHECK: target #0 (Not cat): /bin/ls
-# CHECK-NEXT: * target #1 (cat): /bin/cat
+# CHECK: target #0 (Not cat): [[LS_PATH]]
+# CHECK-NEXT: * target #1 (cat): [[CAT_PATH]]
diff --git a/lldb/test/Shell/helper/toolchain.py b/lldb/test/Shell/helper/toolchain.py
index 255955fc70d8c..32c37536ec8bd 100644
--- a/lldb/test/Shell/helper/toolchain.py
+++ b/lldb/test/Shell/helper/toolchain.py
@@ -1,10 +1,12 @@
import os
import itertools
import platform
+import re
import subprocess
import sys
import lit.util
+from lit.formats import ShTest
from lit.llvm import llvm_config
from lit.llvm.subst import FindTool
from lit.llvm.subst import ToolSubst
@@ -22,6 +24,55 @@ def _disallow(config, execName):
config.substitutions.append((" {0} ".format(execName), warning.format(execName)))
+def get_lldb_args(config, suffix=None):
+ lldb_args = []
+ if "remote-linux" in config.available_features:
+ lldb_args += [
+ "-O",
+ '"platform select remote-linux"',
+ "-O",
+ f'"platform connect {config.lldb_platform_url}"',
+ ]
+ if config.lldb_platform_working_dir:
+ dir = f"{config.lldb_platform_working_dir}/shell"
+ if suffix:
+ dir += f"/{suffix}"
+ lldb_args += [
+ "-O",
+ f'"platform shell mkdir -p {dir}"',
+ "-O",
+ f'"platform settings -w {dir}"',
+ ]
+ lldb_args += ["--no-lldbinit", "-S", _get_lldb_init_path(config)]
+ return lldb_args
+
+
+class ShTestLldb(ShTest):
+ def __init__(
+ self, execute_external=False, extra_substitutions=[], preamble_commands=[]
+ ):
+ super().__init__(execute_external, extra_substitutions, preamble_commands)
+
+ def execute(self, test, litConfig):
+ for i, t in enumerate(test.config.substitutions):
+ try:
+ if re.match(t[0], "%lldb"):
+ cmd = t[1]
+ if '-O "platform settings -w ' in cmd:
+ args_def = " ".join(get_lldb_args(test.config))
+ args_unique = " ".join(
+ get_lldb_args(test.config, "/".join(test.path_in_suite))
+ )
+ test.config.substitutions[i] = (
+ t[0],
+ cmd.replace(args_def, args_unique),
+ )
+ break
+ except:
+ pass
+ return super().execute(test, litConfig)
+
+
def use_lldb_substitutions(config):
# Set up substitutions for primary tools. These tools must come from config.lldb_tools_dir
# which is basically the build output directory. We do not want to find these in path or
@@ -34,7 +85,9 @@ def use_lldb_substitutions(config):
build_script = os.path.join(build_script, "build.py")
build_script_args = [
build_script,
- "--compiler=any", # Default to best compiler
+ (
+ "--compiler=clang" if config.lldb_platform_url else "--compiler=any"
+ ), # Default to best compiler
"--arch=" + str(config.lldb_bitness),
]
if config.lldb_lit_tools_dir:
@@ -56,7 +109,7 @@ def use_lldb_substitutions(config):
ToolSubst(
"%lldb",
command=FindTool("lldb"),
- extra_args=["--no-lldbinit", "-S", lldb_init],
+ extra_args=get_lldb_args(config),
unresolved="fatal",
),
ToolSubst(
@@ -138,7 +191,7 @@ def use_support_substitutions(config):
# Set up substitutions for support tools. These tools can be overridden at the CMake
# level (by specifying -DLLDB_LIT_TOOLS_DIR), installed, or as a last resort, we can use
# the just-built version.
- host_flags = ["--target=" + config.host_triple]
+ host_flags = ["--target=" + config.target_triple]
if platform.system() in ["Darwin"]:
try:
out = subprocess.check_output(["xcrun", "--show-sdk-path"]).strip()
@@ -165,6 +218,14 @@ def use_support_substitutions(config):
if config.cmake_sysroot:
host_flags += ["--sysroot={}".format(config.cmake_sysroot)]
+ if config.lldb_platform_url and config.has_libcxx:
+ host_flags += [
+ "-L{}".format(config.libcxx_libs_dir),
+ "-Wl,-rpath,{}".format(config.libcxx_libs_dir),
+ "-lc++",
+ "-lc++abi",
+ ]
+
host_flags = " ".join(host_flags)
config.substitutions.append(("%clang_host", "%clang " + host_flags))
config.substitutions.append(("%clangxx_host", "%clangxx " + host_flags))
diff --git a/lldb/test/Shell/lit.cfg.py b/lldb/test/Shell/lit.cfg.py
index e24f3fbb4d931..7f832eb3bff4c 100644
--- a/lldb/test/Shell/lit.cfg.py
+++ b/lldb/test/Shell/lit.cfg.py
@@ -21,7 +21,7 @@
config.name = "lldb-shell"
# testFormat: The test format to use to interpret tests.
-config.test_format = lit.formats.ShTest(not llvm_config.use_lit_shell)
+config.test_format = toolchain.ShTestLldb(not llvm_config.use_lit_shell)
# suffixes: A list of file extensions to treat as test files. This is overriden
# by individual lit.local.cfg files in the test subdirectories.
@@ -68,6 +68,10 @@
lit_config.note("Running Shell tests in {} mode.".format(lldb_repro_mode))
toolchain.use_lldb_repro_substitutions(config, lldb_repro_mode)
+if config.lldb_platform_url:
+ if re.match(r".*-linux.*", config.target_triple):
+ config.available_features.add("remote-linux")
+
llvm_config.use_default_substitutions()
toolchain.use_lldb_substitutions(config)
toolchain.use_support_substitutions(config)
diff --git a/lldb/test/Shell/lit.site.cfg.py.in b/lldb/test/Shell/lit.site.cfg.py.in
index b69e7bce1bc0b..dde705f1918cb 100644
--- a/lldb/test/Shell/lit.site.cfg.py.in
+++ b/lldb/test/Shell/lit.site.cfg.py.in
@@ -10,10 +10,14 @@ config.lldb_src_root = "@LLDB_SOURCE_DIR@"
config.lldb_obj_root = "@LLDB_BINARY_DIR@"
config.lldb_libs_dir = lit_config.substitute("@LLDB_LIBS_DIR@")
config.lldb_tools_dir = lit_config.substitute("@LLDB_TOOLS_DIR@")
+config.lldb_platform_url = lit_config.substitute("@LLDB_PLATFORM_URL@")
+config.lldb_platform_working_dir = lit_config.substitute("@LLDB_PLATFORM_WORKING_DIR@")
# Since it comes from the command line, it may have backslashes which
# should not need to be escaped.
config.lldb_lit_tools_dir = lit_config.substitute(r"@LLDB_LIT_TOOLS_DIR@")
-config.cmake_sysroot = lit_config.substitute("@CMAKE_SYSROOT@")
+config.cmake_sysroot = lit_config.substitute("@LLDB_TEST_SYSROOT@" or "@DEFAULT_SYSROOT@")
+config.has_libcxx = @LLDB_HAS_LIBCXX@
+config.libcxx_libs_dir = "@LIBCXX_LIBRARY_DIR@"
config.target_triple = "@LLVM_TARGET_TRIPLE@"
config.python_executable = "@Python3_EXECUTABLE@"
config.have_zlib = @LLVM_ENABLE_ZLIB@
|
|
Can't say I'm excited to see this feature (although I admit it is looking less daunting than I originally feared). Can you explain why you need this feature? Is there some particular aspect of lldb's remote operation that you think isn't well covered by the existing tests? Is there a particular category of tests that you'd like to have more than other? How many of the existing tests would actually exercise the remote platform capability in a meaningful way (a lot of these tests don't even build runnable binaries)? |
I'm sorry, it took a bit longer time to answer than I expected, my apologies. In general, our goal is to maximize the functionality of the testing toolkit in case of cross-compilation+remote launch setup. I agree that not every Shell test launches binaries. According to my grep-based statistics, it's roughly one-fifth of the tests currently (117/533 test files that contain "r", "run", or "process launch" commands). Though it's still a number. Also, launching the "platform select" and "platform connect ..." commands before executing the rest of a Shell test script may be useful. Here is an example case of a potentially unexpected behavior #94165. So it's not always necessary to have a Shell test build and run a binary to reveal some nuances. I haven't opened an RFC since I thought it would be clearer to discuss that idea with the showcase of implementation. I'm open to moving a discussion there if it's considered as a better approach in this case (please let me know😅). |
Thank you for finding that number.
That's true, but this still seems like a pretty big hammer for that.
Maybe. Like I said, it's cleaner than I though, but I'm still somewhat worried about where this will lead. If you can support running these tests on a different machine, then I think the next question becomes "why not run them with a different compiler as well". And then you need to XFAIL the tests for a particular compiler, so you end up replicating all of the I don't know if this is a slippery slope fallacy or an actual slippery slope, but this extra bit of test coverage does not seem worth it to me, and I sort of liked the separation how API tests can run in a multitude of configurations while a Shell test runs in one. I know that isn't really a good separation since "the way I write the tests" is in principle independent of "where I want to run it", but since the two tests use completely separate infrastructures, I think it would make sense to keep this functionality in just one of them. That said I don't want to be only person blocking this, so I'd like to hear what others think as well. @JDevlieghere ?
Maybe the best would be an RFC with a link to a POC, but now that we're here, I think we can stick with it and see how it goes. |
|
Could you please give your opinion @JDevlieghere ? |
|
Created RFC https://discourse.llvm.org/t/rfc-lldb-support-remote-run-of-shell-tests/80072, hope it fosters the discussion. |
dzhidzhoev
force-pushed
the
lldb/remote-shell
branch
from
817caef to
11cabee
Compare
|
Updated: |
dzhidzhoev
force-pushed
the
lldb/remote-shell
branch
2 times, most recently
from
d21c76b to
b747d2c
Compare
|
Update: fixed condition for remote execution. |
There was a problem hiding this comment.
I'm not sure whether we need LLDB_SHELL_TESTS_DISABLE_REMOTE, can you explain how that would be used?
Would I set up a build with all the other flags, do my remote testing, then set LLDB_SHELL_TESTS_DISABLE_REMOTE=OFF and run the shell tests locally, using that same build?
I guess I'm asking whether a given build should be all remote or all local or a mix.
I've added this flag to address the question raised about the support for different compilers. Thus, I've added this flag to disable remote Shell tests execution if tests fail due to unsupported compiler usage. The idea is that if this flag is turned on, tests are run locally as it happens right now. So no "mixed" executions are desired. It should be either "remote execution against the remote target" or "best-effort local execution against the localhost target" (best-effort since if cross toolchain for cross-compilation/remote execution of API tests is built that doesn't support host target and Shell tests can't find another compiler, they fail on current mainline). Also, I think |
|
✅ With the latest revision this PR passed the Python code formatter. |
dzhidzhoev
force-pushed
the
lldb/remote-shell
branch
from
ed18268 to
41b0868
Compare
|
Ok I see what
I'm thinking that final build has unit and shell tests run locally, API tests run on the remote? I suppose we already support unit tests locally and API run remote, so this carries on that idea, but I've just never tried to do that because I'm generally x86 -> AArch64 where the cross compiler is never compatible. |
For example:
|
|
Ok, that seems reasonable. I'm still in a mode of thinking where a given build is all remote or all native. Though another way of thinking about it might be, I have a custom compiler setup but only intend to do host testing. This variable lets me tell the test runner not to assume I'm going to do any remote testing? And if this compiler were the one from the toolchain you were going to ship, and include lldb with it, this lets you test host and remote then ship what you've tested, right? Instead of testing host with the system compiler and hoping nothing changes when you use the newly built cross compiler. |
Yes, considering only Shell tests. By default, host testing is assumed. LLDB_TEST_PLATFORM_NAME/LLDB_TEST_PLATFORM_URL must be set to execute remote API&Shell testing. LLDB_SHELL_TESTS_DISABLE_REMOTE may be used to disable it for Shell tests.
Technically, yes. But I think Shell tests don't aim to test a cross-compiler. It's more like "if the cross-compiler is used, that is not compatible with Shell tests, we can run them only on the host to check at least the part of the functionality". It may make sense since only one-fifth of Shell tests run something remotely. |
|
Looks good to me. @labath should give this another look as the biggest skeptic here :) And @JDevlieghere who was also tagged earlier (start with the RFC though https://discourse.llvm.org/t/rfc-lldb-support-remote-run-of-shell-tests/80072). |
lldb/test/Shell/helper/toolchain.py
Outdated
| "-L{}".format(config.libcxx_libs_dir), | ||
| "-Wl,-rpath,{}".format(config.libcxx_libs_dir), | ||
| "-lc++", | ||
| "-lc++abi", |
There was a problem hiding this comment.
These flags don't seem very consistent. -rpath is only useful for dynamic links but I think you're using static links now. And since we removed -lc++abi (which is only needed for some static links) from the makefile, I don't think it should be used here as well.
I was going to leave it at that, but seeing #101710, I have to add this as it illustrates my point brilliantly. Overall, I am more worried about the reproducibility of lldb tests, then their universality. That bug shows we're not able to create a test for a very simple thing (an unaligned stack pointer) that works reliably for everyone. What makes us think we'll be able to do that for tests that execute remotely? This is important as it slows down the velocity of all developers (old and new). In the end, that also reduces test coverage, because people will start to (even proactively) add |
dzhidzhoev
force-pushed
the
lldb/remote-shell
branch
from
f5601da to
6134721
Compare
Honestly, based on #101710 (comment), I'd say that the mentioned test has a problem of too many redundant dependencies, rather than that it shows the backside of tests universality.
LLDB tests will still somehow depend on at least three targets in the foreseeable future (Linux/Windows/Darwin). If a test fails or is not supported on Linux target, it's unlikely that it will work on Linux target with remote debugging. So I don't see a perspective of having test coverage reduced because of
I see a situation of a development velocity slowdown if a test the developer wrote is not supposed to work for remote debugging, but is missing a proper annotation for that. For other cases, remote debugging support is claimed as an LLDB feature. If tests are written/executed using the LLDB command line, remote execution shouldn't hinder their execution. BTW, most of the cases where tests should've been disabled for remote debugging were the cases where platform mismatch happened. Tests were already disabled for "system-windows", but they were attempted to be run against Linux target. Probably, "system-X" semantics should be redefined to indicate a target platform instead of a host platform, or a feature like "target-system-xxx" should be introduced to show that a test should/shouldn't be run on a Windows/Darwin target regardless of the host. I think this will eliminate the need for Considering the complexity of setting up a remote debugging environment, public buildbots with it will be available https://discourse.llvm.org/t/rfc-lldb-support-remote-run-of-shell-tests/80072/4, https://lab.llvm.org/staging/#/builders/195. Also, the case when no new features are added, but existing ones are changed, shouldn't be neglected. In such cases, it is hard to justify disabling tests without finding out the reason.
From my point of view, the current incentive given is that remote debugging compatibility may be ignored when new commits are made since it's something rare enough/hard to test properly (because testing with it requires writing an API test). |
I'm afraid I don't know what you mean by that. How would you go about removing those dependencies?
I think the main difference here is that you think of the Shell tests as kind of an end to end test which verifies that a certain (user-facing) debugger feature is works the way its supposed to. I don't. I think that's what the API tests are for. I think the Shell tests should verify that a specific piece of debugger code (which is most likely not directly accessible by the user) does what it is supposed to do. I.e., they're more akin to a unit test (which happens to be driven by shell commands). If that sounds too abstract, let me give you an example. And ideal "end to end" test would be "this source code, compiled with the same compiler that the end user will use, running on a machine most similar to the one the user would use produces this debugger output". An ideal unit test would be "feeding these bytes (e.g. DWARF opcodes) into this function produces this result". The difference is that in the second case you want to make the test as specific as possible, whereas in the first one you want to make it as variable as possible (so that people can use the environment they care about). This creates an inherent conflict. If you want the test to be runnable or useful remotely, it needs to be written in the first way. This means compiling for the target of the day, and doing while making sure you don't bake in any assumptions that are only true about the target you work on is very hard. That is what ends up creating OTOH, if someone writes a test that verifies the operation of some feature on a mac even if the test runs on linux, then we can be all sure that the feature will stay working. Yes, this creates a risk that the feature will not work on linux (or maybe just remote linux?), but that's why we have API tests, which can verify that the most important features work in all configurations. And there's nothing stopping downstream for adding additional end-to-end tests (ones which might not even fit into the API test suite) for additional validation for the use cases they are supporting. |
I think the dependency on clang/clang's default pipeline was excessive, considering that the biggest part of that test was written directly in x86 assembly. Probably, rewriting that test with yaml2obj would have been better #101710 (comment).
I think of them as a simplified way of writing the same tests that could have been written with Python API. They are described as integration tests in the documentation https://lldb.llvm.org/resources/test.html#test-suite-structure. From my perspective, the difference between API and Shell tests lies in the tests' scenario complexity, not in test type (if we take aside command line-specific tests). As far as I can see, both |
1. This commit adds LLDB_TEST_PLATFORM_URL, LLDB_TEST_SYSROOT, LLDB_TEST_PLATFORM_WORKING_DIR, LLDB_SHELL_TESTS_DISABLE_REMOTE cmake flags to pass arguments for cross-compilation and remote running of both Shell&API tests. 2. To run Shell tests remotely, it adds 'platform select' and 'platform connect' commands to %lldb substitution. 3. 'remote-linux' feature added to lit to disable tests failing with remote execution. 4. A separate working directory is assigned to each test to avoid conflicts during parallel test execution. 5. Remote Shell testing is run only when LLDB_TEST_SYSROOT is set for building test sources. Recommended compiler for that is Clang.
dzhidzhoev
force-pushed
the
lldb/remote-shell
branch
from
44c25eb to
081f16c
Compare
|
Rebased & added a line to documentation about the usage of API tests for remote testing. |
There was a problem hiding this comment.
LGTM.
Maybe @labath would like to take a last look at it before it lands.
I assume there are no more comments, so I'll merge it. I'm open for further corrections. |
…buntu" builder dotest.py flags replaced with CMake flags introduced in llvm/llvm-project#95986 to run Shell tests remotely. Also, USE_LLVM_TOOLS is removed since it's implied by default that LLVM tools will be used for API tests since llvm/llvm-project#109961.
…buntu" builder (#270) dotest.py flags are replaced with CMake flags introduced in llvm/llvm-project#95986 to run Shell tests remotely. Also, USE_LLVM_TOOLS is removed since it's implied by default that LLVM tools will be used for API tests since llvm/llvm-project#109961.
LLDB_TEST_PLATFORM_WORKING_DIR, LLDB_SHELL_TESTS_DISABLE_REMOTE cmake flags
to pass arguments for cross-compilation and remote running of both Shell&API
tests.
substitution.
conflicts during parallel test execution.
building test sources. The recommended compiler for that is Clang.