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

372

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

377

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?

3314

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

372

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.

377

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

377

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–366

Reverse this logic and switch the if/else contents?

371

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

371

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

 //===-- GDBRemoteCommunicationServerLLGS.h ----------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERLLGS_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERLLGS_H
 
 #include <mutex>
 #include <unordered_map>
 
 #include "lldb/Core/Communication.h"
 #include "lldb/Host/MainLoop.h"
 #include "lldb/Host/common/NativeProcessProtocol.h"
 #include "lldb/lldb-private-forward.h"
 
 #include "GDBRemoteCommunicationServerCommon.h"
 
 class StringExtractorGDBRemote;
 
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

 //===-- GDBRemoteCommunicationServerLLGS.cpp ------------------------------===//
 //
 // 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 <errno.h>
 
 #include "lldb/Host/Config.h"
 
 
 #include <chrono>
 #include <cstring>
 #include <thread>
 
 #include "GDBRemoteCommunicationServerLLGS.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
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);
+      process_name, llvm::sys::path::Style::native);
   match_info.SetNameMatchType(NameMatch::EndsWith);
 
+  if (!include_existing) {
     // 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());
+  } else {
+    LLDB_LOG(log, "including existing processes in search");
+  }
 
   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;
-      };
+  StreamString name_with_slash_stream;
+  name_with_slash_stream.Format("/{0}", process_name);
+  auto name_with_slash = name_with_slash_stream.GetString();
+
+  // Exclude a process if it's in the exclusion list, OR if the process info's
+  // name doesn't exactly match the desired name, nor ends with a slash followed
+  // by the desired name.
+  auto should_exclude_process = [&exclusion_list, &process_name,
+                                 &name_with_slash](
+                                    const ProcessInstanceInfo &info) {
+    auto info_name = info.GetNameAsStringRef();
+    return info_name != process_name && !info_name.endswith(name_with_slash);
 
   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);
+      llvm::erase_if(loop_process_list, should_exclude_process);
 
       // 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());