Fix race condition when launching and attaching.

[clayborg]

Summary

We discovered that when using "launchCommands" or "attachCommands" that there was an issue where these commands were not being run synchronously. There were further problems in this case where we would get thread events for the process that was just launched or attached before the IDE was ready, which is after "configurationDone" was sent to lldb-vscode.

This fix introduces the ability to wait for the process to stop after the run or attach to ensure that we have a stopped process at the entry point that is ready for the debug session to proceed. This also allows us to run the normal launch or attach without needing to play with the async flag the debugger. We spin up the thread that listens for process events before we start the launch or attach, but we stop events from being delivered through the DAP protocol until the "configurationDone" packet is received. This function was manually delivering the first process stopped event in the code already, so this means we don't end up sending two stopped events at the start of a debug session.

Added a flag to the vscode.py protocol classes that detects and ensures that no "stopped" events are sent prior to "configurationDone" has been sent and will raise an error if it does happen.

This should make our launching and attaching more reliable and avoid some deadlocks that were being seen (https://reviews.llvm.org/D119548).

[ilya-nozhkin]

lldb/tools/lldb-vscode/VSCode.cpp

9

Incompatible with MSVC. Maybe use std::chrono instead?

[labath]

Instead of polling, would it be possible for the event handler thread to send a notification (through a condition variable, std::future, etc.), and have the main thread wait for that?

But in general, avoiding flipping the modes back and forth seems like a good idea.

lldb/tools/lldb-vscode/lldb-vscode.cpp

799

is something missing here?

[yinghuitan]

lldb/tools/lldb-vscode/VSCode.cpp

544

Is this a typo? It should be 1000 instead of 10000, right?

[wallace]

I agree with labath. Specially because I wouldn't like the user having aborted debug sessions if their setup takes longer than 10 seconds to get the process ready, which I imagine might happen with android or oculus devices using custom launch configs. What about using a condition variable and at the same time issue a progress notification mentioning that the LLDB is waiting for the process to stop? They could stop LLDB manually if something is not working properly for them in that case.

[clayborg]

• use std::chrono for sleeping the function during polling
• we must use polling in VSCode::WaitForProcessToStop since attach doesn't deliver the initial eStateStopped event
• ignore any eStateStopped events that have a process stop ID of 1. This allows us to easily ignore the first stop that is delivered for launching, and if attaching ever does deliver this first event, it will be ignored. We do this because the IDE expects no process stopped events before "configurationDone" is completed and also because request_configurationDone() will deliver the first stop event manually _if_ we are stopping at the entry ("stopOnEntry" from launch config), else it will auto resume the process for us
• add a new "timeout" launch configuration argument for both launch and attach which defaults to 30 seconds if not specified. This controls how long we poll for the process to stop during launch or attach.

[clayborg]

In D119797#3321924, @labath wrote:

Instead of polling, would it be possible for the event handler thread to send a notification (through a condition variable, std::future, etc.), and have the main thread wait for that?

I tried doing this, but was unable to make it work because attaching doesn't send the a eStateStopped event for the process' stop ID of 1 where launching does. Probably a bug that can be fixed in LLDB at some point, but attaching can be done in so many ways that I didn't want to hold up this patch for this.

In D119797#3324277, @wallace wrote:

I agree with labath. Specially because I wouldn't like the user having aborted debug sessions if their setup takes longer than 10 seconds to get the process ready, which I imagine might happen with android or oculus devices using custom launch configs. What about using a condition variable and at the same time issue a progress notification mentioning that the LLDB is waiting for the process to stop? They could stop LLDB manually if something is not working properly for them in that case.

I default to 30 seconds now and I have added a new "timeout" launch configuration argument that _can_ be specified and the timeout can be made longer. Let me know if the 30 second default timeout should be increased.

lldb/tools/lldb-vscode/VSCode.cpp

9

Done in the current updated patch, thanks for pointing this out.

544

Fixed by using std::chrono

[wallace]

lgtm

lldb/tools/lldb-vscode/VSCode.cpp

533

[omjavaid]

Hi @clayborg this rev has caused some lldb-vscode test failures across all LLDB/Linux buildbots.

[clayborg]

In D119797#3331419, @omjavaid wrote:

Hi @clayborg this rev has caused some lldb-vscode test failures across all LLDB/Linux buildbots.

tracking this down now

[clayborg]

Fixed buildbots with:

commit 38054556a08884aa15d3ebc720e2f43d0cb5a944 (HEAD -> main, origin/main, origin/HEAD)
Author: Greg Clayton <[e-mail]>
Date:   Thu Feb 17 23:59:15 2022 -0800

    Fix buildbots after https://reviews.llvm.org/D119797
    
    This value error is no longer needed with the new version of the patch

[labath]

TestVSCode_attach.py is still failing. Are you able to fix that quickly or shall we revert?

[labath]

There's also a timeout in TestVSCode_coreFile.py

[clayborg]

Here is the diff that should fix things:

https://reviews.llvm.org/rGc41c57468949

[labath]

It worked. Thanks.

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py

def handle_recv_packet(self, packet):
                 # When a new process is attached or launched, remember the
                 # details that are available in the body of the event
                 self.process_event_body = body
             elif event == 'stopped':
                 # Each thread that stops with a reason will send a
                 # 'stopped' event. We need to remember the thread stop
                 # reasons since the 'threads' command doesn't return
                 # that information.
+                if not self.configuration_done_sent:
+                    raise ValueError("'stopped' event received before "
+                                     "configuationDone packet was sent")
                 self._process_stopped()
                 tid = body['threadId']
                 self.thread_stop_reasons[tid] = body
             elif event == 'breakpoint':
                 # Breakpoint events come in when a breakpoint has locations
                 # added or removed. Keep track of them so we can look for them
                 # in tests.
                 self.breakpoint_events.append(packet)

lldb/test/API/tools/lldb-vscode/launch/TestVSCode_launch.py

def test_commands(self):
         output = self.collect_console(duration=1.0)
         self.verify_commands('exitCommands', output, exitCommands)
         self.verify_commands('terminateCommands', output, terminateCommands)
 
     @skipIfWindows
     @skipIfRemote
     def test_extra_launch_commands(self):
         '''
-            Tests the "luanchCommands" with extra launching settings
+            Tests the "launchCommands" with extra launching settings
         '''
         self.build_and_create_debug_adaptor()
         program = self.getBuildArtifact("a.out")
 
         source = 'main.c'
         first_line = line_number(source, '// breakpoint 1')
         second_line = line_number(source, '// breakpoint 2')
         # Set target binary and 2 breakpoints

lldb/tools/lldb-vscode/VSCode.h

struct VSCode {
   /// \param[in] callback
   ///     The callback to execute when the given request is triggered by the
   ///     IDE.
   void RegisterRequestCallback(std::string request, RequestCallback callback);
 
   /// Debuggee will continue from stopped state.
   void WillContinue() { variables.Clear(); }
 
+  /// Poll the process to wait for it to reach the eStateStopped state.
+  ///
+  /// We need to ensure the process is stopped and ready to resume before we
+  /// continue with the launch or attach. This is needed since we no longer play
+  /// with the synchronous mode in the debugger for launching (with or without
+  /// "launchCommands") or attaching (with or without "attachCommands").
+  ///
+  /// \param[in] seconds
+  ///   The number of seconds to poll the process to wait until it is stopped.
+  ///
+  /// \return Error if waiting for the process fails, no error if succeeds.
+  lldb::SBError WaitForProcessToStop(uint32_t seconds);
+
 private:
   // Send the JSON in "json_str" to the "out" stream. Correctly send the
   // "Content-Length:" field followed by the length, followed by the raw
   // JSON bytes.
   void SendJSON(const std::string &json_str);
 };
 
 extern VSCode g_vsc;
 
 } // namespace lldb_vscode
 
 #endif

lldb/tools/lldb-vscode/VSCode.cpp

 //===-- VSCode.cpp ----------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <chrono>
 #include <cstdarg>
 #include <fstream>
 #include <mutex>
 #include <sstream>
 
 #include "LLDBUtils.h"
 #include "VSCode.h"
 #include "llvm/Support/FormatVariadic.h"
PacketStatus VSCode::SendReverseRequest(llvm::json::Object request,
   return PacketStatus::EndOfFile;
 }
 
 void VSCode::RegisterRequestCallback(std::string request,
                                      RequestCallback callback) {
   request_handlers[request] = callback;
 }
 
+lldb::SBError VSCode::WaitForProcessToStop(uint32_t seconds) {
+  // Wait for the process hit a stopped state. When running a launch (with or
+  // without "launchCommands") or attach (with or without)= "attachCommands"),
+  // the calls might take some time to stop at the entry point since the command
+  // is asynchronous. So we need to sync up with the process and make sure it is
+  // stopped before we proceed to do anything else as we will soon be asked to
+  // set breakpoints and other things that require the process to be stopped.
+  // We must use polling because attach doesn't send a process state change
+  // event for the first stop, while launching does. Since both "attachCommands"
+  // and "launchCommands" could end up using any combination of LLDB commands,
+  // we must ensure we can also catch when the process stops, so we must poll
+  // the process to make sure we handle all cases.
+
+  lldb::SBError error;
+  lldb::SBProcess process = target.GetProcess();
+  if (!process.IsValid()) {
+    error.SetErrorString("invalid process");
+    return error;
+  }
+  auto timeout_time =
+      std::chrono::high_resolution_clock::now() + std::chrono::seconds(seconds);
+  while (std::chrono::high_resolution_clock::now() < timeout_time) {
+    const auto state = process.GetState();
+    switch (state) {
+      case lldb::eStateAttaching:
+      case lldb::eStateConnected:
+      case lldb::eStateInvalid:
+      case lldb::eStateLaunching:
+      case lldb::eStateRunning:
+      case lldb::eStateStepping:
+      case lldb::eStateSuspended:
+        break;
+      case lldb::eStateDetached:
+        error.SetErrorString("process detached during launch or attach");
+        return error;
+      case lldb::eStateExited:
+        error.SetErrorString("process exited during launch or attach");
+        return error;
+      case lldb::eStateUnloaded:
+        error.SetErrorString("process unloaded during launch or attach");
+        return error;
+      case lldb::eStateCrashed:
+      case lldb::eStateStopped:
+        return lldb::SBError(); // Success!
+    }
+    std::this_thread::sleep_for(std::chrono::microseconds(250));
+  }
+  error.SetErrorStringWithFormat("process failed to stop within %u seconds",
+                                 seconds);
+  return error;
+}
+
 void Variables::Clear() {
   locals.Clear();
   globals.Clear();
   registers.Clear();
   expandable_variables.clear();
 }
 
 int64_t Variables::GetNewVariableRefence(bool is_permanent) {

lldb/tools/lldb-vscode/lldb-vscode.cpp

if (listener.WaitForEvent(1, event)) {
             break;
           case lldb::eStateCrashed:
             break;
           case lldb::eStateDetached:
             break;
           case lldb::eStateSuspended:
             break;
           case lldb::eStateStopped:
+            // Now that we don't mess with the async setting in the debugger
+            // when launching or attaching we will get the first process stop
+            // event which we do not want to send an event for. This is because
+            // we either manually deliver the event in by calling the
+            // SendThreadStoppedEvent() from request_configuarationDone() if we
+            // want to stop on entry, or we resume from that function.
+            if (process.GetStopID() > 1) {
               // Only report a stopped event if the process was not restarted.
               if (!lldb::SBProcess::GetRestartedFromEvent(event)) {
                 SendStdOutStdErr(process);
                 SendThreadStoppedEvent();
               }
+            }
             break;
           case lldb::eStateRunning:
             g_vsc.WillContinue();
             break;
           case lldb::eStateExited: {
             // Run any exit LLDB commands the user specified in the
             // launch.json
             g_vsc.RunExitCommands();
void request_attach(const llvm::json::Object &request) {
   attach_info.SetWaitForLaunch(wait_for, false /*async*/);
   g_vsc.init_commands = GetStrings(arguments, "initCommands");
   g_vsc.pre_run_commands = GetStrings(arguments, "preRunCommands");
   g_vsc.stop_commands = GetStrings(arguments, "stopCommands");
   g_vsc.exit_commands = GetStrings(arguments, "exitCommands");
   g_vsc.terminate_commands = GetStrings(arguments, "terminateCommands");
   auto attachCommands = GetStrings(arguments, "attachCommands");
   llvm::StringRef core_file = GetString(arguments, "coreFile");
+  const uint64_t timeout_seconds = GetUnsigned(arguments, "timeout", 30);
   g_vsc.stop_at_entry =
       core_file.empty() ? GetBoolean(arguments, "stopOnEntry", false) : true;
   std::vector<std::string> postRunCommands =
       GetStrings(arguments, "postRunCommands");
   const llvm::StringRef debuggerRoot = GetString(arguments, "debuggerRoot");
 
   // This is a hack for loading DWARF in .o files on Mac where the .o files
   // in the debug map of the main executable have relative paths which require
if (pid == LLDB_INVALID_PROCESS_ID && wait_for) {
     auto attach_msg_len = snprintf(attach_msg, sizeof(attach_msg),
                                    "Waiting to attach to \"%s\"...",
                                    g_vsc.target.GetExecutable().GetFilename());
     g_vsc.SendOutput(OutputType::Console,
                      llvm::StringRef(attach_msg, attach_msg_len));
   }
   if (attachCommands.empty()) {
     // No "attachCommands", just attach normally.
-    // Disable async events so the attach will be successful when we return from
-    // the launch call and the launch will happen synchronously
-    g_vsc.debugger.SetAsync(false);
     if (core_file.empty())
       g_vsc.target.Attach(attach_info, error);
     else
       g_vsc.target.LoadCore(core_file.data(), error);
-    // Reenable async events
-    g_vsc.debugger.SetAsync(true);
   } else {
     // 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.
     g_vsc.RunLLDBCommands("Running attachCommands:", attachCommands);
     // The custom commands might have created a new target so we should use the
     // selected target after these commands are run.
     g_vsc.target = g_vsc.debugger.GetSelectedTarget();
   }
+  // Make sure the process is attached and stopped before proceeding.
+  if (error.Success())
+    error = g_vsc.WaitForProcessToStop(timeout_seconds);
 
   if (error.Success() && core_file.empty()) {
     auto attached_pid = g_vsc.target.GetProcess().GetProcessID();
     if (attached_pid == LLDB_INVALID_PROCESS_ID) {
       if (attachCommands.empty())
         error.SetErrorString("failed to attach to a process");
       else
         error.SetErrorString("attachCommands failed to attach to a process");

 //             }]
 // },
 void request_configurationDone(const llvm::json::Object &request) {
   llvm::json::Object response;
   FillResponse(request, response);
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
   if (g_vsc.stop_at_entry)
     SendThreadStoppedEvent();
   else
     g_vsc.target.GetProcess().Continue();
 }
 
 // "DisconnectRequest": {
 //   "allOf": [ { "$ref": "#/definitions/Request" }, {
 //     "type": "object",
 //     "description": "Disconnect request; value of command field is
 //                     'disconnect'.",
void request_launch(const llvm::json::Object &request) {
   g_vsc.stop_commands = GetStrings(arguments, "stopCommands");
   g_vsc.exit_commands = GetStrings(arguments, "exitCommands");
   g_vsc.terminate_commands = GetStrings(arguments, "terminateCommands");
   auto launchCommands = GetStrings(arguments, "launchCommands");
   std::vector<std::string> postRunCommands =
       GetStrings(arguments, "postRunCommands");
   g_vsc.stop_at_entry = GetBoolean(arguments, "stopOnEntry", false);
   const llvm::StringRef debuggerRoot = GetString(arguments, "debuggerRoot");
+  const uint64_t timeout_seconds = GetUnsigned(arguments, "timeout", 30);
 
   // This is a hack for loading DWARF in .o files on Mac where the .o files
   // in the debug map of the main executable have relative paths which require
   // the lldb-vscode binary to have its working directory set to that relative
   // root for the .o files in order to be able to load debug info.
   if (!debuggerRoot.empty())
     llvm::sys::fs::set_current_path(debuggerRoot);
 
void request_launch(const llvm::json::Object &request) {
 
   // Run any pre run LLDB commands the user specified in the launch.json
   g_vsc.RunPreRunCommands();
 
   if (GetBoolean(arguments, "runInTerminal", false)) {
     if (llvm::Error err = request_runInTerminal(request))
       error.SetErrorString(llvm::toString(std::move(err)).c_str());
   } else if (launchCommands.empty()) {
-    // Disable async events so the launch will be successful when we return from
-    // the launch call and the launch will happen synchronously
-    g_vsc.debugger.SetAsync(false);
     g_vsc.target.Launch(launch_info, error);
-    g_vsc.debugger.SetAsync(true);
   } else {
     g_vsc.RunLLDBCommands("Running launchCommands:", launchCommands);
     // The custom commands might have created a new target so we should use the
     // selected target after these commands are run.
     g_vsc.target = g_vsc.debugger.GetSelectedTarget();
   }
+  // Make sure the process is launched and stopped at the entry point before
+  // proceeding.
+  if (error.Success())
+    error = g_vsc.WaitForProcessToStop(timeout_seconds);
 
   if (error.Fail()) {
     response["success"] = llvm::json::Value(false);
     EmplaceSafeString(response, "message", std::string(error.GetCString()));
   } else {
     g_vsc.RunLLDBCommands("Running postRunCommands:", postRunCommands);
   }
 

lldb/tools/lldb-vscode/package.json

"debuggers": [
               },
               "postRunCommands": {
                 "type": "array",
                 "description": "Commands executed just as soon as the program is successfully launched when it's in a stopped state prior to any automatic continuation.",
                 "default": []
               },
               "launchCommands": {
                 "type": "array",
-                "description": "Custom commands that are executed instead of launching a process. A target will be created with the launch arguments prior to executing these commands. The commands may optionally create a new target and must perform a launch. A valid process must exist after these commands complete or the \"launch\" will fail.",
+                "description": "Custom commands that are executed instead of launching a process. A target will be created with the launch arguments prior to executing these commands. The commands may optionally create a new target and must perform a launch. A valid process must exist after these commands complete or the \"launch\" will fail. Launch the process with \"process launch -s\" to make the process to at the entry point since lldb-vscode will auto resume if necessary.",
                 "default": []
               },
               "stopCommands": {
                 "type": "array",
                 "description": "Commands executed each time the program stops.",
                 "default": []
               },
               "exitCommands": {
                 "type": "array",
                 "description": "Commands executed at the end of debugging session.",
                 "default": []
               },
               "runInTerminal": {
                 "type": "boolean",
                 "description": "Launch the program inside an integrated terminal in the IDE. Useful for debugging interactive command line programs",
                 "default": false
+              },
+              "timeout": {
+                "type": "string",
+                "description": "The time in seconds to wait for a program to stop at entry point when launching. Defaults to 30 seconds."
               }
             }
           },
           "attach": {
             "properties": {
               "program": {
                 "type": "string",
                 "description": "Path to the program to attach to."
"debuggers": [
               "exitCommands": {
                 "type": "array",
                 "description": "Commands executed at the end of debugging session.",
                 "default": []
               },
               "coreFile": {
                 "type": "string",
                 "description": "Path to the core file to debug."
+              },
+              "timeout": {
+                "type": "string",
+                "description": "The time in seconds to wait for a program to stop when attaching. Defaults to 30 seconds."
               }
             }
           }
         },
         "initialConfigurations": [
           {
             "type": "lldb-vscode",
             "request": "launch",