[lldb-vscode] Don't hang indefinitely on invalid program

[zturner]

Summary

When you configure your launch settings, if you pass a path to an invalid program, we would previously hand indefinitely. This is because a combination of two bugs working together. The first is that we did not attempt to detect when handling the request whether the file existed, and we would just pass it through to the Process::Launch method. The second is that the Process::Launch method did not properly report an error in the case where the file does not exist. It actually reported that the launch succeeded, which would then cause LLDB to wait on the broadcaster to receive some events, which would obviously never come.

Although fixing this in either place independently will get lldb-vscode working properly when an invalid executable is specified, I'm fixing it in both places because it seems like the right thing to do. Note that for the first fix (the one in lldb-vscode.cpp) we were previously checking the value of error.Fail(), but the previous call did not actually communicate an error via an SBError return, instead it communicated an error via a null SBModule return. So this condition is changed.

[labath]

A test case?

lldb/trunk/source/Target/Process.cpp

Status Process::Launch(ProcessLaunchInfo &launch_info) {
   m_abi_sp.reset();
   m_dyld_up.reset();
   m_jit_loaders_up.reset();
   m_system_runtime_up.reset();
   m_os_up.reset();
   m_process_input_reader.reset();
 
   Module *exe_module = GetTarget().GetExecutableModulePointer();
-  if (exe_module) {
-    char local_exec_file_path[PATH_MAX];
-    char platform_exec_file_path[PATH_MAX];
-    exe_module->GetFileSpec().GetPath(local_exec_file_path,
-                                      sizeof(local_exec_file_path));
-    exe_module->GetPlatformFileSpec().GetPath(platform_exec_file_path,
-                                              sizeof(platform_exec_file_path));
-    if (FileSystem::Instance().Exists(exe_module->GetFileSpec())) {
-      // Install anything that might need to be installed prior to launching.
-      // For host systems, this will do nothing, but if we are connected to a
-      // remote platform it will install any needed binaries
-      error = GetTarget().Install(&launch_info);
-      if (error.Fail())
-        return error;
+  if (!exe_module) {
+    error.SetErrorString("executable module does not exist");
+    return error;
+  }
+
 
-      if (PrivateStateThreadIsValid())
-        PausePrivateStateThread();
 
-      error = WillLaunch(exe_module);
-      if (error.Success()) {
-        const bool restarted = false;
-        SetPublicState(eStateLaunching, restarted);
-        m_should_detach = false;
 
-        if (m_public_run_lock.TrySetRunning()) {
-          // Now launch using these arguments.
-          error = DoLaunch(exe_module, launch_info);
-        } else {
-          // This shouldn't happen
-          error.SetErrorString("failed to acquire process run lock");
-        }
 
-        if (error.Fail()) {
-          if (GetID() != LLDB_INVALID_PROCESS_ID) {
-            SetID(LLDB_INVALID_PROCESS_ID);
-            const char *error_string = error.AsCString();
-            if (error_string == nullptr)
-              error_string = "launch failed";
-            SetExitStatus(-1, error_string);
-          }
-        } else {
-          EventSP event_sp;
 
-          // Now wait for the process to launch and return control to us, and then call
-          // DidLaunch:
-          StateType state = WaitForProcessStopPrivate(event_sp, seconds(10));
+        // Now wait for the process to launch and return control to us, and then
+        // call DidLaunch:
 
-          if (state == eStateInvalid || !event_sp) {
-            // We were able to launch the process, but we failed to catch the
-            // initial stop.
-            error.SetErrorString("failed to catch stop after launch");
-            SetExitStatus(0, "failed to catch stop after launch");
-            Destroy(false);
-          } else if (state == eStateStopped || state == eStateCrashed) {
-            DidLaunch();
 
-            DynamicLoader *dyld = GetDynamicLoader();
-            if (dyld)
-              dyld->DidLaunch();
 
-            GetJITLoaders().DidLaunch();
 
-            SystemRuntime *system_runtime = GetSystemRuntime();
-            if (system_runtime)
-              system_runtime->DidLaunch();
 
-            if (!m_os_up)
-              LoadOperatingSystemPlugin(false);
 
-            // We successfully launched the process and stopped, now it the
-            // right time to set up signal filters before resuming.
-            UpdateAutomaticSignalFiltering();
 
-            // Note, the stop event was consumed above, but not handled. This
-            // was done to give DidLaunch a chance to run. The target is either
-            // stopped or crashed. Directly set the state.  This is done to
-            // prevent a stop message with a bunch of spurious output on thread
-            // status, as well as not pop a ProcessIOHandler.
-            SetPublicState(state, false);
 
-            if (PrivateStateThreadIsValid())
-              ResumePrivateStateThread();
-            else
-              StartPrivateStateThread();
 
-            // Target was stopped at entry as was intended. Need to notify the
-            // listeners about it.
-            if (state == eStateStopped &&
-                launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
-              HandlePrivateEvent(event_sp);
-          } else if (state == eStateExited) {
-            // We exited while trying to launch somehow.  Don't call DidLaunch
-            // as that's not likely to work, and return an invalid pid.
-            HandlePrivateEvent(event_sp);
-          }
-        }
-      }
-    } else {
-      error.SetErrorStringWithFormat("file doesn't exist: '%s'",
-                                     local_exec_file_path);
-    }
-  }
+
   return error;
 }
 
 Status Process::LoadCore() {
   Status error = DoLoadCore();
   if (error.Success()) {
     ListenerSP listener_sp(
         Listener::MakeListener("lldb.process.load_core_listener"));

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

void request_launch(const llvm::json::Object &request) {
   // Grab the current working directory if there is one and set it in the
   // launch info.
   const auto cwd = GetString(arguments, "cwd");
   if (!cwd.empty())
     g_vsc.launch_info.SetWorkingDirectory(cwd.data());
 
   // Grab the name of the program we need to debug and set it as the first
   // argument that will be passed to the program we will debug.
-  const auto program = GetString(arguments, "program");
+  llvm::StringRef program = GetString(arguments, "program");
   if (!program.empty()) {
     lldb::SBFileSpec program_fspec(program.data(), true /*resolve_path*/);
-
     g_vsc.launch_info.SetExecutableFile(program_fspec,
                                         true /*add_as_first_arg*/);
     const char *target_triple = nullptr;
     const char *uuid_cstr = nullptr;
     // Stand alone debug info file if different from executable
     const char *symfile = nullptr;
-    g_vsc.target.AddModule(program.data(), target_triple, uuid_cstr, symfile);
-    if (error.Fail()) {
+    lldb::SBModule module = g_vsc.target.AddModule(
+        program.data(), target_triple, uuid_cstr, symfile);
+    if (!module.IsValid()) {
       response["success"] = llvm::json::Value(false);
-      EmplaceSafeString(response, "message", std::string(error.GetCString()));
+
+      EmplaceSafeString(
+          response, "message",
+          llvm::formatv("Could not load program '{0}'.", program).str());
       g_vsc.SendJSON(llvm::json::Value(std::move(response)));
     }
   }
 
   // Extract any extra arguments and append them to our program arguments for
   // when we launch
   auto args = GetStrings(arguments, "args");
   if (!args.empty())