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

[Simon Marchi <simark@simark.ca>]

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