[lldb] Make `process connect` blocking in synchronous mode.

[JDevlieghere]

Summary

In synchronous mode, the process connect command and its aliases should wait for the stop event before claiming the command is complete. Currently, the stop event is always handled asynchronously by the debugger. The implementation takes the same approach as Process::ResumeSynchronous which hijacks the event and handles it on the current thread. Similarly, after this patch, the stop event is part of the command return object, which is the property used by the test case.

Most of the discussion for this patch took place in D83446

[jingham]

Couple of minor comments.

lldb/include/lldb/Target/Platform.h

855

Even though this is a private method it's still worth documenting the fact that you are switching off of "stream != nullptr" to determine sync vrs. async attach. That's not obvious.

lldb/source/Target/Platform.cpp

1834

If you fail here you leave the process hijacked. That doesn't matter because if "ConnectRemote" fails, you aren't going to have much to listen to anyway. But it still looks odd. I'm surprised we don't have some RAII-dingus for process hijacking, but anyway, it's good practice to undo this in the error branch.

[JDevlieghere]

Address Jim's feedback

[JDevlieghere]

lldb/source/Target/Platform.cpp

1834

Yeah, that's exactly my reasoning. You can't use a RAII object here, because the order of destruction is undefined, so you might end up calling RestoreProcessEvents after the shared pointer has been destructed. Anyway, I've added the call just for consistency.

[jingham]

LGTM

[labath]

The code looks fine. The test could be cleaned up a bit...

lldb/source/Target/Platform.cpp

1834

I'm not sure what you mean by the undefined destruction order. In c++ the destruction order is always the reverse of the construction order. The only "exception" to that that I am aware of is the destruction order for function call arguments. But even there the destruction order is still reverse construction order -- just the construction order itself is not (fully) defined.

lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py

20–44

It looks like this was copied from the "platform process list" test. I'd be very surprised if this is really needed. I guess the default responder should work just fine here...

47

s/s/ss :P

64

Besides the negative output check, it would be also good to test that the event actually gets delivered. I think something like lldbutil.expect_state_changes(self, self.dbg.GetListener(), self.process(), [lldb.eStateStopped]) ought to do the trick.

[JDevlieghere]

lldb/source/Target/Platform.cpp

1834

I was thinking of static destructors, but that is also reverse construction order, the construction order across TUs is just not defined.

lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py

47

My attempt to sneak in some Dutch spelling failed. (/jk)

lldb/include/lldb/Target/Platform.h

public:
   virtual lldb::ProcessSP
   DebugProcess(ProcessLaunchInfo &launch_info, Debugger &debugger,
                Target *target, // Can be nullptr, if nullptr create a new
                                // target, else use existing one
                Status &error);
 
   virtual lldb::ProcessSP ConnectProcess(llvm::StringRef connect_url,
                                          llvm::StringRef plugin_name,
-                                         lldb_private::Debugger &debugger,
-                                         lldb_private::Target *target,
-                                         lldb_private::Status &error);
+                                         Debugger &debugger, Target *target,
+                                         Status &error);
+
+  virtual lldb::ProcessSP
+  ConnectProcessSynchronous(llvm::StringRef connect_url,
+                            llvm::StringRef plugin_name, Debugger &debugger,
+                            Stream &stream, Target *target, Status &error);
 
   /// Attach to an existing process using a process ID.
   ///
   /// Each platform subclass needs to implement this function and attempt to
   /// attach to the process with the process ID of \a pid. The platform
   /// subclass should return an appropriate ProcessSP subclass that is
   /// attached to the process, or an empty shared pointer with an appropriate
   /// error.
public:
   ///     entry value. \b nullptr if not implemented or  if the process has no
   ///     crash information entry. \b error if an error occured.
   virtual llvm::Expected<StructuredData::DictionarySP>
   FetchExtendedCrashInformation(lldb_private::Process &process) {
     return nullptr;
   }
 
 protected:
+  /// Private implementation of connecting to a process. If the stream is set
+  /// we connect synchronously.
+  lldb::ProcessSP DoConnectProcess(llvm::StringRef connect_url,
+                                   llvm::StringRef plugin_name,
+                                   Debugger &debugger, Stream *stream,
+                                   Target *target, Status &error);
   bool m_is_host;
   // Set to true when we are able to actually set the OS version while being
   // connected. For remote platforms, we might set the version ahead of time
   // before we actually connect and this version might change when we actually
   // connect to a remote platform. For the host platform this will be set to
   // the once we call HostInfo::GetOSVersion().
   bool m_os_version_set_while_connected;
   bool m_system_arch_set_while_connected;

lldb/source/Commands/CommandObjectProcess.cpp

bool DoExecute(Args &command, CommandReturnObject &result) override {
 
     const char *plugin_name = nullptr;
     if (!m_options.plugin_name.empty())
       plugin_name = m_options.plugin_name.c_str();
 
     Status error;
     Debugger &debugger = GetDebugger();
     PlatformSP platform_sp = m_interpreter.GetPlatform(true);
-    ProcessSP process_sp = platform_sp->ConnectProcess(
+    ProcessSP process_sp =
+        debugger.GetAsyncExecution()
+            ? platform_sp->ConnectProcess(
                   command.GetArgumentAtIndex(0), plugin_name, debugger,
-        debugger.GetSelectedTarget().get(), error);
+                  debugger.GetSelectedTarget().get(), error)
+            : platform_sp->ConnectProcessSynchronous(
+                  command.GetArgumentAtIndex(0), plugin_name, debugger,
+                  result.GetOutputStream(), debugger.GetSelectedTarget().get(),
+                  error);
     if (error.Fail() || process_sp == nullptr) {
       result.AppendError(error.AsCString("Error connecting to the process"));
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
     return true;
   }
 

lldb/source/Target/Platform.cpp

 //===-- Platform.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 <algorithm>
 #include <csignal>
 #include <fstream>
 #include <memory>
 #include <vector>
 
-#include "llvm/Support/FileSystem.h"
-#include "llvm/Support/Path.h"
-
 #include "lldb/Breakpoint/BreakpointIDList.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Host/FileSystem.h"

 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StructuredData.h"
-
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
 
 // Define these constants from POSIX mman.h rather than include the file so
 // that they will be correct even when compiled on Linux.
 #define MAP_PRIVATE 2
 #define MAP_ANON 0x1000
 
 using namespace lldb;
 using namespace lldb_private;

 
 Status Platform::UnloadImage(lldb_private::Process *process,
                              uint32_t image_token) {
   return Status("UnloadImage is not supported on the current platform");
 }
 
 lldb::ProcessSP Platform::ConnectProcess(llvm::StringRef connect_url,
                                          llvm::StringRef plugin_name,
-                                         lldb_private::Debugger &debugger,
-                                         lldb_private::Target *target,
-                                         lldb_private::Status &error) {
+                                         Debugger &debugger, Target *target,
+                                         Status &error) {
+  return DoConnectProcess(connect_url, plugin_name, debugger, nullptr, target,
+                          error);
+}
+
+lldb::ProcessSP Platform::ConnectProcessSynchronous(
+    llvm::StringRef connect_url, llvm::StringRef plugin_name,
+    Debugger &debugger, Stream &stream, Target *target, Status &error) {
+  return DoConnectProcess(connect_url, plugin_name, debugger, &stream, target,
+                          error);
+}
+
+lldb::ProcessSP Platform::DoConnectProcess(llvm::StringRef connect_url,
+                                           llvm::StringRef plugin_name,
+                                           Debugger &debugger, Stream *stream,
+                                           Target *target, Status &error) {
   error.Clear();
 
   if (!target) {
     ArchSpec arch;
     if (target && target->GetArchitecture().IsValid())
       arch = target->GetArchitecture();
     else
       arch = Target::GetDefaultArchitecture();
lldb::ProcessSP Platform::DoConnectProcess(llvm::StringRef connect_url,
 
   if (!target || error.Fail())
     return nullptr;
 
   debugger.GetTargetList().SetSelectedTarget(target);
 
   lldb::ProcessSP process_sp =
       target->CreateProcess(debugger.GetListener(), plugin_name, nullptr);
+
   if (!process_sp)
     return nullptr;
 
+  // If this private method is called with a stream we are synchronous.
+  const bool synchronous = stream != nullptr;
+
+  ListenerSP listener_sp(
+      Listener::MakeListener("lldb.Process.ConnectProcess.hijack"));
+  if (synchronous)
+    process_sp->HijackProcessEvents(listener_sp);
+
   error = process_sp->ConnectRemote(connect_url);
-  if (error.Fail())
+  if (error.Fail()) {
+    if (synchronous)
+      process_sp->RestoreProcessEvents();
     return nullptr;
+  }
+
+  if (synchronous) {
+    EventSP event_sp;
+    process_sp->WaitForProcessToStop(llvm::None, &event_sp, true, listener_sp,
+                                     nullptr);
+    process_sp->RestoreProcessEvents();
+    bool pop_process_io_handler = false;
+    Process::HandleProcessStateChangedEvent(event_sp, stream,
+                                            pop_process_io_handler);
+  }
 
   return process_sp;
 }
 
 size_t Platform::ConnectToWaitingProcesses(lldb_private::Debugger &debugger,
                                            lldb_private::Status &error) {
   error.Clear();
   return 0;

lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py

+import lldb
+import binascii
+import os
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestProcessConnect(GDBRemoteTestBase):
+    def test_gdb_remote_sync(self):
+        """Test the gdb-remote command in synchronous mode"""
+        try:
+            self.dbg.SetAsync(False)
+            self.expect("gdb-remote %d" % self.server.port,
+                        substrs=['Process', 'stopped'])
+        finally:
+            self.dbg.GetSelectedPlatform().DisconnectRemote()
+
+    def test_gdb_remote_async(self):
+        """Test the gdb-remote command in asynchronous mode"""
+        try:
+            self.dbg.SetAsync(True)
+            self.expect("gdb-remote %d" % self.server.port,
+                        matching=False,
+                        substrs=['Process', 'stopped'])
+            lldbutil.expect_state_changes(self, self.dbg.GetListener(),
+                                          self.process(), [lldb.eStateStopped])
+        finally:
+            self.dbg.GetSelectedPlatform().DisconnectRemote()
+
+    def test_process_connect_sync(self):
+        """Test the gdb-remote command in synchronous mode"""
+        try:
+            self.dbg.SetAsync(False)
+            self.expect("process connect connect://localhost:%d" %
+                        self.server.port,
+                        substrs=['Process', 'stopped'])
+        finally:
+            self.dbg.GetSelectedPlatform().DisconnectRemote()
+
+    def test_process_connect_async(self):
+        """Test the gdb-remote command in asynchronous mode"""
+        try:
+            self.dbg.SetAsync(True)
+            self.expect("process connect connect://localhost:%d" %
+                        self.server.port,
+                        matching=False,
+                        substrs=['Process', 'stopped'])
+            lldbutil.expect_state_changes(self, self.dbg.GetListener(),
+                                          self.process(), [lldb.eStateStopped])
+        finally:
+            self.dbg.GetSelectedPlatform().DisconnectRemote()