[PATCH 00/12] remove some cleanups using a cleanup function

[PATCH 00/12] remove some cleanups using a cleanup function

[Tom Tromey]

This is the series to introduce a new cleanup_function class, that can
be used in situations where a simple function call is needed to do
some cleanup.

As I mentioned in the other thread, I was (and still am) uncertain as
to whether this is a good idea.  My worry is just that this will be
used in ways leading to unclear code.  (You can judge for yourself
whether this series has already entered that territory...)

A couple of rules I used when deciding when to use the new class:

* There should not be too many uses of the existing cleanup (one or
  two).  If there are more than several it should probably be a
  bespoke class.

* Any lambdas should capture only a small number of things (one or
  maybe two), and only capture by value should be used.

Hopefully I actually followed these.

I wonder if the new class should take a std::function rather than a
gdb::function_view.  Some of the changes needs a bit of extra code to
deal with the latter.

Also I wonder if it should be possible to default-construct a
cleanup_function, and then set it later.  This would eliminate some
uses of gdb::optional; and seemed maybe reasonable given that the
class already has to maintain a (sort of-) validity flag.

This series includes a couple of cleanup-related comment fixes for
things I happened to notice along the way.

Regression tested by the buildbot.

Tom

[PATCH 04/12] Remove cleanup_delete_std_terminate_breakpoint

[Tom Tromey]

This removes cleanup_delete_std_terminate_breakpoint, replacing it
with a use of cleanup_function.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* infcall.c (cleanup_delete_std_terminate_breakpoint): Remove.
	(call_function_by_hand_dummy): Use cleanup_function.
---
 gdb/ChangeLog |  5 +++++
 gdb/infcall.c | 13 ++-----------
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/gdb/infcall.c b/gdb/infcall.c
index 14b0cbc716..cd780929fc 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -40,6 +40,7 @@
 #include "interps.h"
 #include "thread-fsm.h"
 #include <algorithm>
+#include "common/cleanup-function.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);
+  cleanup_function terminate_bp_cleanup (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;
 	  }
-- 
2.17.2

[PATCH 12/12] Use cleanup_function in regcache.c

[Tom Tromey]

This removes the regcache_invalidator class in favor of a
cleanup_function.  This seems like an improvement (albeit a minor one)
because regcache_invalidator is only used in a single spot.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* regcache.c (class regcache_invalidator): Remove.
	(regcache::raw_write): Use cleanup_function.
---
 gdb/ChangeLog  |  5 +++++
 gdb/regcache.c | 41 ++++++++---------------------------------
 2 files changed, 13 insertions(+), 33 deletions(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index c51ef771be..47339a0bce 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -28,6 +28,7 @@
 #include "reggroups.h"
 #include "observable.h"
 #include "regset.h"
+#include "common/cleanup-function.h"
 #include <forward_list>
 
 /*
@@ -219,37 +220,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,13 +739,18 @@ 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 do_invalidate
+    = [=] ()
+      {
+	this->invalidate (regnum);
+      };
+  cleanup_function invalidator (do_invalidate);
 
   target_store_registers (this, regnum);
 
   /* The target did not throw an error so we can discard invalidating
      the register.  */
-  invalidator.release ();
+  invalidator.cancel ();
 }
 
 void
-- 
2.17.2

[PATCH 06/12] Remove clear_symtab_users_cleanup

[Tom Tromey]

This removes clear_symtab_users_cleanup, replacing it with uses of
cleanup_function.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* symfile.c (syms_from_objfile_1): Use cleanup_function.
	(clear_symtab_users_cleanup): Remove.
	(reread_symbols): Use cleanup_function.
---
 gdb/ChangeLog |  6 ++++++
 gdb/symfile.c | 31 ++++++++++++++++---------------
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 8858098f48..e5ac521451 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/cleanup-function.h"
 
 #include <sys/types.h>
 #include <fcntl.h>
@@ -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.  */
@@ -923,7 +922,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 +943,12 @@ 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);
+  auto clear_users_func
+    = [] ()
+      {
+	clear_symtab_users (0);
+      };
+  gdb::optional<cleanup_function> defer_clear_users;
   std::unique_ptr<struct objfile> 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 (clear_users_func);
 
       /* Since no error yet, throw away the old symbol table.  */
 
@@ -999,7 +1002,7 @@ syms_from_objfile_1 (struct objfile *objfile,
   /* Discard cleanups as symbol reading was successful.  */
 
   objfile_holder.release ();
-  discard_cleanups (old_chain);
+  defer_clear_users->cancel ();
 }
 
 /* Same as syms_from_objfile_1, but also initializes the objfile
@@ -2434,7 +2437,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;
 
@@ -2454,7 +2456,12 @@ reread_symbols (void)
 	  std::unique_ptr<struct objfile> objfile_holder (objfile);
 
 	  /* We need to do this whenever any symbols go away.  */
-	  old_cleanups = make_cleanup (clear_symtab_users_cleanup, 0 /*ignore*/);
+	  auto clear_users_func
+	    = [] ()
+	      {
+		clear_symtab_users (0);
+	      };
+	  cleanup_function defer_clear_users (clear_users_func);
 
 	  if (exec_bfd != NULL
 	      && filename_cmp (bfd_get_filename (objfile->obfd),
@@ -2618,7 +2625,7 @@ reread_symbols (void)
 
 	  /* Discard cleanups as symbol reading was successful.  */
 	  objfile_holder.release ();
-	  discard_cleanups (old_cleanups);
+	  defer_clear_users.cancel ();
 
 	  /* 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
@@ -2895,12 +2902,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);
-}
 \f
 /* OVERLAYS:
    The following code implements an abstraction for debugging overlay sections.
-- 
2.17.2

[PATCH 07/12] Remove delete_just_stopped_threads_infrun_breakpoints_cleanup

[Tom Tromey]

This removes delete_just_stopped_threads_infrun_breakpoints_cleanup,
replacing it with a use of cleanup_function.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* infrun.c (wait_for_inferior): Use cleanup_function.
	(delete_just_stopped_threads_infrun_breakpoints_cleanup): Remove.
---
 gdb/ChangeLog |  5 +++++
 gdb/infrun.c  | 17 ++---------------
 2 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index d84ed10b61..9e3b1fb6ef 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3248,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
@@ -3539,15 +3531,12 @@ 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);
+  cleanup_function defer_delete_threads
+    (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
@@ -3584,8 +3573,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
-- 
2.17.2

[PATCH 01/12] Remove delete_longjmp_breakpoint_cleanup

[Tom Tromey]

This removes delete_longjmp_breakpoint_cleanup in favor of a new
cleanup function type.  This new type is somewhat generic, and will be
reused in subsequent patches.

Note that cleanup_function::cancel was purposely not named "reset".
An earlier version did this, but it was too easy to mistake
cleanup_function::reset and gdb::optional::reset, as pointed out by
Andrew Burgess when reviewing an earlier version of this series.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* common/cleanup-function.h: New file.
	* breakpoint.c (until_break_command): Use
	cleanup_function.
	* infcmd.c (delete_longjmp_breakpoint_cleanup): Remove.
	(until_next_command): Use cleanup_function.
	* inferior.h (delete_longjmp_breakpoint_cleanup): Don't declare.
---
 gdb/ChangeLog                 |  9 +++++
 gdb/breakpoint.c              | 16 ++++++---
 gdb/common/cleanup-function.h | 62 +++++++++++++++++++++++++++++++++++
 gdb/infcmd.c                  | 18 +++++-----
 gdb/inferior.h                |  2 --
 5 files changed, 90 insertions(+), 17 deletions(-)
 create mode 100644 gdb/common/cleanup-function.h

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d54d496972..24ba4071ff 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/cleanup-function.h"
 
 /* Enums for exception-handling support.  */
 enum exception_event_kind
@@ -11078,7 +11079,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;
@@ -11111,8 +11111,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
@@ -11127,6 +11125,14 @@ until_break_command (const char *arg, int from_tty, int anywhere)
      one.  */
 
   breakpoint_up caller_breakpoint;
+
+  gdb::optional<cleanup_function> lj_deleter;
+  auto lj_deletion_func
+    = [=] ()
+      {
+	delete_longjmp_breakpoint (thread);
+      };
+
   if (frame_id_p (caller_frame_id))
     {
       struct symtab_and_line sal2;
@@ -11141,7 +11147,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 (lj_deletion_func);
     }
 
   /* set_momentary_breakpoint could invalidate FRAME.  */
@@ -11164,7 +11170,7 @@ 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);
+  lj_deleter->cancel ();
 
   proceed (-1, GDB_SIGNAL_DEFAULT);
 }
diff --git a/gdb/common/cleanup-function.h b/gdb/common/cleanup-function.h
new file mode 100644
index 0000000000..89cc6d11c8
--- /dev/null
+++ b/gdb/common/cleanup-function.h
@@ -0,0 +1,62 @@
+/* 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 <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_CLEANUP_FUNCTION_H
+#define COMMON_CLEANUP_FUNCTION_H
+
+#include "common/function-view.h"
+
+/* A cleanup function is one that is run at the end of the current
+   scope.  It is just a function of no arguments.  A cleanup function
+   may be canceled by calling the "cancel" method.  */
+
+class cleanup_function
+{
+public:
+
+  explicit cleanup_function (gdb::function_view<void ()> func)
+    : m_func (func)
+  {
+  }
+
+  DISABLE_COPY_AND_ASSIGN (cleanup_function);
+
+  ~cleanup_function ()
+  {
+    m_func ();
+  }
+
+  /* If this is called, then the wrapped function will not be called
+     on destruction.  */
+  void cancel ()
+  {
+    m_func = do_nothing;
+  }
+
+private:
+
+  /* The function to call.  */
+  gdb::function_view<void ()> m_func;
+
+  /* A helper function that is used as the value of m_func when cancel
+     is called.  */
+  static void do_nothing ()
+  {
+  }
+};
+
+#endif /* COMMON_CLEANUP_FUNCTION_H */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 3c3add89ab..5c4c4171c5 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/cleanup-function.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,16 @@ 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);
+  auto lj_deletion_func
+    = [=] ()
+      {
+	delete_longjmp_breakpoint (thread);
+      };
+  cleanup_function lj_deleter (lj_deletion_func);
 
   sm = new_until_next_fsm (command_interp (), tp->global_num);
   tp->thread_fsm = &sm->thread_fsm;
-  discard_cleanups (old_chain);
+  lj_deleter.cancel ();
 
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
 }
diff --git a/gdb/inferior.h b/gdb/inferior.h
index a82df1a52a..c63fa26b08 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -198,8 +198,6 @@ extern void continue_1 (int all_threads);
 
 extern void interrupt_target_1 (int all_threads);
 
-extern void delete_longjmp_breakpoint_cleanup (void *arg);
-
 extern void detach_command (const char *, int);
 
 extern void notice_new_inferior (struct thread_info *, int, int);
-- 
2.17.2

[PATCH 09/12] Remove remaining cleanup from fetch_inferior_event

[Tom Tromey]

This removes the remaining cleanup from fetch_inferior_event,
replacing it with a cleanup_function.  This required introducing a new
scope and reindenting.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* infrun.c (reinstall_readline_callback_handler_cleanup): Remove
	argument.
	(fetch_inferior_event): Use cleanup_function.
---
 gdb/ChangeLog |   6 ++
 gdb/infrun.c  | 183 +++++++++++++++++++++++++-------------------------
 2 files changed, 99 insertions(+), 90 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 19c6289f8d..f43f39a253 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3586,7 +3586,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;
 
@@ -3688,7 +3688,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;
 
@@ -3700,115 +3699,119 @@ fetch_inferior_event (void *client_data)
   scoped_restore save_ui = make_scoped_restore (&current_ui, main_ui);
 
   /* End up with readline processing input, if necessary.  */
-  make_cleanup (reinstall_readline_callback_handler_cleanup, NULL);
+  {
+    cleanup_function defer_reinstall_readline
+      (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<scoped_restore_current_traceframe> maybe_restore_traceframe;
+    if (non_stop)
+      {
+	maybe_restore_traceframe.emplace ();
+	set_current_traceframe (-1);
+      }
 
-  /* 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<scoped_restore_current_traceframe> maybe_restore_traceframe;
-  if (non_stop)
-    {
-      maybe_restore_traceframe.emplace ();
-      set_current_traceframe (-1);
-    }
+    gdb::optional<scoped_restore_current_thread> maybe_restore_thread;
 
-  gdb::optional<scoped_restore_current_thread> 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 ();
 
-  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 ();
 
-  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 ());
+    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);
+    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 (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);
+    /* 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.  */
-  cleanup_function defer_bpstat_clear (bpstat_clear_actions);
-  cleanup_function defer_delete_threads
-    (delete_just_stopped_threads_infrun_breakpoints);
+    /* Get executed before make_cleanup_restore_current_thread above to apply
+       still for the thread which has thrown the exception.  */
+    cleanup_function defer_bpstat_clear (bpstat_clear_actions);
+    cleanup_function defer_delete_threads
+      (delete_just_stopped_threads_infrun_breakpoints);
 
-  /* Now figure out what to do with the result of the result.  */
-  handle_inferior_event (ecs);
+    /* 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;
+    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;
+	      }
+	  }
+      }
 
-  defer_delete_threads.cancel ();
-  defer_bpstat_clear.cancel ();
+    defer_delete_threads.cancel ();
+    defer_bpstat_clear.cancel ();
 
-  /* 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
-- 
2.17.2

[PATCH 11/12] Update cleanup comment in ui-out.h

[Tom Tromey]

ui-out.h refers to some cleanup functions that no longer exist.  This
updates the reference.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* ui-out.h (class ui_out_emit_type): Update comment.
---
 gdb/ChangeLog | 4 ++++
 gdb/ui-out.h  | 8 +++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

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<ui_out_type Type>
 class ui_out_emit_type
 {
-- 
2.17.2

[PATCH 03/12] Remove make_bpstat_clear_actions_cleanup

[Tom Tromey]

This removes make_bpstat_clear_actions_cleanup, replacing it with uses
of cleanup_function.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* infrun.c (fetch_inferior_event): Use cleanup_function.
	* utils.h (make_bpstat_clear_actions_cleanup): Don't declare.
	* top.c (execute_command): Use cleanup_function.
	* breakpoint.c (bpstat_do_actions): Use cleanup_function.
	* utils.c (do_bpstat_clear_actions_cleanup)
	(make_bpstat_clear_actions_cleanup): Remove.
---
 gdb/ChangeLog    |  9 +++++++++
 gdb/breakpoint.c |  4 ++--
 gdb/infrun.c     | 10 ++++++----
 gdb/top.c        |  8 ++++----
 gdb/utils.c      | 17 -----------------
 gdb/utils.h      |  1 -
 6 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3acb5145b9..4726e452a6 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4474,7 +4474,7 @@ get_bpstat_thread ()
 void
 bpstat_do_actions (void)
 {
-  struct cleanup *cleanup_if_error = make_bpstat_clear_actions_cleanup ();
+  cleanup_function cleanup_if_error (bpstat_clear_actions);
   thread_info *tp;
 
   /* Do any commands attached to breakpoint we are stopped at.  */
@@ -4488,7 +4488,7 @@ bpstat_do_actions (void)
 	break;
     }
 
-  discard_cleanups (cleanup_if_error);
+  cleanup_if_error.cancel ();
 }
 
 /* Print out the (old or new) value associated with a watchpoint.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 150288264f..d84ed10b61 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/cleanup-function.h"
 
 /* Prototypes for local functions */
 
@@ -3758,9 +3759,9 @@ fetch_inferior_event (void *client_data)
 
   /* 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);
+  cleanup_function defer_bpstat_clear (bpstat_clear_actions);
+  cleanup_function defer_delete_threads
+    (delete_just_stopped_threads_infrun_breakpoints);
 
   /* Now figure out what to do with the result of the result.  */
   handle_inferior_event (ecs);
@@ -3813,7 +3814,8 @@ fetch_inferior_event (void *client_data)
 	}
     }
 
-  discard_cleanups (ts_old_chain);
+  defer_delete_threads.cancel ();
+  defer_bpstat_clear.cancel ();
 
   /* No error, don't finish the thread states yet.  */
   finish_state.release ();
diff --git a/gdb/top.c b/gdb/top.c
index 900e78aaec..06911a110f 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/cleanup-function.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 ();
+  cleanup_function cleanup_if_error (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.cancel ();
       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.cancel ();
 }
 
 /* Run execute_command for P and FROM_TTY.  Capture its output into the
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);
 \f
 /* Path utilities.  */
 
-- 
2.17.2

[PATCH 05/12] Remove cleanup from linux-nat.c

[Tom Tromey]

This removes a cleanup from linux-nat.c, replacing it with a
cleanup_function.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* linux-nat.c (cleanup_target_stop): Remove.
	(linux_nat_target::static_tracepoint_markers_by_strid): Use
	cleanup_function.
---
 gdb/ChangeLog   |  6 ++++++
 gdb/linux-nat.c | 23 ++++++++---------------
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 73d4fd193a..d1c4707b4b 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/cleanup-function.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<static_tracepoint_marker>
 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<static_tracepoint_marker> markers;
   const char *p = s;
@@ -4253,7 +4242,13 @@ 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);
+  auto continue_func
+    = [=] ()
+      {
+	/* Unpause all */
+	target_continue_no_signal (ptid);
+      };
+  cleanup_function continue_at_end (continue_func);
 
   while (*p++ == 'm')
     {
@@ -4272,8 +4267,6 @@ linux_nat_target::static_tracepoint_markers_by_strid (const char *strid)
       p = s;
     }
 
-  do_cleanups (old_chain);
-
   return markers;
 }
 
-- 
2.17.2

[PATCH 10/12] Update an obsolete cleanup comment

[Tom Tromey]

This updates a comment in fetch_inferior_event.  The comment refers to
a cleanup that is now a scoped_restore_current_thread.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* infrun.c (fetch_inferior_event): Update comment.
---
 gdb/ChangeLog | 4 ++++
 gdb/infrun.c  | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index f43f39a253..7b8401490a 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3746,7 +3746,7 @@ fetch_inferior_event (void *client_data)
     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
+    /* Get executed before scoped_restore_current_thread above to apply
        still for the thread which has thrown the exception.  */
     cleanup_function defer_bpstat_clear (bpstat_clear_actions);
     cleanup_function defer_delete_threads
-- 
2.17.2

[PATCH 08/12] Remove cleanup from stop_all_threads

[Tom Tromey]

This removes the cleanup from stop_all_threads, replacing it with a
cleanup_function.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* infrun.c (disable_thread_events): Remove argument.
	(stop_all_threads): Use cleanup_function.
---
 gdb/ChangeLog | 5 +++++
 gdb/infrun.c  | 7 ++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 9e3b1fb6ef..19c6289f8d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4276,7 +4276,7 @@ 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)
+disable_thread_events ()
 {
   target_thread_events (0);
 }
@@ -4289,7 +4289,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 ());
 
@@ -4299,7 +4298,7 @@ stop_all_threads (void)
   scoped_restore_current_thread restore_thread;
 
   target_thread_events (1);
-  old_chain = make_cleanup (disable_thread_events, NULL);
+  cleanup_function defer_disable_events (disable_thread_events);
 
   /* Request threads to stop, and then wait for the stops.  Because
      threads we already know about can spawn more threads while we're
@@ -4484,8 +4483,6 @@ stop_all_threads (void)
 	}
     }
 
-  do_cleanups (old_chain);
-
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");
 }
-- 
2.17.2

[PATCH 02/12] Remove remaining cleanup from breakpoint.c

[Tom Tromey]

The remaining null cleanup in breakpoint.c does not seem to protect
anything, so remove it.

gdb/ChangeLog
2019-01-08  Tom Tromey  <tom@tromey.com>

	* breakpoint.c (create_breakpoint): Remove cleanup.
---
 gdb/ChangeLog    |  4 ++++
 gdb/breakpoint.c | 11 -----------
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 24ba4071ff..3acb5145b9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9236,7 +9236,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;
@@ -9286,12 +9285,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)
@@ -9390,10 +9383,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);
 
-- 
2.17.2

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Joel Brobecker]

> This is the series to introduce a new cleanup_function class, that can
> be used in situations where a simple function call is needed to do
> some cleanup.
> 
> As I mentioned in the other thread, I was (and still am) uncertain as
> to whether this is a good idea.  My worry is just that this will be
> used in ways leading to unclear code.  (You can judge for yourself
> whether this series has already entered that territory...)
> 
> A couple of rules I used when deciding when to use the new class:
> 
> * There should not be too many uses of the existing cleanup (one or
>   two).  If there are more than several it should probably be a
>   bespoke class.
> 
> * Any lambdas should capture only a small number of things (one or
>   maybe two), and only capture by value should be used.
> 
> Hopefully I actually followed these.
> 
> I wonder if the new class should take a std::function rather than a
> gdb::function_view.  Some of the changes needs a bit of extra code to
> deal with the latter.
> 
> Also I wonder if it should be possible to default-construct a
> cleanup_function, and then set it later.  This would eliminate some
> uses of gdb::optional; and seemed maybe reasonable given that the
> class already has to maintain a (sort of-) validity flag.
> 
> This series includes a couple of cleanup-related comment fixes for
> things I happened to notice along the way.

I don't know C++ well enough to have an opinion on the general
questions asked here, but FWIW, the patch series in itself seems
like a nice improvement already (compared to before).

-- 
Joel

[PATCH 4/4] gdb/testsuite: Don't allow paths to appear in test name

[Andrew Burgess]

Having paths in the test names makes it harder to compare results
between two runs in different directories.  Give the test a name so
that the path doesn't appear.

gdb/ChangeLog:

	* gdb.base/style.exp: Don't include path in testname.
---
 gdb/testsuite/ChangeLog | 4 ++++
 1 file changed, 4 insertions(+)

[PATCH 3/4] gdb: Remove make_bpstat_clear_actions_cleanup

[Andrew Burgess]

This removes make_bpstat_clear_actions_cleanup, replacing it with uses
of cleanup_function.

gdb/ChangeLog:

	* breakpoint.c (bpstat_do_actions): Use
	bpstat_clear_actions_cleanup.
	* infrun.c (fetch_inferior_event): Likewise.
	* top.c (execute_command): Likewise.
	* breakpoint.h (bpstat_clear_actions_cleanup): Define.
	* utils.h (make_bpstat_clear_actions_cleanup): Don't declare.
	* utils.c (do_bpstat_clear_actions_cleanup): Delete
	(make_bpstat_clear_actions_cleanup): Delete.
---
 gdb/ChangeLog    | 12 ++++++++++++
 gdb/breakpoint.c |  4 ++--
 gdb/breakpoint.h |  9 +++++++++
 gdb/infrun.c     |  7 ++++---
 gdb/top.c        |  7 +++----
 gdb/utils.c      | 17 -----------------
 gdb/utils.h      |  1 -
 7 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1461695df1b..4ea26806449 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4468,7 +4468,7 @@ get_bpstat_thread ()
 void
 bpstat_do_actions (void)
 {
-  struct cleanup *cleanup_if_error = make_bpstat_clear_actions_cleanup ();
+  bpstat_clear_actions_cleanup cleanup_if_error;
   thread_info *tp;
 
   /* Do any commands attached to breakpoint we are stopped at.  */
@@ -4482,7 +4482,7 @@ bpstat_do_actions (void)
 	break;
     }
 
-  discard_cleanups (cleanup_if_error);
+  cleanup_if_error.cancel ();
 }
 
 /* Print out the (old or new) value associated with a watchpoint.  */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 5cbfb1ecfc6..a1d633e900f 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1093,6 +1093,15 @@ extern void bpstat_do_actions (void);
    not be performed.  */
 extern void bpstat_clear_actions (void);
 
+/* Cleanup class that calls bpstat_clear_actions.  */
+#ifdef __cpp_template_auto
+using bpstat_clear_actions_cleanup
+	= cleanup_function <bpstat_clear_actions>;
+#else
+using bpstat_clear_actions_cleanup
+	= cleanup_function <void (*) (void), bpstat_clear_actions>;
+#endif
+
 /* Implementation:  */
 
 /* Values used to tell the printing routine how to behave for this
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 150288264f4..b697145a147 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3758,9 +3758,9 @@ fetch_inferior_event (void *client_data)
 
   /* 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);
+  bpstat_clear_actions_cleanup defer_bpstat_clear;
+  struct cleanup *ts_old_chain
+    = 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);
@@ -3814,6 +3814,7 @@ fetch_inferior_event (void *client_data)
     }
 
   discard_cleanups (ts_old_chain);
+  defer_bpstat_clear.cancel ();
 
   /* No error, don't finish the thread states yet.  */
   finish_state.release ();
diff --git a/gdb/top.c b/gdb/top.c
index 900e78aaec5..1c2f60d4c60 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -539,12 +539,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 ();
+  bpstat_clear_actions_cleanup cleanup_if_error;
   scoped_value_mark cleanup = prepare_execute_command ();
 
   /* Force cleanup of any alloca areas if using C alloca instead of
@@ -554,7 +553,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.cancel ();
       return;
     }
 
@@ -649,7 +648,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.cancel ();
 }
 
 /* Run execute_command for P and FROM_TTY.  Capture its output into the
diff --git a/gdb/utils.c b/gdb/utils.c
index ed8d60fa7b0..4af75e34809 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 f2fe1da832c..896feb973c9 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);
 \f
 /* Path utilities.  */
 
-- 
2.14.5

[PATCH 2/4] gdb: Remove remaining cleanup from breakpoint.c

[Andrew Burgess]

The remaining null cleanup in breakpoint.c does not seem to protect
anything, so remove it.

gdb/ChangeLog:

2019-01-11  Tom Tromey  <tom@tromey.com>

	* breakpoint.c (create_breakpoint): Remove cleanup.
---
 gdb/ChangeLog    |  4 ++++
 gdb/breakpoint.c | 11 -----------
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3e9da1f99fa..1461695df1b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9230,7 +9230,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 +9279,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 +9377,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);
 
-- 
2.14.5

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Andrew Burgess]

I've been thinking about a similar idea for a while too.  I wondered
if there was a way we could make use of templates to generate some of
the common boiler plate cleanups.

This mini-series changes the first 4 of your patches to you this idea
so you can see how it might work.

First, the ideal, description, a new templated class
`cleanup_function` that allows you to do something like this:

    void
    delete_longjmp_breakpoint (int arg)
    {
      /* Blah, blah, blah...  */
    }

    using longjmp_breakpoint_cleanup
        = cleanup_function <delete_longjmp_breakpoint, int>;

This creates a new cleanup class `longjmp_breakpoint_cleanup` than can
then be used like this:

    longjmp_breakpoint_cleanup obj (thread);

    /* Blah, blah, blah...  */

    obj.cancel ();  /* Optional cancel if needed.  */

The cleanup_function class can take any number of arguments, and is
cancellable if needed, it can also take zero arguments.  In theory
this would allow any cleanup that only needs to hold some simple
arguments by value and call a static function to be made into a
`cleanup_function` specialisation.

Now the only slight problem is that the above requires C++17, which
isn't what we use right now. However, there is a C++11 alternative
which we can use for now, so you _actually_ have to write this:

    using longjmp_breakpoint_cleanup
        = cleanup_function <void (*) (int), delete_longjmp_breakpoint, int>;

The function signature is added in as pre-C++17 GCC doesn't support
`auto` template parameters.

The 4 patches below make use of `#ifdef __cpp_template_auto` to
demonstrate the pre and post C++17 code variants.  A final submission
would be based on pre-C++11 and include the function signatures, but
add a comment in the `cleanup_function` class that when we update the
C++ standard, we can update how the class is used.

How do you think this compares to your original patches?  Any interest
in this approach?

Thanks,
Andrew


--

Andrew Burgess (4):
  gdb: Remove delete_longjmp_breakpoint_cleanup
  gdb: Remove remaining cleanup from breakpoint.c
  gdb: Remove make_bpstat_clear_actions_cleanup
  gdb/testsuite: Don't allow paths to appear in test name

 gdb/ChangeLog                 |  29 +++++++++++
 gdb/breakpoint.c              |  26 +++-------
 gdb/breakpoint.h              |  19 +++++++
 gdb/common/cleanup-function.h | 118 ++++++++++++++++++++++++++++++++++++++++++
 gdb/infcmd.c                  |  12 +----
 gdb/inferior.h                |   2 -
 gdb/infrun.c                  |   7 +--
 gdb/testsuite/ChangeLog       |   4 ++
 gdb/top.c                     |   7 ++-
 gdb/utils.c                   |  17 ------
 gdb/utils.h                   |   1 -
 11 files changed, 187 insertions(+), 55 deletions(-)
 create mode 100644 gdb/common/cleanup-function.h

-- 
2.14.5

[PATCH 1/4] gdb: Remove delete_longjmp_breakpoint_cleanup

[Andrew Burgess]

This removes delete_longjmp_breakpoint_cleanup in favor of a new
cleanup function type.  This new type is somewhat generic, and will be
reused in subsequent patches.

Note that cleanup_function::cancel was purposely not named "reset".
An earlier version did this, but it was too easy to mistake
cleanup_function::reset and gdb::optional::reset.

gdb/ChangeLog:

	* breakpoint.c (until_break_command): Delete cleanup, use
	longjmp_breakpoint_cleanup instead.
	* breakpoint.h: Declare longjmp_breakpoint_cleanup class.
	* common/cleanup-function.h: New file.
	* infcmd.c (delete_longjmp_breakpoint_cleanup): Delete.
	(until_next_command): Delete cleanup, use
	longjmp_breakpoint_cleanup instead.
	* inferior.h (delete_longjmp_breakpoint_cleanup): Delete
	declaration.
---
 gdb/ChangeLog                 |  13 +++++
 gdb/breakpoint.c              |  11 ++--
 gdb/breakpoint.h              |  10 ++++
 gdb/common/cleanup-function.h | 118 ++++++++++++++++++++++++++++++++++++++++++
 gdb/infcmd.c                  |  12 +----
 gdb/inferior.h                |   2 -
 6 files changed, 149 insertions(+), 17 deletions(-)
 create mode 100644 gdb/common/cleanup-function.h

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2ab8a76326c..3e9da1f99fa 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11073,7 +11073,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 +11105,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 +11119,9 @@ until_break_command (const char *arg, int from_tty, int anywhere)
      one.  */
 
   breakpoint_up caller_breakpoint;
+
+  gdb::optional<longjmp_breakpoint_cleanup> lj_deleter;
+
   if (frame_id_p (caller_frame_id))
     {
       struct symtab_and_line sal2;
@@ -11136,7 +11136,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 +11159,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->cancel ();
 
   proceed (-1, GDB_SIGNAL_DEFAULT);
 }
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 97b6f3fbc64..5cbfb1ecfc6 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -30,6 +30,7 @@
 #include <vector>
 #include "common/array-view.h"
 #include "cli/cli-script.h"
+#include "common/cleanup-function.h"
 
 struct block;
 struct gdbpy_breakpoint_object;
@@ -1431,6 +1432,15 @@ extern void set_longjmp_breakpoint (struct thread_info *tp,
 				    struct frame_id frame);
 extern void delete_longjmp_breakpoint (int thread);
 
+/* Cleanup class that calls delete_longjmp_breakpoint.  */
+#ifdef __cpp_template_auto
+using longjmp_breakpoint_cleanup
+	= cleanup_function <delete_longjmp_breakpoint, int>;
+#else
+using longjmp_breakpoint_cleanup
+= cleanup_function <void (*) (int), delete_longjmp_breakpoint, int>;
+#endif
+
 /* Mark all longjmp breakpoints from THREAD for later deletion.  */
 extern void delete_longjmp_breakpoint_at_next_stop (int thread);
 
diff --git a/gdb/common/cleanup-function.h b/gdb/common/cleanup-function.h
new file mode 100644
index 00000000000..3a59648457c
--- /dev/null
+++ b/gdb/common/cleanup-function.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 <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_CLEANUP_FUNCTION_H
+#define COMMON_CLEANUP_FUNCTION_H
+
+#include <tuple>
+
+/* This is the general part of a recursive template to unpack the arguments
+   from the tuple PARAMS into ARGS.  The template parameter N is the number
+   of parameters left to unpack from PARAMS.  Each iteration takes an extra
+   parameter from PARAMS and passes it as an extra argument to
+   CALL_FUNCTION_ON_TUPLE::UNPACK_TUPLE, reducing N until we hit the base
+   case below.  */
+template <unsigned int N>
+struct call_function_on_tuple
+{
+  template <typename... FuncArgTypes, typename... UnpackedArgTypes>
+  static void unpack (void (*func) (FuncArgTypes... ),
+		      const std::tuple<FuncArgTypes...>& params,
+		      UnpackedArgTypes... args)
+  {
+    call_function_on_tuple<N-1>::unpack (func, params,
+					 std::get<N-1> (params),
+					 args...);
+  }
+};
+
+/* This is the base case specialisation, when we reach this point all of
+   the arguments have been extracted from PARAMS and are being passed in
+   ARGS.  It is worth noting that ARGS must be non-empty, thus this can't
+   be used to call FUNC if FUNC takes no parameters.  This special case is
+   handled within CLEANUP_FUNCTION::UNPACK_TUPLE below.  */
+template <>
+struct call_function_on_tuple<0>
+{
+  template <typename... FuncArgTypes, typename... UnpackedArgTypes>
+  static void unpack (void (*func) (FuncArgTypes...),
+		      const std::tuple<FuncArgTypes...>& params,
+		      UnpackedArgTypes... args)
+  {
+    func (args...);
+  }
+};
+
+/* In a C++17 world we can make use of the `auto` keyword in the template
+   parameter list to avoid having to pass the function type signature as a
+   separate template parameter.  */
+
+#ifdef __cpp_template_auto
+template <auto function, typename... Args>
+#else
+template <typename f_type, f_type function, typename... Args>
+#endif
+class cleanup_function
+{
+public:
+
+  explicit cleanup_function (Args... args)
+    : m_args (std::forward<Args>(args)...)
+  { /* Nothing.  */ }
+
+  ~cleanup_function ()
+  {
+    /* Maybe we should be try/catch around this?  */
+    if (!m_cancelled)
+      unpack_tuple_and_call (function, m_args);
+  }
+
+  /* Mark this cleanup as cancelled, the cleanup function will not be
+     called.  */
+  void cancel ()
+  {
+    m_cancelled = true;
+  }
+
+private:
+
+  /* This is the entry point for calling function F with the contents of
+     tuple PARAMS as arguments in the case where the tuple actually holds
+     some values.  */
+  template <typename... FuncArgTypes>
+  void unpack_tuple_and_call (void (*f) (FuncArgTypes...),
+			      std::tuple<FuncArgTypes...> const& params)
+  {
+    call_function_on_tuple<sizeof...(FuncArgTypes)>::unpack (f, params);
+  }
+
+  /* This is the entry point for calling function F for the case where F
+     takes no parameters.  */
+  template <typename = void>
+  void unpack_tuple_and_call (void (*f) (void), std::tuple<> const& params)
+  {
+    f ();
+  }
+
+  /* Storage for the arguments to the cleanup function */
+  std::tuple<Args...> m_args;
+
+  /* Has this cleanup been cancelled?  */
+  bool m_cancelled = false;
+};
+
+#endif /* COMMON_CLEANUP_FUNCTION_H */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 3c3add89ab8..9265be5585a 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -947,13 +947,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 +1510,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 +1548,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);
+  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.cancel ();
 
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
 }
diff --git a/gdb/inferior.h b/gdb/inferior.h
index a82df1a52a5..c63fa26b086 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -198,8 +198,6 @@ extern void continue_1 (int all_threads);
 
 extern void interrupt_target_1 (int all_threads);
 
-extern void delete_longjmp_breakpoint_cleanup (void *arg);
-
 extern void detach_command (const char *, int);
 
 extern void notice_new_inferior (struct thread_info *, int, int);
-- 
2.14.5

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> The cleanup_function class can take any number of arguments, and is
Andrew> cancellable if needed, it can also take zero arguments.  In theory
Andrew> this would allow any cleanup that only needs to hold some simple
Andrew> arguments by value and call a static function to be made into a
Andrew> `cleanup_function` specialisation.

It seems reasonable to me.  I think the advantage it has over my
approach is that the resulting classes are more similar to what we would
have written had we implemented a custom class to replace each cleanup.
Also it helps enforce the idea that we don't want cleanup_functions to
be used in an unrestricted way.

Andrew> How do you think this compares to your original patches?  Any interest
Andrew> in this approach?

Yes, let's do this.

Tom

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Tom Tromey]

Andrew> How do you think this compares to your original patches?  Any interest
Andrew> in this approach?

Tom> Yes, let's do this.

I happened to stumble across the message for commit
9bcb1f1630b05594fa86bfd017639cfcc966b11c today.  It references

  http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4189.pdf

... which proposes a generic scope guard class along these lines.
So, now I wonder if we should just reuse that paper's contents, but in
the gdb namespace.

Tom

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2019-01-12 15:41:43 -0700]:

> Andrew> How do you think this compares to your original patches?  Any interest
> Andrew> in this approach?
> 
> Tom> Yes, let's do this.
> 
> I happened to stumble across the message for commit
> 9bcb1f1630b05594fa86bfd017639cfcc966b11c today.  It references
> 
>   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4189.pdf
> 
> ... which proposes a generic scope guard class along these lines.
> So, now I wonder if we should just reuse that paper's contents, but in
> the gdb namespace.

Interesting, there's a revised version here:

  http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0052r5.pdf

Which includes details about how the technique can be expanded to
effectively give us `cleanup_on_failure` and/or `cleanup_on_success`,
which would allow us to completely remove the use of the current
`.reset ()` or `.cancel ()` functions.

Thanks,
Andrew

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Pedro Alves]

On 01/14/2019 11:06 AM, Andrew Burgess wrote:
> * Tom Tromey <tom@tromey.com> [2019-01-12 15:41:43 -0700]:
> 
>> Andrew> How do you think this compares to your original patches?  Any interest
>> Andrew> in this approach?
>>
>> Tom> Yes, let's do this.
>>
>> I happened to stumble across the message for commit
>> 9bcb1f1630b05594fa86bfd017639cfcc966b11c today.  It references
>>
>>   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4189.pdf
>>
>> ... which proposes a generic scope guard class along these lines.
>> So, now I wonder if we should just reuse that paper's contents, but in
>> the gdb namespace.

Ah, I was just about to point at that paper, and at Alexandrescu's
ScopeGuard/SCOPE_EXIT.

> 
> Interesting, there's a revised version here:
> 
>   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0052r5.pdf
> 
> Which includes details about how the technique can be expanded to
> effectively give us `cleanup_on_failure` and/or `cleanup_on_success`,
> which would allow us to completely remove the use of the current
> `.reset ()` or `.cancel ()` functions.

IIRC, to implement that correctly we'd need std::uncaught_exceptions() [plural, C++17],
the older std::uncaught_exception() [singular] is flawed.

See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4152.pdf .

Thanks,
Pedro Alves

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Pedro Alves]

On 01/12/2019 11:50 AM, Andrew Burgess wrote:
> I've been thinking about a similar idea for a while too.  I wondered
> if there was a way we could make use of templates to generate some of
> the common boiler plate cleanups.
> 
> This mini-series changes the first 4 of your patches to you this idea
> so you can see how it might work.
> 
> First, the ideal, description, a new templated class
> `cleanup_function` that allows you to do something like this:
> 
>     void
>     delete_longjmp_breakpoint (int arg)
>     {
>       /* Blah, blah, blah...  */
>     }
> 
>     using longjmp_breakpoint_cleanup
>         = cleanup_function <delete_longjmp_breakpoint, int>;

It seems unnecessary to pass in the types of the arguments of
delete_longjmp_breakpoint.  Couldn't those be extracted from
delete_longjmp_breakpoint's type?  See below.

> 
> This creates a new cleanup class `longjmp_breakpoint_cleanup` than can
> then be used like this:
> 
>     longjmp_breakpoint_cleanup obj (thread);

I think this results in an inferior cleanup_function solution, because this
way you can't pass in a bespoke small lambda on the spot.  I.e. you're
forced to create a cleanup with a named function -- right?

If it's the lambda itself you don't like, I think it should be
possible to add make cleanup_function's ctor have a std::bind-like interface [1],
so that you'd pass cleanup_function's ctor the function and arguments:

  template <typename F, typename... Args>
  cleanup_function (F &&func, Args &&...args);

so you'd create the cleanup like:

  cleanup_function cleanup (delete_longjmp_breakpoint, thread);

Since Tromey's cleanup_function is not a template, to implement such
a ctor, it would have to rely on std::function (or something like it) for
type erasure, which might heap allocate if the resulting callable is large
enough.  The advantage would be that with that you can create a cleanup_function
without passing specifying the called function's type.  The disadvantage is
the cost of the std::function type erasure / potential heap allocation, of course.

But we could avoid the cost/heap if we make cleanup_function a template,
as  Andrew's version is, but I have to say that I don't really like that
version's way of declaring the cleanup_function typedef:

 +/* Cleanup class that calls delete_longjmp_breakpoint.  */
 +#ifdef __cpp_template_auto
 +using longjmp_breakpoint_cleanup
 +	= cleanup_function <delete_longjmp_breakpoint, int>;
 +#else
 +using longjmp_breakpoint_cleanup
 += cleanup_function <void (*) (int), delete_longjmp_breakpoint, int>;
 +#endif

I think it should be possible to code cleanup_function's template
such that you could instantiate it like:

  cleanup_function<void (int)>

similarly to gdb::function_view?

That doesn't tie cleanup_function to the actual function called, just
its type, but I wouldn't see that as a disadvantage, given this way this
works with all sorts of callables, including lambdas.

Now, ctors don't deduce template parameter types until C++17, so with
that template interface and C++11 we wouldn't be able to just write:

  cleanup_function cleanup (delete_longjmp_breakpoint, 0);

But, that is fixable with a helper make_cleanup_function, which would
have us write:

  auto cleanup = make_cleanup_function (delete_longjmp_breakpoint, 0);

For the optional cleanup case, we'd need to somehow spell out the
type, no way around it, but that's not too horrible with that interface,
IMO:
  cleanup_function<void (int)> cleanup;
or:
  cleanup_function<decltype (delete_longjmp_breakpoint)> cleanup;

(and of course a typedef could put the function's type away from view)

I'm not really sure we need a std::bind-like interface though.  I'd
be super fine with the implementation simplicity of having to pass a
lambda, like in scope_exit:

  auto cleanup
    = make_scope_exit ([] { delete_longjmp_breakpoint (0); });
  ... 
  cleanup.release ();

It's simpler to implement, because then all you need for the
template parameter type is a generic callable:

 template <typename Callable>
 class scope_exit
 ...

Note that with Alexandrescu's scope_exit (see
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4152.pdf), if you
don't need to cancel the cleanup, you can write:

 SCOPE_EXIT { delete_longjmp_breakpoint (0); };

which arguably looks cleaner.  Some people prefer avoiding macros
that "extend" the C++ language like that, though, I think.

[1] https://en.cppreference.com/w/cpp/utility/functional/bind

Thanks,
Pedro Alves

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2019-01-14 20:37:41 +0000]:

> On 01/12/2019 11:50 AM, Andrew Burgess wrote:
> > I've been thinking about a similar idea for a while too.  I wondered
> > if there was a way we could make use of templates to generate some of
> > the common boiler plate cleanups.
> > 
> > This mini-series changes the first 4 of your patches to you this idea
> > so you can see how it might work.
> > 
> > First, the ideal, description, a new templated class
> > `cleanup_function` that allows you to do something like this:
> > 
> >     void
> >     delete_longjmp_breakpoint (int arg)
> >     {
> >       /* Blah, blah, blah...  */
> >     }
> > 
> >     using longjmp_breakpoint_cleanup
> >         = cleanup_function <delete_longjmp_breakpoint, int>;
> 
> It seems unnecessary to pass in the types of the arguments of
> delete_longjmp_breakpoint.  Couldn't those be extracted from
> delete_longjmp_breakpoint's type?  See below.
> 
> > 
> > This creates a new cleanup class `longjmp_breakpoint_cleanup` than can
> > then be used like this:
> > 
> >     longjmp_breakpoint_cleanup obj (thread);
> 
> I think this results in an inferior cleanup_function solution, because this
> way you can't pass in a bespoke small lambda on the spot.  I.e. you're
> forced to create a cleanup with a named function -- right?

Thanks for taking the time to provide this feedback.  I think what you
are saying aligns with Tom's final position, that passing a function
pointer or lambda to the cleanup_function constructor is better.

My motivation for bringing this patch set up at all was to address
Tom's comment in this email:

   https://www.sourceware.org/ml/gdb-patches/2019-01/msg00156.html

Where he said:

    I looked at replacing scoped_finish_thread_state, but that one
    seemed reasonable to keep as is, to me, because it is used in
    several spots, and I didn't want to repeat the similar lambdas
    everywhere.

By passing the function pointer into the type creation I had hoped to
address this concern.

Maybe there's some other reason why scoped_finish_thread_state is
different, in which case I apologise for wasting everyone's time, but
right now it appears to me that scoped_finish_thread_state is no
different to cleanup_function, it's just used more.

I think if we're going to put in a generic solution (which I think is
a good thing) then we should either, make sure we understand why
scoped_finish_thread_state is different (and what the rules are for
when to use the generic, and when to create a class), or, make sure
the generic is suitable to replace scoped_finish_thread_state.

(I'm not trying to pick on scoped_finish_thread_state, it was just the
first example I found when originally replying to Tom.)

Thanks,
Andrew


> 
> If it's the lambda itself you don't like, I think it should be
> possible to add make cleanup_function's ctor have a std::bind-like interface [1],
> so that you'd pass cleanup_function's ctor the function and arguments:
> 
>   template <typename F, typename... Args>
>   cleanup_function (F &&func, Args &&...args);
> 
> so you'd create the cleanup like:
> 
>   cleanup_function cleanup (delete_longjmp_breakpoint, thread);
> 
> Since Tromey's cleanup_function is not a template, to implement such
> a ctor, it would have to rely on std::function (or something like it) for
> type erasure, which might heap allocate if the resulting callable is large
> enough.  The advantage would be that with that you can create a cleanup_function
> without passing specifying the called function's type.  The disadvantage is
> the cost of the std::function type erasure / potential heap allocation, of course.
> 
> But we could avoid the cost/heap if we make cleanup_function a template,
> as  Andrew's version is, but I have to say that I don't really like that
> version's way of declaring the cleanup_function typedef:
> 
>  +/* Cleanup class that calls delete_longjmp_breakpoint.  */
>  +#ifdef __cpp_template_auto
>  +using longjmp_breakpoint_cleanup
>  +	= cleanup_function <delete_longjmp_breakpoint, int>;
>  +#else
>  +using longjmp_breakpoint_cleanup
>  += cleanup_function <void (*) (int), delete_longjmp_breakpoint, int>;
>  +#endif
> 
> I think it should be possible to code cleanup_function's template
> such that you could instantiate it like:
> 
>   cleanup_function<void (int)>
> 
> similarly to gdb::function_view?
> 
> That doesn't tie cleanup_function to the actual function called, just
> its type, but I wouldn't see that as a disadvantage, given this way this
> works with all sorts of callables, including lambdas.
> 
> Now, ctors don't deduce template parameter types until C++17, so with
> that template interface and C++11 we wouldn't be able to just write:
> 
>   cleanup_function cleanup (delete_longjmp_breakpoint, 0);
> 
> But, that is fixable with a helper make_cleanup_function, which would
> have us write:
> 
>   auto cleanup = make_cleanup_function (delete_longjmp_breakpoint, 0);
> 
> For the optional cleanup case, we'd need to somehow spell out the
> type, no way around it, but that's not too horrible with that interface,
> IMO:
>   cleanup_function<void (int)> cleanup;
> or:
>   cleanup_function<decltype (delete_longjmp_breakpoint)> cleanup;
> 
> (and of course a typedef could put the function's type away from view)
> 
> I'm not really sure we need a std::bind-like interface though.  I'd
> be super fine with the implementation simplicity of having to pass a
> lambda, like in scope_exit:
> 
>   auto cleanup
>     = make_scope_exit ([] { delete_longjmp_breakpoint (0); });
>   ... 
>   cleanup.release ();
> 
> It's simpler to implement, because then all you need for the
> template parameter type is a generic callable:
> 
>  template <typename Callable>
>  class scope_exit
>  ...
> 
> Note that with Alexandrescu's scope_exit (see
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4152.pdf), if you
> don't need to cancel the cleanup, you can write:
> 
>  SCOPE_EXIT { delete_longjmp_breakpoint (0); };
> 
> which arguably looks cleaner.  Some people prefer avoiding macros
> that "extend" the C++ language like that, though, I think.
> 
> [1] https://en.cppreference.com/w/cpp/utility/functional/bind
> 
> Thanks,
> Pedro Alves

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> 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.

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?

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).

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.

thanks,
Tom

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Pedro Alves]

On 01/15/2019 11:03 PM, Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> 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<decltype (delete_longjmp_breakpoint),
                              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.

> 
> 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?
> 
> 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).
> 
> 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 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 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.

Thanks,
Pedro Alves

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2019-01-15 23:43:07 +0000]:

> On 01/15/2019 11:03 PM, Tom Tromey wrote:
> >>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> 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<decltype (delete_longjmp_breakpoint),
>                               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.
> 
> > 
> > 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?
> > 
> > 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).
> > 
> > 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 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 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'm happy with either approach so long as it opens the door for
removing cleanup classes that are just calling a global function.

Thanks for all the work on this.

Thanks,
Andrew

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Pedro Alves]

On 01/15/2019 11:43 PM, Pedro Alves wrote:
> On 01/15/2019 11:03 PM, Tom Tromey wrote:
>>>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> 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<decltype (delete_longjmp_breakpoint),
>                               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 <palves@redhat.com>
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<delete_longjmp_breakpoint_cleanup> 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 <http://www.gnu.org/licenses/>.  */
+
+#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<longjmp_breakpoint_cleanup> 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<scope_exit<what goes here?>>
+*/
+
+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<typename Function, Function *function, typename Signature>
+struct forward_scope_exit;
+
+template<typename Function, Function *function,
+	 typename Res, typename... Args>
+class forward_scope_exit<Function, function, Res (Args...)>
+  : public scope_exit_base<forward_scope_exit<Function,
+					      function,
+					      Res (Args...)>>
+{
+  /* For access to on_exit().  */
+  friend scope_exit_base<forward_scope_exit<Function,
+					    function,
+					    Res (Args...)>>;
+
+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<Args> ()...))
+    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<decltype (FUNC), FUNC, decltype (FUNC)>
+
+#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 <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_SCOPE_EXIT_H
+#define COMMON_SCOPE_EXIT_H
+
+#include <functional>
+#include <type_traits>
+#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 ( <function, function object, lambda> );
+     ...
+     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 <typename CRTP>
+class scope_exit_base
+{
+public:
+  scope_exit_base () = default;
+
+  ~scope_exit_base ()
+  {
+    if (!m_released)
+      {
+	auto *self = static_cast<CRTP *> (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<typename EF>
+class scope_exit : public scope_exit_base<scope_exit<EF>>
+{
+  /* For access to on_exit().  */
+  friend scope_exit_base<scope_exit<EF>>;
+
+public:
+
+  template<typename EFP,
+	   typename = gdb::Requires<std::is_constructible<EF, EFP>>>
+  scope_exit (EFP &&f)
+    try : m_exit_function ((!std::is_lvalue_reference<EFP>::value
+			    && std::is_nothrow_constructible<EF, EFP>::value)
+			   ? std::move (f)
+			   : f)
+  {
+  }
+  catch (...)
+    {
+      /* "If the initialization of exit_function throws an exception,
+	 calls f()."  */
+      f ();
+    }
+
+  template<typename EFP,
+	   typename = gdb::Requires<std::is_constructible<EF, EFP>>>
+  scope_exit (scope_exit &&rhs)
+    noexcept (std::is_nothrow_move_constructible<EF>::value
+	      || std::is_nothrow_copy_constructible<EF>::value)
+    : m_exit_function (std::is_nothrow_constructible<EFP>::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<scope_exit<EF>> (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 <typename EF>
+scope_exit<typename std::decay<EF>::type>
+make_scope_exit (EF &&f)
+{
+  return scope_exit<typename std::decay<EF>::type> (std::forward<EF> (f));
+}
+
+namespace detail
+{
+
+enum class scope_exit_lhs {};
+
+template<typename EF>
+scope_exit<typename std::decay<EF>::type>
+operator+ (scope_exit_lhs, EF &&rhs)
+{
+  return scope_exit<typename std::decay<EF>::type> (std::forward<EF> (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 <algorithm>
+#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 (&current_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<scoped_restore_current_traceframe> maybe_restore_traceframe;
-  if (non_stop)
-    {
-      maybe_restore_traceframe.emplace ();
-      set_current_traceframe (-1);
-    }
-
-  gdb::optional<scoped_restore_current_thread> 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<scoped_restore_current_traceframe> 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<scoped_restore_current_thread> 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<static_tracepoint_marker>
 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<static_tracepoint_marker> 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 <sys/types.h>
 #include <fcntl.h>
@@ -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<clear_symtab_users_cleanup> defer_clear_users;
+
   std::unique_ptr<struct objfile> 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<struct objfile> 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);
-}
 \f
 /* 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<ui_out_type Type>
 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);
 \f
 /* Path utilities.  */
 
-- 
2.14.4

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> See patch below.  Since cleanup_function turned into scope_exit,
Pedro> this new class became forward_scope_exit, because it's a "forwarding"
Pedro> version of scope_exit?  I'm really not that sure about that name...
Pedro> wrap_scope_exit came to mind too.  Or maybe call it cleanup_function?

The name seems reasonable enough to me.

Pedro> Ideally we'd find a way to implement SCOPE_FAILURE/SCOPE_SUCCESS
Pedro> in C++11 instead...

Maybe we should start a C++17 meta-bug to collect future changes.

Pedro> I think this is the first time I had found a use for a function-try-block,
Pedro> see scope_exit ctor...

TIL!

Pedro> Here is patch with everything together.  WDYT?

I think it all looks good.  One nit and one question...

Pedro> +/* A forward_scope_exit is like scope_exit, but instead of giving it a
Pedro> +   callable, you instead specialize it for a given cleanup function,
Pedro> +   and the generated class automatically has a constructor with the

I think it would make sense to explain the origin of the "forward" name
somewhere in this comment.

Pedro> +#define SCOPE_EXIT \
Pedro> +  auto CONCAT(scope_exit_, __LINE__) = ::detail::scope_exit_lhs () + [&] ()
[...]
Pedro> +  auto invalidator
Pedro> +    = make_scope_exit ([&] { this->invalidate (regnum); });
 
In the current spots it doesn't matter, but I tend to think it's better
to capture by value rather than by reference.  The local being captured
might well be reused in the function.

On the other hand, this would be a difference from the spec.

Tom

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2019-01-16 23:10:23 +0000]:

> On 01/15/2019 11:43 PM, Pedro Alves wrote:
> > On 01/15/2019 11:03 PM, Tom Tromey wrote:
> >>>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> 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<decltype (delete_longjmp_breakpoint),
> >                               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?

I think this patch looks good.

Thanks for taking this on.

Andrew


> 
> From c616fefa268155eea243dedfb6ff54e133ee33f9 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> 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<delete_longjmp_breakpoint_cleanup> 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 <http://www.gnu.org/licenses/>.  */
> +
> +#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<longjmp_breakpoint_cleanup> 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<scope_exit<what goes here?>>
> +*/
> +
> +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<typename Function, Function *function, typename Signature>
> +struct forward_scope_exit;
> +
> +template<typename Function, Function *function,
> +	 typename Res, typename... Args>
> +class forward_scope_exit<Function, function, Res (Args...)>
> +  : public scope_exit_base<forward_scope_exit<Function,
> +					      function,
> +					      Res (Args...)>>
> +{
> +  /* For access to on_exit().  */
> +  friend scope_exit_base<forward_scope_exit<Function,
> +					    function,
> +					    Res (Args...)>>;
> +
> +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<Args> ()...))
> +    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<decltype (FUNC), FUNC, decltype (FUNC)>
> +
> +#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 <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef COMMON_SCOPE_EXIT_H
> +#define COMMON_SCOPE_EXIT_H
> +
> +#include <functional>
> +#include <type_traits>
> +#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 ( <function, function object, lambda> );
> +     ...
> +     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 <typename CRTP>
> +class scope_exit_base
> +{
> +public:
> +  scope_exit_base () = default;
> +
> +  ~scope_exit_base ()
> +  {
> +    if (!m_released)
> +      {
> +	auto *self = static_cast<CRTP *> (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<typename EF>
> +class scope_exit : public scope_exit_base<scope_exit<EF>>
> +{
> +  /* For access to on_exit().  */
> +  friend scope_exit_base<scope_exit<EF>>;
> +
> +public:
> +
> +  template<typename EFP,
> +	   typename = gdb::Requires<std::is_constructible<EF, EFP>>>
> +  scope_exit (EFP &&f)
> +    try : m_exit_function ((!std::is_lvalue_reference<EFP>::value
> +			    && std::is_nothrow_constructible<EF, EFP>::value)
> +			   ? std::move (f)
> +			   : f)
> +  {
> +  }
> +  catch (...)
> +    {
> +      /* "If the initialization of exit_function throws an exception,
> +	 calls f()."  */
> +      f ();
> +    }
> +
> +  template<typename EFP,
> +	   typename = gdb::Requires<std::is_constructible<EF, EFP>>>
> +  scope_exit (scope_exit &&rhs)
> +    noexcept (std::is_nothrow_move_constructible<EF>::value
> +	      || std::is_nothrow_copy_constructible<EF>::value)
> +    : m_exit_function (std::is_nothrow_constructible<EFP>::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<scope_exit<EF>> (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 <typename EF>
> +scope_exit<typename std::decay<EF>::type>
> +make_scope_exit (EF &&f)
> +{
> +  return scope_exit<typename std::decay<EF>::type> (std::forward<EF> (f));
> +}
> +
> +namespace detail
> +{
> +
> +enum class scope_exit_lhs {};
> +
> +template<typename EF>
> +scope_exit<typename std::decay<EF>::type>
> +operator+ (scope_exit_lhs, EF &&rhs)
> +{
> +  return scope_exit<typename std::decay<EF>::type> (std::forward<EF> (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 <algorithm>
> +#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 (&current_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<scoped_restore_current_traceframe> maybe_restore_traceframe;
> -  if (non_stop)
> -    {
> -      maybe_restore_traceframe.emplace ();
> -      set_current_traceframe (-1);
> -    }
> -
> -  gdb::optional<scoped_restore_current_thread> 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<scoped_restore_current_traceframe> 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<scoped_restore_current_thread> 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<static_tracepoint_marker>
>  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<static_tracepoint_marker> 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 <sys/types.h>
>  #include <fcntl.h>
> @@ -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<clear_symtab_users_cleanup> defer_clear_users;
> +
>    std::unique_ptr<struct objfile> 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<struct objfile> 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);
> -}
>  \f
>  /* 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<ui_out_type Type>
>  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);
>  \f
>  /* Path utilities.  */
>  
> -- 
> 2.14.4
> 

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Pedro Alves]

On 01/17/2019 09:39 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> See patch below.  Since cleanup_function turned into scope_exit,
> Pedro> this new class became forward_scope_exit, because it's a "forwarding"
> Pedro> version of scope_exit?  I'm really not that sure about that name...
> Pedro> wrap_scope_exit came to mind too.  Or maybe call it cleanup_function?
> 
> The name seems reasonable enough to me.
> 
> Pedro> Ideally we'd find a way to implement SCOPE_FAILURE/SCOPE_SUCCESS
> Pedro> in C++11 instead...
> 
> Maybe we should start a C++17 meta-bug to collect future changes.
> 
> Pedro> I think this is the first time I had found a use for a function-try-block,
> Pedro> see scope_exit ctor...
> 
> TIL!
> 
> Pedro> Here is patch with everything together.  WDYT?
> 
> I think it all looks good.  One nit and one question...
> 
> Pedro> +/* A forward_scope_exit is like scope_exit, but instead of giving it a
> Pedro> +   callable, you instead specialize it for a given cleanup function,
> Pedro> +   and the generated class automatically has a constructor with the
> 
> I think it would make sense to explain the origin of the "forward" name
> somewhere in this comment.

I added this locally:

diff --git c/gdb/common/forward-scope-exit.h w/gdb/common/forward-scope-exit.h
index 6b51e296b6..5ce230f0dc 100644
--- c/gdb/common/forward-scope-exit.h
+++ w/gdb/common/forward-scope-exit.h
@@ -64,7 +64,11 @@
    callable template type when you create the gdb::optional:
 
      gdb:optional<scope_exit<what goes here?>>
-*/
+
+   The "forward" naming fits both purposes shown above -- the class
+   "forwards" ctor arguments to the wrapped cleanup function at scope
+   exit time, and can also be used to "forward declare"
+   scope_exit-like objects.  */
 
 namespace detail
 {



> 
> Pedro> +#define SCOPE_EXIT \
> Pedro> +  auto CONCAT(scope_exit_, __LINE__) = ::detail::scope_exit_lhs () + [&] ()
> [...]
> Pedro> +  auto invalidator
> Pedro> +    = make_scope_exit ([&] { this->invalidate (regnum); });
>  
> In the current spots it doesn't matter, but I tend to think it's better
> to capture by value rather than by reference.  


Capturing by value would mean that you wouldn't be able to refer to
objects of non-copyable types, though.  E.g., std::unique_ptr.  Unless
we took the effort to capture pointers to such objects, of course.
I don't think the result would be as nice.  The nice thing about capturing
by reference is that it always Just Works with no syntax overhead.


> The local being captured
> might well be reused in the function.
> 

Indeed, that's a valid concern.  How about documenting it, something like:

 -/* Register a block of code to run on scope exit.  */
 +/* Register a block of code to run on scope exit.  Note that the local
 +   context is captured by reference, which means you should be careful
 +   to avoid the case of locals being captured being inadvertently
 +   reused/changed in the function before the scope exit runs.  */
 
 #define SCOPE_EXIT \

?

Maybe there's a way to simplify that sentence.

> On the other hand, this would be a difference from the spec.

Yeah.  The SCOPE_EXIT macro itself isn't part of the p0052r5 proposal AFAICT.

It's part of Alexandruscu's original SCOPE_EXIT idea though, shown on
slide 20/46 (pdf page 14) of:
 http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4152.pdf

IMHO diverging would cause people familiar with the idea confused.

I'm working on merging my changes to each appropriate patch of
your series.

Thanks,
Pedro Alves

Re: [PATCH 00/12] remove some cleanups using a cleanup function

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Capturing by value would mean that you wouldn't be able to refer to
Pedro> objects of non-copyable types, though.  E.g., std::unique_ptr.  Unless
Pedro> we took the effort to capture pointers to such objects, of course.
Pedro> I don't think the result would be as nice.  The nice thing about capturing
Pedro> by reference is that it always Just Works with no syntax overhead.

Yeah, good point.

Pedro> Indeed, that's a valid concern.  How about documenting it, something like:

Pedro>  -/* Register a block of code to run on scope exit.  */
Pedro>  +/* Register a block of code to run on scope exit.  Note that the local
Pedro>  +   context is captured by reference, which means you should be careful
Pedro>  +   to avoid the case of locals being captured being inadvertently
Pedro>  +   reused/changed in the function before the scope exit runs.  */
 
Pedro>  #define SCOPE_EXIT \

Pedro> ?

Pedro> Maybe there's a way to simplify that sentence.

How about:

  ... be careful to avoid inadvertently changing a captured local's
  value before the scope exit runs.

Pedro> I'm working on merging my changes to each appropriate patch of
Pedro> your series.

Thanks!  I think there's one or two more cleanups that can be converted
this way, after this goes in.

Tom