[Lldb-commits] [PATCH] D141605: [lldb] Detach the child process when stepping over a fork

Mon Jan 23 05:18:01 PST 2023

labath added a comment.

Thanks for your response, Jim.

In D141605#4066649 <https://reviews.llvm.org/D141605#4066649>, @jingham wrote:

> The part of handling the fork where we decide we're going to follow the child and so we need to switch the process PID & TID does have to happen on event receipt.  The point there is that until the client pulls an event from the public event queue, it doesn't know that the state change has happened yet.  We don't want the current state that you access with GetState, GetPID, GetSelectedThread, etc. to get out of sync with the event flow, so to the public world it should appear to it as if the process were in the state of the last event it actually received.  Having the PID or TID change before the fork event gets pulled off the queue would break this illusion.
>
> However, the state of the process we aren't following isn't relevant to the state of the process we are  following, so the part of the DidFork dealing with the discarded side of the fork can happen when the fork event is received, in ProcessGDBRemote::SetThreadStopInfo or thereabouts.  That does seem like a better way to do this.

The first part makes sense to me, but I'm not sure how to reconcile it with the second paragraph. It seems to me that, if follow-fork-mode=child, it'd be pretty hard to detach from the original (parent) process early on, but still have the public API report on its state until the fork event gets actually pulled from the event queue. Technically, it's doable, but I think we'd have to maintain separate "private" and "public" copies of most of the process state (thread lists, etc.). And I'm not sure how useful would that be given that when the process is running, it's hard to guarantee the correctness of a piece of information anyway.

> However, I don't think this patch is wrong in itself.  Formally, the step thread plans might want to know that a fork has happened.  But it's not terribly important as they can't really do anything about except maybe report an error.  And the same could be said of exec events which are already in this list.  What's actually going to happen is once we've stopped for the fork, all the step plans will be asked whether they are Stale, and given the thread they were on is no longer present, they should unship themselves.  So we're not losing anything by bypassing the plans for this event delivery.

Ok, so shall we go with this patch then?

> Note, it might be worth checking that the stepping thread plan's ShouldStop check the  PID, not just the TID.  It's possible that the new TID was the same as the old TID and the plan might try to keep operating, which would be wrong.

AFAICT, the plans do not check the process PID, but ShouldStop seems like it's a bit too late for a plan to figure out it's working with a completely different thread that it was supposed to have. Wouldn't it be better to just remove/flush all the thread plans upon forking (if following the child)?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141605