[4/9] fbsd-nat: Add a list of pending events.

Message ID 20230228181845.99936-5-jhb@FreeBSD.org
State New
Headers
Series Fixes for multiprocess for FreeBSD's native target |

Commit Message

John Baldwin Feb. 28, 2023, 6:18 p.m. UTC
  The pending_events list stores a queue of deferred events that might
be reported by the next call to the target's wait method.  The set of
events that are eligible is filtered by the ptid passed to resume.

For now this just replaces the list of vfork_done events.  A
subsequent commit will reuse this to store other events.
---
 gdb/fbsd-nat.c | 146 ++++++++++++++++++++++++++++++++-----------------
 gdb/fbsd-nat.h |   2 +
 2 files changed, 98 insertions(+), 50 deletions(-)
  

Comments

Simon Marchi March 20, 2023, 6:35 p.m. UTC | #1
On 2/28/23 13:18, John Baldwin wrote:
> The pending_events list stores a queue of deferred events that might
> be reported by the next call to the target's wait method.  The set of
> events that are eligible is filtered by the ptid passed to resume.
> 
> For now this just replaces the list of vfork_done events.  A
> subsequent commit will reuse this to store other events.
> ---
>  gdb/fbsd-nat.c | 146 ++++++++++++++++++++++++++++++++-----------------
>  gdb/fbsd-nat.h |   2 +
>  2 files changed, 98 insertions(+), 50 deletions(-)
> 
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index 04d67fc5278..ca278b871ef 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -54,6 +54,66 @@
>  #define	PT_SETREGSET	43	/* Set a target register set */
>  #endif
>  
> +/* Filter for ptid's allowed to report events from wait.  Normally set
> +   in resume, but also reset to minus_one_ptid in create_inferior and
> +   attach.  */
> +
> +static ptid_t resume_ptid;

It doesn't change anything conceptually, but I think this should be a
private field of fbsd_nat_target.

This resume_ptid approach might work now, but it won't work in general,
since it only records the scope ptid passed to the last call to resume.

Even today, do you support multi-inferior?  If the user does:

(gdb) inferior 1
(gdb) continue &
(gdb) inferior 2
(gdb) continue &

Then resume_ptid would be (inf2_pid, 0, 0), and you'd be ignoring any
event from inferior 1, right?

I think the general solution is necessarily to remember the
resume state for each individual thread, somehow.  Up to you to see if
you want to implement that now or defer to later.  If you only support
all-stop with a single inferior, it might be enough for now.

For reference, the linux-nat target maintains its own data structures of
lwps (lwp_lwpid_htab), in which the last_resume_kind field is used for
that.  It would be unfortunate to have to reimplement all of this for
fbsd-nat as I'm sure that a lot of the logic would be very similar.  But
then making the code shareable would also be a lot of work.

> +
> +/* If an event is triggered asynchronously (fake vfork_done events) or
> +   occurs when the core is not expecting it, a pending event is
> +   created.  This event is then returned by a future call to the
> +   target wait method.  */

It can probably also happen if two events happen at the "same" time?
Like if you react to a breakpoint hit by one thread, and while stopping
the other threads, another thread reports a breakpoint stop, you have to
store that one as a pending event?

> +
> +struct pending_event
> +{
> +  pending_event (const ptid_t &_ptid, const target_waitstatus &_status) :
> +    ptid(_ptid), status(_status) {}

Spaces after parentheses.

> +
> +  ptid_t ptid;
> +  target_waitstatus status;
> +};
> +
> +static std::list<pending_event> pending_events;

This could use intrusive_list, which is a bit more lightweight than
std::list (it won't really make a difference in practice, but we have
it, so we might as well use it).

> +
> +/* Add a new pending event to the list.  */
> +
> +static void
> +add_pending_event (const ptid_t &ptid, const target_waitstatus &status)
> +{
> +  pending_events.emplace_back (ptid, status);
> +}
> +
> +/* Return true if there is a pending event matching FILTER.  */
> +
> +static bool
> +have_pending_event (ptid_t filter)
> +{
> +  for (const pending_event &event : pending_events)
> +    if (event.ptid.matches (filter))
> +      return true;
> +  return false;
> +}
> +
> +/* Helper method called by the target wait method.  Returns true if
> +   there is a pending event matching resume_ptid.  If there is a

resume_ptid -> RESUME_PTID

> +   matching event, PTID and *STATUS contain the event details, and the
> +   event is removed from the pending list.  */
> +
> +static bool
> +take_pending_event (ptid_t &ptid, target_waitstatus *status)
> +{
> +  for (auto it = pending_events.begin (); it != pending_events.end (); it++)
> +    if (it->ptid.matches (resume_ptid))
> +      {
> +	ptid = it->ptid;
> +	*status = it->status;
> +	pending_events.erase (it);
> +	return true;
> +      }
> +  return false;
> +}

This could maybe return a gdb::optional<pending_event>?

> +
>  /* Return the name of a file that can be opened to get the symbols for
>     the child process identified by PID.  */
>  
> @@ -1061,47 +1121,20 @@ fbsd_is_child_pending (pid_t pid)
>  }
>  
>  #ifndef PTRACE_VFORK
> -static std::forward_list<ptid_t> fbsd_pending_vfork_done;
> -
>  /* Record a pending vfork done event.  */
>  
>  static void
>  fbsd_add_vfork_done (ptid_t pid)
>  {
> -  fbsd_pending_vfork_done.push_front (pid);
> +  target_waitstatus status;
> +  status.set_vfork_done ();
> +  add_pending_event (ptid, status);

I think you can do this?

  add_pending_event (ptid, target_waitstatus ().set_vfork_done ());

Not a C++ expert, but I believe the temporary target_waitstatus is kept
alive long enough for this to work.

> @@ -1110,21 +1143,18 @@ fbsd_next_vfork_done (void)
>  void
>  fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)

It would be a good time to rename the first parameter from ptid to
scope_ptid, to match what was done here:

https://gitlab.com/gnutools/binutils-gdb/-/commit/d51926f06a7f4854bebdd71dcb0a78dbaa2f4168

I think it helps understand the meaning of the parameter.

>  {
> -#if defined(TDP_RFPPWAIT) && !defined(PTRACE_VFORK)
> -  pid_t pid;
> -
> -  /* Don't PT_CONTINUE a process which has a pending vfork done event.  */
> -  if (minus_one_ptid == ptid)
> -    pid = inferior_ptid.pid ();
> -  else
> -    pid = ptid.pid ();
> -  if (fbsd_is_vfork_done_pending (pid))
> -    return;
> -#endif

Yeah, this old code seems to misinterpret the target_ops::resume
interface, which was clarified in the commit mentioned above.

> +void
> +fbsd_nat_target::attach (const char *args, int from_tty)
> +{
> +  /* Expect a wait for the new process.  */
> +  resume_ptid = minus_one_ptid;
> +  fbsd_nat_debug_printf ("setting resume_ptid to [%s]",
> +			 target_pid_to_str (resume_ptid).c_str ());
> +  inf_ptrace_target::attach (args, from_tty);
> +}
> +

With the "general" solution where you keep rack of the resume state of
all threads, this part would be a bit cleaner I think.  Any thread
created due to an attach or due to create_inferior would start in the
"resumed" state.

Simon
  
John Baldwin March 27, 2023, 8:24 p.m. UTC | #2
On 3/20/23 11:35 AM, Simon Marchi wrote:
> On 2/28/23 13:18, John Baldwin wrote:
>> The pending_events list stores a queue of deferred events that might
>> be reported by the next call to the target's wait method.  The set of
>> events that are eligible is filtered by the ptid passed to resume.
>>
>> For now this just replaces the list of vfork_done events.  A
>> subsequent commit will reuse this to store other events.
>> ---
>>   gdb/fbsd-nat.c | 146 ++++++++++++++++++++++++++++++++-----------------
>>   gdb/fbsd-nat.h |   2 +
>>   2 files changed, 98 insertions(+), 50 deletions(-)
>>
>> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
>> index 04d67fc5278..ca278b871ef 100644
>> --- a/gdb/fbsd-nat.c
>> +++ b/gdb/fbsd-nat.c
>> @@ -54,6 +54,66 @@
>>   #define	PT_SETREGSET	43	/* Set a target register set */
>>   #endif
>>   
>> +/* Filter for ptid's allowed to report events from wait.  Normally set
>> +   in resume, but also reset to minus_one_ptid in create_inferior and
>> +   attach.  */
>> +
>> +static ptid_t resume_ptid;
> 
> It doesn't change anything conceptually, but I think this should be a
> private field of fbsd_nat_target.

This does mean making all the *pending_event functions private methods
of the class or using friend in various ways.  It also doesn't exist
at the end of the series.

> This resume_ptid approach might work now, but it won't work in general,
> since it only records the scope ptid passed to the last call to resume.
> 
> Even today, do you support multi-inferior?  If the user does:
> 
> (gdb) inferior 1
> (gdb) continue &
> (gdb) inferior 2
> (gdb) continue &
> 
> Then resume_ptid would be (inf2_pid, 0, 0), and you'd be ignoring any
> event from inferior 1, right?
> 
> I think the general solution is necessarily to remember the
> resume state for each individual thread, somehow.  Up to you to see if
> you want to implement that now or defer to later.  If you only support
> all-stop with a single inferior, it might be enough for now.
> 
> For reference, the linux-nat target maintains its own data structures of
> lwps (lwp_lwpid_htab), in which the last_resume_kind field is used for
> that.  It would be unfortunate to have to reimplement all of this for
> fbsd-nat as I'm sure that a lot of the logic would be very similar.  But
> then making the code shareable would also be a lot of work.

Yes, patch 6 reworks this further to properly suport multi-process where
it tracks this type of state per-process instead of globally.  I could
squash them all together perhaps but was trying to incrementally solve
multiple problems and trying to keep the patches smaller to ease review.
In particular, patch 5 aims to solve the problem of a new thread showing
up in a process being single-stepped where events are only expected for
the thread being stepped.

>> +
>> +/* If an event is triggered asynchronously (fake vfork_done events) or
>> +   occurs when the core is not expecting it, a pending event is
>> +   created.  This event is then returned by a future call to the
>> +   target wait method.  */
> 
> It can probably also happen if two events happen at the "same" time?
> Like if you react to a breakpoint hit by one thread, and while stopping
> the other threads, another thread reports a breakpoint stop, you have to
> store that one as a pending event?

With the caveat that on FreeBSD (and I think on other BSDs) you'd have to
have those threads be in separate processes, but yes.  This patch aims to
try to make the pending vfork events list more generic so it can be used
to hold other events.
>> +
>> +struct pending_event
>> +{
>> +  pending_event (const ptid_t &_ptid, const target_waitstatus &_status) :
>> +    ptid(_ptid), status(_status) {}
> 
> Spaces after parentheses.

Fixed (along with other style bugs you pointed out)

>> +
>> +  ptid_t ptid;
>> +  target_waitstatus status;
>> +};
>> +
>> +static std::list<pending_event> pending_events;
> 
> This could use intrusive_list, which is a bit more lightweight than
> std::list (it won't really make a difference in practice, but we have
> it, so we might as well use it).

Note though that this is a list of objects, not a list of pointers,
so as a std::list the objects are embedded in a single allocation
along with the pointers (IIUC) (e.g. the current code uses emplace_back
to construct the object in place in the list node).  In order to use an
intrusive list, the code would now have to explicitly new an object
before doing a push_back, and take_pending_event would have to explicitly
delete the object after the call to erase().

>> +   matching event, PTID and *STATUS contain the event details, and the
>> +   event is removed from the pending list.  */
>> +
>> +static bool
>> +take_pending_event (ptid_t &ptid, target_waitstatus *status)
>> +{
>> +  for (auto it = pending_events.begin (); it != pending_events.end (); it++)
>> +    if (it->ptid.matches (resume_ptid))
>> +      {
>> +	ptid = it->ptid;
>> +	*status = it->status;
>> +	pending_events.erase (it);
>> +	return true;
>> +      }
>> +  return false;
>> +}
> 
> This could maybe return a gdb::optional<pending_event>?

Ok.
>> @@ -1061,47 +1121,20 @@ fbsd_is_child_pending (pid_t pid)
>>   }
>>   
>>   #ifndef PTRACE_VFORK
>> -static std::forward_list<ptid_t> fbsd_pending_vfork_done;
>> -
>>   /* Record a pending vfork done event.  */
>>   
>>   static void
>>   fbsd_add_vfork_done (ptid_t pid)
>>   {
>> -  fbsd_pending_vfork_done.push_front (pid);
>> +  target_waitstatus status;
>> +  status.set_vfork_done ();
>> +  add_pending_event (ptid, status);
> 
> I think you can do this?
> 
>    add_pending_event (ptid, target_waitstatus ().set_vfork_done ());
> 
> Not a C++ expert, but I believe the temporary target_waitstatus is kept
> alive long enough for this to work.

Yes, I think this is fine.
  
>> @@ -1110,21 +1143,18 @@ fbsd_next_vfork_done (void)
>>   void
>>   fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
> 
> It would be a good time to rename the first parameter from ptid to
> scope_ptid, to match what was done here:
> 
> https://gitlab.com/gnutools/binutils-gdb/-/commit/d51926f06a7f4854bebdd71dcb0a78dbaa2f4168
> 
> I think it helps understand the meaning of the parameter.

Hmm, I might do the rename as a separate commit first to avoid adding noise
in this patch.

>>   {
>> -#if defined(TDP_RFPPWAIT) && !defined(PTRACE_VFORK)
>> -  pid_t pid;
>> -
>> -  /* Don't PT_CONTINUE a process which has a pending vfork done event.  */
>> -  if (minus_one_ptid == ptid)
>> -    pid = inferior_ptid.pid ();
>> -  else
>> -    pid = ptid.pid ();
>> -  if (fbsd_is_vfork_done_pending (pid))
>> -    return;
>> -#endif
> 
> Yeah, this old code seems to misinterpret the target_ops::resume
> interface, which was clarified in the commit mentioned above.
> 
>> +void
>> +fbsd_nat_target::attach (const char *args, int from_tty)
>> +{
>> +  /* Expect a wait for the new process.  */
>> +  resume_ptid = minus_one_ptid;
>> +  fbsd_nat_debug_printf ("setting resume_ptid to [%s]",
>> +			 target_pid_to_str (resume_ptid).c_str ());
>> +  inf_ptrace_target::attach (args, from_tty);
>> +}
>> +
> 
> With the "general" solution where you keep rack of the resume state of
> all threads, this part would be a bit cleaner I think.  Any thread
> created due to an attach or due to create_inferior would start in the
> "resumed" state.

So later on in patch 6 what happened is that I ended up with a
per-process structure that tracked the state of an inferior and
resume_ptid is per-inferior instead of global.
  
Simon Marchi March 27, 2023, 8:57 p.m. UTC | #3
On 3/27/23 16:24, John Baldwin wrote:
> On 3/20/23 11:35 AM, Simon Marchi wrote:
>> On 2/28/23 13:18, John Baldwin wrote:
>>> The pending_events list stores a queue of deferred events that might
>>> be reported by the next call to the target's wait method.  The set of
>>> events that are eligible is filtered by the ptid passed to resume.
>>>
>>> For now this just replaces the list of vfork_done events.  A
>>> subsequent commit will reuse this to store other events.
>>> ---
>>>   gdb/fbsd-nat.c | 146 ++++++++++++++++++++++++++++++++-----------------
>>>   gdb/fbsd-nat.h |   2 +
>>>   2 files changed, 98 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
>>> index 04d67fc5278..ca278b871ef 100644
>>> --- a/gdb/fbsd-nat.c
>>> +++ b/gdb/fbsd-nat.c
>>> @@ -54,6 +54,66 @@
>>>   #define    PT_SETREGSET    43    /* Set a target register set */
>>>   #endif
>>>   +/* Filter for ptid's allowed to report events from wait.  Normally set
>>> +   in resume, but also reset to minus_one_ptid in create_inferior and
>>> +   attach.  */
>>> +
>>> +static ptid_t resume_ptid;
>>
>> It doesn't change anything conceptually, but I think this should be a
>> private field of fbsd_nat_target.
> 
> This does mean making all the *pending_event functions private methods
> of the class or using friend in various ways.  It also doesn't exist
> at the end of the series.

Ok that's fine with me.

>> This resume_ptid approach might work now, but it won't work in general,
>> since it only records the scope ptid passed to the last call to resume.
>>
>> Even today, do you support multi-inferior?  If the user does:
>>
>> (gdb) inferior 1
>> (gdb) continue &
>> (gdb) inferior 2
>> (gdb) continue &
>>
>> Then resume_ptid would be (inf2_pid, 0, 0), and you'd be ignoring any
>> event from inferior 1, right?
>>
>> I think the general solution is necessarily to remember the
>> resume state for each individual thread, somehow.  Up to you to see if
>> you want to implement that now or defer to later.  If you only support
>> all-stop with a single inferior, it might be enough for now.
>>
>> For reference, the linux-nat target maintains its own data structures of
>> lwps (lwp_lwpid_htab), in which the last_resume_kind field is used for
>> that.  It would be unfortunate to have to reimplement all of this for
>> fbsd-nat as I'm sure that a lot of the logic would be very similar.  But
>> then making the code shareable would also be a lot of work.
> 
> Yes, patch 6 reworks this further to properly suport multi-process where
> it tracks this type of state per-process instead of globally.  I could
> squash them all together perhaps but was trying to incrementally solve
> multiple problems and trying to keep the patches smaller to ease review.
> In particular, patch 5 aims to solve the problem of a new thread showing
> up in a process being single-stepped where events are only expected for
> the thread being stepped.

That's ok.  I review patches linearly, making comments as I go.  I don't
have the mental swap space to look at everything and then comment based
on the whole series (even though it would be better).  So it's
appropriate to tell me "it's irrelevant, it changes later".

> Note though that this is a list of objects, not a list of pointers,
> so as a std::list the objects are embedded in a single allocation
> along with the pointers (IIUC) (e.g. the current code uses emplace_back
> to construct the object in place in the list node).  In order to use an
> intrusive list, the code would now have to explicitly new an object
> before doing a push_back, and take_pending_event would have to explicitly
> delete the object after the call to erase().

Indeed.

Simon
  
John Baldwin March 27, 2023, 9 p.m. UTC | #4
On 3/27/23 1:24 PM, John Baldwin wrote:
> On 3/20/23 11:35 AM, Simon Marchi wrote:
>>> @@ -1110,21 +1143,18 @@ fbsd_next_vfork_done (void)
>>>    void
>>>    fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
>>
>> It would be a good time to rename the first parameter from ptid to
>> scope_ptid, to match what was done here:
>>
>> https://gitlab.com/gnutools/binutils-gdb/-/commit/d51926f06a7f4854bebdd71dcb0a78dbaa2f4168
>>
>> I think it helps understand the meaning of the parameter.
> 
> Hmm, I might do the rename as a separate commit first to avoid adding noise
> in this patch.

Actually, I think I will do this in the patch that reworks resume() to
properly handle multiple inferiors as that is when it starts to really
be treated as scope_ptid.
  

Patch

diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 04d67fc5278..ca278b871ef 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -54,6 +54,66 @@ 
 #define	PT_SETREGSET	43	/* Set a target register set */
 #endif
 
+/* Filter for ptid's allowed to report events from wait.  Normally set
+   in resume, but also reset to minus_one_ptid in create_inferior and
+   attach.  */
+
+static ptid_t resume_ptid;
+
+/* If an event is triggered asynchronously (fake vfork_done events) or
+   occurs when the core is not expecting it, a pending event is
+   created.  This event is then returned by a future call to the
+   target wait method.  */
+
+struct pending_event
+{
+  pending_event (const ptid_t &_ptid, const target_waitstatus &_status) :
+    ptid(_ptid), status(_status) {}
+
+  ptid_t ptid;
+  target_waitstatus status;
+};
+
+static std::list<pending_event> pending_events;
+
+/* Add a new pending event to the list.  */
+
+static void
+add_pending_event (const ptid_t &ptid, const target_waitstatus &status)
+{
+  pending_events.emplace_back (ptid, status);
+}
+
+/* Return true if there is a pending event matching FILTER.  */
+
+static bool
+have_pending_event (ptid_t filter)
+{
+  for (const pending_event &event : pending_events)
+    if (event.ptid.matches (filter))
+      return true;
+  return false;
+}
+
+/* Helper method called by the target wait method.  Returns true if
+   there is a pending event matching resume_ptid.  If there is a
+   matching event, PTID and *STATUS contain the event details, and the
+   event is removed from the pending list.  */
+
+static bool
+take_pending_event (ptid_t &ptid, target_waitstatus *status)
+{
+  for (auto it = pending_events.begin (); it != pending_events.end (); it++)
+    if (it->ptid.matches (resume_ptid))
+      {
+	ptid = it->ptid;
+	*status = it->status;
+	pending_events.erase (it);
+	return true;
+      }
+  return false;
+}
+
 /* Return the name of a file that can be opened to get the symbols for
    the child process identified by PID.  */
 
@@ -1061,47 +1121,20 @@  fbsd_is_child_pending (pid_t pid)
 }
 
 #ifndef PTRACE_VFORK
-static std::forward_list<ptid_t> fbsd_pending_vfork_done;
-
 /* Record a pending vfork done event.  */
 
 static void
 fbsd_add_vfork_done (ptid_t pid)
 {
-  fbsd_pending_vfork_done.push_front (pid);
+  target_waitstatus status;
+  status.set_vfork_done ();
+  add_pending_event (ptid, status);
 
   /* If we're in async mode, need to tell the event loop there's
      something here to process.  */
   if (target_is_async_p ())
     async_file_mark ();
 }
-
-/* Check for a pending vfork done event for a specific PID.  */
-
-static int
-fbsd_is_vfork_done_pending (pid_t pid)
-{
-  for (auto it = fbsd_pending_vfork_done.begin ();
-       it != fbsd_pending_vfork_done.end (); it++)
-    if (it->pid () == pid)
-      return 1;
-  return 0;
-}
-
-/* Check for a pending vfork done event.  If one is found, remove it
-   from the list and return the PTID.  */
-
-static ptid_t
-fbsd_next_vfork_done (void)
-{
-  if (!fbsd_pending_vfork_done.empty ())
-    {
-      ptid_t ptid = fbsd_pending_vfork_done.front ();
-      fbsd_pending_vfork_done.pop_front ();
-      return ptid;
-    }
-  return null_ptid;
-}
 #endif
 #endif
 
@@ -1110,21 +1143,18 @@  fbsd_next_vfork_done (void)
 void
 fbsd_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
 {
-#if defined(TDP_RFPPWAIT) && !defined(PTRACE_VFORK)
-  pid_t pid;
-
-  /* Don't PT_CONTINUE a process which has a pending vfork done event.  */
-  if (minus_one_ptid == ptid)
-    pid = inferior_ptid.pid ();
-  else
-    pid = ptid.pid ();
-  if (fbsd_is_vfork_done_pending (pid))
-    return;
-#endif
-
   fbsd_nat_debug_printf ("[%s], step %d, signo %d (%s)",
 			 target_pid_to_str (ptid).c_str (), step, signo,
 			 gdb_signal_to_name (signo));
+
+  /* Don't PT_CONTINUE a thread or process which has a pending event.  */
+  resume_ptid = ptid;
+  if (have_pending_event (ptid))
+    {
+      fbsd_nat_debug_printf ("found pending event");
+      return;
+    }
+
   if (ptid.lwp_p ())
     {
       /* If ptid is a specific LWP, suspend all other LWPs in the process.  */
@@ -1257,14 +1287,6 @@  fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
 
   while (1)
     {
-#ifndef PTRACE_VFORK
-      wptid = fbsd_next_vfork_done ();
-      if (wptid != null_ptid)
-	{
-	  ourstatus->set_vfork_done ();
-	  return wptid;
-	}
-#endif
       wptid = inf_ptrace_target::wait (ptid, ourstatus, target_options);
       if (ourstatus->kind () == TARGET_WAITKIND_STOPPED)
 	{
@@ -1478,6 +1500,16 @@  fbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   fbsd_nat_debug_printf ("[%s], [%s]", target_pid_to_str (ptid).c_str (),
 			 target_options_to_string (target_options).c_str ());
 
+  /* If there is a valid pending event, return it.  */
+  if (take_pending_event (wptid, ourstatus))
+    {
+      fbsd_nat_debug_printf ("returning pending event [%s], [%s]",
+			     target_pid_to_str (wptid).c_str (),
+			     ourstatus->to_string ().c_str ());
+      gdb_assert (wptid.matches (ptid));
+      return wptid;
+    }
+
   /* Ensure any subsequent events trigger a new event in the loop.  */
   if (is_async_p ())
     async_file_flush ();
@@ -1585,9 +1617,23 @@  fbsd_nat_target::create_inferior (const char *exec_file,
     (disable_randomization);
 #endif
 
+  /* Expect a wait for the new process.  */
+  resume_ptid = minus_one_ptid;
+  fbsd_nat_debug_printf ("setting resume_ptid to [%s]",
+			 target_pid_to_str (resume_ptid).c_str ());
   inf_ptrace_target::create_inferior (exec_file, allargs, env, from_tty);
 }
 
+void
+fbsd_nat_target::attach (const char *args, int from_tty)
+{
+  /* Expect a wait for the new process.  */
+  resume_ptid = minus_one_ptid;
+  fbsd_nat_debug_printf ("setting resume_ptid to [%s]",
+			 target_pid_to_str (resume_ptid).c_str ());
+  inf_ptrace_target::attach (args, from_tty);
+}
+
 #ifdef TDP_RFPPWAIT
 /* Target hook for follow_fork.  On entry and at return inferior_ptid is
    the ptid of the followed inferior.  */
diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
index a19bceaf5e4..cda150ac465 100644
--- a/gdb/fbsd-nat.h
+++ b/gdb/fbsd-nat.h
@@ -76,6 +76,8 @@  class fbsd_nat_target : public inf_ptrace_target
   void create_inferior (const char *, const std::string &,
 			char **, int) override;
 
+  void attach (const char *, int) override;
+
   void resume (ptid_t, int, enum gdb_signal) override;
 
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;