[Lldb-commits] [PATCH] D100256: [lldb] [gdb-remote server] Abstract away getting current process

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D100256#2685807 , @mgorny wrote:

> In D100256#2685632 , @labath wrote:
>
>> Does this actually need to be a function? It seems like this could just be a 
>> pointer variable (or two), pointing into our pool of processes. I'd consider 
>> making a small struct to group the current process and the current thread 
>> within that process.
>
> I suppose a pointer might work for the proposed design. However, I'm 
> wondering why we have the `GetID()` check here — can we have an invalid value 
> of `m_debugged_process_up` then?

I think someone was just being overly cautious. I'm pretty sure the check is 
redundant...


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

https://reviews.llvm.org/D100256

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

[Lldb-commits] [PATCH] D100256: [lldb] [gdb-remote server] Abstract away getting current process

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

mgorny added a comment.

In D100256#2685632 , @labath wrote:

> Does this actually need to be a function? It seems like this could just be a 
> pointer variable (or two), pointing into our pool of processes. I'd consider 
> making a small struct to group the current process and the current thread 
> within that process.

I suppose a pointer might work for the proposed design. However, I'm wondering 
why we have the `GetID()` check here — can we have an invalid value of 
`m_debugged_process_up` then?


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

https://reviews.llvm.org/D100256

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

[Lldb-commits] [PATCH] D100256: [lldb] [gdb-remote server] Abstract away getting current process

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Does this actually need to be a function? It seems like this could just be a 
pointer variable (or two), pointing into our pool of processes. I'd consider 
making a small struct to group the current process and the current thread 
within that process.


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

https://reviews.llvm.org/D100256

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

[Lldb-commits] [PATCH] D100256: [lldb] [gdb-remote server] Abstract away getting current process

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

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

Introduce a new GetCurrentProcess() function to abstract away getting
the current NativeProcessProtocol instance.  At this moment, this merely
streamlines checking for valid m_debugged_process_up.  However, this
lays foundations for the future multiprocess support.


https://reviews.llvm.org/D100256

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

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
@@ -231,6 +231,12 @@
   virtual std::vector
   HandleFeatures(const llvm::ArrayRef client_features) override;
 
+  // Verify and get the process instance selected by the client.
+  // If run is true, the process selected by "Hc" packet will be returned,
+  // otherwise the one selected by "Hg" will be used.  Returns nullptr if there
+  // is no process debugged.
+  NativeProcessProtocol *GetCurrentProcess(bool run) const;
+
 private:
   llvm::Expected> BuildTargetXml();
 
@@ -256,13 +262,18 @@
 
   void StopSTDIOForwarding();
 
-  // Read thread-id from packet.  If the thread-id is correct, returns it.
-  // Otherwise, returns the error.
+  // Read thread-id from packet.  If run is true, a TID without PID will be
+  // associated with the process selected via "Hc" packet.  Otherwise, the PID
+  // selected by "Hg" will be used.
   //
   // If allow_all is true, then the pid/tid value of -1 ('all') will be allowed.
   // In any case, the function assumes that exactly one inferior is being
   // debugged and rejects pid values that do no match that inferior.
+  //
+  // If the thread-id is correct, it is returned.  Otherwise, the error is
+  // returned.
   llvm::Expected ReadTid(StringExtractorGDBRemote ,
+  bool run,
   bool allow_all = false);
 
   // Call SetEnabledExtensions() with appropriate flags on the process.
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
@@ -1198,21 +1198,18 @@
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_jLLDBTraceSupported(
 StringExtractorGDBRemote ) {
-
-  // Fail if we don't have a current process.
-  if (!m_debugged_process_up ||
-  (m_debugged_process_up->GetID() == LLDB_INVALID_PROCESS_ID))
+  auto *process = GetCurrentProcess(false);
+  if (!process)
 return SendErrorResponse(Status("Process not running."));
 
-  return SendJSONResponse(m_debugged_process_up->TraceSupported());
+  return SendJSONResponse(process->TraceSupported());
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_jLLDBTraceStop(
 StringExtractorGDBRemote ) {
-  // Fail if we don't have a current process.
-  if (!m_debugged_process_up ||
-  (m_debugged_process_up->GetID() == LLDB_INVALID_PROCESS_ID))
+  auto *process = GetCurrentProcess(false);
+  if (!process)
 return SendErrorResponse(Status("Process not running."));
 
   packet.ConsumeFront("jLLDBTraceStop:");
@@ -1221,7 +1218,7 @@
   if (!stop_request)
 return SendErrorResponse(stop_request.takeError());
 
-  if (Error err = m_debugged_process_up->TraceStop(*stop_request))
+  if (Error err = process->TraceStop(*stop_request))
 return SendErrorResponse(std::move(err));
 
   return SendOKResponse();
@@ -1230,10 +1227,8 @@
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_jLLDBTraceStart(
 StringExtractorGDBRemote ) {
-
-  // Fail if we don't have a current process.
-  if (!m_debugged_process_up ||
-  (m_debugged_process_up->GetID() == LLDB_INVALID_PROCESS_ID))
+  auto *process = GetCurrentProcess(false);
+  if (!process)
 return SendErrorResponse(Status("Process not running."));
 
   packet.ConsumeFront("jLLDBTraceStart:");
@@ -1242,8 +1237,7 @@
   if (!request)
 return SendErrorResponse(request.takeError());
 
-  if (Error err =
-  m_debugged_process_up->TraceStart(packet.Peek(), request->type))
+  if (Error err = process->TraceStart(packet.Peek(), request->type))
 return SendErrorResponse(std::move(err));
 
   return SendOKResponse();
@@ -1252,10 +1246,8 @@
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_jLLDBTraceGetState(
 StringExtractorGDBRemote ) {
-
-  // Fail if we don't have a current process.
-  if (!m_debugged_process_up ||
-