From 1edb66d856c82c389edfd7610143236a68c76846 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Thu, 27 May 2021 23:37:03 -0400 Subject: gdb: make thread_info::suspend private, add getters / setters 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. Change-Id: Iedab9653238eaf8231abcf0baa20145acc8b77a7 --- gdb/elf-none-tdep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'gdb/elf-none-tdep.c') diff --git a/gdb/elf-none-tdep.c b/gdb/elf-none-tdep.c index 4cbb664..52a6281 100644 --- a/gdb/elf-none-tdep.c +++ b/gdb/elf-none-tdep.c @@ -92,7 +92,7 @@ elf_none_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, that stopped SIGNALLED_THR. */ gdb_signal stop_signal; if (signalled_thr != nullptr) - stop_signal = signalled_thr->suspend.stop_signal; + stop_signal = signalled_thr->stop_signal (); else stop_signal = GDB_SIGNAL_0; -- cgit v1.1