[v2,0/8] Fixes for multiprocess for FreeBSD's native target

Message ID 20230717192039.13976-1-jhb@FreeBSD.org
Headers
Series Fixes for multiprocess for FreeBSD's native target |

Message

John Baldwin July 17, 2023, 7:20 p.m. UTC
  While this series is not perfect, it does result in a net improvement
in testsuite results on FreeBSD/amd64 and is also a significantly
better experience debugging forking processes.  I'm less confident about
the last two patches but would like to get the first 6 patches into
GDB 14.

Changes since V1:

- A few early trivial patches were merged.

- The list of pending events are now stored as private class members
  of class fbsd_nat_target in Patch 1 along with various style fixes
  noted by Simon.  I also added an assertion that a pending event
  always has an associated inferior when it is added to the list.

- Patch 2 adds a bug trailer.

- Patch 3 is a bit simpler since the methods to work with the pending
  list are now class methods so the "this" pointer doesn't have to be
  passed around as often.  The ptid argument to resume is now renamed
  to scope_ptid.  I also added handling for TARGET_WAITKIND_IGNORE in
  the stop_process helper to avoid adding a bogus pending event if the
  process has disappeared (so wait() fails with ECHILD).  This case
  was caught by the new assertion in Patch 1.

- In patch 4, a workaround in pending_ptrace_events for a kernel bug
  is now conditional since I've merged an upstream fix to FreeBSD's
  kernel since posting the first version at
  https://cgit.freebsd.org/src/commit/?id=653738e895ba022be1179a95a85089e7bc66dbbe

- Patches 5 and 6 are unchanged.

- Patches 7 is new and replaces the ptid_t to identify single LWP vs
  entire process being resumed with an unordered_set<> of resumed LWPs
  permitting multiple individual LWPs in a process to be resumed while
  other LWPs are still suspended.

  Since FreeBSD can only PT_CONTINUE entire processes at a time, this
  means that the resume target method can no longer use ptrace
  directly, but instead stores the requested actions as a set of
  pending actions in the per-inferior data structure.  Later when
  either commit_resumed or wait is called, the target runs through all
  its non-exited inferiors comitting the cumulative results of earlier
  resume calls.

  Because that change is a bit hairy I've kept separate from the
  previous round of multiprocess fixes in patch 3 to make it easier to
  bisect in the future if there are regressions.

- Patch 8 is new and adds an implementation of the stop target method.
  It wasn't clear to me if this is really needed for an all-stop-only
  target, but at least one test in the testsuite was calling this
  method when I made it gdb_assert() instead of being the default.

John Baldwin (8):
  fbsd-nat: Add a list of pending events.
  fbsd-nat: Defer any ineligible events reported by wait.
  fbsd-nat: Fix resuming and waiting with multiple processes.
  fbsd-nat: Fix several issues with detaching.
  fbsd-nat: Fix thread_alive against a running thread.
  fbsd-nat: Stop a process if it is running before killing it.
  fbsd-nat: Defer resume of inferiors.
  fbsd-nat: Implement the target stop method.

 gdb/fbsd-nat.c | 971 ++++++++++++++++++++++++++++++++++++++++++-------
 gdb/fbsd-nat.h |  68 ++++
 2 files changed, 915 insertions(+), 124 deletions(-)
  

Comments

Tom Tromey Aug. 4, 2023, 4:45 p.m. UTC | #1
>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> While this series is not perfect, it does result in a net improvement
John> in testsuite results on FreeBSD/amd64 and is also a significantly
John> better experience debugging forking processes.  I'm less confident about
John> the last two patches but would like to get the first 6 patches into
John> GDB 14.

I skimmed through this, but I don't really know anything about FreeBSD.
Anyway I didn't see any real red flags, and I sent the one comment I
had.

John> - Patch 8 is new and adds an implementation of the stop target method.
John>   It wasn't clear to me if this is really needed for an all-stop-only
John>   target, but at least one test in the testsuite was calling this
John>   method when I made it gdb_assert() instead of being the default.

That is interesting.  I had thought it was only used for non-stop.
windows-nat doesn't implement it... but then again, I've never managed
to run the test suite on Windows.

Tom
  
John Baldwin Aug. 14, 2023, 5:51 p.m. UTC | #2
On 8/4/23 9:45 AM, Tom Tromey wrote:
>>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:
> 
> John> While this series is not perfect, it does result in a net improvement
> John> in testsuite results on FreeBSD/amd64 and is also a significantly
> John> better experience debugging forking processes.  I'm less confident about
> John> the last two patches but would like to get the first 6 patches into
> John> GDB 14.
> 
> I skimmed through this, but I don't really know anything about FreeBSD.
> Anyway I didn't see any real red flags, and I sent the one comment I
> had.

Ok, thanks.  I'm going to push the first six patches (which I'm more confident
of) with your suggestion applied (using inferior::priv_field).