Re: [PATCH 06/11] gdb: make thread_info::suspend private, add getters / setters

[Pedro Alves <pedro@palves.net>]

On 2021-06-22 5:56 p.m., Simon Marchi via Gdb-patches wrote:
> A following patch will want to take some action when a pending wait
> status is set on or removed from a thread.  Add a getter and a setter on
> thread_info for the pending waitstatus, so that we can add some code in
> the setter later.
> 
> The thing is, the pending wait status field is in the
> thread_suspend_state, along with other fields that we need to backup
> before and restore after the thread does an inferior function call.
> Therefore, make the thread_suspend_state member private
> (thread_info::suspend becomes thread_info::m_suspend), and add getters /
> setters for all of its fields:
> 
>  - pending wait status
>  - stop signal
>  - stop reason
>  - stop pc
> 
> For the pending wait status, add the additional has_pending_waitstatus
> and clear_pending_waitstatus methods.
> 
> I think this makes the thread_info interface a bit nicer, because we
> now access the fields as:
> 
>   thread->stop_pc ()
> 
> rather than
> 
>   thread->suspend.stop_pc
> 
> The stop_pc field being in the `suspend` structure is an implementation
> detail of thread_info that callers don't need to be aware of.
> 
> For the backup / restore of the thread_suspend_state structure, add
> save_suspend_to and restore_suspend_from methods.  You might wonder why
> `save_suspend_to`, as opposed to a simple getter like
> 
>   thread_suspend_state &suspend ();
> 
> I want to make it clear that this is to be used only for backing up and
> restoring the suspend state, _not_ to access fields like:
> 
>   thread->suspend ()->stop_pc
> 
> Adding some getters / setters allows adding some assertions.  I find
> that this helps understand how things are supposed to work.  Add:
> 
>  - When getting the pending status (pending_waitstatus method), ensure
>    that there is a pending status.
>  - When setting a pending status (set_pending_waitstatus method), ensure
>    there is no pending status.
> 
> There is one case I found where this wasn't true - in
> remote_target::process_initial_stop_replies - which needed adjustments
> to respect that contract.  I think it's because
> process_initial_stop_replies is kind of (ab)using the
> thread_info::suspend::waitstatus to store some statuses temporarily, for
> its internal use (statuses it doesn't intent on leaving pending).
> 
> process_initial_stop_replies pulls out stop replies received during the
> initial connection using target_wait.  It always stores the received
> event in `evthread->suspend.waitstatus`.  But it only sets
> waitstatus_pending_p, if it deems the event interesting enough to leave
> pending, to be reported to the core:
> 
>       if (ws.kind != TARGET_WAITKIND_STOPPED
> 	  || ws.value.sig != GDB_SIGNAL_0)
> 	evthread->suspend.waitstatus_pending_p = 1;
> 
> It later uses this flag a bit below, to choose which thread to make the
> "selected" one:
> 
>       if (selected == NULL
> 	  && thread->suspend.waitstatus_pending_p)
> 	selected = thread;
> 
> And ultimately that's used if the user-visible mode is all-stop, so that
> we print the stop for that interesting thread:
> 
>   /* In all-stop, we only print the status of one thread, and leave
>      others with their status pending.  */
>   if (!non_stop)
>     {
>       thread_info *thread = selected;
>       if (thread == NULL)
> 	thread = lowest_stopped;
>       if (thread == NULL)
> 	thread = first;
> 
>       print_one_stopped_thread (thread);
>     }
> 
> But in any case (all-stop or non-stop), print_one_stopped_thread needs
> to access the waitstatus value of these threads that don't have a
> pending waitstatus (those that had TARGET_WAITKIND_STOPPED +
> GDB_SIGNAL_0).  This doesn't work with the assertions I've
> put.
> 
> So, change the code to only set the thread's wait status if it is an
> interesting one that we are going to leave pending.  If the thread
> stopped due to a non-interesting event (TARGET_WAITKIND_STOPPED +
> GDB_SIGNAL_0), don't store it.  Adjust print_one_stopped_thread to
> understand that if a thread has no pending waitstatus, it's because it
> stopped with TARGET_WAITKIND_STOPPED + GDB_SIGNAL_0.
> 
> The call to set_last_target_status also uses the pending waitstatus.
> However, given that the pending waitstatus for the thread may have been
> cleared in print_one_stopped_thread (and that there might not even be a
> pending waitstatus in the first place, as explained above), it is no
> longer possible to do it at this point.  To fix that, move the call to
> set_last_target_status in print_one_stopped_thread.  I think this will
> preserve the existing behavior, because set_last_target_status is
> currently using the current thread's wait status.  And the current
> thread is the last one for which print_one_stopped_thread is called.  So
> by calling set_last_target_status in print_one_stopped_thread, we'll get
> the same result.  set_last_target_status will possibly be called
> multiple times, but only the last call will matter.  It just means
> possibly more calls to set_last_target_status, but those are cheap.
> 
> gdb/ChangeLog:
> 
> 	* gdbthread.h (class thread_info) <save_suspend_to,
> 	restore_suspend_from, stop_pc, set_stop_pc,
> 	has_pending_waitstatus, pending_waitstatus,
> 	set_pending_waitstatus, clear_pending_waitstatus, stop_signal,
> 	set_stop_signal, stop_reason, set_stop_reason): New.
> 	<suspend>: Rename to...
> 	<m_suspend>: ... this, make private.  Adjust all uses to use one
> 	of the new methods above.
> 	* thread.c (thread_info::set_pending_waitstatus,
> 	thread_info::clear_pending_waitstatus): New.
> 	* remote.c (class remote_target) <print_one_stopped_thread>: New
> 	method.
> 	(print_one_stopped_thread): Rename to...
> 	(remote_target::print_one_stopped_thread): ... this.  Assume
> 	that if thread has no pending waitstatus, it's
> 	TARGET_WAITKIND_STOPPED + GDB_SIGNAL_0.  Call
> 	set_last_target_status.
> 	(remote_target::process_initial_stop_replies): Don't set pending
> 	waitstatus if TARGET_WAITKIND_STOPPED + GDB_SIGNAL_0.  Don't
> 	call set_last_target_status.
> 	(is_pending_fork_parent): Constify param.
> 	(thread_pending_fork_status): Constify return.
> 	(is_pending_fork_parent_thread): Adjust.
> 	(remote_target::remove_new_fork_children): Adjust.

OK.