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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

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.

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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141605