Improving error reporting for attachCommands and adding a test for attachCommands.

Applying reviewer suggestions.

Author: ashgti

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

@@ -79,7 +79,7 @@ def test_by_name(self):
     def test_by_name_waitFor(self):
         """
         Tests attaching to a process by process name and waiting for the
-        next instance of a process to be launched, ingoring all current
+        next instance of a process to be launched, ignoring all current
         ones.
         """
         program = self.build_and_create_debug_adapter_for_attach()
@@ -102,7 +102,7 @@ def test_commands(self):
         that can be passed during attach.
 
         "initCommands" are a list of LLDB commands that get executed
-        before the targt is created.
+        before the target is created.
         "preRunCommands" are a list of LLDB commands that get executed
         after the target has been created and before the launch.
         "stopCommands" are a list of LLDB commands that get executed each
@@ -180,6 +180,24 @@ def test_commands(self):
         self.verify_commands("exitCommands", output, exitCommands)
         self.verify_commands("terminateCommands", output, terminateCommands)
 
+    def test_attach_command_process_failures(self):
+        """
+        Tests that a 'attachCommands' is expected to leave the debugger's
+        selected target with a valid process.
+        """
+        program = self.build_and_create_debug_adapter_for_attach()
+        attachCommands = ['script print("oops, forgot to attach to a process...")']
+        resp = self.attach(
+            program=program,
+            attachCommands=attachCommands,
+            expectFailure=True,
+        )
+        self.assertFalse(resp["success"])
+        self.assertIn(
+            "attachCommands failed to attach to a process",
+            resp["body"]["error"]["format"],
+        )
+
     @skipIfNetBSD  # Hangs on NetBSD as well
     @skipIf(
         archs=["arm", "aarch64"]

lldb/tools/lldb-dap/DAP.cpp

@@ -675,11 +675,12 @@ lldb::SBTarget DAP::CreateTarget(lldb::SBError &error) {
   // enough information to determine correct arch and platform (or ELF can be
   // omitted at all), so it is good to leave the user an opportunity to specify
   // those. Any of those three can be left empty.
-  auto target = this->debugger.CreateTarget(configuration.program.data(),
-                                            configuration.targetTriple.data(),
-                                            configuration.platformName.data(),
-                                            true, // Add dependent modules.
-                                            error);
+  auto target = this->debugger.CreateTarget(
+      /*filename=*/configuration.program.data(),
+      /*target_triple=*/configuration.targetTriple.data(),
+      /*platform_name=*/configuration.platformName.data(),
+      /*add_dependent_modules=*/true, // Add dependent modules.
+      error);
 
   return target;
 }

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

@@ -77,11 +77,10 @@ Error AttachRequestHandler::Run(const AttachRequestArguments &args) const {
   if ((args.pid == LLDB_INVALID_PROCESS_ID ||
        args.gdbRemotePort == LLDB_DAP_INVALID_PORT) &&
       args.waitFor) {
-    char attach_msg[256];
-    auto attach_msg_len = snprintf(attach_msg, sizeof(attach_msg),
-                                   "Waiting to attach to \"%s\"...",
-                                   dap.target.GetExecutable().GetFilename());
-    dap.SendOutput(OutputType::Console, StringRef(attach_msg, attach_msg_len));
+    dap.SendOutput(OutputType::Console,
+                   llvm::formatv("Waiting to attach to \"{0}\"...",
+                                 dap.target.GetExecutable().GetFilename())
+                       .str());
   }
 
   {
@@ -96,7 +95,13 @@ Error AttachRequestHandler::Run(const AttachRequestArguments &args) const {
       // state.
       if (llvm::Error err = dap.RunAttachCommands(args.attachCommands))
         return err;
+
       dap.target = dap.debugger.GetSelectedTarget();
+
+      // Validate the attachCommand results.
+      if (!dap.target.GetProcess().IsValid())
+        return make_error<DAPError>(
+            "attachCommands failed to attach to a process");
     } else if (!args.coreFile.empty()) {
       dap.target.LoadCore(args.coreFile.data(), error);
     } else if (args.gdbRemotePort != LLDB_DAP_INVALID_PORT) {
@@ -116,7 +121,7 @@ Error AttachRequestHandler::Run(const AttachRequestArguments &args) const {
         attach_info.SetProcessID(args.pid);
       else if (!dap.configuration.program.empty())
         attach_info.SetExecutable(dap.configuration.program.data());
-      attach_info.SetWaitForLaunch(args.waitFor, false /*async*/);
+      attach_info.SetWaitForLaunch(args.waitFor, /*async=*/false);
       dap.target.Attach(attach_info, error);
     }
   }

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

@@ -168,6 +168,9 @@ class RequestHandler : public BaseRequestHandler {
 
   /// A hook for a request handler to run additional operations after the
   /// request response is sent but before the next request handler.
+  ///
+  /// *NOTE*: PostRun will be invoked even if the `Run` operation returned an
+  /// error.
   virtual void PostRun() const {};
 
   protocol::ErrorResponseBody ToResponse(llvm::Error err) const {

lldb/tools/lldb-dap/Protocol/ProtocolRequests.h

@@ -151,7 +151,7 @@ struct Configuration {
   /// information in your executable contains relative paths, this option can be
   /// used so that `lldb-dap` can find source files and object files that have
   /// relative paths.
-  std::string debuggerRoot = "";
+  std::string debuggerRoot;
 
   /// Enable auto generated summaries for variables when no summaries exist for
   /// a given type. This feature can cause performance delays in large projects
@@ -194,7 +194,7 @@ struct Configuration {
 
   /// Specify a source path to remap "./" to allow full paths to be used when
   /// setting breakpoints in binaries that have relative source paths.
-  std::string sourcePath = "";
+  std::string sourcePath;
 
   /// Specify an array of path re-mappings. Each element in the array must be a
   /// two element array containing a source and destination pathname. Overrides
@@ -230,15 +230,15 @@ struct Configuration {
   ///
   /// *NOTE:* When launching, either `launchCommands` or `program` must be
   /// configured. If both are configured then `launchCommands` takes priority.
-  std::string program = "";
+  std::string program;
 
   /// Target triple for the program (arch-vendor-os). If not set, inferred from
   /// the binary.
-  std::string targetTriple = "";
+  std::string targetTriple;
 
   /// Specify name of the platform to use for this target, creating the platform
   /// if necessary.
-  std::string platformName = "";
+  std::string platformName;
 };
 
 /// lldb-dap specific launch arguments.
@@ -264,7 +264,7 @@ struct LaunchRequestArguments {
   std::vector<std::string> launchCommands;
 
   /// The program working directory.
-  std::string cwd = "";
+  std::string cwd;
 
   /// An array of command line argument strings to be passed to the program
   /// being launched.
@@ -336,7 +336,7 @@ struct AttachRequestArguments {
   std::string gdbRemoteHostname = "localhost";
 
   /// Path to the core file to debug.
-  std::string coreFile = "";
+  std::string coreFile;
 
   /// @}
 };