[lldb-dap] Fix raciness in launch and attach tests

We've gotten multiple reports of the launch and attach test being flaky,
both in CI and locally when running the tests. I believe the flakiness
is due to a race between the main thread and the event handler thread.

Launching and attaching is done in synchronous mode so the corresponding
requests return only after the respective operation has been completed.
In synchronous mode, no stop event is emitted. When we have launch or
attach commands, we cannot use synchronous mode. To hide the stop events
in this case, the default event handler thread was ignoring stop events
before the "configuration done" request was answered. The problem with
that is that there's no guarantee that we have handled the stop event
before we have handled the configuration done request.

When looking at the logs, you can see that we're still in the process of
sending module events (which I recently added) when we receive, and
respond to the "configuration done" request, before it sees the launch
or attach stop event. At that point `dap.configuration_done_sent` is
true and the event is sent, which the test doesn't expect.

This PR fixes the raciness by using an atomic flag to signal that the
next stop event should be ignored. An alternative approach could be to
stop trying to hide the initial stop event, and instead report it to the
client unconditionally. Instead of ignoring the stop for the
asynchronous case, we could send a stop event after we're done handling
the synchronous case.

Fixes #137660

Author: JDevlieghere

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py

@@ -103,7 +103,7 @@ def verify_breakpoint_hit(self, breakpoint_ids):
                     match_desc = "breakpoint %s." % (breakpoint_id)
                     if match_desc in description:
                         return
-        self.assertTrue(False, "breakpoint not hit")
+        self.assertTrue(False, f"breakpoint not hit: stopped_events={stopped_events}")
 
     def verify_stop_exception_info(self, expected_description, timeout=timeoutval):
         """Wait for the process we are debugging to stop, and verify the stop

lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py

@@ -25,7 +25,6 @@ def spawn_and_wait(program, delay):
     process.wait()
 
 
-@skipIf
 class TestDAP_attach(lldbdap_testcase.DAPTestCaseBase):
     def set_and_hit_breakpoint(self, continueToExit=True):
         source = "main.c"

lldb/test/API/tools/lldb-dap/attach/TestDAP_attachByPortNum.py

@@ -19,7 +19,6 @@
 import socket
 
 
-@skip
 class TestDAP_attachByPortNum(lldbdap_testcase.DAPTestCaseBase):
     default_timeout = 20
 

lldb/tools/lldb-dap/DAP.cpp

@@ -85,7 +85,8 @@ DAP::DAP(Log *log, const ReplMode default_repl_mode,
       exception_breakpoints(), focus_tid(LLDB_INVALID_THREAD_ID),
       stop_at_entry(false), is_attach(false),
       restarting_process_id(LLDB_INVALID_PROCESS_ID),
-      configuration_done_sent(false), waiting_for_run_in_terminal(false),
+      configuration_done_sent(false), ignore_next_stop(false),
+      waiting_for_run_in_terminal(false),
       progress_event_reporter(
           [&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }),
       reverse_request_seq(0), repl_mode(default_repl_mode) {

lldb/tools/lldb-dap/DAP.h

@@ -189,6 +189,7 @@ struct DAP {
   // the old process here so we can detect this case and keep running.
   lldb::pid_t restarting_process_id;
   bool configuration_done_sent;
+  std::atomic<bool> ignore_next_stop;
   llvm::StringMap<std::unique_ptr<BaseRequestHandler>> request_handlers;
   bool waiting_for_run_in_terminal;
   ProgressEventReporter progress_event_reporter;

lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp

@@ -155,6 +155,9 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
         std::string connect_url =
             llvm::formatv("connect://{0}:", gdb_remote_hostname);
         connect_url += std::to_string(gdb_remote_port);
+        // Connect remote will generate a stopped event even in synchronous
+        // mode.
+        dap.ignore_next_stop = true;
         dap.target.ConnectRemote(listener, connect_url.c_str(), "gdb-remote",
                                  error);
       } else {
@@ -166,6 +169,7 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const {
     // Reenable async events
     dap.debugger.SetAsync(true);
   } else {
+    dap.ignore_next_stop = true;
     // We have "attachCommands" that are a set of commands that are expected
     // to execute the commands after which a process should be created. If there
     // is no valid process after running these commands, we have failed.

lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp

@@ -167,7 +167,7 @@ static void EventThreadFunction(DAP &dap) {
             // stop events which we do not want to send an event for. We will
             // manually send a stopped event in request_configurationDone(...)
             // so don't send any before then.
-            if (dap.configuration_done_sent) {
+            if (!dap.ignore_next_stop.exchange(false)) {
               // Only report a stopped event if the process was not
               // automatically restarted.
               if (!lldb::SBProcess::GetRestartedFromEvent(event)) {

lldb/tools/lldb-dap/Handler/RequestHandler.cpp

@@ -250,6 +250,7 @@ llvm::Error BaseRequestHandler::LaunchProcess(
     if (error.Fail())
       return llvm::make_error<DAPError>(error.GetCString());
   } else {
+    dap.ignore_next_stop = true;
     // Set the launch info so that run commands can access the configured
     // launch details.
     dap.target.SetLaunchInfo(launch_info);