From patchwork Thu May 4 14:48:56 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 20275 Received: (qmail 62297 invoked by alias); 4 May 2017 14:49:10 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 58801 invoked by uid 89); 4 May 2017 14:49:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=desire, freedom, recheck, honestly X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 04 May 2017 14:49:00 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 88F6C7F3F8; Thu, 4 May 2017 14:49:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 88F6C7F3F8 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 88F6C7F3F8 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1739B17DC6; Thu, 4 May 2017 14:48:57 +0000 (UTC) Subject: Re: [PATCH v2] RAII-fy make_cleanup_restore_current_thread & friends To: Simon Marchi References: <1492618687-11704-1-git-send-email-palves@redhat.com> <005a86ac021df04f3a9c8883e3f8c211@polymtl.ca> Cc: gdb-patches@sourceware.org, Sergio Durigan Junior From: Pedro Alves Message-ID: Date: Thu, 4 May 2017 15:48:56 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <005a86ac021df04f3a9c8883e3f8c211@polymtl.ca> On 04/27/2017 04:48 PM, Simon Marchi wrote: > On 2017-04-19 12:18, Pedro Alves wrote: >> New in v2: >> - v1 had missed deleting save_current_program_space / >> restore_program_space. >> >> After all the make_cleanup_restore_current_thread fixing, I thought >> I'd convert that and its relatives (which are all cleanups) to RAII >> classes. >> >> scoped_restore_current_pspace_and_thread was put in a separate file to >> avoid a circular dependency. >> >> Tested on x86-64 Fedora 23, native and gdbserver. > > I agree with Sergio's comment. Replying to that here: On 04/25/2017 08:50 PM, Sergio Durigan Junior wrote: >> > @@ -3066,7 +3067,7 @@ update_inserted_breakpoint_locations (void) >> > there was an error. */ >> > tmp_error_stream.puts ("Warning:\n"); >> > >> > - struct cleanup *cleanups = save_current_space_and_thread (); >> > + scoped_restore_current_pspace_and_thread restore; > "restore" doesn't seem like a very good name; I think it's too generic. > How about naming the variable "restore_pspace_thread" or something like > that, like you did with other variables? I should perhaps remark that "restore" is no more generic than "cleanups". :-) Seriously, I made that change, throughout. Thanks. It does clarify the code when you need to reference the variable, in the cases the scoped_restore is wrapped in a gdb::optional. (Otherwise, I'm borderline. All the info you need is in the type name; the more you put in the variable name, the more you risk getting out of sync via refactoring.) > >> @@ -922,16 +936,17 @@ handle_vfork_child_exec_or_exit (int exec) >> >> inf->vfork_parent->pending_detach = 0; >> >> + gdb::optional >> + maybe_restore_inferior; >> + gdb::optional >> + maybe_restore_thread; >> + >> + /* If we're handling a child exit, then inferior_ptid points >> + at the inferior's pid, not to a thread. */ > > I wonder if this is still the right place for this comment. The other > logical place would be in scoped_restore_exited_inferior. I think it is, otherwise it's not clear why we need the optionals dance and the different scoped_restores instead of just always using scoped_restore_current_pspace_and_thread. I had already added this comment to scoped_restore_exited_inferior: /* Save/restore inferior_ptid, current program space and current inferior. Only use this if the current context points at an exited inferior (and therefore there's no current thread to save). */ (and yes, we have too many of these inferior/thread/whatnot state save/restorers. :-/ ) > > >> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c >> index c3e7bf7..8145eb5 100644 >> --- a/gdb/mi/mi-main.c >> +++ b/gdb/mi/mi-main.c >> @@ -59,6 +59,7 @@ >> #include >> #include "run-time-clock.h" >> #include >> +#include "progspace-and-thread.h" >> >> enum >> { >> @@ -274,9 +275,9 @@ exec_continue (char **argv, int argc) >> See comment on infcmd.c:proceed_thread_callback for rationale. */ >> if (current_context->all || current_context->thread_group != -1) >> { >> - int pid = 0; >> - struct cleanup *back_to = make_cleanup_restore_current_thread (); >> + scoped_restore_current_thread restore_thread; >> >> + int pid = 0; > > I'd keep "int pid" above the empty line. Honestly, I've been thinking more and more that the "empty line after declarations" no longer makes sense nowaways, given types with constructors, which I kind of read like a function call, and also with the desire to exercise the freedom (from a language perspective) to declare variables not-just-at-the-start-of-a-scope. In this case, the resulting code looked like this: if (current_context->all || current_context->thread_group != -1) { scoped_restore_current_thread restore_thread; int pid = 0; if (!current_context->all) { struct inferior *inf = find_inferior_id (current_context->thread_group); pid = inf->pid; } iterate_over_threads (proceed_thread_callback, &pid); } The idea was put the "pid" variable closer to where it's used, and the scoped_restore's ctor at the top of the scope, kind of mirroring the fact that the dtor wil run at the end of the scope. Anyway, it's certainly a very minor, arguable detail, so I've moved pid above the empty line. > >> @@ -2794,7 +2793,7 @@ mi_cmd_trace_frame_collected (const char >> *command, char **argv, int argc) >> >> /* This command only makes sense for the current frame, not the >> selected frame. */ >> - old_chain = make_cleanup_restore_current_thread (); > > The declaration of old_chain can be removed. Good catch, thanks. > > >> @@ -1571,77 +1575,53 @@ restore_selected_frame (struct frame_id >> a_frame_id, int frame_level) >> } >> } >> >> -/* Data used by the cleanup installed by >> - 'make_cleanup_restore_current_thread'. */ >> - >> -struct current_thread_cleanup >> -{ >> - thread_info *thread; >> - struct frame_id selected_frame_id; >> - int selected_frame_level; >> - int was_stopped; >> - inferior *inf; >> -}; >> - >> -static void >> -do_restore_current_thread_cleanup (void *arg) >> +scoped_restore_current_thread::~scoped_restore_current_thread () >> { >> - struct current_thread_cleanup *old = (struct current_thread_cleanup >> *) arg; >> - >> /* If an entry of thread_info was previously selected, it won't be >> deleted because we've increased its refcount. The thread >> represented >> by this thread_info entry may have already exited (due to normal >> exit, >> detach, etc), so the thread_info.state is THREAD_EXITED. */ >> - if (old->thread != NULL >> + if (m_thread != NULL >> /* If the previously selected thread belonged to a process that >> has >> in the mean time exited (or killed, detached, etc.), then don't >> revert >> back to it, but instead simply drop back to no thread selected. */ >> - && old->inf->pid != 0) >> - switch_to_thread (old->thread); >> + && m_inf->pid != 0) >> + switch_to_thread (m_thread); >> else >> { >> switch_to_no_thread (); >> - set_current_inferior (old->inf); >> + set_current_inferior (m_inf); >> } >> >> /* The running state of the originally selected thread may have >> changed, so we have to recheck it here. */ >> if (inferior_ptid != null_ptid >> - && old->was_stopped >> + && m_was_stopped >> && is_stopped (inferior_ptid) >> && target_has_registers >> && target_has_stack >> && target_has_memory) >> - restore_selected_frame (old->selected_frame_id, >> - old->selected_frame_level); >> -} >> - >> -static void >> -restore_current_thread_cleanup_dtor (void *arg) >> -{ >> - struct current_thread_cleanup *old = (struct current_thread_cleanup >> *) arg; >> + restore_selected_frame (m_selected_frame_id, >> m_selected_frame_level); >> >> - if (old->thread != NULL) >> - old->thread->decref (); >> - >> - old->inf->decref (); >> - xfree (old); >> + if (m_thread != NULL) >> + m_thread->decref (); >> + m_inf->decref (); >> } > > The cleanup did the decref in the dtor, I assume because we still want > to decref when the cleanup is discarded. If we ever add a release > method to this restore, we need to remember to do that. Yeah. > Or by that time we'll be using shared_ptr everywhere :). Maybe. :-) shared_ptr has the atomics cost, though. I'm kind of wary of the "just throw shared_ptr at the ownership problem" mindset, though it has its uses, of course. > > This does a bit more than 1:1 translating (calling find_thread_ptid > earlier, the assert). I think it's good, but can you put it in a > separate patch before this one, so it's not lost in the sea of > mechanical changes of the current patch? Good point. I've done that as a separate thread, here: https://sourceware.org/ml/gdb-patches/2017-05/msg00108.html And here's what I pushed for make_cleanup_restore_current_thread: From 5ed8105e02d0c6648b7faea9f4e941237b932717 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 4 May 2017 12:46:44 +0100 Subject: [PATCH] RAII-fy make_cleanup_restore_current_thread & friends After all the make_cleanup_restore_current_thread fixing, I thought I'd convert that and its relatives (which are all cleanups) to RAII classes. scoped_restore_current_pspace_and_thread was put in a separate file to avoid a circular dependency. Tested on x86-64 Fedora 23, native and gdbserver. gdb/ChangeLog: 2017-05-04 Pedro Alves * Makefile.in (SFILES): Add progspace-and-thread.c. (HFILES_NO_SRCDIR): Add progspace-and-thread.h. (COMMON_OBS): Add progspace-and-thread.o. * breakpoint.c: Include "progspace-and-thread.h". (update_inserted_breakpoint_locations) (insert_breakpoint_locations, create_longjmp_master_breakpoint): Use scoped_restore_current_pspace_and_thread. (create_std_terminate_master_breakpoint): Use scoped_restore_current_program_space. (remove_breakpoint): Use scoped_restore_current_pspace_and_thread. (print_breakpoint_location): Use scoped_restore_current_program_space. (bp_loc_is_permanent): Use scoped_restore_current_pspace_and_thread. (resolve_sal_pc): Use scoped_restore_current_pspace_and_thread. (download_tracepoint_locations): Use scoped_restore_current_pspace_and_thread. (breakpoint_re_set): Use scoped_restore_current_pspace_and_thread. * exec.c (exec_close_1): Use scoped_restore_current_program_space. (enum step_over_calls_kind): Moved from inferior.h. (class scoped_restore_current_thread): New class. * gdbthread.h (make_cleanup_restore_current_thread): Delete declaration. (scoped_restore_current_thread): New class. * infcmd.c: Include "common/gdb_optional.h". (continue_1, proceed_after_attach): Use scoped_restore_current_thread. (notice_new_inferior): Use scoped_restore_current_thread. * inferior.c: Include "progspace-and-thread.h". (restore_inferior, save_current_inferior): Delete. (add_inferior_command, clone_inferior_command): Use scoped_restore_current_pspace_and_thread. * inferior.h (scoped_restore_current_inferior): New class. * infrun.c: Include "progspace-and-thread.h" and "common/gdb_optional.h". (follow_fork_inferior): Use scoped_restore_current_pspace_and_thread. (scoped_restore_exited_inferior): New class. (handle_vfork_child_exec_or_exit): Use scoped_restore_exited_inferior, scoped_restore_current_pspace_and_thread, scoped_restore_current_thread and scoped_restore. (fetch_inferior_event): Use scoped_restore_current_thread. * linespec.c (decode_line_full, decode_line_1): Use scoped_restore_current_program_space. * mi/mi-main.c: Include "progspace-and-thread.h". (exec_continue): Use scoped_restore_current_thread. (mi_cmd_exec_run): Use scoped_restore_current_pspace_and_thread. (mi_cmd_trace_frame_collected): Use scoped_restore_current_thread. * proc-service.c (ps_pglobal_lookup): Use scoped_restore_current_program_space. * progspace-and-thread.c: New file. * progspace-and-thread.h: New file. * progspace.c (release_program_space, clone_program_space): Use scoped_restore_current_program_space. (restore_program_space, save_current_program_space) (save_current_space_and_thread): Delete. (switch_to_program_space_and_thread): Moved to progspace-and-thread.c. * progspace.h (save_current_program_space) (save_current_space_and_thread): Delete declarations. (scoped_restore_current_program_space): New class. * remote.c (remote_btrace_maybe_reopen): Use scoped_restore_current_thread. * symtab.c: Include "progspace-and-thread.h". (skip_prologue_sal): Use scoped_restore_current_pspace_and_thread. * thread.c (print_thread_info_1): Use scoped_restore_current_thread. (struct current_thread_cleanup): Delete. (do_restore_current_thread_cleanup) (restore_current_thread_cleanup_dtor): Rename/convert both to ... (scoped_restore_current_thread::~scoped_restore_current_thread): ... this new dtor. (make_cleanup_restore_current_thread): Rename/convert to ... (scoped_restore_current_thread::scoped_restore_current_thread): ... this new ctor. (thread_apply_all_command): Use scoped_restore_current_thread. (thread_apply_command): Use scoped_restore_current_thread. * tracepoint.c (tdump_command): Use scoped_restore_current_thread. * varobj.c (value_of_root_1): Use scoped_restore_current_thread. --- gdb/ChangeLog | 83 ++++++++++++++++ gdb/Makefile.in | 3 + gdb/breakpoint.c | 97 +++++++----------- gdb/exec.c | 15 ++- gdb/gdbthread.h | 22 ++++- gdb/infcmd.c | 31 ++---- gdb/inferior.c | 29 +----- gdb/inferior.h | 23 ++++- gdb/infrun.c | 65 ++++++------ gdb/linespec.c | 6 +- gdb/mi/mi-main.c | 12 +-- gdb/proc-service.c | 18 ++-- gdb/progspace-and-thread.c | 43 ++++++++ gdb/progspace-and-thread.h | 40 ++++++++ gdb/progspace.c | 80 +-------------- gdb/progspace.h | 32 +++--- gdb/remote.c | 5 +- gdb/symtab.c | 12 +-- gdb/thread.c | 241 ++++++++++++++++++++------------------------- gdb/tracepoint.c | 6 +- gdb/varobj.c | 5 +- 21 files changed, 453 insertions(+), 415 deletions(-) create mode 100644 gdb/progspace-and-thread.c create mode 100644 gdb/progspace-and-thread.h diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 7c48c3d..6e3a51e 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,88 @@ 2017-05-04 Pedro Alves + * Makefile.in (SFILES): Add progspace-and-thread.c. + (HFILES_NO_SRCDIR): Add progspace-and-thread.h. + (COMMON_OBS): Add progspace-and-thread.o. + * breakpoint.c: Include "progspace-and-thread.h". + (update_inserted_breakpoint_locations) + (insert_breakpoint_locations, create_longjmp_master_breakpoint): + Use scoped_restore_current_pspace_and_thread. + (create_std_terminate_master_breakpoint): Use + scoped_restore_current_program_space. + (remove_breakpoint): Use scoped_restore_current_pspace_and_thread. + (print_breakpoint_location): Use + scoped_restore_current_program_space. + (bp_loc_is_permanent): Use + scoped_restore_current_pspace_and_thread. + (resolve_sal_pc): Use scoped_restore_current_pspace_and_thread. + (download_tracepoint_locations): Use + scoped_restore_current_pspace_and_thread. + (breakpoint_re_set): Use scoped_restore_current_pspace_and_thread. + * exec.c (exec_close_1): Use scoped_restore_current_program_space. + (enum step_over_calls_kind): Moved from inferior.h. + (class scoped_restore_current_thread): New class. + * gdbthread.h (make_cleanup_restore_current_thread): Delete + declaration. + (scoped_restore_current_thread): New class. + * infcmd.c: Include "common/gdb_optional.h". + (continue_1, proceed_after_attach): Use + scoped_restore_current_thread. + (notice_new_inferior): Use scoped_restore_current_thread. + * inferior.c: Include "progspace-and-thread.h". + (restore_inferior, save_current_inferior): Delete. + (add_inferior_command, clone_inferior_command): Use + scoped_restore_current_pspace_and_thread. + * inferior.h (scoped_restore_current_inferior): New class. + * infrun.c: Include "progspace-and-thread.h" and + "common/gdb_optional.h". + (follow_fork_inferior): Use + scoped_restore_current_pspace_and_thread. + (scoped_restore_exited_inferior): New class. + (handle_vfork_child_exec_or_exit): Use + scoped_restore_exited_inferior, + scoped_restore_current_pspace_and_thread, + scoped_restore_current_thread and scoped_restore. + (fetch_inferior_event): Use scoped_restore_current_thread. + * linespec.c (decode_line_full, decode_line_1): Use + scoped_restore_current_program_space. + * mi/mi-main.c: Include "progspace-and-thread.h". + (exec_continue): Use scoped_restore_current_thread. + (mi_cmd_exec_run): Use scoped_restore_current_pspace_and_thread. + (mi_cmd_trace_frame_collected): Use scoped_restore_current_thread. + * proc-service.c (ps_pglobal_lookup): Use + scoped_restore_current_program_space. + * progspace-and-thread.c: New file. + * progspace-and-thread.h: New file. + * progspace.c (release_program_space, clone_program_space): Use + scoped_restore_current_program_space. + (restore_program_space, save_current_program_space) + (save_current_space_and_thread): Delete. + (switch_to_program_space_and_thread): Moved to + progspace-and-thread.c. + * progspace.h (save_current_program_space) + (save_current_space_and_thread): Delete declarations. + (scoped_restore_current_program_space): New class. + * remote.c (remote_btrace_maybe_reopen): Use + scoped_restore_current_thread. + * symtab.c: Include "progspace-and-thread.h". + (skip_prologue_sal): Use scoped_restore_current_pspace_and_thread. + * thread.c (print_thread_info_1): Use + scoped_restore_current_thread. + (struct current_thread_cleanup): Delete. + (do_restore_current_thread_cleanup) + (restore_current_thread_cleanup_dtor): Rename/convert both to ... + (scoped_restore_current_thread::~scoped_restore_current_thread): + ... this new dtor. + (make_cleanup_restore_current_thread): Rename/convert to ... + (scoped_restore_current_thread::scoped_restore_current_thread): + ... this new ctor. + (thread_apply_all_command): Use scoped_restore_current_thread. + (thread_apply_command): Use scoped_restore_current_thread. + * tracepoint.c (tdump_command): Use scoped_restore_current_thread. + * varobj.c (value_of_root_1): Use scoped_restore_current_thread. + +2017-05-04 Pedro Alves + * thread.c (make_cleanup_restore_current_thread): Move find_thread_ptid call before the is_stopped call. Assert that the thread is found. Replace is_stopped call by checking the thread's diff --git a/gdb/Makefile.in b/gdb/Makefile.in index cc93485..2f7d355 100644 --- a/gdb/Makefile.in +++ b/gdb/Makefile.in @@ -1148,6 +1148,7 @@ SFILES = \ probe.c \ proc-service.list \ progspace.c \ + progspace-and-thread.c \ prologue-value.c \ psymtab.c \ record.c \ @@ -1403,6 +1404,7 @@ HFILES_NO_SRCDIR = \ proc-utils.h \ procfs.h \ progspace.h \ + progspace-and-thread.h \ prologue-value.h \ psympriv.h \ psymtab.h \ @@ -1757,6 +1759,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \ printcmd.o \ probe.o \ progspace.o \ + progspace-and-thread.o \ prologue-value.o \ psymtab.o \ ptid.o \ diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e8d8d09..9d6a2f4 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -80,6 +80,7 @@ #include "mi/mi-common.h" #include "extension.h" #include +#include "progspace-and-thread.h" /* Enums for exception-handling support. */ enum exception_event_kind @@ -3065,7 +3066,7 @@ update_inserted_breakpoint_locations (void) there was an error. */ tmp_error_stream.puts ("Warning:\n"); - struct cleanup *cleanups = save_current_space_and_thread (); + scoped_restore_current_pspace_and_thread restore_pspace_thread; ALL_BP_LOCATIONS (bl, blp_tmp) { @@ -3101,8 +3102,6 @@ update_inserted_breakpoint_locations (void) target_terminal_ours_for_output (); error_stream (tmp_error_stream); } - - do_cleanups (cleanups); } /* Used when starting or continuing the program. */ @@ -3124,7 +3123,7 @@ insert_breakpoint_locations (void) there was an error. */ tmp_error_stream.puts ("Warning:\n"); - struct cleanup *cleanups = save_current_space_and_thread (); + scoped_restore_current_pspace_and_thread restore_pspace_thread; ALL_BP_LOCATIONS (bl, blp_tmp) { @@ -3202,8 +3201,6 @@ You may have requested too many hardware breakpoints/watchpoints.\n"); target_terminal_ours_for_output (); error_stream (tmp_error_stream); } - - do_cleanups (cleanups); } /* Used when the program stops. @@ -3489,9 +3486,8 @@ static void create_longjmp_master_breakpoint (void) { struct program_space *pspace; - struct cleanup *old_chain; - old_chain = save_current_program_space (); + scoped_restore_current_program_space restore_pspace; ALL_PSPACES (pspace) { @@ -3595,8 +3591,6 @@ create_longjmp_master_breakpoint (void) } } } - - do_cleanups (old_chain); } /* Create a master std::terminate breakpoint. */ @@ -3604,10 +3598,9 @@ static void create_std_terminate_master_breakpoint (void) { struct program_space *pspace; - struct cleanup *old_chain; const char *const func_name = "std::terminate()"; - old_chain = save_current_program_space (); + scoped_restore_current_program_space restore_pspace; ALL_PSPACES (pspace) { @@ -3652,8 +3645,6 @@ create_std_terminate_master_breakpoint (void) b->enable_state = bp_disabled; } } - - do_cleanups (old_chain); } /* Install a master breakpoint on the unwinder's debug hook. */ @@ -4077,9 +4068,6 @@ remove_breakpoint_1 (struct bp_location *bl, enum remove_bp_reason reason) static int remove_breakpoint (struct bp_location *bl) { - int ret; - struct cleanup *old_chain; - /* BL is never in moribund_locations by our callers. */ gdb_assert (bl->owner != NULL); @@ -4087,14 +4075,11 @@ remove_breakpoint (struct bp_location *bl) This should not ever happen. */ gdb_assert (bl->owner->type != bp_none); - old_chain = save_current_space_and_thread (); + scoped_restore_current_pspace_and_thread restore_pspace_thread; switch_to_program_space_and_thread (bl->pspace); - ret = remove_breakpoint_1 (bl, REMOVE_BREAKPOINT); - - do_cleanups (old_chain); - return ret; + return remove_breakpoint_1 (bl, REMOVE_BREAKPOINT); } /* Clear the "inserted" flag in all breakpoints. */ @@ -6129,7 +6114,8 @@ print_breakpoint_location (struct breakpoint *b, struct bp_location *loc) { struct ui_out *uiout = current_uiout; - struct cleanup *old_chain = save_current_program_space (); + + scoped_restore_current_program_space restore_pspace; if (loc != NULL && loc->shlib_disabled) loc = NULL; @@ -6194,8 +6180,6 @@ print_breakpoint_location (struct breakpoint *b, bp_location_condition_evaluator (loc)); uiout->text (")"); } - - do_cleanups (old_chain); } static const char * @@ -9071,9 +9055,6 @@ program_breakpoint_here_p (struct gdbarch *gdbarch, CORE_ADDR address) static int bp_loc_is_permanent (struct bp_location *loc) { - struct cleanup *cleanup; - int retval; - gdb_assert (loc != NULL); /* If we have a catchpoint or a watchpoint, just return 0. We should not @@ -9083,14 +9064,9 @@ bp_loc_is_permanent (struct bp_location *loc) if (!breakpoint_address_is_meaningful (loc->owner)) return 0; - cleanup = save_current_space_and_thread (); + scoped_restore_current_pspace_and_thread restore_pspace_thread; switch_to_program_space_and_thread (loc->pspace); - - retval = program_breakpoint_here_p (loc->gdbarch, loc->address); - - do_cleanups (cleanup); - - return retval; + return program_breakpoint_here_p (loc->gdbarch, loc->address); } /* Build a command list for the dprintf corresponding to the current @@ -9974,16 +9950,12 @@ resolve_sal_pc (struct symtab_and_line *sal) if we have line numbers but no functions (as can happen in assembly source). */ - struct bound_minimal_symbol msym; - struct cleanup *old_chain = save_current_space_and_thread (); - + scoped_restore_current_pspace_and_thread restore_pspace_thread; switch_to_program_space_and_thread (sal->pspace); - msym = lookup_minimal_symbol_by_pc (sal->pc); + bound_minimal_symbol msym = lookup_minimal_symbol_by_pc (sal->pc); if (msym.minsym) sal->section = MSYMBOL_OBJ_SECTION (msym.objfile, msym.minsym); - - do_cleanups (old_chain); } } } @@ -12177,10 +12149,9 @@ static void download_tracepoint_locations (void) { struct breakpoint *b; - struct cleanup *old_chain; enum tribool can_download_tracepoint = TRIBOOL_UNKNOWN; - old_chain = save_current_space_and_thread (); + scoped_restore_current_pspace_and_thread restore_pspace_thread; ALL_TRACEPOINTS (b) { @@ -12224,8 +12195,6 @@ download_tracepoint_locations (void) if (bp_location_downloaded) observer_notify_breakpoint_modified (b); } - - do_cleanups (old_chain); } /* Swap the insertion/duplication state between two locations. */ @@ -14506,32 +14475,32 @@ breakpoint_re_set (void) struct breakpoint *b, *b_tmp; enum language save_language; int save_input_radix; - struct cleanup *old_chain; save_language = current_language->la_language; save_input_radix = input_radix; - old_chain = save_current_space_and_thread (); - - /* Note: we must not try to insert locations until after all - breakpoints have been re-set. Otherwise, e.g., when re-setting - breakpoint 1, we'd insert the locations of breakpoint 2, which - hadn't been re-set yet, and thus may have stale locations. */ - ALL_BREAKPOINTS_SAFE (b, b_tmp) { - /* Format possible error msg. */ - char *message = xstrprintf ("Error in re-setting breakpoint %d: ", - b->number); - struct cleanup *cleanups = make_cleanup (xfree, message); - catch_errors (breakpoint_re_set_one, b, message, RETURN_MASK_ALL); - do_cleanups (cleanups); - } - set_language (save_language); - input_radix = save_input_radix; + scoped_restore_current_pspace_and_thread restore_pspace_thread; - jit_breakpoint_re_set (); + /* Note: we must not try to insert locations until after all + breakpoints have been re-set. Otherwise, e.g., when re-setting + breakpoint 1, we'd insert the locations of breakpoint 2, which + hadn't been re-set yet, and thus may have stale locations. */ - do_cleanups (old_chain); + ALL_BREAKPOINTS_SAFE (b, b_tmp) + { + /* Format possible error msg. */ + char *message = xstrprintf ("Error in re-setting breakpoint %d: ", + b->number); + struct cleanup *cleanups = make_cleanup (xfree, message); + catch_errors (breakpoint_re_set_one, b, message, RETURN_MASK_ALL); + do_cleanups (cleanups); + } + set_language (save_language); + input_radix = save_input_radix; + + jit_breakpoint_re_set (); + } create_overlay_event_breakpoint (); create_longjmp_master_breakpoint (); diff --git a/gdb/exec.c b/gdb/exec.c index 2c9a74b..05ecb1b 100644 --- a/gdb/exec.c +++ b/gdb/exec.c @@ -113,17 +113,14 @@ static void exec_close_1 (struct target_ops *self) { struct program_space *ss; - struct cleanup *old_chain; + scoped_restore_current_program_space restore_pspace; - old_chain = save_current_program_space (); ALL_PSPACES (ss) - { - set_current_program_space (ss); - clear_section_table (current_target_sections); - exec_close (); - } - - do_cleanups (old_chain); + { + set_current_program_space (ss); + clear_section_table (current_target_sections); + exec_close (); + } } void diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 68427e9..7bf2070 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -592,7 +592,27 @@ extern int print_thread_events; extern void print_thread_info (struct ui_out *uiout, char *requested_threads, int pid); -extern struct cleanup *make_cleanup_restore_current_thread (void); +/* Save/restore current inferior/thread/frame. */ + +class scoped_restore_current_thread +{ +public: + scoped_restore_current_thread (); + ~scoped_restore_current_thread (); + + /* Disable copy. */ + scoped_restore_current_thread + (const scoped_restore_current_thread &) = delete; + void operator= + (const scoped_restore_current_thread &) = delete; + +private: + thread_info *m_thread; + inferior *m_inf; + frame_id m_selected_frame_id; + int m_selected_frame_level; + bool m_was_stopped; +}; /* Returns a pointer into the thread_info corresponding to INFERIOR_PTID. INFERIOR_PTID *must* be in the thread list. */ diff --git a/gdb/infcmd.c b/gdb/infcmd.c index 56da56f..f42c6d1 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -58,6 +58,7 @@ #include "thread-fsm.h" #include "top.h" #include "interps.h" +#include "common/gdb_optional.h" /* Local functions: */ @@ -730,10 +731,10 @@ continue_1 (int all_threads) { /* Don't error out if the current thread is running, because there may be other stopped threads. */ - struct cleanup *old_chain; - /* Backup current thread and selected frame. */ - old_chain = make_cleanup_restore_current_thread (); + /* Backup current thread and selected frame and restore on scope + exit. */ + scoped_restore_current_thread restore_thread; iterate_over_threads (proceed_thread_callback, NULL); @@ -754,9 +755,6 @@ continue_1 (int all_threads) */ target_terminal_inferior (); } - - /* Restore selected ptid. */ - do_cleanups (old_chain); } else { @@ -2619,15 +2617,11 @@ proceed_after_attach (int pid) { /* Don't error out if the current thread is running, because there may be other stopped threads. */ - struct cleanup *old_chain; /* Backup current thread and selected frame. */ - old_chain = make_cleanup_restore_current_thread (); + scoped_restore_current_thread restore_thread; iterate_over_threads (proceed_after_attach_callback, &pid); - - /* Restore selected ptid. */ - do_cleanups (old_chain); } /* See inferior.h. */ @@ -2914,15 +2908,13 @@ attach_command (char *args, int from_tty) void notice_new_inferior (ptid_t ptid, int leave_running, int from_tty) { - struct cleanup* old_chain; - enum attach_post_wait_mode mode; - - old_chain = make_cleanup (null_cleanup, NULL); + enum attach_post_wait_mode mode + = leave_running ? ATTACH_POST_WAIT_RESUME : ATTACH_POST_WAIT_NOTHING; - mode = leave_running ? ATTACH_POST_WAIT_RESUME : ATTACH_POST_WAIT_NOTHING; + gdb::optional restore_thread; - if (!ptid_equal (inferior_ptid, null_ptid)) - make_cleanup_restore_current_thread (); + if (inferior_ptid != null_ptid) + restore_thread.emplace (); /* Avoid reading registers -- we haven't fetched the target description yet. */ @@ -2951,13 +2943,10 @@ notice_new_inferior (ptid_t ptid, int leave_running, int from_tty) add_inferior_continuation (attach_command_continuation, a, attach_command_continuation_free_args); - do_cleanups (old_chain); return; } attach_post_wait ("" /* args */, from_tty, mode); - - do_cleanups (old_chain); } /* diff --git a/gdb/inferior.c b/gdb/inferior.c index db23df9..0b655f4 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -35,6 +35,7 @@ #include "arch-utils.h" #include "target-descriptions.h" #include "readline/tilde.h" +#include "progspace-and-thread.h" void _initialize_inferiors (void); @@ -72,30 +73,6 @@ set_current_inferior (struct inferior *inf) current_inferior_ = inf; } -/* A cleanups callback, helper for save_current_program_space - below. */ - -static void -restore_inferior (void *arg) -{ - struct inferior *saved_inferior = (struct inferior *) arg; - - set_current_inferior (saved_inferior); -} - -/* Save the current program space so that it may be restored by a later - call to do_cleanups. Returns the struct cleanup pointer needed for - later doing the cleanup. */ - -struct cleanup * -save_current_inferior (void) -{ - struct cleanup *old_chain = make_cleanup (restore_inferior, - current_inferior_); - - return old_chain; -} - inferior::~inferior () { inferior *inf = this; @@ -862,7 +839,7 @@ add_inferior_command (char *args, int from_tty) } } - save_current_space_and_thread (); + scoped_restore_current_pspace_and_thread restore_pspace_thread; for (i = 0; i < copies; ++i) { @@ -942,7 +919,7 @@ clone_inferior_command (char *args, int from_tty) if (orginf == NULL) orginf = current_inferior (); - save_current_space_and_thread (); + scoped_restore_current_pspace_and_thread restore_pspace_thread; for (i = 0; i < copies; ++i) { diff --git a/gdb/inferior.h b/gdb/inferior.h index c6fb9d3..7ee92ed 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -540,7 +540,28 @@ extern int number_of_live_inferiors (void); (not cores, not executables, real live processes). */ extern int have_live_inferiors (void); -extern struct cleanup *save_current_inferior (void); +/* Save/restore the current inferior. */ + +class scoped_restore_current_inferior +{ +public: + scoped_restore_current_inferior () + : m_saved_inf (current_inferior ()) + {} + + ~scoped_restore_current_inferior () + { set_current_inferior (m_saved_inf); } + + /* Disable copy. */ + scoped_restore_current_inferior + (const scoped_restore_current_inferior &) = delete; + void operator= + (const scoped_restore_current_inferior &) = delete; + +private: + inferior *m_saved_inf; +}; + /* Traverse all inferiors. */ diff --git a/gdb/infrun.c b/gdb/infrun.c index fcafdc1..d0504de 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -64,6 +64,8 @@ #include "event-loop.h" #include "thread-fsm.h" #include "common/enum-flags.h" +#include "progspace-and-thread.h" +#include "common/gdb_optional.h" /* Prototypes for local functions */ @@ -487,7 +489,6 @@ holding the child stopped. Try \"set detach-on-fork\" or \ else { struct inferior *parent_inf, *child_inf; - struct cleanup *old_chain; /* Add process to GDB's tables. */ child_inf = add_inferior (ptid_get_pid (child_ptid)); @@ -498,7 +499,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \ child_inf->gdbarch = parent_inf->gdbarch; copy_inferior_target_desc_info (child_inf, parent_inf); - old_chain = save_current_space_and_thread (); + scoped_restore_current_pspace_and_thread restore_pspace_thread; inferior_ptid = child_ptid; add_thread (inferior_ptid); @@ -536,8 +537,6 @@ holding the child stopped. Try \"set detach-on-fork\" or \ required. */ solib_create_inferior_hook (0); } - - do_cleanups (old_chain); } if (has_vforked) @@ -895,6 +894,22 @@ proceed_after_vfork_done (struct thread_info *thread, return 0; } +/* Save/restore inferior_ptid, current program space and current + inferior. Only use this if the current context points at an exited + inferior (and therefore there's no current thread to save). */ +class scoped_restore_exited_inferior +{ +public: + scoped_restore_exited_inferior () + : m_saved_ptid (&inferior_ptid) + {} + +private: + scoped_restore_tmpl m_saved_ptid; + scoped_restore_current_program_space m_pspace; + scoped_restore_current_inferior m_inferior; +}; + /* Called whenever we notice an exec or exit event, to handle detaching or resuming a vfork parent. */ @@ -914,7 +929,6 @@ handle_vfork_child_exec_or_exit (int exec) if (inf->vfork_parent->pending_detach) { struct thread_info *tp; - struct cleanup *old_chain; struct program_space *pspace; struct address_space *aspace; @@ -922,16 +936,17 @@ handle_vfork_child_exec_or_exit (int exec) inf->vfork_parent->pending_detach = 0; + gdb::optional + maybe_restore_inferior; + gdb::optional + maybe_restore_thread; + + /* If we're handling a child exit, then inferior_ptid points + at the inferior's pid, not to a thread. */ if (!exec) - { - /* If we're handling a child exit, then inferior_ptid - points at the inferior's pid, not to a thread. */ - old_chain = save_inferior_ptid (); - save_current_program_space (); - save_current_inferior (); - } + maybe_restore_inferior.emplace (); else - old_chain = save_current_space_and_thread (); + maybe_restore_thread.emplace (); /* We're letting loose of the parent. */ tp = any_live_thread_of_process (inf->vfork_parent->pid); @@ -979,8 +994,6 @@ handle_vfork_child_exec_or_exit (int exec) /* Put it back. */ inf->pspace = pspace; inf->aspace = aspace; - - do_cleanups (old_chain); } else if (exec) { @@ -998,7 +1011,6 @@ handle_vfork_child_exec_or_exit (int exec) } else { - struct cleanup *old_chain; struct program_space *pspace; /* If this is a vfork child exiting, then the pspace and @@ -1010,10 +1022,11 @@ handle_vfork_child_exec_or_exit (int exec) go ahead and create a new one for this exiting inferior. */ - /* Switch to null_ptid, so that clone_program_space doesn't want - to read the selected frame of a dead process. */ - old_chain = save_inferior_ptid (); - inferior_ptid = null_ptid; + /* Switch to null_ptid while running clone_program_space, so + that clone_program_space doesn't want to read the + selected frame of a dead process. */ + scoped_restore restore_ptid + = make_scoped_restore (&inferior_ptid, null_ptid); /* This inferior is dead, so avoid giving the breakpoints module the option to write through to it (cloning a @@ -1028,10 +1041,6 @@ handle_vfork_child_exec_or_exit (int exec) inf->pspace = pspace; inf->aspace = pspace->aspace; - /* Put back inferior_ptid. We'll continue mourning this - inferior. */ - do_cleanups (old_chain); - resume_parent = inf->vfork_parent->pid; /* Break the bonds. */ inf->vfork_parent->vfork_child = NULL; @@ -1045,7 +1054,7 @@ handle_vfork_child_exec_or_exit (int exec) { /* If the user wanted the parent to be running, let it go free now. */ - struct cleanup *old_chain = make_cleanup_restore_current_thread (); + scoped_restore_current_thread restore_thread; if (debug_infrun) fprintf_unfiltered (gdb_stdlog, @@ -1053,8 +1062,6 @@ handle_vfork_child_exec_or_exit (int exec) resume_parent); iterate_over_threads (proceed_after_vfork_done, &resume_parent); - - do_cleanups (old_chain); } } } @@ -3889,12 +3896,14 @@ fetch_inferior_event (void *client_data) set_current_traceframe (-1); } + gdb::optional maybe_restore_thread; + if (non_stop) /* In non-stop mode, the user/frontend should not notice a thread switch due to internal events. Make sure we reverse to the user selected thread and frame after handling the event and running any breakpoint commands. */ - make_cleanup_restore_current_thread (); + maybe_restore_thread.emplace (); overlay_cache_invalid = 1; /* Flush target cache before starting to handle each event. Target diff --git a/gdb/linespec.c b/gdb/linespec.c index acf4900..4c076fe 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -2554,7 +2554,8 @@ decode_line_full (const struct event_location *location, int flags, search_pspace, default_symtab, default_line, canonical); cleanups = make_cleanup (linespec_parser_delete, &parser); - save_current_program_space (); + + scoped_restore_current_program_space restore_pspace; result = event_location_to_sals (&parser, location); state = PARSER_STATE (&parser); @@ -2616,7 +2617,8 @@ decode_line_1 (const struct event_location *location, int flags, search_pspace, default_symtab, default_line, NULL); cleanups = make_cleanup (linespec_parser_delete, &parser); - save_current_program_space (); + + scoped_restore_current_program_space restore_pspace; result = event_location_to_sals (&parser, location); diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index cb68fd6..bdc5dda 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -59,6 +59,7 @@ #include #include "run-time-clock.h" #include +#include "progspace-and-thread.h" enum { @@ -274,8 +275,8 @@ exec_continue (char **argv, int argc) See comment on infcmd.c:proceed_thread_callback for rationale. */ if (current_context->all || current_context->thread_group != -1) { + scoped_restore_current_thread restore_thread; int pid = 0; - struct cleanup *back_to = make_cleanup_restore_current_thread (); if (!current_context->all) { @@ -285,7 +286,6 @@ exec_continue (char **argv, int argc) pid = inf->pid; } iterate_over_threads (proceed_thread_callback, &pid); - do_cleanups (back_to); } else { @@ -468,10 +468,9 @@ mi_cmd_exec_run (const char *command, char **argv, int argc) if (current_context->all) { - struct cleanup *back_to = save_current_space_and_thread (); + scoped_restore_current_pspace_and_thread restore_pspace_thread; iterate_over_inferiors (run_one_inferior, &start_p); - do_cleanups (back_to); } else { @@ -2699,7 +2698,6 @@ print_variable_or_computed (const char *expression, enum print_values values) void mi_cmd_trace_frame_collected (const char *command, char **argv, int argc) { - struct cleanup *old_chain; struct bp_location *tloc; int stepping_frame; struct collection_list *clist; @@ -2762,7 +2760,7 @@ mi_cmd_trace_frame_collected (const char *command, char **argv, int argc) /* This command only makes sense for the current frame, not the selected frame. */ - old_chain = make_cleanup_restore_current_thread (); + scoped_restore_current_thread restore_thread; select_frame (get_current_frame ()); encode_actions (tloc, &tracepoint_list, &stepping_list); @@ -2918,8 +2916,6 @@ mi_cmd_trace_frame_collected (const char *command, char **argv, int argc) do_cleanups (list_cleanup); } - - do_cleanups (old_chain); } void diff --git a/gdb/proc-service.c b/gdb/proc-service.c index 415ba0a..5e5eee0 100644 --- a/gdb/proc-service.c +++ b/gdb/proc-service.c @@ -112,25 +112,19 @@ ps_err_e ps_pglobal_lookup (gdb_ps_prochandle_t ph, const char *obj, const char *name, psaddr_t *sym_addr) { - struct bound_minimal_symbol ms; - struct cleanup *old_chain = save_current_program_space (); struct inferior *inf = find_inferior_ptid (ph->ptid); - ps_err_e result; + + scoped_restore_current_program_space restore_pspace; set_current_program_space (inf->pspace); /* FIXME: kettenis/2000-09-03: What should we do with OBJ? */ - ms = lookup_minimal_symbol (name, NULL, NULL); + bound_minimal_symbol ms = lookup_minimal_symbol (name, NULL, NULL); if (ms.minsym == NULL) - result = PS_NOSYM; - else - { - *sym_addr = core_addr_to_ps_addr (BMSYMBOL_VALUE_ADDRESS (ms)); - result = PS_OK; - } + return PS_NOSYM; - do_cleanups (old_chain); - return result; + *sym_addr = core_addr_to_ps_addr (BMSYMBOL_VALUE_ADDRESS (ms)); + return PS_OK; } /* Read SIZE bytes from the target process PH at address ADDR and copy diff --git a/gdb/progspace-and-thread.c b/gdb/progspace-and-thread.c new file mode 100644 index 0000000..a48faa0 --- /dev/null +++ b/gdb/progspace-and-thread.c @@ -0,0 +1,43 @@ +/* Copyright (C) 2009-2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +#include "defs.h" +#include "progspace-and-thread.h" + +/* See progspace-and-thread.h */ + +void +switch_to_program_space_and_thread (program_space *pspace) +{ + inferior *inf = find_inferior_for_program_space (pspace); + + if (inf != NULL && inf->pid != 0) + { + thread_info *tp = any_live_thread_of_process (inf->pid); + + if (tp != NULL) + { + switch_to_thread (tp->ptid); + /* Switching thread switches pspace implicitly. We're + done. */ + return; + } + } + + switch_to_thread (null_ptid); + set_current_program_space (pspace); +} diff --git a/gdb/progspace-and-thread.h b/gdb/progspace-and-thread.h new file mode 100644 index 0000000..c68c79f --- /dev/null +++ b/gdb/progspace-and-thread.h @@ -0,0 +1,40 @@ +/* Copyright (C) 2009-2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + + +#ifndef PROGSPACE_AND_THREAD_H +#define PROGSPACE_AND_THREAD_H + +#include "progspace.h" +#include "gdbthread.h" + +/* Save/restore the current program space, thread, inferior and frame. + Use this when you need to call + switch_to_program_space_and_thread. */ + +class scoped_restore_current_pspace_and_thread +{ + scoped_restore_current_program_space m_restore_pspace; + scoped_restore_current_thread m_restore_thread; +}; + +/* Switches full context to program space PSPACE. Switches to the + first thread found bound to PSPACE, giving preference to the + current thread, if there's one and it isn't executing. */ +void switch_to_program_space_and_thread (program_space *pspace); + +#endif diff --git a/gdb/progspace.c b/gdb/progspace.c index b37701e..f6602b7b 100644 --- a/gdb/progspace.c +++ b/gdb/progspace.c @@ -156,10 +156,10 @@ add_program_space (struct address_space *aspace) static void release_program_space (struct program_space *pspace) { - struct cleanup *old_chain = save_current_program_space (); - gdb_assert (pspace != current_program_space); + scoped_restore_current_program_space restore_pspace; + set_current_program_space (pspace); breakpoint_program_space_exit (pspace); @@ -173,8 +173,6 @@ release_program_space (struct program_space *pspace) /* Discard any data modules have associated with the PSPACE. */ program_space_free_data (pspace); xfree (pspace); - - do_cleanups (old_chain); } /* Copies program space SRC to DEST. Copies the main executable file, @@ -183,9 +181,7 @@ release_program_space (struct program_space *pspace) struct program_space * clone_program_space (struct program_space *dest, struct program_space *src) { - struct cleanup *old_chain; - - old_chain = save_current_program_space (); + scoped_restore_current_program_space restore_pspace; set_current_program_space (dest); @@ -195,7 +191,6 @@ clone_program_space (struct program_space *dest, struct program_space *src) if (src->symfile_object_file != NULL) symbol_file_add_main (objfile_name (src->symfile_object_file), 0); - do_cleanups (old_chain); return dest; } @@ -217,30 +212,6 @@ set_current_program_space (struct program_space *pspace) reinit_frame_cache (); } -/* A cleanups callback, helper for save_current_program_space - below. */ - -static void -restore_program_space (void *arg) -{ - struct program_space *saved_pspace = (struct program_space *) arg; - - set_current_program_space (saved_pspace); -} - -/* Save the current program space so that it may be restored by a later - call to do_cleanups. Returns the struct cleanup pointer needed for - later doing the cleanup. */ - -struct cleanup * -save_current_program_space (void) -{ - struct cleanup *old_chain = make_cleanup (restore_program_space, - current_program_space); - - return old_chain; -} - /* Returns true iff there's no inferior bound to PSPACE. */ int @@ -447,51 +418,6 @@ update_address_spaces (void) inf->aspace = inf->pspace->aspace; } -/* Save the current program space so that it may be restored by a later - call to do_cleanups. Returns the struct cleanup pointer needed for - later doing the cleanup. */ - -struct cleanup * -save_current_space_and_thread (void) -{ - struct cleanup *old_chain; - - /* If restoring to null thread, we need to restore the pspace as - well, hence, we need to save the current program space first. */ - old_chain = save_current_program_space (); - /* There's no need to save the current inferior here. - That is handled by make_cleanup_restore_current_thread. */ - make_cleanup_restore_current_thread (); - - return old_chain; -} - -/* See progspace.h */ - -void -switch_to_program_space_and_thread (struct program_space *pspace) -{ - struct inferior *inf; - - inf = find_inferior_for_program_space (pspace); - if (inf != NULL && inf->pid != 0) - { - struct thread_info *tp; - - tp = any_live_thread_of_process (inf->pid); - if (tp != NULL) - { - switch_to_thread (tp->ptid); - /* Switching thread switches pspace implicitly. We're - done. */ - return; - } - } - - switch_to_thread (null_ptid); - set_current_program_space (pspace); -} - /* See progspace.h. */ diff --git a/gdb/progspace.h b/gdb/progspace.h index 8925f56..ec1f482 100644 --- a/gdb/progspace.h +++ b/gdb/progspace.h @@ -251,11 +251,6 @@ extern int program_space_empty_p (struct program_space *pspace); extern struct program_space *clone_program_space (struct program_space *dest, struct program_space *src); -/* Save the current program space so that it may be restored by a later - call to do_cleanups. Returns the struct cleanup pointer needed for - later doing the cleanup. */ -extern struct cleanup *save_current_program_space (void); - /* Sets PSPACE as the current program space. This is usually used instead of set_current_space_and_thread when the current thread/inferior is not important for the operations that follow. @@ -266,14 +261,27 @@ extern struct cleanup *save_current_program_space (void); space. */ extern void set_current_program_space (struct program_space *pspace); -/* Saves the current thread (may be null), frame and program space in - the current cleanup chain. */ -extern struct cleanup *save_current_space_and_thread (void); +/* Save/restore the current program space. */ + +class scoped_restore_current_program_space +{ +public: + scoped_restore_current_program_space () + : m_saved_pspace (current_program_space) + {} + + ~scoped_restore_current_program_space () + { set_current_program_space (m_saved_pspace); } + + /* Disable copy. */ + scoped_restore_current_program_space + (const scoped_restore_current_program_space &) = delete; + void operator= + (const scoped_restore_current_program_space &) = delete; -/* Switches full context to program space PSPACE. Switches to the - first thread found bound to PSPACE, giving preference to the - current thread, if there's one and it isn't executing. */ -extern void switch_to_program_space_and_thread (struct program_space *pspace); +private: + program_space *m_saved_pspace; +}; /* Create a new address space object, and add it to the list. */ extern struct address_space *new_address_space (void); diff --git a/gdb/remote.c b/gdb/remote.c index 2cd9850..de52ac3 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -13210,12 +13210,12 @@ static void remote_btrace_maybe_reopen (void) { struct remote_state *rs = get_remote_state (); - struct cleanup *cleanup; struct thread_info *tp; int btrace_target_pushed = 0; int warned = 0; - cleanup = make_cleanup_restore_current_thread (); + scoped_restore_current_thread restore_thread; + ALL_NON_EXITED_THREADS (tp) { set_general_thread (tp->ptid); @@ -13255,7 +13255,6 @@ remote_btrace_maybe_reopen (void) tp->btrace.target->ptid = tp->ptid; tp->btrace.target->conf = rs->btrace_config; } - do_cleanups (cleanup); } /* Enable branch tracing. */ diff --git a/gdb/symtab.c b/gdb/symtab.c index 20ef76d..22d81fa 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -61,6 +61,7 @@ #include "parser-defs.h" #include "completer.h" +#include "progspace-and-thread.h" /* Forward declarations for local functions. */ @@ -3551,7 +3552,6 @@ skip_prologue_sal (struct symtab_and_line *sal) { struct symbol *sym; struct symtab_and_line start_sal; - struct cleanup *old_chain; CORE_ADDR pc, saved_pc; struct obj_section *section; const char *name; @@ -3564,7 +3564,8 @@ skip_prologue_sal (struct symtab_and_line *sal) if (sal->explicit_pc) return; - old_chain = save_current_space_and_thread (); + scoped_restore_current_pspace_and_thread restore_pspace_thread; + switch_to_program_space_and_thread (sal->pspace); sym = find_pc_sect_function (sal->pc, sal->section); @@ -3583,10 +3584,7 @@ skip_prologue_sal (struct symtab_and_line *sal) = lookup_minimal_symbol_by_pc_section (sal->pc, sal->section); if (msymbol.minsym == NULL) - { - do_cleanups (old_chain); - return; - } + return; objfile = msymbol.objfile; pc = BMSYMBOL_VALUE_ADDRESS (msymbol); @@ -3678,8 +3676,6 @@ skip_prologue_sal (struct symtab_and_line *sal) start_sal = find_pc_sect_line (pc, section, 0); } - do_cleanups (old_chain); - /* If we're already past the prologue, leave SAL unchanged. Otherwise forward SAL to the end of the prologue. */ if (sal->pc >= pc) diff --git a/gdb/thread.c b/gdb/thread.c index fce37c5..3cf1b94 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1240,7 +1240,6 @@ print_thread_info_1 (struct ui_out *uiout, char *requested_threads, { struct thread_info *tp; ptid_t current_ptid; - struct cleanup *old_chain; const char *extra_info, *name, *target_id; struct inferior *inf; int default_inf_num = current_inferior ()->num; @@ -1248,8 +1247,7 @@ print_thread_info_1 (struct ui_out *uiout, char *requested_threads, update_thread_list (); current_ptid = inferior_ptid; - /* We'll be switching threads temporarily. */ - old_chain = make_cleanup_restore_current_thread (); + struct cleanup *old_chain = make_cleanup (null_cleanup, NULL); /* For backward compatibility, we make a list for MI. A table is preferable for the CLI, though, because it shows table @@ -1296,98 +1294,104 @@ print_thread_info_1 (struct ui_out *uiout, char *requested_threads, uiout->table_body (); } - ALL_THREADS_BY_INFERIOR (inf, tp) - { - int core; + /* We'll be switching threads temporarily. */ + { + scoped_restore_current_thread restore_thread; - if (!should_print_thread (requested_threads, default_inf_num, - global_ids, pid, tp)) - continue; + ALL_THREADS_BY_INFERIOR (inf, tp) + { + int core; - ui_out_emit_tuple tuple_emitter (uiout, NULL); + if (!should_print_thread (requested_threads, default_inf_num, + global_ids, pid, tp)) + continue; - if (!uiout->is_mi_like_p ()) - { - if (tp->ptid == current_ptid) - uiout->field_string ("current", "*"); - else - uiout->field_skip ("current"); - } + ui_out_emit_tuple tuple_emitter (uiout, NULL); - if (!uiout->is_mi_like_p ()) - uiout->field_string ("id-in-tg", print_thread_id (tp)); + if (!uiout->is_mi_like_p ()) + { + if (tp->ptid == current_ptid) + uiout->field_string ("current", "*"); + else + uiout->field_skip ("current"); + } - if (show_global_ids || uiout->is_mi_like_p ()) - uiout->field_int ("id", tp->global_num); + if (!uiout->is_mi_like_p ()) + uiout->field_string ("id-in-tg", print_thread_id (tp)); - /* For the CLI, we stuff everything into the target-id field. - This is a gross hack to make the output come out looking - correct. The underlying problem here is that ui-out has no - way to specify that a field's space allocation should be - shared by several fields. For MI, we do the right thing - instead. */ + if (show_global_ids || uiout->is_mi_like_p ()) + uiout->field_int ("id", tp->global_num); - target_id = target_pid_to_str (tp->ptid); - extra_info = target_extra_thread_info (tp); - name = tp->name ? tp->name : target_thread_name (tp); + /* For the CLI, we stuff everything into the target-id field. + This is a gross hack to make the output come out looking + correct. The underlying problem here is that ui-out has no + way to specify that a field's space allocation should be + shared by several fields. For MI, we do the right thing + instead. */ - if (uiout->is_mi_like_p ()) - { - uiout->field_string ("target-id", target_id); - if (extra_info) - uiout->field_string ("details", extra_info); - if (name) - uiout->field_string ("name", name); - } - else - { - struct cleanup *str_cleanup; - char *contents; - - if (extra_info && name) - contents = xstrprintf ("%s \"%s\" (%s)", target_id, - name, extra_info); - else if (extra_info) - contents = xstrprintf ("%s (%s)", target_id, extra_info); - else if (name) - contents = xstrprintf ("%s \"%s\"", target_id, name); - else - contents = xstrdup (target_id); - str_cleanup = make_cleanup (xfree, contents); + target_id = target_pid_to_str (tp->ptid); + extra_info = target_extra_thread_info (tp); + name = tp->name ? tp->name : target_thread_name (tp); - uiout->field_string ("target-id", contents); - do_cleanups (str_cleanup); - } + if (uiout->is_mi_like_p ()) + { + uiout->field_string ("target-id", target_id); + if (extra_info) + uiout->field_string ("details", extra_info); + if (name) + uiout->field_string ("name", name); + } + else + { + struct cleanup *str_cleanup; + char *contents; + + if (extra_info && name) + contents = xstrprintf ("%s \"%s\" (%s)", target_id, + name, extra_info); + else if (extra_info) + contents = xstrprintf ("%s (%s)", target_id, extra_info); + else if (name) + contents = xstrprintf ("%s \"%s\"", target_id, name); + else + contents = xstrdup (target_id); + str_cleanup = make_cleanup (xfree, contents); + + uiout->field_string ("target-id", contents); + do_cleanups (str_cleanup); + } - if (tp->state == THREAD_RUNNING) - uiout->text ("(running)\n"); - else - { - /* The switch below puts us at the top of the stack (leaf - frame). */ - switch_to_thread (tp->ptid); - print_stack_frame (get_selected_frame (NULL), - /* For MI output, print frame level. */ - uiout->is_mi_like_p (), - LOCATION, 0); - } + if (tp->state == THREAD_RUNNING) + uiout->text ("(running)\n"); + else + { + /* The switch below puts us at the top of the stack (leaf + frame). */ + switch_to_thread (tp->ptid); + print_stack_frame (get_selected_frame (NULL), + /* For MI output, print frame level. */ + uiout->is_mi_like_p (), + LOCATION, 0); + } - if (uiout->is_mi_like_p ()) - { - const char *state = "stopped"; + if (uiout->is_mi_like_p ()) + { + const char *state = "stopped"; - if (tp->state == THREAD_RUNNING) - state = "running"; - uiout->field_string ("state", state); - } + if (tp->state == THREAD_RUNNING) + state = "running"; + uiout->field_string ("state", state); + } - core = target_core_of_thread (tp->ptid); - if (uiout->is_mi_like_p () && core != -1) - uiout->field_int ("core", core); + core = target_core_of_thread (tp->ptid); + if (uiout->is_mi_like_p () && core != -1) + uiout->field_int ("core", core); } - /* Restores the current thread and the frame selected before - the "info threads" command. */ + /* This end scope restores the current thread and the frame + selected before the "info threads" command. */ + } + do_cleanups (old_chain); if (pid == -1 && requested_threads == NULL) @@ -1559,70 +1563,43 @@ restore_selected_frame (struct frame_id a_frame_id, int frame_level) } } -/* Data used by the cleanup installed by - 'make_cleanup_restore_current_thread'. */ - -struct current_thread_cleanup -{ - thread_info *thread; - struct frame_id selected_frame_id; - int selected_frame_level; - int was_stopped; - inferior *inf; -}; - -static void -do_restore_current_thread_cleanup (void *arg) +scoped_restore_current_thread::~scoped_restore_current_thread () { - struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg; - /* If an entry of thread_info was previously selected, it won't be deleted because we've increased its refcount. The thread represented by this thread_info entry may have already exited (due to normal exit, detach, etc), so the thread_info.state is THREAD_EXITED. */ - if (old->thread != NULL + if (m_thread != NULL /* If the previously selected thread belonged to a process that has in the mean time exited (or killed, detached, etc.), then don't revert back to it, but instead simply drop back to no thread selected. */ - && old->inf->pid != 0) - switch_to_thread (old->thread); + && m_inf->pid != 0) + switch_to_thread (m_thread); else { switch_to_no_thread (); - set_current_inferior (old->inf); + set_current_inferior (m_inf); } /* The running state of the originally selected thread may have changed, so we have to recheck it here. */ if (inferior_ptid != null_ptid - && old->was_stopped + && m_was_stopped && is_stopped (inferior_ptid) && target_has_registers && target_has_stack && target_has_memory) - restore_selected_frame (old->selected_frame_id, - old->selected_frame_level); -} + restore_selected_frame (m_selected_frame_id, m_selected_frame_level); -static void -restore_current_thread_cleanup_dtor (void *arg) -{ - struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg; - - if (old->thread != NULL) - old->thread->decref (); - - old->inf->decref (); - xfree (old); + if (m_thread != NULL) + m_thread->decref (); + m_inf->decref (); } -struct cleanup * -make_cleanup_restore_current_thread (void) +scoped_restore_current_thread::scoped_restore_current_thread () { - struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup); - - old->thread = NULL; - old->inf = current_inferior (); + m_thread = NULL; + m_inf = current_inferior (); if (inferior_ptid != null_ptid) { @@ -1631,8 +1608,8 @@ make_cleanup_restore_current_thread (void) gdb_assert (tp != NULL); - old->was_stopped = tp->state == THREAD_STOPPED; - if (old->was_stopped + m_was_stopped = tp->state == THREAD_STOPPED; + if (m_was_stopped && target_has_registers && target_has_stack && target_has_memory) @@ -1647,17 +1624,14 @@ make_cleanup_restore_current_thread (void) else frame = NULL; - old->selected_frame_id = get_frame_id (frame); - old->selected_frame_level = frame_relative_level (frame); + m_selected_frame_id = get_frame_id (frame); + m_selected_frame_level = frame_relative_level (frame); tp->incref (); - old->thread = tp; + m_thread = tp; } - old->inf->incref (); - - return make_cleanup_dtor (do_restore_current_thread_cleanup, old, - restore_current_thread_cleanup_dtor); + m_inf->incref (); } /* See gdbthread.h. */ @@ -1727,7 +1701,6 @@ tp_array_compar (const thread_info *a, const thread_info *b) static void thread_apply_all_command (char *cmd, int from_tty) { - struct cleanup *old_chain; char *saved_cmd; tp_array_compar_ascending = false; @@ -1743,8 +1716,6 @@ thread_apply_all_command (char *cmd, int from_tty) update_thread_list (); - old_chain = make_cleanup_restore_current_thread (); - /* Save a copy of the command in case it is clobbered by execute_command. */ saved_cmd = xstrdup (cmd); @@ -1778,6 +1749,8 @@ thread_apply_all_command (char *cmd, int from_tty) std::sort (thr_list_cpy.begin (), thr_list_cpy.end (), tp_array_compar); + scoped_restore_current_thread restore_thread; + for (thread_info *thr : thr_list_cpy) if (thread_alive (thr)) { @@ -1791,8 +1764,6 @@ thread_apply_all_command (char *cmd, int from_tty) strcpy (cmd, saved_cmd); } } - - do_cleanups (old_chain); } /* Implementation of the "thread apply" command. */ @@ -1831,7 +1802,7 @@ thread_apply_command (char *tidlist, int from_tty) saved_cmd = xstrdup (cmd); old_chain = make_cleanup (xfree, saved_cmd); - make_cleanup_restore_current_thread (); + scoped_restore_current_thread restore_thread; parser.init (tidlist, current_inferior ()->num); while (!parser.finished () && parser.cur_tok () < cmd) diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c index 808afde..98213cf 100644 --- a/gdb/tracepoint.c +++ b/gdb/tracepoint.c @@ -2925,7 +2925,6 @@ tdump_command (char *args, int from_tty) { int stepping_frame = 0; struct bp_location *loc; - struct cleanup *old_chain; struct command_line *actions; /* This throws an error is not inspecting a trace frame. */ @@ -2936,14 +2935,13 @@ tdump_command (char *args, int from_tty) /* This command only makes sense for the current frame, not the selected frame. */ - old_chain = make_cleanup_restore_current_thread (); + scoped_restore_current_thread restore_thread; + select_frame (get_current_frame ()); actions = all_tracepoint_actions_and_cleanup (loc->owner); trace_dump_actions (actions, 0, stepping_frame, from_tty); - - do_cleanups (old_chain); } /* Encode a piece of a tracepoint's source-level definition in a form diff --git a/gdb/varobj.c b/gdb/varobj.c index 5f21d84..7bd549d 100644 --- a/gdb/varobj.c +++ b/gdb/varobj.c @@ -2218,14 +2218,13 @@ value_of_root_1 (struct varobj **var_handle) struct value *new_val = NULL; struct varobj *var = *var_handle; int within_scope = 0; - struct cleanup *back_to; /* Only root variables can be updated... */ if (!is_root_p (var)) /* Not a root var. */ return NULL; - back_to = make_cleanup_restore_current_thread (); + scoped_restore_current_thread restore_thread; /* Determine whether the variable is still around. */ if (var->root->valid_block == NULL || var->root->floating) @@ -2264,8 +2263,6 @@ value_of_root_1 (struct varobj **var_handle) END_CATCH } - do_cleanups (back_to); - return new_val; }