[Lldb-commits] [PATCH] D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP]

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This is going to be pretty big, so I'd say we start splitting this up. The client & server bits can go into separate patches, and it would make things simpler if the qSupported emission and parsing also got its own patches, as that is going to be a lot less controversial.

I'm going to have more questions, but let me start with this: What should the (v)fork-events extension actually mean? In the client, I guess it demonstrates its willingness to receive the appropriate type of event. But what should it mean on the server? Is it like "I promise to stop on every (v)fork", or just something like "hey, I know this word"? The second meaning only makes sense if these (v)fork-events come with some new protocol extensions/packets that the client can use when communicating with the server. Such is the case with the multiprocess extension (new thread id syntax), but I am not aware of anything similar in for these features. Are there any? The gdb documentation (//This feature indicates whether GDB supports fork event extensions to the remote protocol. GDB does not use such extensions unless the stub also reports that it supports them by including ‘fork-events+’ in its ‘qSupported’ reply. //) seems to indicate they might exist, but I am not sure what would they be...

If there aren't any, then it would probably be better to use the first "definition" of the feature (or something similar), as the client can legitimately be interested in whether it will be able to intercept forks or not. And if that's the case, then we should first consult the relevant NativeProcess class before we reply (v)fork-events+.



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:455
 
+    if (::strstr(response_cstr, "fork-events+"))
+      m_supports_fork_events = eLazyBoolYes;
----------------
mgorny wrote:
> Kamil noticed that this will also catch `vfork-events+`. I suppose I'll rewrite this function to iterate over split data as well.
Yeah, that should have been done a long time ago.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:1314-1322
+    "qXfer:features:read+",
+#if defined(__linux__) || defined(__NetBSD__) || defined(__FreeBSD__)
+    "QPassSignals+",
+    "qXfer:auxv:read+",
+    "qXfer:libraries-svr4:read+",
+#endif
+    "multiprocess+",
----------------
Ideally, all of these would be handled in the GDBRemoteCommunicationServerLLGS class, since these features are meaningless for the platform server. Then you also wouldn't need to define the m_(v)fork_events_supported members in this class.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h:152
+  // to be returned in response.
+  virtual llvm::SmallVector<std::string, 16>
+  HandleFeatures(const llvm::ArrayRef<llvm::StringRef> client_features);
----------------
A SmallVector as a return type is fairly odd. One tends to use SmallVectorImpl by-ref argument in this case, but a plain std::vector result is also fine.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2342-2344
+        StreamString ostr;
+        ostr.Printf("%" PRIu64 " %" PRIu64, pid_tid->first, pid_tid->second);
+        description = std::string(ostr.GetString());
----------------
description = llvm::formatv("{0} {1}", pid_tid->first, pid_tid->second).str()


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

https://reviews.llvm.org/D99864