Implement vAttachOrWait

[augusto2112]

Summary

Implements the required functions on gdb-remote so the '--include-existing' flag of process attach works correctly on Linux.

[augusto2112]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

379

Is there an easier way to format a StringRef such that if I have a StringRef "foo" I get a new StringRef "/foo"?

384

There was an oversight on my last patch, where we could attach to a different process if it ended with the name of the process we want to attach to. For example if we want to attach to "bar", it could also attach to "foobar". I changed it so we exclude processes whose names aren't a perfect match ("bar") or end with a slash + process name ("path/to/bar"). Is this enough, or could there be other false positives?

3306

Handle_vAttachOrWait and Handle_vAttachWait are pretty much the same. The only differences are the argument we pass to packet.SetFilePos and to AttachWaitProcess. Since they're so small I decided to keep them as separate functions, but I could make one common function for both.

[labath]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

379

Not exactly, since StringRef always needs a persistent materialized string to refer to. But in the cases you don't have one handy, you can use a std::string. You could create it as ("/" + foo).str(), but per the comment below, I don't think this is the right solution.

384

That is odd, because GetNameAsStringRef should already strip the path:

llvm::StringRef ProcessInfo::GetNameAsStringRef() const {
  return m_executable.GetFilename().GetStringRef();
}

If that's not working for you, then I guess you need to figure out why is that function failing.

[augusto2112]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

384

Oh I see. I just assumed that it could potentially return the full path, my bad. I believe simply comparing info_name != process_name will be sufficient then.

[clayborg]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

359–367

Reverse this logic and switch the if/else contents?

378

Do we need to get the correct directory delimiter for the current platform here? This will work for everything except windows. Just wanted to bring this up in case we do.

[augusto2112]

Addresses comments, includes tests.

[augusto2112]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

378

Turns out that was unnecessary since match_infos are compared by file name and not full path.

[clayborg]

LGTM. Pavel?

[augusto2112]

@labath, @clayborg since both of you approved of it, I think this patch is ready to be merged, right? Could one of you do it? I don't have commit access.

[augusto2112]

@labath sorry for the repeated messages. I believe this is ready to be merged, right?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

public:
   ///
   /// This method supports waiting for the next instance of a process
   /// with a given name and attaching llgs to that via the configured
   /// Platform.
   ///
   /// \return
   ///     An Status object indicating the success or failure of the
   ///     attach operation.
-  Status AttachWaitProcess(llvm::StringRef process_name);
+  Status AttachWaitProcess(llvm::StringRef process_name, bool include_existing);
 
   // NativeProcessProtocol::NativeDelegate overrides
   void InitializeDelegate(NativeProcessProtocol *process) override;
 
   void ProcessStateChanged(NativeProcessProtocol *process,
                            lldb::StateType state) override;
 
   void DidExec(NativeProcessProtocol *process) override;
protected:
   PacketResult Handle_jLLDBTraceSupportedType(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_QRestoreRegisterState(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_vAttach(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_vAttachWait(StringExtractorGDBRemote &packet);
 
+  PacketResult Handle_qVAttachOrWaitSupported(StringExtractorGDBRemote &packet);
+
+  PacketResult Handle_vAttachOrWait(StringExtractorGDBRemote &packet);
+
   PacketResult Handle_D(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_qThreadStopInfo(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_jThreadsInfo(StringExtractorGDBRemote &packet);
 
   PacketResult Handle_qWatchpointSupportInfo(StringExtractorGDBRemote &packet);
 

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

RegisterMemberFunctionHandler(
       &GDBRemoteCommunicationServerLLGS::Handle_stop_reason); // ?
   RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_vAttach,
       &GDBRemoteCommunicationServerLLGS::Handle_vAttach);
   RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_vAttachWait,
       &GDBRemoteCommunicationServerLLGS::Handle_vAttachWait);
   RegisterMemberFunctionHandler(
+      StringExtractorGDBRemote::eServerPacketType_qVAttachOrWaitSupported,
+      &GDBRemoteCommunicationServerLLGS::Handle_qVAttachOrWaitSupported);
+  RegisterMemberFunctionHandler(
+      StringExtractorGDBRemote::eServerPacketType_vAttachOrWait,
+      &GDBRemoteCommunicationServerLLGS::Handle_vAttachOrWait);
+  RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_vCont,
       &GDBRemoteCommunicationServerLLGS::Handle_vCont);
   RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_vCont_actions,
       &GDBRemoteCommunicationServerLLGS::Handle_vCont_actions);
   RegisterMemberFunctionHandler(
       StringExtractorGDBRemote::eServerPacketType_x,
       &GDBRemoteCommunicationServerLLGS::Handle_memory_read);
LLDB_LOGF(log,
               __FUNCTION__, terminal_fd);
   }
 
   printf("Attached to process %" PRIu64 "...\n", pid);
   return Status();
 }
 
 Status GDBRemoteCommunicationServerLLGS::AttachWaitProcess(
-    llvm::StringRef process_name) {
+    llvm::StringRef process_name, bool include_existing) {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
 
   std::chrono::milliseconds polling_interval = std::chrono::milliseconds(1);
 
   // Create the matcher used to search the process list.
   ProcessInstanceInfoList exclusion_list;
   ProcessInstanceInfoMatch match_info;
   match_info.GetProcessInfo().GetExecutableFile().SetFile(
-      process_name, llvm::sys::path::Style::posix);
-  match_info.SetNameMatchType(NameMatch::EndsWith);
+      process_name, llvm::sys::path::Style::native);
+  match_info.SetNameMatchType(NameMatch::Equals);
 
+  if (include_existing) {
+    LLDB_LOG(log, "including existing processes in search");
+  } else {
     // Create the excluded process list before polling begins.
     Host::FindProcesses(match_info, exclusion_list);
     LLDB_LOG(log, "placed '{0}' processes in the exclusion list.",
              exclusion_list.size());
+  }
 
   LLDB_LOG(log, "waiting for '{0}' to appear", process_name);
 
   auto is_in_exclusion_list =
       [&exclusion_list](const ProcessInstanceInfo &info) {
         for (auto &excluded : exclusion_list) {
           if (excluded.GetProcessID() == info.GetProcessID())
             return true;
         }
         return false;
       };
 
   ProcessInstanceInfoList loop_process_list;
   while (true) {
     loop_process_list.clear();
     if (Host::FindProcesses(match_info, loop_process_list)) {
       // Remove all the elements that are in the exclusion list.
       llvm::erase_if(loop_process_list, is_in_exclusion_list);
 
       // One match! We found the desired process.
       if (loop_process_list.size() == 1) {
         auto matching_process_pid = loop_process_list[0].GetProcessID();
         LLDB_LOG(log, "found pid {0}", matching_process_pid);
         return AttachToProcess(matching_process_pid);
       }
 
GDBRemoteCommunicationServerLLGS::Handle_vAttachWait(
   // Allocate the buffer for the process name from vAttachWait.
   std::string process_name;
   if (!packet.GetHexByteString(process_name))
     return SendIllFormedResponse(packet,
                                  "vAttachWait failed to parse process name");
 
   LLDB_LOG(log, "attempting to attach to process named '{0}'", process_name);
 
-  Status error = AttachWaitProcess(process_name);
+  Status error = AttachWaitProcess(process_name, false);
+  if (error.Fail()) {
+    LLDB_LOG(log, "failed to attach to process named '{0}': {1}", process_name,
+             error);
+    return SendErrorResponse(error);
+  }
+
+  // Notify we attached by sending a stop packet.
+  return SendStopReasonForState(m_debugged_process_up->GetState());
+}
+
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServerLLGS::Handle_qVAttachOrWaitSupported(
+    StringExtractorGDBRemote &packet) {
+  return SendOKResponse();
+}
+
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunicationServerLLGS::Handle_vAttachOrWait(
+    StringExtractorGDBRemote &packet) {
+  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
+
+  // Consume the ';' after the identifier.
+  packet.SetFilePos(strlen("vAttachOrWait"));
+
+  if (!packet.GetBytesLeft() || packet.GetChar() != ';')
+    return SendIllFormedResponse(packet, "vAttachOrWait missing expected ';'");
+
+  // Allocate the buffer for the process name from vAttachWait.
+  std::string process_name;
+  if (!packet.GetHexByteString(process_name))
+    return SendIllFormedResponse(packet,
+                                 "vAttachOrWait failed to parse process name");
+
+  LLDB_LOG(log, "attempting to attach to process named '{0}'", process_name);
+
+  Status error = AttachWaitProcess(process_name, true);
   if (error.Fail()) {
     LLDB_LOG(log, "failed to attach to process named '{0}': {1}", process_name,
              error);
     return SendErrorResponse(error);
   }
 
   // Notify we attached by sending a stop packet.
   return SendStopReasonForState(m_debugged_process_up->GetState());

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

+
+import os
+from time import sleep
+
+import gdbremote_testcase
+import lldbgdbserverutils
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestGdbRemoteAttachOrWait(gdbremote_testcase.GdbRemoteTestCaseBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test_launch_before_attach_with_vAttachOrWait(self):
+        exe = '%s_%d' % (self.testMethodName, os.getpid())
+        self.build(dictionary={'EXE': exe})
+        self.set_inferior_startup_attach_manually()
+
+        # Start the inferior, start the debug monitor, nothing is attached yet.
+        procs = self.prep_debug_monitor_and_inferior(
+            inferior_args=["sleep:60"],
+            inferior_exe_path=self.getBuildArtifact(exe))
+        self.assertIsNotNone(procs)
+
+        # Make sure the target process has been launched.
+        inferior = procs.get("inferior")
+        self.assertIsNotNone(inferior)
+        self.assertTrue(inferior.pid > 0)
+        self.assertTrue(
+            lldbgdbserverutils.process_is_running(
+                inferior.pid, True))
+
+        # Add attach packets.
+        self.test_sequence.add_log_lines([
+            # Do the attach.
+            "read packet: $vAttachOrWait;{}#00".format(lldbgdbserverutils.gdbremote_hex_encode_string(exe)),
+            # Expect a stop notification from the attach.
+            {"direction": "send",
+             "regex": r"^\$T([0-9a-fA-F]{2})[^#]*#[0-9a-fA-F]{2}$",
+             "capture": {1: "stop_signal_hex"}},
+        ], True)
+        self.add_process_info_collection_packets()
+
+        # Run the stream
+        context = self.expect_gdbremote_sequence()
+        self.assertIsNotNone(context)
+
+        # Gather process info response
+        process_info = self.parse_process_info_response(context)
+        self.assertIsNotNone(process_info)
+
+        # Ensure the process id matches what we expected.
+        pid_text = process_info.get('pid', None)
+        self.assertIsNotNone(pid_text)
+        reported_pid = int(pid_text, base=16)
+        self.assertEqual(reported_pid, inferior.pid)
+
+    def test_launch_after_attach_with_vAttachOrWait(self):
+        exe = '%s_%d' % (self.testMethodName, os.getpid())
+        self.build(dictionary={'EXE': exe})
+        self.set_inferior_startup_attach_manually()
+
+        server = self.connect_to_debug_monitor()
+        self.assertIsNotNone(server)
+
+        self.add_no_ack_remote_stream()
+        self.test_sequence.add_log_lines([
+            # Do the attach.
+            "read packet: $vAttachOrWait;{}#00".format(lldbgdbserverutils.gdbremote_hex_encode_string(exe)),
+        ], True)
+        # Run the stream until attachWait.
+        context = self.expect_gdbremote_sequence()
+        self.assertIsNotNone(context)
+
+        # Sleep so we're sure that the inferior is launched after we ask for the attach.
+        sleep(1)
+
+        # Launch the inferior.
+        inferior = self.launch_process_for_attach(
+            inferior_args=["sleep:60"],
+            exe_path=self.getBuildArtifact(exe))
+        self.assertIsNotNone(inferior)
+        self.assertTrue(inferior.pid > 0)
+        self.assertTrue(
+            lldbgdbserverutils.process_is_running(
+                inferior.pid, True))
+
+        # Make sure the attach succeeded.
+        self.test_sequence.add_log_lines([
+            {"direction": "send",
+             "regex": r"^\$T([0-9a-fA-F]{2})[^#]*#[0-9a-fA-F]{2}$",
+             "capture": {1: "stop_signal_hex"}},
+        ], True)
+        self.add_process_info_collection_packets()
+
+
+        # Run the stream sending the response..
+        context = self.expect_gdbremote_sequence()
+        self.assertIsNotNone(context)
+
+        # Gather process info response.
+        process_info = self.parse_process_info_response(context)
+        self.assertIsNotNone(process_info)
+
+        # Ensure the process id matches what we expected.
+        pid_text = process_info.get('pid', None)
+        self.assertIsNotNone(pid_text)
+        reported_pid = int(pid_text, base=16)
+        self.assertEqual(reported_pid, inferior.pid)