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

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

mgorny added inline comments.


================
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;
+  };
----------------
labath wrote:
> The llvm way to do this is via an enum class in combination with LLVM_MARK_AS_BITMASK_ENUM.
Heh, and you've just told me to use `constexpr`s instead of `enum`s ;-).


================
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);
----------------
labath wrote:
> 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?
`tid 118029 belongs to a different tgid 4295085325, assuming child`

When tgid was 0, it also printed this huge number. If I cast it to `int`, it prints fine. But then, first arg is also `lldb::pid_t` and it prints fine.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:670
+      data = 0;
+
+    MonitorFork(data, true, PTRACE_EVENT_FORK);
----------------
labath wrote:
> I guess we should also do something like the `WaitForNewThread` call in the clone case.
Yeah, I was wondering about it. On the other hand, won't the main loop handle it anyway?

Does doing it explicitly have any real gain? One thing I was worried about is that the case of parent signal first is much more common than child signal first, so having a different logic on both branches would make it more likely for bugs in the child-first branch not to be noticed.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:187
       &GDBRemoteCommunicationServerLLGS::Handle_QPassSignals);
+  RegisterMemberFunctionHandler(
+      StringExtractorGDBRemote::eServerPacketType_qSupported,
----------------
labath wrote:
> 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. 
Yeah, that's better than my ideas ;-).


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

https://reviews.llvm.org/D98822