From 573269a87c89ae866db556428fe9ea63d6c4db5f Mon Sep 17 00:00:00 2001 From: Lancelot SIX Date: Tue, 11 Jan 2022 10:10:11 -0500 Subject: [PATCH] gdb: make thread_info::m_thread_fsm a std::unique_ptr While working on function calls, I realized that the thread_fsm member of struct thread_info is a raw pointer to a resource it owns. This commit changes the type of the thread_fsm member to a std::unique_ptr in order to signify this ownership relationship and slightly ease resource management (no need to manually call delete). To ensure consistent use, the field is made a private member (m_thread_fsm). The setter method (set_thread_fsm) can then check that it is incorrect to associate a FSM to a thread_info object if another one is already in place. This is ensured by an assertion. The function run_inferior_call takes an argument as a pointer to a call_thread_fsm and installs it in it in a thread_info instance. Also change this function's signature to accept a unique_ptr in order to signify that the ownership of the call_thread_fsm is transferred during the call. No user visible change expected after this commit. Tested on x86_64-linux with no regression observed. Change-Id: Ia1224f72a4afa247801ce6650ce82f90224a9ae8 --- gdb/breakpoint.c | 6 ++++-- gdb/cli/cli-interp.c | 6 +++--- gdb/gdbthread.h | 37 ++++++++++++++++++++++++++++----- gdb/infcall.c | 49 +++++++++++++++++++++++++------------------- gdb/infcmd.c | 8 ++++---- gdb/infrun.c | 44 +++++++++++++++++---------------------- gdb/mi/mi-interp.c | 6 +++--- gdb/thread.c | 7 +++---- 8 files changed, 96 insertions(+), 67 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index b1130acbab2..9ff2bf82374 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -10830,8 +10830,10 @@ until_break_command (const char *arg, int from_tty, int anywhere) breakpoints.emplace_back (std::move (location_breakpoint)); } - tp->thread_fsm = new until_break_fsm (command_interp (), tp->global_num, - std::move (breakpoints)); + tp->set_thread_fsm + (std::unique_ptr + (new until_break_fsm (command_interp (), tp->global_num, + std::move (breakpoints)))); if (lj_deleter) lj_deleter->release (); diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c index dd56e12e696..0190b4d32bc 100644 --- a/gdb/cli/cli-interp.c +++ b/gdb/cli/cli-interp.c @@ -108,9 +108,9 @@ should_print_stop_to_console (struct interp *console_interp, { if ((bpstat_what (tp->control.stop_bpstat).main_action == BPSTAT_WHAT_STOP_NOISY) - || tp->thread_fsm == NULL - || tp->thread_fsm->command_interp == console_interp - || !tp->thread_fsm->finished_p ()) + || tp->thread_fsm () == nullptr + || tp->thread_fsm ()->command_interp == console_interp + || !tp->thread_fsm ()->finished_p ()) return 1; return 0; } diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 9921dae7a71..1a33eb61221 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -34,6 +34,7 @@ struct symtab; #include "gdbsupport/forward-scope-exit.h" #include "displaced-stepping.h" #include "gdbsupport/intrusive_list.h" +#include "thread-fsm.h" struct inferior; struct process_stratum_target; @@ -443,6 +444,32 @@ class thread_info : public refcounted_object, m_suspend.stop_reason = reason; } + /* Get the FSM associated with the thread. */ + + struct thread_fsm *thread_fsm () const + { + return m_thread_fsm.get (); + } + + /* Get the owning reference to the FSM associated with the thread. + + After a call to this method, "thread_fsm () == nullptr". */ + + std::unique_ptr release_thread_fsm () + { + return std::move (m_thread_fsm); + } + + /* Set the FSM associated with the current thread. + + It is invalid to set the FSM if another FSM is already installed. */ + + void set_thread_fsm (std::unique_ptr fsm) + { + gdb_assert (m_thread_fsm == nullptr); + m_thread_fsm = std::move (fsm); + } + int current_line = 0; struct symtab *current_symtab = NULL; @@ -480,11 +507,6 @@ class thread_info : public refcounted_object, when GDB gets back SIGTRAP from step_resume_breakpoint. */ int step_after_step_resume_breakpoint = 0; - /* Pointer to the state machine manager object that handles what is - left to do for the thread's execution command after the target - stops. Several execution commands use it. */ - struct thread_fsm *thread_fsm = NULL; - /* This is used to remember when a fork or vfork event was caught by a catchpoint, and thus the event is to be followed at the next resume of the thread, and not immediately. */ @@ -550,6 +572,11 @@ class thread_info : public refcounted_object, Nullptr if the thread does not have a user-given name. */ gdb::unique_xmalloc_ptr m_name; + + /* Pointer to the state machine manager object that handles what is + left to do for the thread's execution command after the target + stops. Several execution commands use it. */ + std::unique_ptr m_thread_fsm; }; using thread_info_resumed_with_pending_wait_status_node diff --git a/gdb/infcall.c b/gdb/infcall.c index 05cf18f0a7f..8e896296b96 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -574,7 +574,7 @@ call_thread_fsm::should_notify_stop () thrown errors. The caller should rethrow if there's an error. */ static struct gdb_exception -run_inferior_call (struct call_thread_fsm *sm, +run_inferior_call (std::unique_ptr sm, struct thread_info *call_thread, CORE_ADDR real_pc) { struct gdb_exception caught_error; @@ -597,9 +597,8 @@ run_inferior_call (struct call_thread_fsm *sm, clear_proceed_status (0); /* Associate the FSM with the thread after clear_proceed_status - (otherwise it'd clear this FSM), and before anything throws, so - we don't leak it (and any resources it manages). */ - call_thread->thread_fsm = sm; + (otherwise it'd clear this FSM). */ + call_thread->set_thread_fsm (std::move (sm)); disable_watchpoints_before_interactive_call_start (); @@ -1251,12 +1250,9 @@ call_function_by_hand_dummy (struct value *function, just below is the place to chop this function in two.. */ { - struct thread_fsm *saved_sm; - struct call_thread_fsm *sm; - /* Save the current FSM. We'll override it. */ - saved_sm = call_thread->thread_fsm; - call_thread->thread_fsm = NULL; + std::unique_ptr saved_sm = call_thread->release_thread_fsm (); + struct call_thread_fsm *sm; /* Save this thread's ptid, we need it later but the thread may have exited. */ @@ -1273,17 +1269,19 @@ call_function_by_hand_dummy (struct value *function, values_type, return_method != return_method_normal, struct_addr); - - e = run_inferior_call (sm, call_thread.get (), real_pc); + { + std::unique_ptr sm_up (sm); + e = run_inferior_call (std::move (sm_up), call_thread.get (), real_pc); + } gdb::observers::inferior_call_post.notify (call_thread_ptid, funaddr); if (call_thread->state != THREAD_EXITED) { /* The FSM should still be the same. */ - gdb_assert (call_thread->thread_fsm == sm); + gdb_assert (call_thread->thread_fsm () == sm); - if (call_thread->thread_fsm->finished_p ()) + if (call_thread->thread_fsm ()->finished_p ()) { struct value *retval; @@ -1297,11 +1295,16 @@ call_function_by_hand_dummy (struct value *function, /* Get the return value. */ retval = sm->return_value; - /* Clean up / destroy the call FSM, and restore the - original one. */ - call_thread->thread_fsm->clean_up (call_thread.get ()); - delete call_thread->thread_fsm; - call_thread->thread_fsm = saved_sm; + /* Restore the original FSM and clean up / destroh the call FSM. + Doing it in this order ensures that if the call to clean_up + throws, the original FSM is properly restored. */ + { + std::unique_ptr finalizing + = call_thread->release_thread_fsm (); + call_thread->set_thread_fsm (std::move (saved_sm)); + + finalizing->clean_up (call_thread.get ()); + } maybe_remove_breakpoints (); @@ -1315,9 +1318,13 @@ call_function_by_hand_dummy (struct value *function, /* Didn't complete. Clean up / destroy the call FSM, and restore the previous state machine, and handle the error. */ - call_thread->thread_fsm->clean_up (call_thread.get ()); - delete call_thread->thread_fsm; - call_thread->thread_fsm = saved_sm; + { + std::unique_ptr finalizing + = call_thread->release_thread_fsm (); + call_thread->set_thread_fsm (std::move (saved_sm)); + + finalizing->clean_up (call_thread.get ()); + } } } diff --git a/gdb/infcmd.c b/gdb/infcmd.c index b9fb121dbbe..48dda9b23ee 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -848,7 +848,7 @@ step_1 (int skip_subroutines, int single_inst, const char *count_string) steps. */ thr = inferior_thread (); step_sm = new step_command_fsm (command_interp ()); - thr->thread_fsm = step_sm; + thr->set_thread_fsm (std::unique_ptr (step_sm)); step_command_fsm_prepare (step_sm, skip_subroutines, single_inst, count, thr); @@ -865,7 +865,7 @@ step_1 (int skip_subroutines, int single_inst, const char *count_string) /* Stepped into an inline frame. Pretend that we've stopped. */ - thr->thread_fsm->clean_up (thr); + thr->thread_fsm ()->clean_up (thr); proceeded = normal_stop (); if (!proceeded) inferior_event_handler (INF_EXEC_COMPLETE); @@ -1355,7 +1355,7 @@ until_next_command (int from_tty) delete_longjmp_breakpoint_cleanup lj_deleter (thread); sm = new until_next_fsm (command_interp (), tp->global_num); - tp->thread_fsm = sm; + tp->set_thread_fsm (std::unique_ptr (sm)); lj_deleter.release (); proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); @@ -1762,7 +1762,7 @@ finish_command (const char *arg, int from_tty) sm = new finish_command_fsm (command_interp ()); - tp->thread_fsm = sm; + tp->set_thread_fsm (std::unique_ptr (sm)); /* Finishing from an inline frame is completely different. We don't try to show the "return value" - no way to locate it. */ diff --git a/gdb/infrun.c b/gdb/infrun.c index f9420d54990..61af16238de 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -698,7 +698,6 @@ follow_fork () int current_line = 0; symtab *current_symtab = NULL; struct frame_id step_frame_id = { 0 }; - struct thread_fsm *thread_fsm = NULL; if (!non_stop) { @@ -741,6 +740,7 @@ follow_fork () case TARGET_WAITKIND_VFORKED: { ptid_t parent, child; + std::unique_ptr thread_fsm; /* If the user did a next/step, etc, over a fork call, preserve the stepping state in the fork child. */ @@ -755,7 +755,7 @@ follow_fork () step_frame_id = tp->control.step_frame_id; exception_resume_breakpoint = clone_momentary_breakpoint (tp->control.exception_resume_breakpoint); - thread_fsm = tp->thread_fsm; + thread_fsm = tp->release_thread_fsm (); /* For now, delete the parent's sr breakpoint, otherwise, parent/child sr breakpoints are considered duplicates, @@ -767,7 +767,6 @@ follow_fork () tp->control.step_range_end = 0; tp->control.step_frame_id = null_frame_id; delete_exception_resume_breakpoint (tp); - tp->thread_fsm = NULL; } parent = inferior_ptid; @@ -809,7 +808,7 @@ follow_fork () tp->control.step_frame_id = step_frame_id; tp->control.exception_resume_breakpoint = exception_resume_breakpoint; - tp->thread_fsm = thread_fsm; + tp->set_thread_fsm (std::move (thread_fsm)); } else { @@ -2651,8 +2650,7 @@ clear_proceed_status_thread (struct thread_info *tp) if (!signal_pass_state (tp->stop_signal ())) tp->set_stop_signal (GDB_SIGNAL_0); - delete tp->thread_fsm; - tp->thread_fsm = NULL; + tp->release_thread_fsm (); tp->control.trap_expected = 0; tp->control.step_range_start = 0; @@ -3935,24 +3933,24 @@ reinstall_readline_callback_handler_cleanup () static void clean_up_just_stopped_threads_fsms (struct execution_control_state *ecs) { - if (ecs->event_thread != NULL - && ecs->event_thread->thread_fsm != NULL) - ecs->event_thread->thread_fsm->clean_up (ecs->event_thread); + if (ecs->event_thread != nullptr + && ecs->event_thread->thread_fsm () != nullptr) + ecs->event_thread->thread_fsm ()->clean_up (ecs->event_thread); if (!non_stop) { for (thread_info *thr : all_non_exited_threads ()) { - if (thr->thread_fsm == NULL) + if (thr->thread_fsm () == nullptr) continue; if (thr == ecs->event_thread) continue; switch_to_thread (thr); - thr->thread_fsm->clean_up (thr); + thr->thread_fsm ()->clean_up (thr); } - if (ecs->event_thread != NULL) + if (ecs->event_thread != nullptr) switch_to_thread (ecs->event_thread); } } @@ -4103,13 +4101,8 @@ fetch_inferior_event () delete_just_stopped_threads_infrun_breakpoints (); - if (thr != NULL) - { - struct thread_fsm *thread_fsm = thr->thread_fsm; - - if (thread_fsm != NULL) - should_stop = thread_fsm->should_stop (thr); - } + if (thr != nullptr && thr->thread_fsm () != nullptr) + should_stop = thr->thread_fsm ()->should_stop (thr); if (!should_stop) { @@ -4122,8 +4115,9 @@ fetch_inferior_event () clean_up_just_stopped_threads_fsms (ecs); - if (thr != NULL && thr->thread_fsm != NULL) - should_notify_stop = thr->thread_fsm->should_notify_stop (); + if (thr != nullptr && thr->thread_fsm () != nullptr) + should_notify_stop + = thr->thread_fsm ()->should_notify_stop (); if (should_notify_stop) { @@ -8340,13 +8334,13 @@ print_stop_event (struct ui_out *uiout, bool displays) } tp = inferior_thread (); - if (tp->thread_fsm != NULL - && tp->thread_fsm->finished_p ()) + if (tp->thread_fsm () != nullptr + && tp->thread_fsm ()->finished_p ()) { struct return_value_info *rv; - rv = tp->thread_fsm->return_value (); - if (rv != NULL) + rv = tp->thread_fsm ()->return_value (); + if (rv != nullptr) print_return_value (uiout, rv); } } diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c index e69ad9aff2d..f02a7cf3f10 100644 --- a/gdb/mi/mi-interp.c +++ b/gdb/mi/mi-interp.c @@ -630,12 +630,12 @@ mi_on_normal_stop_1 (struct bpstat *bs, int print_frame) tp = inferior_thread (); - if (tp->thread_fsm != NULL - && tp->thread_fsm->finished_p ()) + if (tp->thread_fsm () != nullptr + && tp->thread_fsm ()->finished_p ()) { enum async_reply_reason reason; - reason = tp->thread_fsm->async_reply_reason (); + reason = tp->thread_fsm ()->async_reply_reason (); mi_uiout->field_string ("reason", async_reason_lookup (reason)); } diff --git a/gdb/thread.c b/gdb/thread.c index 611be3f4633..12f3abd57bd 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -160,11 +160,10 @@ thread_has_single_step_breakpoint_here (struct thread_info *tp, void thread_cancel_execution_command (struct thread_info *thr) { - if (thr->thread_fsm != NULL) + if (thr->thread_fsm () != nullptr) { - thr->thread_fsm->clean_up (thr); - delete thr->thread_fsm; - thr->thread_fsm = NULL; + std::unique_ptr fsm = thr->release_thread_fsm (); + fsm->clean_up (thr); } }