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

[John Baldwin <jhb@FreeBSD.org>]

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.

-- 
John Baldwin