[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbbae0c1f7b4f: [lldb] [llgs] Support owning and detaching 
extra processes (authored by mgorny).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
  lldb/test/API/tools/lldb-server/main.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,15 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocessImpl,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
+  // This is a hack to avoid MOCK_METHOD2 incompatibility with std::unique_ptr
+  // passed as value.
+  void NewSubprocess(NativeProcessProtocol *parent_process,
+ std::unique_ptr child_process) {
+NewSubprocessImpl(parent_process, child_process);
+  }
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/test/API/tools/lldb-server/main.cpp
===
--- lldb/test/API/tools/lldb-server/main.cpp
+++ lldb/test/API/tools/lldb-server/main.cpp
@@ -216,7 +216,13 @@
 
   sig_result = signal(SIGSEGV, signal_handler);
   if (sig_result == SIG_ERR) {
-fprintf(stderr, "failed to set SIGUSR1 handler: errno=%d\n", errno);
+fprintf(stderr, "failed to set SIGSEGV handler: errno=%d\n", errno);
+exit(1);
+  }
+
+  sig_result = signal(SIGCHLD, SIG_IGN);
+  if (sig_result == SIG_ERR) {
+fprintf(stderr, "failed to set SIGCHLD handler: errno=%d\n", errno);
 exit(1);
   }
 #endif
@@ -293,6 +299,14 @@
   else if (arg == "swap_chars")
 func_p = swap_chars;
   func_p();
+#if !defined(_WIN32)
+} else if (arg == "fork") {
+  if (fork() == 0)
+_exit(0);
+} else if (arg == "vfork") {
+  if (vfork() == 0)
+_exit(0);
+#endif
 } else if (consume_front(arg, "thread:new")) {
 threads.push_back(std::thread(thread_func, nullptr));
 } else if (consume_front(arg, "thread:print-ids")) {
Index: lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
@@ -0,0 +1,59 @@
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestGdbRemoteFork(gdbremote_testcase.GdbRemoteTestCaseBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def fork_and_detach_test(self, variant):
+self.build()
+self.prep_debug_monitor_and_inferior(inferior_args=[variant])
+self.add_qSupported_packets(["multiprocess+",
+ "{}-events+".format(variant)])
+ret = self.expect_gdbremote_sequence()
+self.assertIn("{}-events+".format(variant), ret["qSupported_response"])
+self.reset_test_sequence()
+
+# continue and expect fork
+fork_regex = "[$]T.*;{}:p([0-9a-f]*)[.]([0-9a-f]*).*".format(variant)
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": fork_regex,
+ "capture": {1: "pid", 2: "tid"}},
+], True)
+ret = self.expect_gdbremote_sequence()
+pid = int(ret["pid"], 16)
+self.reset_test_sequence()
+
+# detach the forked child
+self.test_sequence.add_log_lines([
+"read packet: $D;{:x}#00".format(pid),
+{"direction": "send", "regex": r"[$]OK#.*"},
+], True)
+ret = self.expect_gdbremote_sequence()
+self.reset_test_sequence()
+
+@add_test_categories(["fork"])
+def test_fork(self):
+self.fork_and_detach_test("fork")
+
+# resume the parent
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": r"[$]W00#.*"},
+], True)
+self.expect_gdbremote_sequence()
+
+@add_test_categories(["fork"])
+def test_vfork(self):
+self.fork_and_detach_test("vfork")
+
+   

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 340186.
mgorny added a comment.

Fix tests to include `multiprocess+` in `qSupported`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
  lldb/test/API/tools/lldb-server/main.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,15 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocessImpl,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
+  // This is a hack to avoid MOCK_METHOD2 incompatibility with std::unique_ptr
+  // passed as value.
+  void NewSubprocess(NativeProcessProtocol *parent_process,
+ std::unique_ptr child_process) {
+NewSubprocessImpl(parent_process, child_process);
+  }
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/test/API/tools/lldb-server/main.cpp
===
--- lldb/test/API/tools/lldb-server/main.cpp
+++ lldb/test/API/tools/lldb-server/main.cpp
@@ -216,7 +216,13 @@
 
   sig_result = signal(SIGSEGV, signal_handler);
   if (sig_result == SIG_ERR) {
-fprintf(stderr, "failed to set SIGUSR1 handler: errno=%d\n", errno);
+fprintf(stderr, "failed to set SIGSEGV handler: errno=%d\n", errno);
+exit(1);
+  }
+
+  sig_result = signal(SIGCHLD, SIG_IGN);
+  if (sig_result == SIG_ERR) {
+fprintf(stderr, "failed to set SIGCHLD handler: errno=%d\n", errno);
 exit(1);
   }
 #endif
@@ -293,6 +299,14 @@
   else if (arg == "swap_chars")
 func_p = swap_chars;
   func_p();
+#if !defined(_WIN32)
+} else if (arg == "fork") {
+  if (fork() == 0)
+_exit(0);
+} else if (arg == "vfork") {
+  if (vfork() == 0)
+_exit(0);
+#endif
 } else if (consume_front(arg, "thread:new")) {
 threads.push_back(std::thread(thread_func, nullptr));
 } else if (consume_front(arg, "thread:print-ids")) {
Index: lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
@@ -0,0 +1,59 @@
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestGdbRemoteFork(gdbremote_testcase.GdbRemoteTestCaseBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def fork_and_detach_test(self, variant):
+self.build()
+self.prep_debug_monitor_and_inferior(inferior_args=[variant])
+self.add_qSupported_packets(["multiprocess+",
+ "{}-events+".format(variant)])
+ret = self.expect_gdbremote_sequence()
+self.assertIn("{}-events+".format(variant), ret["qSupported_response"])
+self.reset_test_sequence()
+
+# continue and expect fork
+fork_regex = "[$]T.*;{}:p([0-9a-f]*)[.]([0-9a-f]*).*".format(variant)
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": fork_regex,
+ "capture": {1: "pid", 2: "tid"}},
+], True)
+ret = self.expect_gdbremote_sequence()
+pid = int(ret["pid"], 16)
+self.reset_test_sequence()
+
+# detach the forked child
+self.test_sequence.add_log_lines([
+"read packet: $D;{:x}#00".format(pid),
+{"direction": "send", "regex": r"[$]OK#.*"},
+], True)
+ret = self.expect_gdbremote_sequence()
+self.reset_test_sequence()
+
+@add_test_categories(["fork"])
+def test_fork(self):
+self.fork_and_detach_test("fork")
+
+# resume the parent
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": r"[$]W00#.*"},
+], True)
+self.expect_gdbremote_sequence()
+
+@add_test_categories(["fork"])
+def test_vfork(self):
+self.fork_and_detach_test("vfork")
+
+# resume the parent
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": r"[$]T.*vforkdone.*"},
+"read

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 340184.
mgorny marked an inline comment as done.
mgorny added a comment.

Add the comment for mock stuff.

Move tests from the later patch here and skip them based on category.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
  lldb/test/API/tools/lldb-server/main.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,15 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocessImpl,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
+  // This is a hack to avoid MOCK_METHOD2 incompatibility with std::unique_ptr
+  // passed as value.
+  void NewSubprocess(NativeProcessProtocol *parent_process,
+ std::unique_ptr child_process) {
+NewSubprocessImpl(parent_process, child_process);
+  }
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/test/API/tools/lldb-server/main.cpp
===
--- lldb/test/API/tools/lldb-server/main.cpp
+++ lldb/test/API/tools/lldb-server/main.cpp
@@ -216,7 +216,13 @@
 
   sig_result = signal(SIGSEGV, signal_handler);
   if (sig_result == SIG_ERR) {
-fprintf(stderr, "failed to set SIGUSR1 handler: errno=%d\n", errno);
+fprintf(stderr, "failed to set SIGSEGV handler: errno=%d\n", errno);
+exit(1);
+  }
+
+  sig_result = signal(SIGCHLD, SIG_IGN);
+  if (sig_result == SIG_ERR) {
+fprintf(stderr, "failed to set SIGCHLD handler: errno=%d\n", errno);
 exit(1);
   }
 #endif
@@ -293,6 +299,14 @@
   else if (arg == "swap_chars")
 func_p = swap_chars;
   func_p();
+#if !defined(_WIN32)
+} else if (arg == "fork") {
+  if (fork() == 0)
+_exit(0);
+} else if (arg == "vfork") {
+  if (vfork() == 0)
+_exit(0);
+#endif
 } else if (consume_front(arg, "thread:new")) {
 threads.push_back(std::thread(thread_func, nullptr));
 } else if (consume_front(arg, "thread:print-ids")) {
Index: lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/TestGdbRemoteFork.py
@@ -0,0 +1,58 @@
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestGdbRemoteFork(gdbremote_testcase.GdbRemoteTestCaseBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def fork_and_detach_test(self, variant):
+self.build()
+self.prep_debug_monitor_and_inferior(inferior_args=[variant])
+self.add_qSupported_packets(["{}-events+".format(variant)])
+ret = self.expect_gdbremote_sequence()
+self.assertIn("{}-events+".format(variant), ret["qSupported_response"])
+self.reset_test_sequence()
+
+# continue and expect fork
+fork_regex = "[$]T.*;{}:p([0-9a-f]*)[.]([0-9a-f]*).*".format(variant)
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": fork_regex,
+ "capture": {1: "pid", 2: "tid"}},
+], True)
+ret = self.expect_gdbremote_sequence()
+pid = int(ret["pid"], 16)
+self.reset_test_sequence()
+
+# detach the forked child
+self.test_sequence.add_log_lines([
+"read packet: $D;{:x}#00".format(pid),
+{"direction": "send", "regex": r"[$]OK#.*"},
+], True)
+ret = self.expect_gdbremote_sequence()
+self.reset_test_sequence()
+
+@add_test_categories(["fork"])
+def test_fork(self):
+self.fork_and_detach_test("fork")
+
+# resume the parent
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": r"[$]W00#.*"},
+], True)
+self.expect_gdbremote_sequence()
+
+@add_test_categories(["fork"])
+def test_vfork(self):
+self.fork_and_detach_test("vfork")
+
+# resume the parent
+self.test_sequence.add_log_lines([
+"read packet: $c#00",
+{"direction": "send", "regex": r"[$]T

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h:31-34
+  void NewSubprocess(NativeProcessProtocol *parent_process,
+ std::unique_ptr child_process) {
+NewSubprocessImpl(parent_process, child_process);
+  }

Maybe add a quick comment to explain why this is here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 339217.
mgorny marked an inline comment as done.
mgorny added a comment.

Remove the test, update the API to pass `std::unique_ptr<>` instance instead of 
ref.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,13 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocessImpl,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
+  void NewSubprocess(NativeProcessProtocol *parent_process,
+ std::unique_ptr child_process) {
+NewSubprocessImpl(parent_process, child_process);
+  }
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===
--- lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -378,9 +378,7 @@
 return eServerPacketType_C;
 
   case 'D':
-if (packet_size == 1)
-  return eServerPacketType_D;
-break;
+return eServerPacketType_D;
 
   case 'g':
 return eServerPacketType_g;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -78,6 +78,10 @@
 
   void DidExec(NativeProcessProtocol *process) override;
 
+  void
+  NewSubprocess(NativeProcessProtocol *parent_process,
+std::unique_ptr child_process) override;
+
   Status InitializeConnection(std::unique_ptr connection);
 
 protected:
@@ -89,7 +93,8 @@
   NativeProcessProtocol *m_current_process;
   NativeProcessProtocol *m_continue_process;
   std::recursive_mutex m_debugged_process_mutex;
-  std::unique_ptr m_debugged_process_up;
+  std::unordered_map>
+  m_debugged_processes;
 
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -249,14 +249,14 @@
 
   {
 std::lock_guard guard(m_debugged_process_mutex);
-assert(!m_debugged_process_up && "lldb-server creating debugged "
- "process but one already exists");
+assert(m_debugged_processes.empty() && "lldb-server creating debugged "
+   "process but one already exists");
 auto process_or =
 m_process_factory.Launch(m_process_launch_info, *this, m_mainloop);
 if (!process_or)
   return Status(process_or.takeError());
-m_debugged_process_up = std::move(*process_or);
-m_continue_process = m_current_process = m_debugged_process_up.get();
+m_continue_process = m_current_process = process_or->get();
+m_debugged_processes[m_current_process->GetID()] = std::move(*process_or);
   }
 
   SetEnabledExtensions(*m_current_process);
@@ -273,10 +273,10 @@
 LLDB_LOG(log,
  "pid = {0}: setting up stdout/stderr redirection via $O "
  "gdb-remote commands",
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
 
 // Setup stdout/stderr mapping from inferior to $O
-auto terminal_fd = m_debugged_process_up->GetTerminalFileDescriptor();
+auto terminal_fd = m_current_process->GetTerminalFileDescriptor();
 if (terminal_fd >= 0) {
   LLDB_LOGF(log,
 "ProcessGDBRemoteCommunicationServerLLGS::%s setting "
@@ -295,12 +295,12 @@
 LLDB_LOG(log,
  "pid = {0} skipping stdout/stderr redirection via $O: inferior "
  "will communicate over client-provided file descriptors",
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
   }
 
   printf("Launched '%s' as process %" PRIu64 "...\n",
  m_process_launch_info.GetArgument

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D100191#2704748 , @mgorny wrote:

> In D100191#2704681 , @labath wrote:
>
>> In D100191#2704492 , @mgorny wrote:
>>
>>> In D100191#2704403 , @labath 
>>> wrote:
>>>
 Let's identify the set of patches needed to make this testable via the 
 lldb-server suite (this one, D100153 , 
 D100208  (or equivalent for some other 
 os), and what else?) and test that?
>>>
>>> In its current form, D100208  relies at 
>>> least on D100196  as well. I suppose we 
>>> might get away without other patches for now. My logic is that as long as 
>>> client doesn't indicate fork support, the regular LLDB behavior won't 
>>> change. We can mock-enable `fork-events` in the server tests to get things 
>>> rolling, unless I'm missing something.
>>
>> Sounds great.
>>
>>> I think we could avoid merging D100196  
>>> if I split D100208  into two parts, the 
>>> earlier part just calling `NewSubprocess()` without actually reporting 
>>> stop. This won't be really functional (or used in real sessions) but should 
>>> suffice for testing.
>>
>> I'm not sure how would that work -- you'd still have to provide a way to 
>> notify the client (test) about the new process and its pid, so that the test 
>> can assert something meaningful. Let's just keep D100196 
>> , but leave out the client specific parts 
>> -- just keep it about adding the new enums and relevant server code (trivial 
>> case switches are fine).
>
> Do you mean the `DidFoo()` methods, or the whole `StopInfo` stuff along with 
> the logic in `ProcessGDBRemote`?

All of it.

> If the latter, should I add some asserts for the unhandled stop reasons or 
> just ignore them for now?

Nah, just ignore it completely lldb-server will not be sending those as long as 
the client does not advertise support. It's possible lldb will misbehave if a 
broken/evil server sends reasons like these, but that's not a problem for this 
patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D100191#2704681 , @labath wrote:

> In D100191#2704492 , @mgorny wrote:
>
>> In D100191#2704403 , @labath wrote:
>>
>>> Let's identify the set of patches needed to make this testable via the 
>>> lldb-server suite (this one, D100153 , 
>>> D100208  (or equivalent for some other 
>>> os), and what else?) and test that?
>>
>> In its current form, D100208  relies at 
>> least on D100196  as well. I suppose we 
>> might get away without other patches for now. My logic is that as long as 
>> client doesn't indicate fork support, the regular LLDB behavior won't 
>> change. We can mock-enable `fork-events` in the server tests to get things 
>> rolling, unless I'm missing something.
>
> Sounds great.
>
>> I think we could avoid merging D100196  if 
>> I split D100208  into two parts, the 
>> earlier part just calling `NewSubprocess()` without actually reporting stop. 
>> This won't be really functional (or used in real sessions) but should 
>> suffice for testing.
>
> I'm not sure how would that work -- you'd still have to provide a way to 
> notify the client (test) about the new process and its pid, so that the test 
> can assert something meaningful. Let's just keep D100196 
> , but leave out the client specific parts 
> -- just keep it about adding the new enums and relevant server code (trivial 
> case switches are fine).

Do you mean the `DidFoo()` methods, or the whole `StopInfo` stuff along with 
the logic in `ProcessGDBRemote`? If the latter, should I add some asserts for 
the unhandled stop reasons or just ignore them for now?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D100191#2704492 , @mgorny wrote:

> In D100191#2704403 , @labath wrote:
>
>> Let's identify the set of patches needed to make this testable via the 
>> lldb-server suite (this one, D100153 , 
>> D100208  (or equivalent for some other 
>> os), and what else?) and test that?
>
> In its current form, D100208  relies at 
> least on D100196  as well. I suppose we 
> might get away without other patches for now. My logic is that as long as 
> client doesn't indicate fork support, the regular LLDB behavior won't change. 
> We can mock-enable `fork-events` in the server tests to get things rolling, 
> unless I'm missing something.

Sounds great.

> I think we could avoid merging D100196  if 
> I split D100208  into two parts, the 
> earlier part just calling `NewSubprocess()` without actually reporting stop. 
> This won't be really functional (or used in real sessions) but should suffice 
> for testing.

I'm not sure how would that work -- you'd still have to provide a way to notify 
the client (test) about the new process and its pid, so that the test can 
assert something meaningful. Let's just keep D100196 
, but leave out the client specific parts -- 
just keep it about adding the new enums and relevant server code (trivial case 
switches are fine).

> To be honest, I really like to keep these patches small, even if it means it 
> takes 2 or 3 patches to make a test. I would prefer just adding the test to 
> the last patch in series.

I don't mind the multiple patches. These are a bit on the smaller side, but 
they are definitely easier to review than a single giant patch). It does create 
confusion though, since that is not the usual mode of operation around here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D100191#2704403 , @labath wrote:

> Let's identify the set of patches needed to make this testable via the 
> lldb-server suite (this one, D100153 , 
> D100208  (or equivalent for some other os), 
> and what else?) and test that?

In its current form, D100208  relies at least 
on D100196  as well. I suppose we might get 
away without other patches for now. My logic is that as long as client doesn't 
indicate fork support, the regular LLDB behavior won't change. We can 
mock-enable `fork-events` in the server tests to get things rolling, unless I'm 
missing something.

I think we could avoid merging D100196  if I 
split D100208  into two parts, the earlier 
part just calling `NewSubprocess()` without actually reporting stop. This won't 
be really functional (or used in real sessions) but should suffice for testing.

To be honest, I really like to keep these patches small, even if it means it 
takes 2 or 3 patches to make a test. I would prefer just adding the test to the 
last patch in series.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D100191#2692882 , @mgorny wrote:

> In D100191#2689489 , @labath wrote:
>
>> We should also start thinking about tests. I suppose the smallest piece of 
>> functionality that could be usefully tested (with a lldb-server test) is 
>> debugging a process that forks, stopping after the fork, and detaching from 
>> the child. Shall we try making that work first?
>
> How about using a `MockProcess` to unittest server's behavior wrt getting 
> `NewSubprocess()` and `Detach()` calls?

I have mixed feelings about that. On one hand, these kinds of tests really 
enable you to test scenarios (often to do with various borderline and error 
conditions) which would not be testable in other ways. And they can run on any 
platform. However, you're not actually testing the error conditions, and I also 
have to put a bunch of other things on the other tip of the scale:

- this test is not a very good example of how tests should be written -- it 
doesn't test the public interface of the class, and it doesn't even set it up 
in a very realistic way. For example, the class does not have a connection 
object initialized. I'm guessing you cannot check the result of the `Handle_D` 
function (which is directly touched by this patch, and it is at least close to 
the public API), because it would always return an error due to a missing 
connection. All of this makes the test very fragile
- writing this test wouldn't "absolve" us of the need to write a test at a 
higher level, which would test the integration with a real process class. All 
of the checks you do here could be done at the lldb-server test level, and 
within the existing test framework (you couldn't check that `m_current_process` 
is null, but you could for example check that a `g` packet returns an error, 
which should be close enough).

So overall, I think that David was right that this patch is too small to be 
tested independently. Though I didn't say that explicitly, this is what I was 
also getting at with the "minimal usefully testable functionality" remark. So, 
while I hate to waste the effort you put into this test (and I'm very sorry 
that my slow response made you waste it), I don't think it would be wise to 
waste more of it in trying to make the test look good.

Let's identify the set of patches needed to make this testable via the 
lldb-server suite (this one, D100153 , 
D100208  (or equivalent for some other os), 
and what else?) and test that?




Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:226
+NewSubprocess(NativeProcessProtocol *parent_process,
+  std::unique_ptr &child_process) = 0;
   };

mgorny wrote:
> labath wrote:
> > That way, the delegate _must_ do something with the child process.
> Indeed, it must. Unfortunately, this breaks `MockDelegate`:
> 
> ```
> /home/mgorny/git/llvm-project/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h:
>  In member function ‘virtual testing::internal::Function::Result 
> lldb_private::MockDelegate::NewSubprocess(testing::internal::Function::Argument1,
>  testing::internal::Function std::unique_ptr)>::Argument2)’:
> /home/mgorny/git/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:405:73:
>  error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const 
> std::unique_ptr<_Tp, _Dp>&) [with _Tp = lldb_private::NativeProcessProtocol; 
> _Dp = std::default_delete]’
>   405 | return GMOCK_MOCKER_(2, constness, Method).Invoke(gmock_a1, 
> gmock_a2); \
>   |   
>   ^
> /home/mgorny/git/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:679:30:
>  note: in expansion of macro ‘GMOCK_METHOD2_’
>   679 | #define MOCK_METHOD2(m, ...) GMOCK_METHOD2_(, , , m, __VA_ARGS__)
>   |  ^~
> /home/mgorny/git/llvm-project/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h:28:3:
>  note: in expansion of macro ‘MOCK_METHOD2’
>28 |   MOCK_METHOD2(NewSubprocess,
>   |   ^~~~
> [...]
> ```
> 
> and then there are a few pages of errors.
Right. I see. In that case, I think we should fix the problem inside 
MockDelegate. The simplest way to do that is to add a forwarding method which 
repackages the arguments in a gmock-friendly way. Something like this ought to 
work:
```
class MockDelegate {
  MOCK_METHOD2(NewSubprocessImpl,
   void(NativeProcessProtocol *parent_process,
std::unique_ptr &child_proces /*or 
NativeProcessProtocol &, or whatever works best*/));
  void NewSubprocess(NativeProcessProtocol *parent_process,
std::unique_ptr child_proces) { 
NewSubprocessImpl(parent_process, child_pr

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 338232.
mgorny added a comment.

Added a unittest covering the whole server life cycle, including attaching a 
(mocked) process, forking, detaching a child, detaching the parent, forking 
from the child and detaching all processes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/unittests/Process/gdb-remote/CMakeLists.txt
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerLLGSTest.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,9 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocess,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerLLGSTest.cpp
===
--- /dev/null
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerLLGSTest.cpp
@@ -0,0 +1,139 @@
+//===-- GDBRemoteCommunicationServerLLGSTest.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 "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Host/common/NativeProcessProtocol.h"
+
+#include "GDBRemoteTestUtils.h"
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h"
+#include "TestingSupport/Host/NativeProcessTestUtils.h"
+
+namespace lldb_private {
+namespace process_gdb_remote {
+
+class MockFactory : public NativeProcessProtocol::Factory {
+public:
+  llvm::Expected>
+  Launch(ProcessLaunchInfo &launch_info,
+ NativeProcessProtocol::NativeDelegate &native_delegate,
+ MainLoop &mainloop) const override {
+return llvm::createStringError(llvm::inconvertibleErrorCode(), "Not supported");
+  }
+
+  virtual llvm::Expected>
+  Attach(lldb::pid_t pid,
+ NativeProcessProtocol::NativeDelegate &native_delegate,
+ MainLoop &mainloop) const override {
+NativeProcessProtocol *process = new MockProcess(native_delegate, ArchSpec("x86_64-pc-linux"), pid);
+return std::unique_ptr(process);
+  }
+};
+
+class TestServer : public GDBRemoteCommunicationServerLLGS {
+  MainLoop m_main_loop;
+  MockFactory m_process_factory;
+
+public:
+  TestServer() : GDBRemoteCommunicationServerLLGS(m_main_loop, m_process_factory) {}
+
+  using GDBRemoteCommunicationServerLLGS::m_debugged_processes;
+  using GDBRemoteCommunicationServerLLGS::m_current_process;
+  using GDBRemoteCommunicationServerLLGS::m_continue_process;
+
+  using GDBRemoteCommunicationServerLLGS::Handle_D;
+};
+
+TEST(GDBRemoteCommunicationServerLLGSTest, NewSubprocess) {
+  TestServer server;
+  Status error;
+
+  // Initial state: no process.
+  EXPECT_EQ(server.m_debugged_processes.size(), 0u);
+  EXPECT_EQ(server.m_current_process, nullptr);
+  EXPECT_EQ(server.m_continue_process, nullptr);
+
+  // Attach the main process.
+  error = server.AttachToProcess(1234);
+  ASSERT_TRUE(error.Success());
+  EXPECT_EQ(server.m_debugged_processes.size(), 1u);
+  auto it = server.m_debugged_processes.find(1234);
+  ASSERT_NE(it, server.m_debugged_processes.end());
+  NativeProcessProtocol *parent = it->second.get();
+  EXPECT_EQ(parent->GetID(), 1234u);
+  EXPECT_EQ(server.m_current_process, parent);
+  EXPECT_EQ(server.m_continue_process, parent);
+
+  // Simulate a fork.
+  std::unique_ptr child_up{new MockProcess(server, ArchSpec("x86_64-pc-linux"), 1235)};
+  server.NewSubprocess(parent, child_up);
+  EXPECT_EQ(child_up, nullptr);
+  EXPECT_EQ(server.m_debugged_processes.size(), 2u);
+  it = server.m_debugged_processes.find(1235);
+  ASSERT_NE(it, server.m_debugged_processes.end());
+  NativeProcessProtocol *child = it->second.get();
+  EXPECT_EQ(child->GetID(), 1235u);
+  EXPECT_EQ(server.m_current_process, parent);
+  EXPECT_EQ(server.m_continue_process, parent);
+
+  // Detach the child.
+  StringExtractorGDBRemote child_detach{"D;04D3"};
+  EXPECT_CALL(*static_cast *>(child), Detach()

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D100191#2689489 , @labath wrote:

> We should also start thinking about tests. I suppose the smallest piece of 
> functionality that could be usefully tested (with a lldb-server test) is 
> debugging a process that forks, stopping after the fork, and detaching from 
> the child. Shall we try making that work first?

How about using a `MockProcess` to unittest server's behavior wrt getting 
`NewSubprocess()` and `Detach()` calls?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:226
+NewSubprocess(NativeProcessProtocol *parent_process,
+  std::unique_ptr &child_process) = 0;
   };

labath wrote:
> That way, the delegate _must_ do something with the child process.
Indeed, it must. Unfortunately, this breaks `MockDelegate`:

```
/home/mgorny/git/llvm-project/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h:
 In member function ‘virtual testing::internal::Function::Result 
lldb_private::MockDelegate::NewSubprocess(testing::internal::Function::Argument1,
 testing::internal::Function)>::Argument2)’:
/home/mgorny/git/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:405:73:
 error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const 
std::unique_ptr<_Tp, _Dp>&) [with _Tp = lldb_private::NativeProcessProtocol; 
_Dp = std::default_delete]’
  405 | return GMOCK_MOCKER_(2, constness, Method).Invoke(gmock_a1, 
gmock_a2); \
  | 
^
/home/mgorny/git/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:679:30:
 note: in expansion of macro ‘GMOCK_METHOD2_’
  679 | #define MOCK_METHOD2(m, ...) GMOCK_METHOD2_(, , , m, __VA_ARGS__)
  |  ^~
/home/mgorny/git/llvm-project/lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h:28:3:
 note: in expansion of macro ‘MOCK_METHOD2’
   28 |   MOCK_METHOD2(NewSubprocess,
  |   ^~~~
[...]
```

and then there are a few pages of errors.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3199
 
   StopSTDIOForwarding();
 

Hmm, I suppose this should only happen if we're detaching all processes, or...?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 337535.
mgorny marked 3 inline comments as done.
mgorny added a comment.

All pending changes, except for the change from `std::unique_ptr<...> &` to 
`std::unique_ptr<...>`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,9 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocess,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===
--- lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -378,9 +378,7 @@
 return eServerPacketType_C;
 
   case 'D':
-if (packet_size == 1)
-  return eServerPacketType_D;
-break;
+return eServerPacketType_D;
 
   case 'g':
 return eServerPacketType_g;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -78,6 +78,10 @@
 
   void DidExec(NativeProcessProtocol *process) override;
 
+  void
+  NewSubprocess(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process) override;
+
   Status InitializeConnection(std::unique_ptr connection);
 
 protected:
@@ -89,7 +93,8 @@
   NativeProcessProtocol *m_current_process;
   NativeProcessProtocol *m_continue_process;
   std::recursive_mutex m_debugged_process_mutex;
-  std::unique_ptr m_debugged_process_up;
+  std::unordered_map>
+  m_debugged_processes;
 
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -243,14 +243,14 @@
 
   {
 std::lock_guard guard(m_debugged_process_mutex);
-assert(!m_debugged_process_up && "lldb-server creating debugged "
- "process but one already exists");
+assert(m_debugged_processes.empty() && "lldb-server creating debugged "
+   "process but one already exists");
 auto process_or =
 m_process_factory.Launch(m_process_launch_info, *this, m_mainloop);
 if (!process_or)
   return Status(process_or.takeError());
-m_debugged_process_up = std::move(*process_or);
-m_continue_process = m_current_process = m_debugged_process_up.get();
+m_continue_process = m_current_process = process_or->get();
+m_debugged_processes[m_current_process->GetID()] = std::move(*process_or);
   }
 
   // Handle mirroring of inferior stdout/stderr over the gdb-remote protocol as
@@ -265,10 +265,10 @@
 LLDB_LOG(log,
  "pid = {0}: setting up stdout/stderr redirection via $O "
  "gdb-remote commands",
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
 
 // Setup stdout/stderr mapping from inferior to $O
-auto terminal_fd = m_debugged_process_up->GetTerminalFileDescriptor();
+auto terminal_fd = m_current_process->GetTerminalFileDescriptor();
 if (terminal_fd >= 0) {
   LLDB_LOGF(log,
 "ProcessGDBRemoteCommunicationServerLLGS::%s setting "
@@ -287,12 +287,12 @@
 LLDB_LOG(log,
  "pid = {0} skipping stdout/stderr redirection via $O: inferior "
  "will communicate over client-provided file descriptors",
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
   }
 
   printf("Launched '%s' as process %" PRIu64 "...\n",
  m_process_launch_info.GetArguments().GetArgumentAtIndex(0),
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
 
   return Status()

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D100191#2689543 , @mgorny wrote:

> In D100191#2689489 , @labath wrote:
>
>> We should also start thinking about tests. I suppose the smallest piece of 
>> functionality that could be usefully tested (with a lldb-server test) is 
>> debugging a process that forks, stopping after the fork, and detaching from 
>> the child. Shall we try making that work first?
>
> I'm really too exhausted on this to write more tests. A few commits later I'm 
> adding a full set of basic tests for various fork scenarios. I think the 
> patches are simple enough to justify limiting ourselves to integration 
> testing for the whole thing, at least for the time being.

Generally LLVM patches must have accompanying tests for any code they're adding 
- if it's not possible to test the patch because it's too isolated/lacks 
related infrastructure then it's possible the patch is too small. But we try 
pretty hard to isolate patches and find ways to test work incrementally as it's 
added.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D100191#2689489 , @labath wrote:

> We should also start thinking about tests. I suppose the smallest piece of 
> functionality that could be usefully tested (with a lldb-server test) is 
> debugging a process that forks, stopping after the fork, and detaching from 
> the child. Shall we try making that work first?

I'm really too exhausted on this to write more tests. A few commits later I'm 
adding a full set of basic tests for various fork scenarios. I think the 
patches are simple enough to justify limiting ourselves to integration testing 
for the whole thing, at least for the time being.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

We should also start thinking about tests. I suppose the smallest piece of 
functionality that could be usefully tested (with a lldb-server test) is 
debugging a process that forks, stopping after the fork, and detaching from the 
child. Shall we try making that work first?




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3249
 
+  if (!detached)
+return SendErrorResponse(Status("PID %" PRIu64 " not traced", pid));

mgorny wrote:
> labath wrote:
> > Open question: Should we return an error for a plain `D` packet, if we 
> > don't have _any_ processes around?
> Practically, it probably doesn't matter. However, if client sends `D` without 
> actually having process attached, then something has probably gone wrong, so 
> might make sense to return some error.
True. I suppose we could just go with whatever falls out naturally from the 
code, and not add any special code on account of that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny marked 2 inline comments as done.
mgorny added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3202-3209
   if (!m_current_process ||
   (m_current_process->GetID() == LLDB_INVALID_PROCESS_ID)) {
 LLDB_LOGF(
 log,
 "GDBRemoteCommunicationServerLLGS::%s failed, no process available",
 __FUNCTION__);
 return SendErrorResponse(0x15);

labath wrote:
> I don't think this makes sense anymore
Good catch. I must've accidentally reintroduced it in rebase.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3249
 
+  if (!detached)
+return SendErrorResponse(Status("PID %" PRIu64 " not traced", pid));

labath wrote:
> Open question: Should we return an error for a plain `D` packet, if we don't 
> have _any_ processes around?
Practically, it probably doesn't matter. However, if client sends `D` without 
actually having process attached, then something has probably gone wrong, so 
might make sense to return some error.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:226
+NewSubprocess(NativeProcessProtocol *parent_process,
+  std::unique_ptr &child_process) = 0;
   };

That way, the delegate _must_ do something with the child process.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3202-3209
   if (!m_current_process ||
   (m_current_process->GetID() == LLDB_INVALID_PROCESS_ID)) {
 LLDB_LOGF(
 log,
 "GDBRemoteCommunicationServerLLGS::%s failed, no process available",
 __FUNCTION__);
 return SendErrorResponse(0x15);

I don't think this makes sense anymore



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3237
+  __FUNCTION__, it->first, error.AsCString());
+return SendErrorResponse(0x01);
+  }

It might be better to carry on detaching even if one process fails, and then 
return the errors in batch (?)

Something like:
```
Error err = Error::success();
for (p : processes) {
  if (Error e = p.second.Detach().ToError())
err = joinErrors(std::move(err), std::move(e));
}
if (err)
  SendErrorResonse(std::move(err));
```



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3249
 
+  if (!detached)
+return SendErrorResponse(Status("PID %" PRIu64 " not traced", pid));

Open question: Should we return an error for a plain `D` packet, if we don't 
have _any_ processes around?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 337236.
mgorny added a comment.

Now in a version that actually compiles.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,9 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocess,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===
--- lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -378,9 +378,7 @@
 return eServerPacketType_C;
 
   case 'D':
-if (packet_size == 1)
-  return eServerPacketType_D;
-break;
+return eServerPacketType_D;
 
   case 'g':
 return eServerPacketType_g;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -78,6 +78,10 @@
 
   void DidExec(NativeProcessProtocol *process) override;
 
+  void
+  NewSubprocess(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process) override;
+
   Status InitializeConnection(std::unique_ptr connection);
 
 protected:
@@ -89,7 +93,8 @@
   NativeProcessProtocol *m_current_process;
   NativeProcessProtocol *m_continue_process;
   std::recursive_mutex m_debugged_process_mutex;
-  std::unique_ptr m_debugged_process_up;
+  std::unordered_map>
+  m_debugged_processes;
 
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -243,14 +243,14 @@
 
   {
 std::lock_guard guard(m_debugged_process_mutex);
-assert(!m_debugged_process_up && "lldb-server creating debugged "
- "process but one already exists");
+assert(m_debugged_processes.empty() && "lldb-server creating debugged "
+   "process but one already exists");
 auto process_or =
 m_process_factory.Launch(m_process_launch_info, *this, m_mainloop);
 if (!process_or)
   return Status(process_or.takeError());
-m_debugged_process_up = std::move(*process_or);
-m_continue_process = m_current_process = m_debugged_process_up.get();
+m_continue_process = m_current_process = process_or->get();
+m_debugged_processes[m_current_process->GetID()] = std::move(*process_or);
   }
 
   // Handle mirroring of inferior stdout/stderr over the gdb-remote protocol as
@@ -265,10 +265,10 @@
 LLDB_LOG(log,
  "pid = {0}: setting up stdout/stderr redirection via $O "
  "gdb-remote commands",
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
 
 // Setup stdout/stderr mapping from inferior to $O
-auto terminal_fd = m_debugged_process_up->GetTerminalFileDescriptor();
+auto terminal_fd = m_current_process->GetTerminalFileDescriptor();
 if (terminal_fd >= 0) {
   LLDB_LOGF(log,
 "ProcessGDBRemoteCommunicationServerLLGS::%s setting "
@@ -287,12 +287,12 @@
 LLDB_LOG(log,
  "pid = {0} skipping stdout/stderr redirection via $O: inferior "
  "will communicate over client-provided file descriptors",
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
   }
 
   printf("Launched '%s' as process %" PRIu64 "...\n",
  m_process_launch_info.GetArguments().GetArgumentAtIndex(0),
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
 
   return Status();
 }
@@ -304,12 +304,11 @@
 
   // Before we try to attach, make sure we aren't already monitoring so

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 337228.
mgorny added a comment.

Unset `m_current_process` and `m_continue_process` if we're about to detach it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,9 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocess,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===
--- lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -378,9 +378,7 @@
 return eServerPacketType_C;
 
   case 'D':
-if (packet_size == 1)
-  return eServerPacketType_D;
-break;
+return eServerPacketType_D;
 
   case 'g':
 return eServerPacketType_g;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -78,6 +78,10 @@
 
   void DidExec(NativeProcessProtocol *process) override;
 
+  void
+  NewSubprocess(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process) override;
+
   Status InitializeConnection(std::unique_ptr connection);
 
 protected:
@@ -89,7 +93,8 @@
   NativeProcessProtocol *m_current_process;
   NativeProcessProtocol *m_continue_process;
   std::recursive_mutex m_debugged_process_mutex;
-  std::unique_ptr m_debugged_process_up;
+  std::unordered_map>
+  m_debugged_processes;
 
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -243,14 +243,14 @@
 
   {
 std::lock_guard guard(m_debugged_process_mutex);
-assert(!m_debugged_process_up && "lldb-server creating debugged "
- "process but one already exists");
+assert(m_debugged_processes.empty() && "lldb-server creating debugged "
+   "process but one already exists");
 auto process_or =
 m_process_factory.Launch(m_process_launch_info, *this, m_mainloop);
 if (!process_or)
   return Status(process_or.takeError());
-m_debugged_process_up = std::move(*process_or);
-m_continue_process = m_current_process = m_debugged_process_up.get();
+m_continue_process = m_current_process = process_or->get();
+m_debugged_processes[m_current_process->GetID()] = std::move(*process_or);
   }
 
   // Handle mirroring of inferior stdout/stderr over the gdb-remote protocol as
@@ -265,10 +265,10 @@
 LLDB_LOG(log,
  "pid = {0}: setting up stdout/stderr redirection via $O "
  "gdb-remote commands",
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
 
 // Setup stdout/stderr mapping from inferior to $O
-auto terminal_fd = m_debugged_process_up->GetTerminalFileDescriptor();
+auto terminal_fd = m_current_process->GetTerminalFileDescriptor();
 if (terminal_fd >= 0) {
   LLDB_LOGF(log,
 "ProcessGDBRemoteCommunicationServerLLGS::%s setting "
@@ -287,12 +287,12 @@
 LLDB_LOG(log,
  "pid = {0} skipping stdout/stderr redirection via $O: inferior "
  "will communicate over client-provided file descriptors",
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
   }
 
   printf("Launched '%s' as process %" PRIu64 "...\n",
  m_process_launch_info.GetArguments().GetArgumentAtIndex(0),
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
 
   return Status();
 }
@@ -304,12 +304,11 @@
 
   // Before we try to attach, ma

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 337164.
mgorny edited the summary of this revision.
mgorny added a comment.

Updated to keep a single list of all attached processes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,9 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocess,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===
--- lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -378,9 +378,7 @@
 return eServerPacketType_C;
 
   case 'D':
-if (packet_size == 1)
-  return eServerPacketType_D;
-break;
+return eServerPacketType_D;
 
   case 'g':
 return eServerPacketType_g;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -78,6 +78,10 @@
 
   void DidExec(NativeProcessProtocol *process) override;
 
+  void
+  NewSubprocess(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process) override;
+
   Status InitializeConnection(std::unique_ptr connection);
 
 protected:
@@ -89,7 +93,8 @@
   NativeProcessProtocol *m_current_process;
   NativeProcessProtocol *m_continue_process;
   std::recursive_mutex m_debugged_process_mutex;
-  std::unique_ptr m_debugged_process_up;
+  std::unordered_map>
+  m_debugged_processes;
 
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -243,14 +243,14 @@
 
   {
 std::lock_guard guard(m_debugged_process_mutex);
-assert(!m_debugged_process_up && "lldb-server creating debugged "
- "process but one already exists");
+assert(m_debugged_processes.empty() && "lldb-server creating debugged "
+   "process but one already exists");
 auto process_or =
 m_process_factory.Launch(m_process_launch_info, *this, m_mainloop);
 if (!process_or)
   return Status(process_or.takeError());
-m_debugged_process_up = std::move(*process_or);
-m_continue_process = m_current_process = m_debugged_process_up.get();
+m_continue_process = m_current_process = process_or->get();
+m_debugged_processes[m_current_process->GetID()] = std::move(*process_or);
   }
 
   // Handle mirroring of inferior stdout/stderr over the gdb-remote protocol as
@@ -265,10 +265,10 @@
 LLDB_LOG(log,
  "pid = {0}: setting up stdout/stderr redirection via $O "
  "gdb-remote commands",
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
 
 // Setup stdout/stderr mapping from inferior to $O
-auto terminal_fd = m_debugged_process_up->GetTerminalFileDescriptor();
+auto terminal_fd = m_current_process->GetTerminalFileDescriptor();
 if (terminal_fd >= 0) {
   LLDB_LOGF(log,
 "ProcessGDBRemoteCommunicationServerLLGS::%s setting "
@@ -287,12 +287,12 @@
 LLDB_LOG(log,
  "pid = {0} skipping stdout/stderr redirection via $O: inferior "
  "will communicate over client-provided file descriptors",
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
   }
 
   printf("Launched '%s' as process %" PRIu64 "...\n",
  m_process_launch_info.GetArguments().GetArgumentAtIndex(0),
- m_debugged_process_up->GetID());
+ m_current_process->GetID());
 
   return Status();
 }
@@ -304,12 +304,11 @@
 
   // Before

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

@labath, if we're going to remove `m_debugged_process_up`, then I suppose it 
makes sense to merge D100256  first. Could 
you review that one, please?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1036
+std::unique_ptr &child_process) {
+  // Apparently the process has been dealt with by another delegate.
+  if (!child_process)

labath wrote:
> mgorny wrote:
> > labath wrote:
> > > You no longer have to worry about that...
> > Don't I? The process plugin puts the new instance in an `std::unique_ptr`, 
> > and passes that to all delegates. Only one delegate can take the pointer 
> > over. While I don't think we really have a case for multiple delegates 
> > doing `NewSubprocess()`, I suppose we should check rather than crash. Or 
> > maybe just put an `assert` for it.
> I deleted the multi-delegate thingy in c9cf394f796e1 ;)
Ah, cool, I'll get to rebasing shortly.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1036
+std::unique_ptr &child_process) {
+  // Apparently the process has been dealt with by another delegate.
+  if (!child_process)

mgorny wrote:
> labath wrote:
> > You no longer have to worry about that...
> Don't I? The process plugin puts the new instance in an `std::unique_ptr`, 
> and passes that to all delegates. Only one delegate can take the pointer 
> over. While I don't think we really have a case for multiple delegates doing 
> `NewSubprocess()`, I suppose we should check rather than crash. Or maybe just 
> put an `assert` for it.
I deleted the multi-delegate thingy in c9cf394f796e1 ;)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D100191#2685039 , @labath wrote:

> I like this a lot more than the previous version. The thing I'd like to know 
> is, whether we can replace `m_additional_processes` with something like 
> `m_debugged_processes` (i.e., just have a single list of all processes we are 
> debugging. We could replace all occurrences of `m_debugged_process_up` with 
> `m_current_process`, which just points inside that list. Is there any reason 
> for the "privileged" position of the original process? The way I see it, most 
> of the operations would just work even if we treated them as equals. The only 
> thing which might not work is resuming (or TBE, reporting the stops) of the 
> subprocesses, but if we wanted to be safe, we could prevent that in some 
> other way (preventing the changing of process through Hc; returning 
> "crippled" process instances for subprocess, which return errors on resume 
> operations; ...).
>
> WDYT?

I've tried to limit changes to the minimum but sure, I can try doing that. I 
suppose it's going to make following children cleaner.




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1036
+std::unique_ptr &child_process) {
+  // Apparently the process has been dealt with by another delegate.
+  if (!child_process)

labath wrote:
> You no longer have to worry about that...
Don't I? The process plugin puts the new instance in an `std::unique_ptr`, and 
passes that to all delegates. Only one delegate can take the pointer over. 
While I don't think we really have a case for multiple delegates doing 
`NewSubprocess()`, I suppose we should check rather than crash. Or maybe just 
put an `assert` for it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like this a lot more than the previous version. The thing I'd like to know 
is, whether we can replace `m_additional_processes` with something like 
`m_debugged_processes` (i.e., just have a single list of all processes we are 
debugging. We could replace all occurrences of `m_debugged_process_up` with 
`m_current_process`, which just points inside that list. Is there any reason 
for the "privileged" position of the original process? The way I see it, most 
of the operations would just work even if we treated them as equals. The only 
thing which might not work is resuming (or TBE, reporting the stops) of the 
subprocesses, but if we wanted to be safe, we could prevent that in some other 
way (preventing the changing of process through Hc; returning "crippled" 
process instances for subprocess, which return errors on resume operations; 
...).

WDYT?




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1036
+std::unique_ptr &child_process) {
+  // Apparently the process has been dealt with by another delegate.
+  if (!child_process)

You no longer have to worry about that...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 336443.
mgorny added a comment.

Permit more-than-one-char 'D' packets.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100191/new/

https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,9 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocess,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===
--- lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -378,9 +378,7 @@
 return eServerPacketType_C;
 
   case 'D':
-if (packet_size == 1)
-  return eServerPacketType_D;
-break;
+return eServerPacketType_D;
 
   case 'g':
 return eServerPacketType_g;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -78,6 +78,10 @@
 
   void DidExec(NativeProcessProtocol *process) override;
 
+  void
+  NewSubprocess(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process) override;
+
   Status InitializeConnection(std::unique_ptr connection);
 
 protected:
@@ -88,6 +92,8 @@
   lldb::tid_t m_continue_tid = LLDB_INVALID_THREAD_ID;
   std::recursive_mutex m_debugged_process_mutex;
   std::unique_ptr m_debugged_process_up;
+  std::unordered_map>
+  m_additional_processes;
 
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -1030,6 +1030,18 @@
   ClearProcessSpecificData();
 }
 
+void GDBRemoteCommunicationServerLLGS::NewSubprocess(
+NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process) {
+  // Apparently the process has been dealt with by another delegate.
+  if (!child_process)
+return;
+  lldb::pid_t child_pid = child_process->GetID();
+  assert(child_pid != LLDB_INVALID_PROCESS_ID);
+  assert(m_additional_processes.find(child_pid) == m_additional_processes.end());
+  m_additional_processes[child_pid] = std::move(child_process);
+}
+
 void GDBRemoteCommunicationServerLLGS::DataAvailableCallback() {
   Log *log(GetLogIfAnyCategoriesSet(GDBR_LOG_COMM));
 
@@ -3213,19 +3225,43 @@
   return SendIllFormedResponse(packet, "D failed to parse the process id");
   }
 
-  if (pid != LLDB_INVALID_PROCESS_ID && m_debugged_process_up->GetID() != pid) {
-return SendIllFormedResponse(packet, "Invalid pid");
+  // Detach forked children if their PID was specified *or* no PID was requested
+  // (i.e. detach-all packet).
+  bool detached = false;
+  for (auto it = m_additional_processes.begin(); it != m_additional_processes.end();) {
+if (pid == LLDB_INVALID_PROCESS_ID || pid == it->first) {
+  const Status error = it->second->Detach();
+  if (error.Fail()) {
+LLDB_LOGF(log,
+  "GDBRemoteCommunicationServerLLGS::%s failed to detach from "
+  "pid %" PRIu64 ": %s\n",
+  __FUNCTION__, m_debugged_process_up->GetID(), error.AsCString());
+return SendErrorResponse(0x01);
+  }
+  it = m_additional_processes.erase(it);
+  detached = true;
+} else
+  ++it;
   }
 
-  const Status error = m_debugged_process_up->Detach();
-  if (error.Fail()) {
-LLDB_LOGF(log,
-  "GDBRemoteCommunicationServerLLGS::%s failed to detach from "
-  "pid %" PRIu64 ": %s\n",
-  __FUNCTION__, m_debugged_process_up->GetID(), error.AsCString());
-return SendErrorResponse(0x01);
+  // Detach the main process if PID matches or no PID was requested.
+  if (pid == LLDB_INVALID_PROCESS_ID || m_

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added reviewers: labath, emaste, krytarowski.
mgorny requested review of this revision.

Add a NativeDelegate API to pass new processes (forks) to LLGS,
and support detaching them via the 'D' packet.  A 'D' packet without
a specific PID detaches all processes, otherwise it detaches either
the specified subprocess or the main process, depending on the passed
PID.  However, the main process can only be detached if all subprocesses
were detached as well.


https://reviews.llvm.org/D100191

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -25,6 +25,9 @@
   MOCK_METHOD2(ProcessStateChanged,
void(NativeProcessProtocol *Process, StateType State));
   MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(NewSubprocess,
+   void(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process));
 };
 
 // NB: This class doesn't use the override keyword to avoid
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -78,6 +78,10 @@
 
   void DidExec(NativeProcessProtocol *process) override;
 
+  void
+  NewSubprocess(NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process) override;
+
   Status InitializeConnection(std::unique_ptr connection);
 
 protected:
@@ -88,6 +92,8 @@
   lldb::tid_t m_continue_tid = LLDB_INVALID_THREAD_ID;
   std::recursive_mutex m_debugged_process_mutex;
   std::unique_ptr m_debugged_process_up;
+  std::unordered_map>
+  m_additional_processes;
 
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -1030,6 +1030,18 @@
   ClearProcessSpecificData();
 }
 
+void GDBRemoteCommunicationServerLLGS::NewSubprocess(
+NativeProcessProtocol *parent_process,
+std::unique_ptr &child_process) {
+  // Apparently the process has been dealt with by another delegate.
+  if (!child_process)
+return;
+  lldb::pid_t child_pid = child_process->GetID();
+  assert(child_pid != LLDB_INVALID_PROCESS_ID);
+  assert(m_additional_processes.find(child_pid) == m_additional_processes.end());
+  m_additional_processes[child_pid] = std::move(child_process);
+}
+
 void GDBRemoteCommunicationServerLLGS::DataAvailableCallback() {
   Log *log(GetLogIfAnyCategoriesSet(GDBR_LOG_COMM));
 
@@ -3213,19 +3225,43 @@
   return SendIllFormedResponse(packet, "D failed to parse the process id");
   }
 
-  if (pid != LLDB_INVALID_PROCESS_ID && m_debugged_process_up->GetID() != pid) {
-return SendIllFormedResponse(packet, "Invalid pid");
+  // Detach forked children if their PID was specified *or* no PID was requested
+  // (i.e. detach-all packet).
+  bool detached = false;
+  for (auto it = m_additional_processes.begin(); it != m_additional_processes.end();) {
+if (pid == LLDB_INVALID_PROCESS_ID || pid == it->first) {
+  const Status error = it->second->Detach();
+  if (error.Fail()) {
+LLDB_LOGF(log,
+  "GDBRemoteCommunicationServerLLGS::%s failed to detach from "
+  "pid %" PRIu64 ": %s\n",
+  __FUNCTION__, m_debugged_process_up->GetID(), error.AsCString());
+return SendErrorResponse(0x01);
+  }
+  it = m_additional_processes.erase(it);
+  detached = true;
+} else
+  ++it;
   }
 
-  const Status error = m_debugged_process_up->Detach();
-  if (error.Fail()) {
-LLDB_LOGF(log,
-  "GDBRemoteCommunicationServerLLGS::%s failed to detach from "
-  "pid %" PRIu64 ": %s\n",
-  __FUNCTION__, m_debugged_process_up->GetID(), error.AsCString());
-return SendErrorResponse(0x01);
+  // Detach the main process if PID matches or no PID was requested.
+  if (pid == LLDB_INVALID_PROCESS_ID || m_debugged_process_up->GetID() == pid) {
+if (!m_additional_processes.empty())
+  return SendErrorResponse(Status("Unable to detach the main process while children are still traced"