[Lldb-commits] [PATCH] D98822: [lldb] follow-fork/vfork support [WIP]

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Seems mostly ok. The getPidForTid business does not seem completely ideal, though. I think it may be cleaner to keep the threads for which we have received the creation event in some kind of a purgatory list until we receive the corresponding parent event (which will clarify what kind of a child we are dealing with).

We actually did something like that a very long time ago, but I removed it for being too complicated. Now, I think it may be worth going back to it.

BTW, this made me realize of a funky scenario we may need to handle somehow. I think it's possible (on linux) that two threads will initiate a fork operation concurrently, and so by the time that everything stops, we end up with three (or more) processes...



================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:401-404
+  struct Extension {
+    static constexpr uint32_t fork = 1;
+    static constexpr uint32_t vfork = 2;
+  };
----------------
The llvm way to do this is via an enum class in combination with LLVM_MARK_AS_BITMASK_ENUM.


================
Comment at: lldb/source/Host/linux/Host.cpp:318
+namespace lldb_private {
+llvm::Optional<::pid_t> getPIDForTID(::pid_t tid) {
+  ::pid_t tracerpid, tgid = LLDB_INVALID_PROCESS_ID;
----------------
just say `lldb_private::getPIDForTID`


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:456
+      LLDB_LOG(log, "tid {0} belongs to a different tgid {1}, assuming child",
+               pid, tgid.getValue());
+      MonitorFork(pid, false, 0);
----------------
mgorny wrote:
> For some reason, this doesn't print the correct tgid value (plain printf works). Any clue what's wrong here?
what does it print?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:670
+      data = 0;
+
+    MonitorFork(data, true, PTRACE_EVENT_FORK);
----------------
I guess we should also do something like the `WaitForNewThread` call in the clone case.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:187
       &GDBRemoteCommunicationServerLLGS::Handle_QPassSignals);
+  RegisterMemberFunctionHandler(
+      StringExtractorGDBRemote::eServerPacketType_qSupported,
----------------
mgorny wrote:
> @labath, I'm not convinced this is the best way of doing it. Do you have any suggestions?
I'd probably do this by creating a new virtual function (with a signature like vector<string>(ArrayRef<StringRef>)`), which would receive the client features and return the server features. The base qSupported handler would be implemented in terms of this function.

This would also allow us to avoid sending features that clearly have no place in the platform connections. 


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

https://reviews.llvm.org/D98822