From patchwork Wed Jan 16 23:10:23 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 31082 Received: (qmail 52430 invoked by alias); 16 Jan 2019 23:10:39 -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 52393 invoked by uid 89); 16 Jan 2019 23:10:38 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, TIME_LIMIT_EXCEEDED autolearn=unavailable version=3.3.2 spammy=revealed, prompt, pcs, Guard 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; Wed, 16 Jan 2019 23:10:28 +0000 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 067832C7CD; Wed, 16 Jan 2019 23:10:27 +0000 (UTC) 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 55E8D19748; Wed, 16 Jan 2019 23:10:25 +0000 (UTC) Subject: Re: [PATCH 00/12] remove some cleanups using a cleanup function To: Tom Tromey , Andrew Burgess References: <20190109033426.16062-1-tom@tromey.com> <961d9501-23e6-9adb-a11b-da41702c4fa0@redhat.com> <20190115094157.GP3456@embecosm.com> <87ef9dttfl.fsf@tromey.com> <1014fa7f-bbce-cbea-f54f-480373299809@redhat.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Wed, 16 Jan 2019 23:10:23 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1014fa7f-bbce-cbea-f54f-480373299809@redhat.com> On 01/15/2019 11:43 PM, Pedro Alves wrote: > On 01/15/2019 11:03 PM, Tom Tromey wrote: >>>>>>> "Andrew" == Andrew Burgess writes: >> >> Andrew> Maybe there's some other reason why scoped_finish_thread_state is >> Andrew> different, in which case I apologise for wasting everyone's time, but >> Andrew> right now it appears to me that scoped_finish_thread_state is no >> Andrew> different to cleanup_function, it's just used more. >> >> FWIW I don't think it's a waste of time at all. There's no particular >> rush for these patches and I think it's more valuable for us to agree on >> what we'd like the result to look like than it is to land them quickly. > > Definitely agreed. Not a waste at all! > > I've been playing with this today, and I have a different > implementation of Andrew's class that allows writing: > > using delete_longjmp_breakpoint_cleanup > = forward_cleanup_function delete_longjmp_breakpoint>; > > Or, with a macro to eliminate redundancy: > > using delete_longjmp_breakpoint_cleanup > = FORWARD_CLEANUP_FUNCTION (delete_longjmp_breakpoint); > > Naming up in the air, I just picked that as straw man. See patch below. Since cleanup_function turned into scope_exit, this new class became forward_scope_exit, because it's a "forwarding" version of scope_exit? I'm really not that sure about that name... wrap_scope_exit came to mind too. Or maybe call it cleanup_function? This version builds on top of std::bind, which eliminates the need for manually storing the args in the tuple and the recursive call_function_on_tuple template code. As shown above, it also doesn't require manually passing the wrapped-function's parameter types to the template, they're extracted automatically. > >> >> Andrew> I think if we're going to put in a generic solution (which I think is >> Andrew> a good thing) then we should either, make sure we understand why >> Andrew> scoped_finish_thread_state is different (and what the rules are for >> Andrew> when to use the generic, and when to create a class), or, make sure >> Andrew> the generic is suitable to replace scoped_finish_thread_state. >> >> Andrew> (I'm not trying to pick on scoped_finish_thread_state, it was just the >> Andrew> first example I found when originally replying to Tom.) >> >> Maybe I was just making too big a deal out of it, but my thinking was >> that writing the finish_thread_state call at each spot would be bad, >> since it would be multiple copies of the same thing. But, maybe it is >> actually no big deal? Yeah, I think that writing SCOPE_EXIT { finish_thread_state (ptid); }; in the multiple places wouldn't be a big deal. However, I found that Andrew's idea is most useful for the gdb::optional case, since in that case we need the cleanup's type upfront. The finish_thread_state cleanup is in that situation, so I converted it to a forward_scope_exit. >> >> Using a template, as Pedro suggested, would remove some of the ugliness >> from the series. Stuff like this: >> >> + auto do_invalidate >> + = [=] () >> + { >> + this->invalidate (regnum); >> + }; >> + cleanup_function invalidator (do_invalidate); >> >> Could instead just be: >> >> SCOPE_EXIT { this->invalidate (regnum); } >> >> ... assuming we like SCOPE_EXIT (to me it seems reasonable enough). In this particular case, it can't be SCOPE_EXIT because we need to be able to cancel the scope, i.e., we need to scope_exit's name. So we end up with: auto invalidator = make_scope_exit ([&] { this->invalidate (regnum); }); I guess we should add an alternative macro like to be used like: auto invalidator = NAMED_SCOPE_EXIT { this->invalidate (regnum); }; I didn't do that though. Doesn't save that much? Ideally we'd find a way to implement SCOPE_FAILURE/SCOPE_SUCCESS in C++11 instead... >> >> Anyway, I tend to think we should simply copy the scope_exit paper. If >> it's accepted into C++ then someday we can just remove the gdb variant. >> >> Let me know if you agree; if so I can implement this. >> > I've also played with the template idea, basically implemented > scope_exit / make_scope_exit. Seems to work nicely. I did this more fully today, following the description in the paper. I think this is the first time I had found a use for a function-try-block, see scope_exit ctor... > > I hadn't done the SCOPE_EXIT macro though, not sure it is worth > it to have yet another way to write these things (thinking about > newcomers' cognitive load, having to learn all the different > things) -- of all the make_scope_exit calls I have, most either > take the form: > > auto cleanup = make_scope_exit (function); > > i.e., are passed a function pointer, can do without a > lambda, and/or need access to the scope_exit object to > cancel it. But I can give it a try for sure. It may > be clearer code to standardize writing: > > auto cleanup = make_scope_exit (function); > cleanup.release (); > > when the cleanup may need to be canceled and > > SCOPE_EXIT { function (); } > > when it doesn't? I did this. > > I won't be able to finish this today (I'd like to clean up a couple hacks > here and there), but I'll post something tomorrow so we can all see > and decide a way forward. I remembered struct on_exit I had added to gdbarch-selftests.c, thinking of a generic SCOPE_EXIT at the time. :-) So I converted that too now. This revealed that the ESC macro in common/preprocessor.h conflicts with a macro of the same name in readline.h... That should go in a separate patch. Since both scope_exit and forward_scope_exit share some functionality, I put the shared code/structure in a common base class, using CRTP. Here is patch with everything together. WDYT? From c616fefa268155eea243dedfb6ff54e133ee33f9 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 16 Jan 2019 20:45:52 +0000 Subject: [PATCH] scope_exit --- gdb/breakpoint.c | 27 ++--- gdb/common/forward-scope-exit.h | 118 +++++++++++++++++++++ gdb/common/preprocessor.h | 2 +- gdb/common/scope-exit.h | 185 ++++++++++++++++++++++++++++++++ gdb/common/valid-expr.h | 18 ++-- gdb/gdbarch-selftests.c | 8 +- gdb/gdbthread.h | 28 +---- gdb/infcall.c | 13 +-- gdb/infcmd.c | 13 +-- gdb/inferior.h | 4 +- gdb/infrun.c | 230 ++++++++++++++++++---------------------- gdb/linux-nat.c | 18 +--- gdb/regcache.c | 34 +----- gdb/symfile.c | 27 +++-- gdb/top.c | 8 +- gdb/ui-out.h | 8 +- gdb/utils.c | 17 --- gdb/utils.h | 1 - 18 files changed, 463 insertions(+), 296 deletions(-) create mode 100644 gdb/common/forward-scope-exit.h create mode 100644 gdb/common/scope-exit.h diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 2ab8a76326..7f6cfc4828 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -83,6 +83,7 @@ #include "progspace-and-thread.h" #include "common/array-view.h" #include "common/gdb_optional.h" +#include "common/scope-exit.h" /* Enums for exception-handling support. */ enum exception_event_kind @@ -4468,7 +4469,7 @@ get_bpstat_thread () void bpstat_do_actions (void) { - struct cleanup *cleanup_if_error = make_bpstat_clear_actions_cleanup (); + auto cleanup_if_error = make_scope_exit (bpstat_clear_actions); thread_info *tp; /* Do any commands attached to breakpoint we are stopped at. */ @@ -4482,7 +4483,7 @@ bpstat_do_actions (void) break; } - discard_cleanups (cleanup_if_error); + cleanup_if_error.release (); } /* Print out the (old or new) value associated with a watchpoint. */ @@ -9230,7 +9231,6 @@ create_breakpoint (struct gdbarch *gdbarch, unsigned flags) { struct linespec_result canonical; - struct cleanup *bkpt_chain = NULL; int pending = 0; int task = 0; int prev_bkpt_count = breakpoint_count; @@ -9280,12 +9280,6 @@ create_breakpoint (struct gdbarch *gdbarch, if (!pending && canonical.lsals.empty ()) return 0; - /* ----------------------------- SNIP ----------------------------- - Anything added to the cleanup chain beyond this point is assumed - to be part of a breakpoint. If the breakpoint create succeeds - then the memory is not reclaimed. */ - bkpt_chain = make_cleanup (null_cleanup, 0); - /* Resolve all line numbers to PC's and verify that the addresses are ok for the target. */ if (!pending) @@ -9384,10 +9378,6 @@ create_breakpoint (struct gdbarch *gdbarch, prev_breakpoint_count = prev_bkpt_count; } - /* That's it. Discard the cleanups for data inserted into the - breakpoint. */ - discard_cleanups (bkpt_chain); - /* error call may happen here - have BKPT_CHAIN already discarded. */ update_global_location_list (UGLL_MAY_INSERT); @@ -11073,7 +11063,6 @@ until_break_command (const char *arg, int from_tty, int anywhere) struct gdbarch *frame_gdbarch; struct frame_id stack_frame_id; struct frame_id caller_frame_id; - struct cleanup *old_chain; int thread; struct thread_info *tp; struct until_break_fsm *sm; @@ -11106,8 +11095,6 @@ until_break_command (const char *arg, int from_tty, int anywhere) tp = inferior_thread (); thread = tp->global_num; - old_chain = make_cleanup (null_cleanup, NULL); - /* Note linespec handling above invalidates the frame chain. Installing a breakpoint also invalidates the frame chain (as it may need to switch threads), so do any frame handling before @@ -11122,6 +11109,9 @@ until_break_command (const char *arg, int from_tty, int anywhere) one. */ breakpoint_up caller_breakpoint; + + gdb::optional lj_deleter; + if (frame_id_p (caller_frame_id)) { struct symtab_and_line sal2; @@ -11136,7 +11126,7 @@ until_break_command (const char *arg, int from_tty, int anywhere) bp_until); set_longjmp_breakpoint (tp, caller_frame_id); - make_cleanup (delete_longjmp_breakpoint_cleanup, &thread); + lj_deleter.emplace (thread); } /* set_momentary_breakpoint could invalidate FRAME. */ @@ -11159,7 +11149,8 @@ until_break_command (const char *arg, int from_tty, int anywhere) std::move (caller_breakpoint)); tp->thread_fsm = &sm->thread_fsm; - discard_cleanups (old_chain); + if (lj_deleter) + lj_deleter->release (); proceed (-1, GDB_SIGNAL_DEFAULT); } diff --git a/gdb/common/forward-scope-exit.h b/gdb/common/forward-scope-exit.h new file mode 100644 index 0000000000..6b51e296b6 --- /dev/null +++ b/gdb/common/forward-scope-exit.h @@ -0,0 +1,118 @@ +/* Copyright (C) 2019 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 COMMON_FORWARD_SCOPE_EXIT_H +#define COMMON_FORWARD_SCOPE_EXIT_H + +#include "common/scope-exit.h" + +/* A forward_scope_exit is like scope_exit, but instead of giving it a + callable, you instead specialize it for a given cleanup function, + and the generated class automatically has a constructor with the + same interface as the cleanup function. forward_scope_exit + captures the arguments passed to the ctor, and in turn passes those + as arguments to the wrapped cleanup function, when it is called at + scope exit time, from within the forward_scope_exit dtor. The + forward_scope_exit class can take any number of arguments, and is + cancelable if needed. + + This allows usage like this: + + void + delete_longjmp_breakpoint (int arg) + { + // Blah, blah, blah... + } + + using longjmp_breakpoint_cleanup + = FORWARD_SCOPE_EXIT (delete_longjmp_breakpoint); + + This above created a new cleanup class `longjmp_breakpoint_cleanup` + than can then be used like this: + + longjmp_breakpoint_cleanup obj (thread); + + // Blah, blah, blah... + + obj.release (); // Optional cancel if needed. + + forward_scope_exit is also handy when you would need to wrap a + scope_exit in a gdb::optional: + + gdb::optional cleanup; + if (some condition) + cleanup.emplace (thread); + ... + if (cleanup) + cleanup->release (); + + since with scope exit, you would have to know the scope_exit's + callable template type when you create the gdb::optional: + + gdb:optional> +*/ + +namespace detail +{ + +/* Function and Signature are passed in the same type, in order to + extract Function's arguments' types in the specialization below. + Those are used to generate the constructor. */ + +template +struct forward_scope_exit; + +template +class forward_scope_exit + : public scope_exit_base> +{ + /* For access to on_exit(). */ + friend scope_exit_base>; + +public: + explicit forward_scope_exit (Args ...args) + : m_bind_function (std::bind (function, args...)) + { + /* Nothing. */ + } + +private: + void on_exit () + { + m_bind_function (); + } + + /* The function and the arguments passed to the ctor, all packed in + a std::bind. */ + decltype (std::bind (function, std::declval ()...)) + m_bind_function; +}; + +} /* namespace detail */ + +/* This is the "public" entry point. It's a macro to avoid having to + name FUNC more than once. */ + +#define FORWARD_SCOPE_EXIT(FUNC) \ + detail::forward_scope_exit + +#endif /* COMMON_FORWARD_SCOPE_EXIT_H */ diff --git a/gdb/common/preprocessor.h b/gdb/common/preprocessor.h index 647568ede6..a7c33d0978 100644 --- a/gdb/common/preprocessor.h +++ b/gdb/common/preprocessor.h @@ -30,6 +30,6 @@ /* Escape parens out. Useful if you need to pass an argument that includes commas to another macro. */ -#define ESC(...) __VA_ARGS__ +#define ESC_PARENS(...) __VA_ARGS__ #endif /* COMMON_PREPROC */ diff --git a/gdb/common/scope-exit.h b/gdb/common/scope-exit.h new file mode 100644 index 0000000000..a484304372 --- /dev/null +++ b/gdb/common/scope-exit.h @@ -0,0 +1,185 @@ +/* Copyright (C) 2019 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 COMMON_SCOPE_EXIT_H +#define COMMON_SCOPE_EXIT_H + +#include +#include +#include "common/preprocessor.h" + +/* scope_exit is a general-purpose scope guard that calls its exit + function at the end of the current scope. A scope_exit may be + canceled by calling the "release" method. The API is modeled on + P0052R5 - Generic Scope Guard and RAII Wrapper for the Standard + Library, which is itself based on Andrej Alexandrescu's + ScopeGuard/SCOPE_EXIT. + + There are two forms available: + + - The "make_scope_exit" form allows canceling the scope guard. Use + it like this: + + auto cleanup = make_scope_exit ( ); + ... + cleanup.release (); // cancel + + - If you don't need to cancel the guard, you can use the SCOPE_EXIT + macro, like this: + + SCOPE_EXIT + { + // any code you like here. + } + + See also forward_scope_exit. +*/ + +/* CRTP base class for cancelable scope_exit-like classes. Implements + the common call-custom-function-from-dtor functionality. Classes + that inherit this implement the on_exit() method, which is called + from scope_exit_base's dtor. */ + +template +class scope_exit_base +{ +public: + scope_exit_base () = default; + + ~scope_exit_base () + { + if (!m_released) + { + auto *self = static_cast (this); + self->on_exit (); + } + } + + /* This is needed for make_scope_exit because copy elision isn't + guaranteed until C++17. An optimizing compiler will usually skip + calling this, but it must exist. */ + scope_exit_base (const scope_exit_base &other) + : m_released (other.m_released) + { + other.m_released = true; + } + + void operator= (const scope_exit_base &) = delete; + + /* If this is called, then the wrapped function will not be called + on destruction. */ + void release () noexcept + { + m_released = true; + } + +private: + + /* True if released. Mutable because of the copy ctor hack + above. */ + mutable bool m_released = false; +}; + +/* A cleanup function is one that is run at the end of the current + scope. A cleanup function may be canceled by calling the "cancel" + method. */ + +template +class scope_exit : public scope_exit_base> +{ + /* For access to on_exit(). */ + friend scope_exit_base>; + +public: + + template>> + scope_exit (EFP &&f) + try : m_exit_function ((!std::is_lvalue_reference::value + && std::is_nothrow_constructible::value) + ? std::move (f) + : f) + { + } + catch (...) + { + /* "If the initialization of exit_function throws an exception, + calls f()." */ + f (); + } + + template>> + scope_exit (scope_exit &&rhs) + noexcept (std::is_nothrow_move_constructible::value + || std::is_nothrow_copy_constructible::value) + : m_exit_function (std::is_nothrow_constructible::value + ? std::move (rhs) + : rhs) + { + rhs.release (); + } + + /* This is needed for make_scope_exit because copy elision isn't + guaranteed until C++17. An optimizing compiler will usually skip + calling this, but it must exist. */ + scope_exit (const scope_exit &other) + : scope_exit_base> (other), + m_exit_function (other.m_exit_function) + { + } + + void operator= (const scope_exit &) = delete; + void operator= (scope_exit &&) = delete; + +private: + void on_exit () + { + m_exit_function (); + } + + /* The function to call on scope exit. */ + EF m_exit_function; +}; + +template +scope_exit::type> +make_scope_exit (EF &&f) +{ + return scope_exit::type> (std::forward (f)); +} + +namespace detail +{ + +enum class scope_exit_lhs {}; + +template +scope_exit::type> +operator+ (scope_exit_lhs, EF &&rhs) +{ + return scope_exit::type> (std::forward (rhs)); +} + +} + +/* Register a block of code to run on scope exit. */ + +#define SCOPE_EXIT \ + auto CONCAT(scope_exit_, __LINE__) = ::detail::scope_exit_lhs () + [&] () + +#endif /* COMMON_SCOPE_EXIT_H */ diff --git a/gdb/common/valid-expr.h b/gdb/common/valid-expr.h index 9289448365..603c76e8f1 100644 --- a/gdb/common/valid-expr.h +++ b/gdb/common/valid-expr.h @@ -85,24 +85,24 @@ another variant. */ #define CHECK_VALID_EXPR_1(T1, VALID, EXPR_TYPE, EXPR) \ - CHECK_VALID_EXPR_INT (ESC (typename T1), \ - ESC (T1), \ + CHECK_VALID_EXPR_INT (ESC_PARENS (typename T1), \ + ESC_PARENS (T1), \ VALID, EXPR_TYPE, EXPR) #define CHECK_VALID_EXPR_2(T1, T2, VALID, EXPR_TYPE, EXPR) \ - CHECK_VALID_EXPR_INT (ESC (typename T1, typename T2), \ - ESC (T1, T2), \ + CHECK_VALID_EXPR_INT (ESC_PARENS(typename T1, typename T2), \ + ESC_PARENS (T1, T2), \ VALID, EXPR_TYPE, EXPR) #define CHECK_VALID_EXPR_3(T1, T2, T3, VALID, EXPR_TYPE, EXPR) \ - CHECK_VALID_EXPR_INT (ESC (typename T1, typename T2, typename T3), \ - ESC (T1, T2, T3), \ + CHECK_VALID_EXPR_INT (ESC_PARENS (typename T1, typename T2, typename T3), \ + ESC_PARENS (T1, T2, T3), \ VALID, EXPR_TYPE, EXPR) #define CHECK_VALID_EXPR_4(T1, T2, T3, T4, VALID, EXPR_TYPE, EXPR) \ - CHECK_VALID_EXPR_INT (ESC (typename T1, typename T2, \ - typename T3, typename T4), \ - ESC (T1, T2, T3, T4), \ + CHECK_VALID_EXPR_INT (ESC_PARENS (typename T1, typename T2, \ + typename T3, typename T4), \ + ESC_PARENS (T1, T2, T3, T4), \ VALID, EXPR_TYPE, EXPR) #endif /* COMMON_VALID_EXPR_H */ diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c index 50062abe8a..64d1e543e4 100644 --- a/gdb/gdbarch-selftests.c +++ b/gdb/gdbarch-selftests.c @@ -104,13 +104,7 @@ register_to_value_test (struct gdbarch *gdbarch) push_target (&mock_target); /* Pop it again on exit (return/exception). */ - struct on_exit - { - ~on_exit () - { - pop_all_targets_at_and_above (process_stratum); - } - } pop_targets; + SCOPE_EXIT { pop_all_targets_at_and_above (process_stratum); }; /* Switch to the mock thread. */ scoped_restore restore_inferior_ptid diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 95db69605e..c35a54e75d 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -32,6 +32,7 @@ struct symtab; #include "cli/cli-utils.h" #include "common/refcounted-object.h" #include "common-gdbthread.h" +#include "common/forward-scope-exit.h" struct inferior; @@ -612,31 +613,8 @@ extern void finish_thread_state (ptid_t ptid); /* Calls finish_thread_state on scope exit, unless release() is called to disengage. */ -class scoped_finish_thread_state -{ -public: - explicit scoped_finish_thread_state (ptid_t ptid) - : m_ptid (ptid) - {} - - ~scoped_finish_thread_state () - { - if (!m_released) - finish_thread_state (m_ptid); - } - - /* Disengage. */ - void release () - { - m_released = true; - } - - DISABLE_COPY_AND_ASSIGN (scoped_finish_thread_state); - -private: - bool m_released = false; - ptid_t m_ptid; -}; +using scoped_finish_thread_state + = FORWARD_SCOPE_EXIT (finish_thread_state); /* Commands with a prefix of `thread'. */ extern struct cmd_list_element *thread_cmd_list; diff --git a/gdb/infcall.c b/gdb/infcall.c index 14b0cbc716..6ca479ac1f 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -40,6 +40,7 @@ #include "interps.h" #include "thread-fsm.h" #include +#include "common/scope-exit.h" /* If we can't find a function's name from its address, we print this instead. */ @@ -675,13 +676,6 @@ run_inferior_call (struct call_thread_fsm *sm, return caught_error; } -/* A cleanup function that calls delete_std_terminate_breakpoint. */ -static void -cleanup_delete_std_terminate_breakpoint (void *ignore) -{ - delete_std_terminate_breakpoint (); -} - /* See infcall.h. */ struct value * @@ -727,7 +721,6 @@ call_function_by_hand_dummy (struct value *function, struct frame_id dummy_id; struct frame_info *frame; struct gdbarch *gdbarch; - struct cleanup *terminate_bp_cleanup; ptid_t call_thread_ptid; struct gdb_exception e; char name_buf[RAW_FUNCTION_ADDRESS_SIZE]; @@ -1122,8 +1115,7 @@ call_function_by_hand_dummy (struct value *function, dummy_dtor, dummy_dtor_data); /* Register a clean-up for unwind_on_terminating_exception_breakpoint. */ - terminate_bp_cleanup = make_cleanup (cleanup_delete_std_terminate_breakpoint, - NULL); + SCOPE_EXIT { delete_std_terminate_breakpoint (); }; /* - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - If you're looking to implement asynchronous dummy-frames, then @@ -1184,7 +1176,6 @@ call_function_by_hand_dummy (struct value *function, maybe_remove_breakpoints (); - do_cleanups (terminate_bp_cleanup); gdb_assert (retval != NULL); return retval; } diff --git a/gdb/infcmd.c b/gdb/infcmd.c index 3c3add89ab..a75388c0af 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -59,6 +59,7 @@ #include "top.h" #include "interps.h" #include "common/gdb_optional.h" +#include "common/scope-exit.h" #include "source.h" /* Local functions: */ @@ -947,13 +948,6 @@ nexti_command (const char *count_string, int from_tty) step_1 (1, 1, count_string); } -void -delete_longjmp_breakpoint_cleanup (void *arg) -{ - int thread = * (int *) arg; - delete_longjmp_breakpoint (thread); -} - /* Data for the FSM that manages the step/next/stepi/nexti commands. */ @@ -1517,7 +1511,6 @@ until_next_command (int from_tty) struct symtab_and_line sal; struct thread_info *tp = inferior_thread (); int thread = tp->global_num; - struct cleanup *old_chain; struct until_next_fsm *sm; clear_proceed_status (0); @@ -1556,11 +1549,11 @@ until_next_command (int from_tty) tp->control.step_over_calls = STEP_OVER_ALL; set_longjmp_breakpoint (tp, get_frame_id (frame)); - old_chain = make_cleanup (delete_longjmp_breakpoint_cleanup, &thread); + delete_longjmp_breakpoint_cleanup lj_deleter (thread); sm = new_until_next_fsm (command_interp (), tp->global_num); tp->thread_fsm = &sm->thread_fsm; - discard_cleanups (old_chain); + lj_deleter.release (); proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); } diff --git a/gdb/inferior.h b/gdb/inferior.h index a82df1a52a..6ce21e3ddd 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -51,6 +51,7 @@ struct thread_info; #include "symfile-add-flags.h" #include "common/refcounted-object.h" +#include "common/scope-exit.h" #include "common-inferior.h" #include "gdbthread.h" @@ -198,7 +199,8 @@ extern void continue_1 (int all_threads); extern void interrupt_target_1 (int all_threads); -extern void delete_longjmp_breakpoint_cleanup (void *arg); +using delete_longjmp_breakpoint_cleanup + = FORWARD_SCOPE_EXIT (delete_longjmp_breakpoint); extern void detach_command (const char *, int); diff --git a/gdb/infrun.c b/gdb/infrun.c index 150288264f..cdfdf49070 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -67,6 +67,7 @@ #include "progspace-and-thread.h" #include "common/gdb_optional.h" #include "arch-utils.h" +#include "common/scope-exit.h" /* Prototypes for local functions */ @@ -3247,14 +3248,6 @@ delete_just_stopped_threads_single_step_breakpoints (void) for_each_just_stopped_thread (delete_single_step_breakpoints); } -/* A cleanup wrapper. */ - -static void -delete_just_stopped_threads_infrun_breakpoints_cleanup (void *arg) -{ - delete_just_stopped_threads_infrun_breakpoints (); -} - /* See infrun.h. */ void @@ -3538,15 +3531,11 @@ prepare_for_detach (void) void wait_for_inferior (void) { - struct cleanup *old_cleanups; - if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: wait_for_inferior ()\n"); - old_cleanups - = make_cleanup (delete_just_stopped_threads_infrun_breakpoints_cleanup, - NULL); + SCOPE_EXIT { delete_just_stopped_threads_infrun_breakpoints (); }; /* If an error happens while handling the event, propagate GDB's knowledge of the executing state to the frontend/user running @@ -3583,8 +3572,6 @@ wait_for_inferior (void) /* No error, don't finish the state yet. */ finish_state.release (); - - do_cleanups (old_cleanups); } /* Cleanup that reinstalls the readline callback handler, if the @@ -3598,7 +3585,7 @@ wait_for_inferior (void) input. */ static void -reinstall_readline_callback_handler_cleanup (void *arg) +reinstall_readline_callback_handler_cleanup () { struct ui *ui = current_ui; @@ -3700,7 +3687,6 @@ fetch_inferior_event (void *client_data) { struct execution_control_state ecss; struct execution_control_state *ecs = &ecss; - struct cleanup *old_chain = make_cleanup (null_cleanup, NULL); int cmd_done = 0; ptid_t waiton_ptid = minus_one_ptid; @@ -3712,114 +3698,119 @@ fetch_inferior_event (void *client_data) scoped_restore save_ui = make_scoped_restore (¤t_ui, main_ui); /* End up with readline processing input, if necessary. */ - make_cleanup (reinstall_readline_callback_handler_cleanup, NULL); - - /* We're handling a live event, so make sure we're doing live - debugging. If we're looking at traceframes while the target is - running, we're going to need to get back to that mode after - handling the event. */ - gdb::optional maybe_restore_traceframe; - if (non_stop) - { - maybe_restore_traceframe.emplace (); - 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. */ - maybe_restore_thread.emplace (); - - overlay_cache_invalid = 1; - /* Flush target cache before starting to handle each event. Target - was running and cache could be stale. This is just a heuristic. - Running threads may modify target memory, but we don't get any - event. */ - target_dcache_invalidate (); - - scoped_restore save_exec_dir - = make_scoped_restore (&execution_direction, target_execution_direction ()); - - ecs->ptid = do_target_wait (waiton_ptid, &ecs->ws, - target_can_async_p () ? TARGET_WNOHANG : 0); - - if (debug_infrun) - print_target_wait_results (waiton_ptid, ecs->ptid, &ecs->ws); - - /* If an error happens while handling the event, propagate GDB's - knowledge of the executing state to the frontend/user running - state. */ - ptid_t finish_ptid = !target_is_non_stop_p () ? minus_one_ptid : ecs->ptid; - scoped_finish_thread_state finish_state (finish_ptid); - - /* Get executed before make_cleanup_restore_current_thread above to apply - still for the thread which has thrown the exception. */ - struct cleanup *ts_old_chain = make_bpstat_clear_actions_cleanup (); - - make_cleanup (delete_just_stopped_threads_infrun_breakpoints_cleanup, NULL); - - /* Now figure out what to do with the result of the result. */ - handle_inferior_event (ecs); + { + SCOPE_EXIT { reinstall_readline_callback_handler_cleanup (); }; + + /* We're handling a live event, so make sure we're doing live + debugging. If we're looking at traceframes while the target is + running, we're going to need to get back to that mode after + handling the event. */ + gdb::optional maybe_restore_traceframe; + if (non_stop) + { + maybe_restore_traceframe.emplace (); + set_current_traceframe (-1); + } - if (!ecs->wait_some_more) - { - struct inferior *inf = find_inferior_ptid (ecs->ptid); - int should_stop = 1; - struct thread_info *thr = ecs->event_thread; + 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. */ + maybe_restore_thread.emplace (); + + overlay_cache_invalid = 1; + /* Flush target cache before starting to handle each event. Target + was running and cache could be stale. This is just a heuristic. + Running threads may modify target memory, but we don't get any + event. */ + target_dcache_invalidate (); + + scoped_restore save_exec_dir + = make_scoped_restore (&execution_direction, + target_execution_direction ()); + + ecs->ptid = do_target_wait (waiton_ptid, &ecs->ws, + target_can_async_p () ? TARGET_WNOHANG : 0); + + if (debug_infrun) + print_target_wait_results (waiton_ptid, ecs->ptid, &ecs->ws); + + /* If an error happens while handling the event, propagate GDB's + knowledge of the executing state to the frontend/user running + state. */ + ptid_t finish_ptid = !target_is_non_stop_p () ? minus_one_ptid : ecs->ptid; + scoped_finish_thread_state finish_state (finish_ptid); + + /* Get executed before scoped_restore_current_thread above to apply + still for the thread which has thrown the exception. */ + auto defer_bpstat_clear + = make_scope_exit (bpstat_clear_actions); + auto defer_delete_threads + = make_scope_exit (delete_just_stopped_threads_infrun_breakpoints); + + /* Now figure out what to do with the result of the result. */ + handle_inferior_event (ecs); + + if (!ecs->wait_some_more) + { + struct inferior *inf = find_inferior_ptid (ecs->ptid); + int should_stop = 1; + struct thread_info *thr = ecs->event_thread; - delete_just_stopped_threads_infrun_breakpoints (); + delete_just_stopped_threads_infrun_breakpoints (); - if (thr != NULL) - { - struct thread_fsm *thread_fsm = thr->thread_fsm; + if (thr != NULL) + { + struct thread_fsm *thread_fsm = thr->thread_fsm; - if (thread_fsm != NULL) - should_stop = thread_fsm_should_stop (thread_fsm, thr); - } + if (thread_fsm != NULL) + should_stop = thread_fsm_should_stop (thread_fsm, thr); + } - if (!should_stop) - { - keep_going (ecs); - } - else - { - int should_notify_stop = 1; - int proceeded = 0; + if (!should_stop) + { + keep_going (ecs); + } + else + { + int should_notify_stop = 1; + int proceeded = 0; - clean_up_just_stopped_threads_fsms (ecs); + clean_up_just_stopped_threads_fsms (ecs); - if (thr != NULL && thr->thread_fsm != NULL) - { - should_notify_stop - = thread_fsm_should_notify_stop (thr->thread_fsm); - } + if (thr != NULL && thr->thread_fsm != NULL) + { + should_notify_stop + = thread_fsm_should_notify_stop (thr->thread_fsm); + } - if (should_notify_stop) - { - /* We may not find an inferior if this was a process exit. */ - if (inf == NULL || inf->control.stop_soon == NO_STOP_QUIETLY) - proceeded = normal_stop (); - } + if (should_notify_stop) + { + /* We may not find an inferior if this was a process exit. */ + if (inf == NULL || inf->control.stop_soon == NO_STOP_QUIETLY) + proceeded = normal_stop (); + } - if (!proceeded) - { - inferior_event_handler (INF_EXEC_COMPLETE, NULL); - cmd_done = 1; - } - } - } + if (!proceeded) + { + inferior_event_handler (INF_EXEC_COMPLETE, NULL); + cmd_done = 1; + } + } + } - discard_cleanups (ts_old_chain); + defer_delete_threads.release (); + defer_bpstat_clear.release (); - /* No error, don't finish the thread states yet. */ - finish_state.release (); + /* No error, don't finish the thread states yet. */ + finish_state.release (); - /* Revert thread and frame. */ - do_cleanups (old_chain); + /* This scope is used to ensure that readline callbacks are + reinstalled here. */ + } /* If a UI was in sync execution mode, and now isn't, restore its prompt (a synchronous execution command has finished, and we're @@ -4284,14 +4275,6 @@ save_waitstatus (struct thread_info *tp, struct target_waitstatus *ws) } } -/* A cleanup that disables thread create/exit events. */ - -static void -disable_thread_events (void *arg) -{ - target_thread_events (0); -} - /* See infrun.h. */ void @@ -4300,7 +4283,6 @@ stop_all_threads (void) /* We may need multiple passes to discover all threads. */ int pass; int iterations = 0; - struct cleanup *old_chain; gdb_assert (target_is_non_stop_p ()); @@ -4310,7 +4292,7 @@ stop_all_threads (void) scoped_restore_current_thread restore_thread; target_thread_events (1); - old_chain = make_cleanup (disable_thread_events, NULL); + SCOPE_EXIT { target_thread_events (0); }; /* Request threads to stop, and then wait for the stops. Because threads we already know about can spawn more threads while we're @@ -4495,8 +4477,6 @@ stop_all_threads (void) } } - do_cleanups (old_chain); - if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n"); } diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index c0d5f8dc66..45da9fa997 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -66,6 +66,7 @@ #include "objfiles.h" #include "nat/linux-namespaces.h" #include "fileio.h" +#include "common/scope-exit.h" #ifndef SPUFS_MAGIC #define SPUFS_MAGIC 0x23c9b64e @@ -4223,22 +4224,10 @@ linux_nat_xfer_osdata (enum target_object object, return TARGET_XFER_OK; } -static void -cleanup_target_stop (void *arg) -{ - ptid_t *ptid = (ptid_t *) arg; - - gdb_assert (arg != NULL); - - /* Unpause all */ - target_continue_no_signal (*ptid); -} - std::vector linux_nat_target::static_tracepoint_markers_by_strid (const char *strid) { char s[IPA_CMD_BUF_SIZE]; - struct cleanup *old_chain; int pid = inferior_ptid.pid (); std::vector markers; const char *p = s; @@ -4253,7 +4242,8 @@ linux_nat_target::static_tracepoint_markers_by_strid (const char *strid) agent_run_command (pid, s, strlen (s) + 1); - old_chain = make_cleanup (cleanup_target_stop, &ptid); + /* Unpause all. */ + SCOPE_EXIT { target_continue_no_signal (ptid); }; while (*p++ == 'm') { @@ -4272,8 +4262,6 @@ linux_nat_target::static_tracepoint_markers_by_strid (const char *strid) p = s; } - do_cleanups (old_chain); - return markers; } diff --git a/gdb/regcache.c b/gdb/regcache.c index c51ef771be..bbaca73ff8 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -219,37 +219,6 @@ reg_buffer::arch () const return m_descr->gdbarch; } -/* Cleanup class for invalidating a register. */ - -class regcache_invalidator -{ -public: - - regcache_invalidator (struct regcache *regcache, int regnum) - : m_regcache (regcache), - m_regnum (regnum) - { - } - - ~regcache_invalidator () - { - if (m_regcache != nullptr) - m_regcache->invalidate (m_regnum); - } - - DISABLE_COPY_AND_ASSIGN (regcache_invalidator); - - void release () - { - m_regcache = nullptr; - } - -private: - - struct regcache *m_regcache; - int m_regnum; -}; - /* Return a pointer to register REGNUM's buffer cache. */ gdb_byte * @@ -769,7 +738,8 @@ regcache::raw_write (int regnum, const gdb_byte *buf) /* Invalidate the register after it is written, in case of a failure. */ - regcache_invalidator invalidator (this, regnum); + auto invalidator + = make_scope_exit ([&] { this->invalidate (regnum); }); target_store_registers (this, regnum); diff --git a/gdb/symfile.c b/gdb/symfile.c index 04197120db..a9f03790b6 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -59,6 +59,7 @@ #include "common/byte-vector.h" #include "selftest.h" #include "cli/cli-style.h" +#include "common/scope-exit.h" #include #include @@ -79,8 +80,6 @@ void (*deprecated_show_load_progress) (const char *section, void (*deprecated_pre_add_symbol_hook) (const char *); void (*deprecated_post_add_symbol_hook) (void); -static void clear_symtab_users_cleanup (void *ignore); - /* Global variables owned by this file. */ int readnow_symbol_files; /* Read full symbols immediately. */ int readnever_symbol_files; /* Never read full symbols. */ @@ -893,6 +892,9 @@ init_entry_point_info (struct objfile *objfile) } } +using clear_symtab_users_cleanup + = FORWARD_SCOPE_EXIT (clear_symtab_users); + /* Process a symbol file, as either the main file or as a dynamically loaded file. @@ -923,7 +925,6 @@ syms_from_objfile_1 (struct objfile *objfile, symfile_add_flags add_flags) { section_addr_info local_addr; - struct cleanup *old_chain; const int mainline = add_flags & SYMFILE_MAINLINE; objfile_set_sym_fns (objfile, find_sym_fns (objfile->obfd)); @@ -945,7 +946,9 @@ syms_from_objfile_1 (struct objfile *objfile, /* Make sure that partially constructed symbol tables will be cleaned up if an error occurs during symbol reading. */ - old_chain = make_cleanup (null_cleanup, NULL); + + gdb::optional defer_clear_users; + std::unique_ptr objfile_holder (objfile); /* If ADDRS is NULL, put together a dummy address list. @@ -958,7 +961,7 @@ syms_from_objfile_1 (struct objfile *objfile, { /* We will modify the main symbol table, make sure that all its users will be cleaned up if an error occurs during symbol reading. */ - make_cleanup (clear_symtab_users_cleanup, 0 /*ignore*/); + defer_clear_users.emplace ((symfile_add_flag) 0); /* Since no error yet, throw away the old symbol table. */ @@ -999,7 +1002,8 @@ syms_from_objfile_1 (struct objfile *objfile, /* Discard cleanups as symbol reading was successful. */ objfile_holder.release (); - discard_cleanups (old_chain); + if (defer_clear_users) + defer_clear_users->release (); } /* Same as syms_from_objfile_1, but also initializes the objfile @@ -2441,7 +2445,6 @@ reread_symbols (void) new_modtime = new_statbuf.st_mtime; if (new_modtime != objfile->mtime) { - struct cleanup *old_cleanups; struct section_offsets *offsets; int num_offsets; @@ -2461,7 +2464,7 @@ reread_symbols (void) std::unique_ptr objfile_holder (objfile); /* We need to do this whenever any symbols go away. */ - old_cleanups = make_cleanup (clear_symtab_users_cleanup, 0 /*ignore*/); + clear_symtab_users_cleanup defer_clear_users (0); if (exec_bfd != NULL && filename_cmp (bfd_get_filename (objfile->obfd), @@ -2615,7 +2618,7 @@ reread_symbols (void) /* Discard cleanups as symbol reading was successful. */ objfile_holder.release (); - discard_cleanups (old_cleanups); + defer_clear_users.release (); /* If the mtime has changed between the time we set new_modtime and now, we *want* this to be out of date, so don't call stat @@ -2892,12 +2895,6 @@ clear_symtab_users (symfile_add_flags add_flags) if ((add_flags & SYMFILE_DEFER_BP_RESET) == 0) breakpoint_re_set (); } - -static void -clear_symtab_users_cleanup (void *ignore) -{ - clear_symtab_users (0); -} /* OVERLAYS: The following code implements an abstraction for debugging overlay sections. diff --git a/gdb/top.c b/gdb/top.c index 900e78aaec..cf6a6abc7d 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -52,6 +52,7 @@ #include "frame.h" #include "buffer.h" #include "gdb_select.h" +#include "common/scope-exit.h" /* readline include files. */ #include "readline/readline.h" @@ -539,12 +540,11 @@ set_repeat_arguments (const char *args) void execute_command (const char *p, int from_tty) { - struct cleanup *cleanup_if_error; struct cmd_list_element *c; const char *line; const char *cmd_start = p; - cleanup_if_error = make_bpstat_clear_actions_cleanup (); + auto cleanup_if_error = make_scope_exit (bpstat_clear_actions); scoped_value_mark cleanup = prepare_execute_command (); /* Force cleanup of any alloca areas if using C alloca instead of @@ -554,7 +554,7 @@ execute_command (const char *p, int from_tty) /* This can happen when command_line_input hits end of file. */ if (p == NULL) { - discard_cleanups (cleanup_if_error); + cleanup_if_error.release (); return; } @@ -649,7 +649,7 @@ execute_command (const char *p, int from_tty) if (has_stack_frames () && inferior_thread ()->state != THREAD_RUNNING) check_frame_language_change (); - discard_cleanups (cleanup_if_error); + cleanup_if_error.release (); } /* Run execute_command for P and FROM_TTY. Capture its output into the diff --git a/gdb/ui-out.h b/gdb/ui-out.h index 5f4eea5491..8d183060b5 100644 --- a/gdb/ui-out.h +++ b/gdb/ui-out.h @@ -195,11 +195,9 @@ class ui_out ui_out_level *current_level () const; }; -/* This is similar to make_cleanup_ui_out_tuple_begin_end and - make_cleanup_ui_out_list_begin_end, but written as an RAII template - class. It takes the ui_out_type as a template parameter. Normally - this is used via the typedefs ui_out_emit_tuple and - ui_out_emit_list. */ +/* Start a new tuple or list on construction, and end it on + destruction. Normally this is used via the typedefs + ui_out_emit_tuple and ui_out_emit_list. */ template class ui_out_emit_type { diff --git a/gdb/utils.c b/gdb/utils.c index ed8d60fa7b..4af75e3480 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -3057,23 +3057,6 @@ parse_pid_to_attach (const char *args) return pid; } -/* Helper for make_bpstat_clear_actions_cleanup. */ - -static void -do_bpstat_clear_actions_cleanup (void *unused) -{ - bpstat_clear_actions (); -} - -/* Call bpstat_clear_actions for the case an exception is throw. You should - discard_cleanups if no exception is caught. */ - -struct cleanup * -make_bpstat_clear_actions_cleanup (void) -{ - return make_cleanup (do_bpstat_clear_actions_cleanup, NULL); -} - /* Substitute all occurences of string FROM by string TO in *STRINGP. *STRINGP must come from xrealloc-compatible allocator and it may be updated. FROM needs to be delimited by IS_DIR_SEPARATOR or DIRNAME_SEPARATOR (or be diff --git a/gdb/utils.h b/gdb/utils.h index f2fe1da832..896feb973c 100644 --- a/gdb/utils.h +++ b/gdb/utils.h @@ -286,7 +286,6 @@ private: int m_save_batch_flag; }; -extern struct cleanup *make_bpstat_clear_actions_cleanup (void); /* Path utilities. */