[lldb] Split some lldb-server tests to avoid timeout #129614
Conversation
|
@llvm/pr-subscribers-lldb Author: David Peixotto (dmpots) ChangesThis test is running right up to the 600s timeout limit. Split it in two to avoid hitting a timeout. Patch is 72.16 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/129614.diff 2 Files Affected:
diff --git a/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py b/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
index 592037db502aa..81639ed96597f 100644
--- a/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ b/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -5,9 +5,9 @@
lldb-server tests run where the lldb-server exe is
available.
-This class will be broken into smaller test case classes by
-gdb remote packet functional areas. For now it contains
-the initial set of tests implemented.
+The tests are split between the LldbGdbServerTestCase and
+LldbGdbServerTestCase2 classes to avoid timeouts in the
+test suite.
"""
import binascii
@@ -602,898 +602,3 @@ def test_Hc_then_Csignal_signals_correct_thread_launch(self):
self.Hc_then_Csignal_signals_correct_thread(
lldbutil.get_signal_number("SIGSEGV")
)
-
- @skipIfWindows # No pty support to test any inferior output
- def test_m_packet_reads_memory(self):
- self.build()
- self.set_inferior_startup_launch()
- # This is the memory we will write into the inferior and then ensure we
- # can read back with $m.
- MEMORY_CONTENTS = "Test contents 0123456789 ABCDEFGHIJKLMNOPQRSTUVWXYZ abcdefghijklmnopqrstuvwxyz"
-
- # Start up the inferior.
- procs = self.prep_debug_monitor_and_inferior(
- inferior_args=[
- "set-message:%s" % MEMORY_CONTENTS,
- "get-data-address-hex:g_message",
- "sleep:5",
- ]
- )
-
- # Run the process
- self.test_sequence.add_log_lines(
- [
- # Start running after initial stop.
- "read packet: $c#63",
- # Match output line that prints the memory address of the message buffer within the inferior.
- # Note we require launch-only testing so we can get inferior otuput.
- {
- "type": "output_match",
- "regex": self.maybe_strict_output_regex(
- r"data address: 0x([0-9a-fA-F]+)\r\n"
- ),
- "capture": {1: "message_address"},
- },
- # Now stop the inferior.
- "read packet: {}".format(chr(3)),
- # And wait for the stop notification.
- {
- "direction": "send",
- "regex": r"^\$T([0-9a-fA-F]{2})thread:([0-9a-fA-F]+);",
- "capture": {1: "stop_signo", 2: "stop_thread_id"},
- },
- ],
- True,
- )
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
-
- # Grab the message address.
- self.assertIsNotNone(context.get("message_address"))
- message_address = int(context.get("message_address"), 16)
-
- # Grab contents from the inferior.
- self.reset_test_sequence()
- self.test_sequence.add_log_lines(
- [
- "read packet: $m{0:x},{1:x}#00".format(
- message_address, len(MEMORY_CONTENTS)
- ),
- {
- "direction": "send",
- "regex": r"^\$(.+)#[0-9a-fA-F]{2}$",
- "capture": {1: "read_contents"},
- },
- ],
- True,
- )
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
-
- # Ensure what we read from inferior memory is what we wrote.
- self.assertIsNotNone(context.get("read_contents"))
- read_contents = seven.unhexlify(context.get("read_contents"))
- self.assertEqual(read_contents, MEMORY_CONTENTS)
-
- def test_qMemoryRegionInfo_is_supported(self):
- self.build()
- self.set_inferior_startup_launch()
- # Start up the inferior.
- procs = self.prep_debug_monitor_and_inferior()
-
- # Ask if it supports $qMemoryRegionInfo.
- self.test_sequence.add_log_lines(
- ["read packet: $qMemoryRegionInfo#00", "send packet: $OK#00"], True
- )
- self.expect_gdbremote_sequence()
-
- @skipIfWindows # No pty support to test any inferior output
- def test_qMemoryRegionInfo_reports_code_address_as_executable(self):
- self.build()
- self.set_inferior_startup_launch()
-
- # Start up the inferior.
- procs = self.prep_debug_monitor_and_inferior(
- inferior_args=["get-code-address-hex:hello", "sleep:5"]
- )
-
- # Run the process
- self.test_sequence.add_log_lines(
- [
- # Start running after initial stop.
- "read packet: $c#63",
- # Match output line that prints the memory address of the message buffer within the inferior.
- # Note we require launch-only testing so we can get inferior otuput.
- {
- "type": "output_match",
- "regex": self.maybe_strict_output_regex(
- r"code address: 0x([0-9a-fA-F]+)\r\n"
- ),
- "capture": {1: "code_address"},
- },
- # Now stop the inferior.
- "read packet: {}".format(chr(3)),
- # And wait for the stop notification.
- {
- "direction": "send",
- "regex": r"^\$T([0-9a-fA-F]{2})thread:([0-9a-fA-F]+);",
- "capture": {1: "stop_signo", 2: "stop_thread_id"},
- },
- ],
- True,
- )
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
-
- # Grab the code address.
- self.assertIsNotNone(context.get("code_address"))
- code_address = int(context.get("code_address"), 16)
-
- # Grab memory region info from the inferior.
- self.reset_test_sequence()
- self.add_query_memory_region_packets(code_address)
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
- mem_region_dict = self.parse_memory_region_packet(context)
-
- # Ensure there are no errors reported.
- self.assertNotIn("error", mem_region_dict)
-
- # Ensure code address is readable and executable.
- self.assertIn("permissions", mem_region_dict)
- self.assertIn("r", mem_region_dict["permissions"])
- self.assertIn("x", mem_region_dict["permissions"])
-
- # Ensure the start address and size encompass the address we queried.
- self.assert_address_within_memory_region(code_address, mem_region_dict)
-
- @skipIfWindows # No pty support to test any inferior output
- def test_qMemoryRegionInfo_reports_stack_address_as_rw(self):
- self.build()
- self.set_inferior_startup_launch()
-
- # Start up the inferior.
- procs = self.prep_debug_monitor_and_inferior(
- inferior_args=["get-stack-address-hex:", "sleep:5"]
- )
-
- # Run the process
- self.test_sequence.add_log_lines(
- [
- # Start running after initial stop.
- "read packet: $c#63",
- # Match output line that prints the memory address of the message buffer within the inferior.
- # Note we require launch-only testing so we can get inferior otuput.
- {
- "type": "output_match",
- "regex": self.maybe_strict_output_regex(
- r"stack address: 0x([0-9a-fA-F]+)\r\n"
- ),
- "capture": {1: "stack_address"},
- },
- # Now stop the inferior.
- "read packet: {}".format(chr(3)),
- # And wait for the stop notification.
- {
- "direction": "send",
- "regex": r"^\$T([0-9a-fA-F]{2})thread:([0-9a-fA-F]+);",
- "capture": {1: "stop_signo", 2: "stop_thread_id"},
- },
- ],
- True,
- )
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
-
- # Grab the address.
- self.assertIsNotNone(context.get("stack_address"))
- stack_address = int(context.get("stack_address"), 16)
-
- # Grab memory region info from the inferior.
- self.reset_test_sequence()
- self.add_query_memory_region_packets(stack_address)
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
- mem_region_dict = self.parse_memory_region_packet(context)
-
- # Ensure there are no errors reported.
- self.assertNotIn("error", mem_region_dict)
-
- # Ensure address is readable and executable.
- self.assertIn("permissions", mem_region_dict)
- self.assertIn("r", mem_region_dict["permissions"])
- self.assertIn("w", mem_region_dict["permissions"])
-
- # Ensure the start address and size encompass the address we queried.
- self.assert_address_within_memory_region(stack_address, mem_region_dict)
-
- @skipIfWindows # No pty support to test any inferior output
- def test_qMemoryRegionInfo_reports_heap_address_as_rw(self):
- self.build()
- self.set_inferior_startup_launch()
-
- # Start up the inferior.
- procs = self.prep_debug_monitor_and_inferior(
- inferior_args=["get-heap-address-hex:", "sleep:5"]
- )
-
- # Run the process
- self.test_sequence.add_log_lines(
- [
- # Start running after initial stop.
- "read packet: $c#63",
- # Match output line that prints the memory address of the message buffer within the inferior.
- # Note we require launch-only testing so we can get inferior otuput.
- {
- "type": "output_match",
- "regex": self.maybe_strict_output_regex(
- r"heap address: 0x([0-9a-fA-F]+)\r\n"
- ),
- "capture": {1: "heap_address"},
- },
- # Now stop the inferior.
- "read packet: {}".format(chr(3)),
- # And wait for the stop notification.
- {
- "direction": "send",
- "regex": r"^\$T([0-9a-fA-F]{2})thread:([0-9a-fA-F]+);",
- "capture": {1: "stop_signo", 2: "stop_thread_id"},
- },
- ],
- True,
- )
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
-
- # Grab the address.
- self.assertIsNotNone(context.get("heap_address"))
- heap_address = int(context.get("heap_address"), 16)
-
- # Grab memory region info from the inferior.
- self.reset_test_sequence()
- self.add_query_memory_region_packets(heap_address)
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
- mem_region_dict = self.parse_memory_region_packet(context)
-
- # Ensure there are no errors reported.
- self.assertNotIn("error", mem_region_dict)
-
- # Ensure address is readable and executable.
- self.assertIn("permissions", mem_region_dict)
- self.assertIn("r", mem_region_dict["permissions"])
- self.assertIn("w", mem_region_dict["permissions"])
-
- # Ensure the start address and size encompass the address we queried.
- self.assert_address_within_memory_region(heap_address, mem_region_dict)
-
- def breakpoint_set_and_remove_work(self, want_hardware):
- # Start up the inferior.
- procs = self.prep_debug_monitor_and_inferior(
- inferior_args=[
- "get-code-address-hex:hello",
- "sleep:1",
- "call-function:hello",
- ]
- )
-
- # Run the process
- self.add_register_info_collection_packets()
- self.add_process_info_collection_packets()
- self.test_sequence.add_log_lines(
- [ # Start running after initial stop.
- "read packet: $c#63",
- # Match output line that prints the memory address of the function call entry point.
- # Note we require launch-only testing so we can get inferior otuput.
- {
- "type": "output_match",
- "regex": self.maybe_strict_output_regex(
- r"code address: 0x([0-9a-fA-F]+)\r\n"
- ),
- "capture": {1: "function_address"},
- },
- # Now stop the inferior.
- "read packet: {}".format(chr(3)),
- # And wait for the stop notification.
- {
- "direction": "send",
- "regex": r"^\$T([0-9a-fA-F]{2})thread:([0-9a-fA-F]+);",
- "capture": {1: "stop_signo", 2: "stop_thread_id"},
- },
- ],
- True,
- )
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
-
- # Gather process info - we need endian of target to handle register
- # value conversions.
- process_info = self.parse_process_info_response(context)
- endian = process_info.get("endian")
- self.assertIsNotNone(endian)
-
- # Gather register info entries.
- reg_infos = self.parse_register_info_packets(context)
- (pc_lldb_reg_index, pc_reg_info) = self.find_pc_reg_info(reg_infos)
- self.assertIsNotNone(pc_lldb_reg_index)
- self.assertIsNotNone(pc_reg_info)
-
- # Grab the function address.
- self.assertIsNotNone(context.get("function_address"))
- function_address = int(context.get("function_address"), 16)
-
- # Get current target architecture
- target_arch = self.getArchitecture()
-
- # Set the breakpoint.
- if target_arch in ["arm", "arm64", "aarch64"]:
- # TODO: Handle case when setting breakpoint in thumb code
- BREAKPOINT_KIND = 4
- else:
- BREAKPOINT_KIND = 1
-
- # Set default packet type to Z0 (software breakpoint)
- z_packet_type = 0
-
- # If hardware breakpoint is requested set packet type to Z1
- if want_hardware:
- z_packet_type = 1
-
- self.reset_test_sequence()
- self.add_set_breakpoint_packets(
- function_address,
- z_packet_type,
- do_continue=True,
- breakpoint_kind=BREAKPOINT_KIND,
- )
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
-
- # Verify the stop signal reported was the breakpoint signal number.
- stop_signo = context.get("stop_signo")
- self.assertIsNotNone(stop_signo)
- self.assertEqual(int(stop_signo, 16), lldbutil.get_signal_number("SIGTRAP"))
-
- # Ensure we did not receive any output. If the breakpoint was not set, we would
- # see output (from a launched process with captured stdio) printing a hello, world message.
- # That would indicate the breakpoint didn't take.
- self.assertEqual(len(context["O_content"]), 0)
-
- # Verify that the PC for the main thread is where we expect it - right at the breakpoint address.
- # This acts as a another validation on the register reading code.
- self.reset_test_sequence()
- self.test_sequence.add_log_lines(
- [
- # Print the PC. This should match the breakpoint address.
- "read packet: $p{0:x}#00".format(pc_lldb_reg_index),
- # Capture $p results.
- {
- "direction": "send",
- "regex": r"^\$([0-9a-fA-F]+)#",
- "capture": {1: "p_response"},
- },
- ],
- True,
- )
-
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
-
- # Verify the PC is where we expect. Note response is in endianness of
- # the inferior.
- p_response = context.get("p_response")
- self.assertIsNotNone(p_response)
-
- # Convert from target endian to int.
- returned_pc = lldbgdbserverutils.unpack_register_hex_unsigned(
- endian, p_response
- )
- self.assertEqual(returned_pc, function_address)
-
- # Verify that a breakpoint remove and continue gets us the expected
- # output.
- self.reset_test_sequence()
-
- # Add breakpoint remove packets
- self.add_remove_breakpoint_packets(
- function_address, z_packet_type, breakpoint_kind=BREAKPOINT_KIND
- )
-
- self.test_sequence.add_log_lines(
- [
- # Continue running.
- "read packet: $c#63",
- # We should now receive the output from the call.
- {"type": "output_match", "regex": r"^hello, world\r\n$"},
- # And wait for program completion.
- {"direction": "send", "regex": r"^\$W00(.*)#[0-9a-fA-F]{2}$"},
- ],
- True,
- )
-
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
-
- @skipIfWindows # No pty support to test any inferior output
- def test_software_breakpoint_set_and_remove_work(self):
- if self.getArchitecture() == "arm":
- # TODO: Handle case when setting breakpoint in thumb code
- self.build(dictionary={"CFLAGS_EXTRAS": "-marm"})
- else:
- self.build()
- self.set_inferior_startup_launch()
- self.breakpoint_set_and_remove_work(want_hardware=False)
-
- @skipUnlessPlatform(oslist=["linux"])
- @skipIf(archs=no_match(["arm", "aarch64"]))
- def test_hardware_breakpoint_set_and_remove_work(self):
- if self.getArchitecture() == "arm":
- # TODO: Handle case when setting breakpoint in thumb code
- self.build(dictionary={"CFLAGS_EXTRAS": "-marm"})
- else:
- self.build()
- self.set_inferior_startup_launch()
- self.breakpoint_set_and_remove_work(want_hardware=True)
-
- def get_qSupported_dict(self, features=[]):
- self.build()
- self.set_inferior_startup_launch()
-
- # Start up the stub and start/prep the inferior.
- procs = self.prep_debug_monitor_and_inferior()
- self.add_qSupported_packets(features)
-
- # Run the packet stream.
- context = self.expect_gdbremote_sequence()
- self.assertIsNotNone(context)
-
- # Retrieve the qSupported features.
- return self.parse_qSupported_response(context)
-
- def test_qSupported_returns_known_stub_features(self):
- supported_dict = self.get_qSupported_dict()
- self.assertIsNotNone(supported_dict)
- self.assertGreater(len(supported_dict), 0)
-
- def test_qSupported_auvx(self):
- expected = (
- "+"
- if lldbplatformutil.getPlatform() in ["freebsd", "linux", "netbsd"]
- else "-"
- )
- supported_dict = self.get_qSupported_dict()
- self.assertEqual(supported_dict.get("qXfer:auxv:read", "-"), expected)
-
- def test_qSupported_libraries_svr4(self):
- expected = (
- "+"
- if lldbplatformutil.getPlatform() in ["freebsd", "linux", "netbsd"]
- else "-"
- )
- supported_dict = self.get_qSupported_dict()
- self.assertEqual(supported_dict.get("qXfer:libraries-svr4:read", "-"), expected)
-
- def test_qSupported_siginfo_read(self):
- expected = (
- "+" if lldbplatformutil.getPlatform() in ["freebsd", "linux"] else "-"
- )
- supported_dict = self.get_qSupported_dict()
- self.assertEqual(supp...
[truncated]
|
No, it looks like the timeout comes from the lit runner which is one level up from the dotest runner that runs the individual
We could try removing some of those, but it seems like it could lead to timing issues when running under stress. Looking at all the sleeps it only adds up to < 1m, which is not too bad for the 10m test timeout we currently have (a couple of these are in helper functions but do not look to be called very often). |
On what configuration exactly. I do not recall this happening on Linaro's buildbots at least. (I have seen parts of this fail on Windows on Arm, but not timeouts) |
DavidSpickett
changed the title
This is a Debug build on x86_64-linux running on a zen3 processor. It doesn't seem to be load related because it will still timeout running that single test. |
|
Even for a debug build, timeouts aren't a good experience. The sleeps as you say are a minority of the time, and I would rather not disturb them anyway. So I agree with splitting this. How many others are close to the limit that you've found, just this one? |
I was only seeing the one timeout in the runs so I had not thought to measure. It's a good idea though. Based on the timings it looks like maybe
|
|
If this PR is approved, and I suspect folks will be on board with splitting these extremely long tests, but I will hold off approving for a few days in case that's not the case. |
DavidSpickett
changed the title
|
I looked into why this test (and TestGdbRemoteFork.py) is so slow. Turns out its nothing related to lldb itself, but the build step that occurs as part of the test. These tests methods pretty much all call to to build the test lldb-server And it takes about 20s to compile this file with a debug build! So each test is running for at least 20s. The actual test part (after the build) seems to usually be quite fast (< 1s). The tests use the just-built clang to compile the file. Looking at the output from Looking at the build() implementation, it looks like it builds a separate binary for each
which is defined so that the test to include name as part of the path Any suggestions on how to improve this? It seems like we could maybe share the built executable amongst all tests in the file, but the test framework does put them into separate per-test locations. |
|
You could make the test binary a cmake dependency, see This means if you ran the test directly without |
|
The test really is big and splitting it up is a good idea, but instead of an arbitrary 1/2/3 split, I think it be better to do it by functionality. Looking at the part of the code you moved, I can see several "themes" that we could use to split this up. One of them is "memory operations" (TestGdbRemoteMemory?) where we could put all of the memory and qMemoryRegionInfo tests. Another might be "qSupported" tests. And a third test for the register operations ( If that doesn't reduce the file sufficiently, I'm sure we can find some more. |
Just so it's clear, it is not lldb-server itself that is the slow dependency, but the cpp file in the test directory. I think you mean we could create a new binary from the test file. Pulling out the cpp file into a separate tool might work. I don't know if there are any issues trying that. It's now building with the just-built clang binaries hooks into the dotest build system which seems to allow more flexibility in building it in different ways at test runtime (although I'm not sure we actually rely on that flexibility). |
Thanks for the suggestion. I wasn't familiar enough to do a proper split, but I can take a look based on your suggestion. It seems we already have some memory tests split out: |
I think that would be a problem for remote tests (where the architecture of the binary needs to be different from the lldb architecture) and would be a significant change in how the tests work. I really wouldn't want to go in that direction. What I could imagine is building the binary in the "static" |
Sounds good. And if you do that, then we can also create another/simpler binary for those tests. For example, all of the qSupported tests don't really care about the binary they're running. They just need a binary. It might as well just be |
dmpots
changed the title
This test is running right up to the 600s timeout limit. Split it in two to avoid hitting a timeout.
|
@labath @DavidSpickett I split the tests based on functional area. Please take a look, thanks! |
|
✅ With the latest revision this PR passed the Python code formatter. |
There was a problem hiding this comment.
let's make the folder name qSupported, as this is just too generic.
DavidSpickett
changed the title
Split test cases out of TestLldbGdbServer.py and TestGdbRemoteFork.py into separate files to avoid hitting the 600s timeout limit. The inferior used by these tests (main.cpp) takes approximately 20s to compile with a Debug build of clang, causing timeouts when a single test file contains many tests. By grouping similar tests into separate files, we can prevent timeouts and improve overall test efficiency.