[lldb/test] Remove sleeps from some lldb-server tests

[labath]

Summary

Instead of using sleeps, have the inferior notify us (via a signal) that
the requested number of threads have been created.

This allows us to get rid of some fairly dodgy test utility code --
wait_for_thread_count seemed like it was waiting for the threads to
appear, but it never actually let the inferior run, so it only succeeded
if the threads were already started when the function was called. Since
the function was called after a fairly small delay (1s, usually), this
is probably the reason why the tests were failing on some bots.

[DavidSpickett]

I'm pretty sure I've seen this sort of failure before but can't remember if it was locally or on one of many bots. (which probably means all of them!) This seems like a good improvement.

lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py

699

I don't think this is an issue but what would cause you to have > the number of threads?

Are there automatically created threads sometimes that we don't explicitly ask for.

lldb/test/API/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py

22

If there's a very slim chance this text is "" you could assertTrue and catch both situations.

34

Afaict parse_key_val_dict will never return None. At worst an empty dictionary.

[labath]

Speaking of windows, I just realized that this method will (obviously) not work there. I should have occurred to me there was a reason I didn't do this earlier. Let me try a different synchronization method instead.

lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py

699

Windows std::threads maintain a thread pool of spun-up threads, presumably to speed up their creation. It is somewhat annoying because, for some of these lower-level tests, we would really want to know the exact number of threads and their properties. One day, I'm going to create some some thread creation wrappers to avoid this...

lldb/test/API/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py

22

technically, "" might be correct, if we decide to return a bare Txx packet for some reason.

34

Yeah, this code is being overly defensive.

[labath]

Use a trap opcode instead of a signal. This has two main drawbacks:

• needs some architecture-specific code (gcc does not have __builtin_debugtrap)
• if we ever need to resume from this state, we will need to add special handling to make progress on systems where the trap opcode does not increment the PC

I still think it beats sleeps, though.

[DavidSpickett]

LGTM

lldb/test/API/tools/lldb-server/main.cpp

144

I thought this might need to be different for Windows on Arm64 but no, it's the same instruction https://docs.microsoft.com/en-us/cpp/intrinsics/debugbreak?redirectedfrom=MSDN&view=msvc-170.

[aprantl]

This looks amazing! Thanks, Pavel.

lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py

def parse_threadinfo_packets(self, context):
         self.assertIsNotNone(threadinfo_responses)
 
         thread_ids = []
         for threadinfo_response in threadinfo_responses:
             new_thread_infos = parse_threadinfo_response(threadinfo_response)
             thread_ids.extend(new_thread_infos)
         return thread_ids
 
-    def wait_for_thread_count(self, thread_count):
-        start_time = time.time()
-        timeout_time = start_time + self.DEFAULT_TIMEOUT
+    def launch_with_threads(self, thread_count):
+        procs = self.prep_debug_monitor_and_inferior(
+                inferior_args=["thread:new"]*(thread_count-1) + ["raise:SIGUSR1"])
 
-        actual_thread_count = 0
-        while actual_thread_count < thread_count:
-            self.reset_test_sequence()
-            self.add_threadinfo_collection_packets()
-
-            context = self.expect_gdbremote_sequence()
-            self.assertIsNotNone(context)
-
-            threads = self.parse_threadinfo_packets(context)
-            self.assertIsNotNone(threads)
-
-            actual_thread_count = len(threads)
-
-            if time.time() > timeout_time:
-                raise Exception(
-                    'timed out after {} seconds while waiting for threads: waiting for at least {} threads, found {}'.format(
-                        self.DEFAULT_TIMEOUT, thread_count, actual_thread_count))
-
-        return threads
+        self.test_sequence.add_log_lines([
+                "read packet: $c#00",
+                {"direction": "send",
+                    "regex": r"^\$T{0:02x}([^#]*)#..$".format(lldbutil.get_signal_number('SIGUSR1')),
+                    "capture": {1: "stop_reply_kv"}}], True)
+        self.assertGreaterEqual(len(threads), thread_count)
+        return context, threads
 
     def add_set_breakpoint_packets(
             self,
             address,
             z_packet_type=0,
             do_continue=True,
             breakpoint_kind=1):
         self.test_sequence.add_log_lines(
def parse_qSupported_response(self, context):
             # Ensure we know the supported element
             if key not in self._KNOWN_QSUPPORTED_STUB_FEATURES:
                 raise Exception(
                     "unknown qSupported stub feature reported: %s" %
                     key)
 
         return supported_dict
 
-    def run_process_then_stop(self, run_seconds=1):
-        # Tell the stub to continue.
-        self.test_sequence.add_log_lines(
-            ["read packet: $vCont;c#a8"],
-            True)
-        context = self.expect_gdbremote_sequence()
-
-        # Wait for run_seconds.
-        time.sleep(run_seconds)
-
-        # Send an interrupt, capture a T response.
-        self.reset_test_sequence()
-        self.test_sequence.add_log_lines(
-            ["read packet: {}".format(chr(3)),
-             {"direction": "send", "regex": r"^\$T([0-9a-fA-F]+)([^#]+)#[0-9a-fA-F]{2}$", "capture": {1: "stop_result"}}],
-            True)
-        context = self.expect_gdbremote_sequence()
-        self.assertIsNotNone(context)
-        self.assertIsNotNone(context.get("stop_result"))
-
-        return context
-
     def continue_process_and_wait_for_stop(self):
         self.test_sequence.add_log_lines(
             [
                 "read packet: $vCont;c#a8",
                 {
                     "direction": "send",
                     "regex": r"^\$T([0-9a-fA-F]{2})(.*)#[0-9a-fA-F]{2}$",
                     "capture": {1: "stop_signo", 2: "stop_key_val_text"},

lldb/test/API/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py

class TestGdbRemoteThreadsInStopReply(
 
     mydir = TestBase.compute_mydir(__file__)
 
     ENABLE_THREADS_IN_STOP_REPLY_ENTRIES = [
         "read packet: $QListThreadsInStopReply#21",
         "send packet: $OK#00",
     ]
 
-    def gather_stop_reply_fields(self, post_startup_log_lines, thread_count,
-            field_names):
-        # Set up the inferior args.
-        inferior_args = []
-        for i in range(thread_count - 1):
-            inferior_args.append("thread:new")
-        inferior_args.append("sleep:10")
-        procs = self.prep_debug_monitor_and_inferior(
-            inferior_args=inferior_args)
+    def gather_stop_reply_fields(self, thread_count, field_names):
+        context, threads = self.launch_with_threads(thread_count)
+        key_vals_text = context.get("stop_reply_kv")
+        self.assertIsNotNone(key_vals_text)
 
+        self.reset_test_sequence()
         self.add_register_info_collection_packets()
         self.add_process_info_collection_packets()
 
-        # Assumes test_sequence has anything added needed to setup the initial state.
-        # (Like optionally enabling QThreadsInStopReply.)
-        if post_startup_log_lines:
-            self.test_sequence.add_log_lines(post_startup_log_lines, True)
-        self.test_sequence.add_log_lines([
-            "read packet: $c#63"
-        ], True)
         context = self.expect_gdbremote_sequence()
         self.assertIsNotNone(context)
         hw_info = self.parse_hw_info(context)
 
-        # Give threads time to start up, then break.
-        time.sleep(self.DEFAULT_SLEEP)
-        self.reset_test_sequence()
-        self.test_sequence.add_log_lines(
-            [
-                "read packet: {}".format(
-                    chr(3)),
-                {
-                    "direction": "send",
-                    "regex": r"^\$T([0-9a-fA-F]+)([^#]+)#[0-9a-fA-F]{2}$",
-                    "capture": {
-                        1: "stop_result",
-                        2: "key_vals_text"}},
-            ],
-            True)
-        context = self.expect_gdbremote_sequence()
-        self.assertIsNotNone(context)
-
-        # Wait until all threads have started.
-        threads = self.wait_for_thread_count(thread_count)
-        self.assertIsNotNone(threads)
-        self.assertEqual(len(threads), thread_count)
-
-        # Run, then stop the process, grab the stop reply content.
-        self.reset_test_sequence()
-        self.test_sequence.add_log_lines(["read packet: $c#63",
-                                          "read packet: {}".format(chr(3)),
-                                          {"direction": "send",
-                                           "regex": r"^\$T([0-9a-fA-F]+)([^#]+)#[0-9a-fA-F]{2}$",
-                                           "capture": {1: "stop_result",
-                                                       2: "key_vals_text"}},
-                                          ],
-                                         True)
-        context = self.expect_gdbremote_sequence()
-        self.assertIsNotNone(context)
-
         # Parse the stop reply contents.
-        key_vals_text = context.get("key_vals_text")
-        self.assertIsNotNone(key_vals_text)
         kv_dict = self.parse_key_val_dict(key_vals_text)
         self.assertIsNotNone(kv_dict)
 
         result = dict();
         result["pc_register"] = hw_info["pc_register"]
         result["little_endian"] = hw_info["little_endian"]
         for key_field in field_names:
             result[key_field] = kv_dict.get(key_field)
 
         return result
 
-    def gather_stop_reply_threads(self, post_startup_log_lines, thread_count):
+    def gather_stop_reply_threads(self, thread_count):
         # Pull out threads from stop response.
         stop_reply_threads_text = self.gather_stop_reply_fields(
-                post_startup_log_lines, thread_count, ["threads"])["threads"]
+                thread_count, ["threads"])["threads"]
         if stop_reply_threads_text:
             return [int(thread_id, 16)
                     for thread_id in stop_reply_threads_text.split(",")]
         else:
             return []
 
-    def gather_stop_reply_pcs(self, post_startup_log_lines, thread_count):
-        results = self.gather_stop_reply_fields( post_startup_log_lines,
-                thread_count, ["threads", "thread-pcs"])
+    def gather_stop_reply_pcs(self, thread_count):
+        results = self.gather_stop_reply_fields(thread_count, ["threads", "thread-pcs"])
         if not results:
             return []
 
         threads_text = results["threads"]
         pcs_text = results["thread-pcs"]
         thread_ids = threads_text.split(",")
         pcs = pcs_text.split(",")
         self.assertEquals(len(thread_ids), len(pcs))
class TestGdbRemoteThreadsInStopReply(
     # delegate, e.g. llgs.  So tests below to assert the stop threads number will all fail.
     @expectedFailureAll(oslist=["windows"])
     @skipIfNetBSD
     def test_stop_reply_reports_multiple_threads(self):
         self.build()
         self.set_inferior_startup_launch()
         # Gather threads from stop notification when QThreadsInStopReply is
         # enabled.
-        stop_reply_threads = self.gather_stop_reply_threads(
-            self.ENABLE_THREADS_IN_STOP_REPLY_ENTRIES, 5)
+        self.test_sequence.add_log_lines(
+            self.ENABLE_THREADS_IN_STOP_REPLY_ENTRIES, True)
+        stop_reply_threads = self.gather_stop_reply_threads(5)
         self.assertEqual(len(stop_reply_threads), 5)
 
     @expectedFailureAll(oslist=["windows"])
     @skipIfNetBSD
     def test_no_QListThreadsInStopReply_supplies_no_threads(self):
         self.build()
         self.set_inferior_startup_launch()
         # Gather threads from stop notification when QThreadsInStopReply is not
         # enabled.
-        stop_reply_threads = self.gather_stop_reply_threads(None, 5)
+        stop_reply_threads = self.gather_stop_reply_threads(5)
         self.assertEqual(len(stop_reply_threads), 0)
 
     @expectedFailureAll(oslist=["windows"])
     @skipIfNetBSD
     def test_stop_reply_reports_correct_threads(self):
         self.build()
         self.set_inferior_startup_launch()
         # Gather threads from stop notification when QThreadsInStopReply is
         # enabled.
         thread_count = 5
-        stop_reply_threads = self.gather_stop_reply_threads(
-            self.ENABLE_THREADS_IN_STOP_REPLY_ENTRIES, thread_count)
-        self.assertEqual(len(stop_reply_threads), thread_count)
+        self.test_sequence.add_log_lines(
+            self.ENABLE_THREADS_IN_STOP_REPLY_ENTRIES, True)
+        stop_reply_threads = self.gather_stop_reply_threads(thread_count)
 
         # Gather threads from q{f,s}ThreadInfo.
         self.reset_test_sequence()
         self.add_threadinfo_collection_packets()
 
         context = self.expect_gdbremote_sequence()
         self.assertIsNotNone(context)
 
         threads = self.parse_threadinfo_packets(context)
         self.assertIsNotNone(threads)
         self.assertEqual(len(threads), thread_count)
 
         # Ensure each thread in q{f,s}ThreadInfo appears in stop reply threads
         for tid in threads:
             self.assertIn(tid, stop_reply_threads)
 
     @expectedFailureAll(oslist=["windows"])
     @skipIfNetBSD
     def test_stop_reply_contains_thread_pcs(self):
         self.build()
         self.set_inferior_startup_launch()
         thread_count = 5
-        results = self.gather_stop_reply_pcs(
-                self.ENABLE_THREADS_IN_STOP_REPLY_ENTRIES, thread_count)
+        self.test_sequence.add_log_lines(
+            self.ENABLE_THREADS_IN_STOP_REPLY_ENTRIES, True)
+        results = self.gather_stop_reply_pcs(thread_count)
         stop_reply_pcs = results["thread_pcs"]
         pc_register = results["pc_register"]
         little_endian = results["little_endian"]
         self.assertEqual(len(stop_reply_pcs), thread_count)
 
         threads_info_pcs = self.gather_threads_info_pcs(pc_register,
                 little_endian)
 
         self.assertEqual(len(threads_info_pcs), thread_count)
         for thread_id in stop_reply_pcs:
             self.assertIn(thread_id, threads_info_pcs)
             self.assertEqual(int(stop_reply_pcs[thread_id], 16),
                     int(threads_info_pcs[thread_id], 16))

lldb/test/API/tools/lldb-server/TestGdbRemote_qThreadStopInfo.py

 import unittest2
 import gdbremote_testcase
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 class TestGdbRemote_qThreadStopInfo(gdbremote_testcase.GdbRemoteTestCaseBase):
 
     mydir = TestBase.compute_mydir(__file__)
     THREAD_COUNT = 5
 
     def gather_stop_replies_via_qThreadStopInfo(self, thread_count):
-        # Set up the inferior args.
-        inferior_args = []
-        for i in range(thread_count - 1):
-            inferior_args.append("thread:new")
-        inferior_args.append("sleep:10")
-        procs = self.prep_debug_monitor_and_inferior(
-            inferior_args=inferior_args)
-
-        # Assumes test_sequence has anything added needed to setup the initial state.
-        # (Like optionally enabling QThreadsInStopReply.)
-        self.test_sequence.add_log_lines([
-            "read packet: $c#63"
-        ], True)
-        context = self.expect_gdbremote_sequence()
-        self.assertIsNotNone(context)
-
-        # Give threads time to start up, then break.
-        time.sleep(self.DEFAULT_SLEEP)
-        self.reset_test_sequence()
-        self.test_sequence.add_log_lines(
-            [
-                "read packet: {}".format(
-                    chr(3)),
-                {
-                    "direction": "send",
-                    "regex": r"^\$T([0-9a-fA-F]+)([^#]+)#[0-9a-fA-F]{2}$",
-                    "capture": {
-                        1: "stop_result",
-                        2: "key_vals_text"}},
-            ],
-            True)
-        context = self.expect_gdbremote_sequence()
-        self.assertIsNotNone(context)
-
-        # Wait until all threads have started.
-        threads = self.wait_for_thread_count(thread_count)
-        self.assertIsNotNone(threads)
+        context, threads = self.launch_with_threads(thread_count)
 
         # On Windows, there could be more threads spawned. For example, DebugBreakProcess will
         # create a new thread from the debugged process to handle an exception event. So here we
         # assert 'GreaterEqual' condition.
         triple = self.dbg.GetSelectedPlatform().GetTriple()
         if re.match(".*-.*-windows", triple):
             self.assertGreaterEqual(len(threads), thread_count)
         else:

lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

def test_p_returns_correct_data_size_for_each_qRegisterInfo_launch(self):
                 self.updateRegInfoBitsize(reg_info, byte_order)
             self.assertEqual(len(p_response), 2 * int(reg_info["bitsize"]) / 8,
                              reg_info)
 
             # Increment loop
             reg_index += 1
 
     def Hg_switches_to_3_threads(self, pass_pid=False):
-        # Startup the inferior with three threads (main + 2 new ones).
-        procs = self.prep_debug_monitor_and_inferior(
-            inferior_args=["thread:new", "thread:new"])
-
-        # Let the inferior process have a few moments to start up the thread
-        # when launched.  (The launch scenario has no time to run, so threads
-        # won't be there yet.)
-        self.run_process_then_stop(run_seconds=1)
-
-        # Wait at most x seconds for 3 threads to be present.
-        threads = self.wait_for_thread_count(3)
-        self.assertEqual(len(threads), 3)
+        _, threads = self.launch_with_threads(3)
 
         pid_str = ""
         if pass_pid:
             pid_str = "p{0:x}.".format(procs["inferior"].pid)
 
         # verify we can $H to each thead, and $qC matches the thread we set.
         for thread in threads:
             # Change to each thread, verify current thread id.
class LldbGdbServerTestCase(gdbremote_testcase.GdbRemoteTestCaseBase, DwarfOpcodeParser):
 
     @expectedFailureAll(oslist=["windows"]) # expect 4 threads
     @skipIf(compiler="clang", compiler_version=['<', '11.0'])
     def test_Hg_switches_to_3_threads_launch(self):
         self.build()
         self.set_inferior_startup_launch()
         self.Hg_switches_to_3_threads()
 
-    @expectedFailureAll(oslist=["windows"]) # expecting one more thread
-    @skipIf(compiler="clang", compiler_version=['<', '11.0'])
-    def test_Hg_switches_to_3_threads_attach(self):
-        self.build()
-        self.set_inferior_startup_attach()
-        self.Hg_switches_to_3_threads()
-
-    @expectedFailureAll(oslist=["windows"]) # expect 4 threads
-    @add_test_categories(["llgs"])
-    @skipIf(compiler="clang", compiler_version=['<', '11.0'])
-    def test_Hg_switches_to_3_threads_attach_pass_correct_pid(self):
-        self.build()
-        self.set_inferior_startup_attach()
-        self.Hg_switches_to_3_threads(pass_pid=True)
-
     def Hg_fails_on_pid(self, pass_pid):
-        # Start the inferior.
-        procs = self.prep_debug_monitor_and_inferior(
-            inferior_args=["thread:new"])
-
-        self.run_process_then_stop(run_seconds=1)
-
-        threads = self.wait_for_thread_count(2)
-        self.assertEqual(len(threads), 2)
+        _, threads = self.launch_with_threads(2)
 
         if pass_pid == -1:
             pid_str = "p-1."
         else:
             pid_str = "p{0:x}.".format(pass_pid)
         thread = threads[1]
 
         self.test_sequence.add_log_lines(
def Hc_then_Csignal_signals_correct_thread(self, segfault_signo):
 
         # Launch/attach.  (In our case, this should only ever be launched since
         # we need inferior stdout/stderr).
         procs = self.prep_debug_monitor_and_inferior(
             inferior_args=inferior_args)
         self.test_sequence.add_log_lines(["read packet: $c#63"], True)
         context = self.expect_gdbremote_sequence()
 
-        # Let the inferior process have a few moments to start up the thread when launched.
-        # context = self.run_process_then_stop(run_seconds=1)
-
-        # Wait at most x seconds for all threads to be present.
-        # threads = self.wait_for_thread_count(NUM_THREADS)
-        # self.assertEquals(len(threads), NUM_THREADS)
-
         signaled_tids = {}
         print_thread_ids = {}
 
         # Switch to each thread, deliver a signal, and verify signal delivery
         for i in range(NUM_THREADS - 1):
             # Run until SIGSEGV comes in.
             self.reset_test_sequence()
             self.test_sequence.add_log_lines([{"direction": "send",

     # Note: as of this moment, a hefty number of the GPR writes are failing
     # with E32 (everything except rax-rdx, rdi, rsi, rbp).
     @skipIfWindows
     def test_P_and_p_thread_suffix_work(self):
         self.build()
         self.set_inferior_startup_launch()
 
         # Startup the inferior with three threads.
-        procs = self.prep_debug_monitor_and_inferior(
-            inferior_args=["thread:new", "thread:new"])
+        _, threads = self.launch_with_threads(3)
+
+        self.reset_test_sequence()
         self.add_thread_suffix_request_packets()
         self.add_register_info_collection_packets()
         self.add_process_info_collection_packets()
 
         context = self.expect_gdbremote_sequence()
         self.assertIsNotNone(context)
 
         process_info = self.parse_process_info_response(context)
         self.assertIsNotNone(process_info)
         endian = process_info.get("endian")
         self.assertIsNotNone(endian)
 
         reg_infos = self.parse_register_info_packets(context)
         self.assertIsNotNone(reg_infos)
         self.add_lldb_register_index(reg_infos)
 
         reg_index = self.select_modifiable_register(reg_infos)
         self.assertIsNotNone(reg_index)
         reg_byte_size = int(reg_infos[reg_index]["bitsize"]) // 8
         self.assertTrue(reg_byte_size > 0)
 
-        # Run the process a bit so threads can start up, and collect register
-        # info.
-        context = self.run_process_then_stop(run_seconds=1)
-        self.assertIsNotNone(context)
-
-        # Wait for 3 threads to be present.
-        threads = self.wait_for_thread_count(3)
-        self.assertEqual(len(threads), 3)
-
         expected_reg_values = []
         register_increment = 1
         next_value = None
 
         # Set the same register in each of 3 threads to a different value.
         # Verify each one has the unique value.
         for thread in threads:
             # If we don't have a next value yet, start it with the initial read

lldb/test/API/tools/lldb-server/main.cpp

#warning Please add appropriate assembly for your target.
   g_c2 = '1';
 #endif
 }
 
 static void hello() {
   std::lock_guard<std::mutex> lock(g_print_mutex);
   printf("hello, world\n");
 }
 
 static void *thread_func(void *arg) {
   static std::atomic<int> s_thread_index(1);
   const int this_thread_index = s_thread_index++;
   if (g_print_thread_ids) {
     std::lock_guard<std::mutex> lock(g_print_mutex);
     printf("thread %d id: %" PRIx64 "\n", this_thread_index, get_thread_id());
   }
 
#endif
     } else if (consume_front(arg, "thread:segfault")) {
       g_threads_do_segfault = true;
     } else if (consume_front(arg, "print-pid")) {
       print_pid();
     } else if (consume_front(arg, "print-env:")) {
       // Print the value of specified envvar to stdout.
       const char *value = getenv(arg.c_str());
       printf("%s\n", value ? value : "__unset__");
+    } else if (consume_front(arg, "raise:SIGUSR1")) {
+      raise(SIGUSR1);
     } else {
       // Treat the argument as text for stdout.
       printf("%s\n", argv[i]);
     }
   }
 
   // If we launched any threads, join them
   for (std::vector<std::thread>::iterator it = threads.begin();
        it != threads.end(); ++it)
     it->join();
 
   return return_value;
 }