diff mbox

[RFA,02/22] Use RAII to save and restore scalars

Message ID 1474949330-4307-3-git-send-email-tom@tromey.com
State New
Headers show

Commit Message

Tom Tromey Sept. 27, 2016, 4:08 a.m. UTC
This patch replaces many (but not all) uses of
make_cleanup_restore_integer with a simple RAII-based template class.
It also removes the similar restore_execution_direction cleanup in
favor of this new class.  Subsequent patches will replace other
similar cleanups with this class.

I chose the name "scoped_restore" for this class.  I considered that
perhaps "auto_restore" might be clearer.  Or maybe something else;
it's simple enough to change.

I had hoped that template parameter deduction would work here, but it
did not, and so the patch uses explicit template parameters
everywhere.

2016-09-26  Tom Tromey  <tom@tromey.com>

	* utils.h (class scoped_restore): New class.
	* top.c (execute_command_to_string): Use scoped_restore.
	* python/python.c (python_interactive_command): Use
	scoped_restore.
	(python_command, execute_gdb_command): Likewise.
	* printcmd.c (do_one_display): Use scoped_restore.
	* mi/mi-main.c (exec_continue): Use scoped_restore.
	* mi/mi-cmd-var.c (mi_cmd_var_assign): Use scoped_restore.
	* linux-fork.c (checkpoint_command): Use scoped_restore.
	* infrun.c (restore_execution_direction): Remove.
	(fetch_inferior_event): Use scoped_restore.
	* compile/compile.c (compile_file_command): Use
	scoped_restore.
	(compile_code_command, compile_print_command): Likewise.
	* cli/cli-script.c (execute_user_command): Use
	scoped_restore.
	(while_command, if_command, script_from_file): Likewise.
	* arm-tdep.c (arm_insert_single_step_breakpoint): Use
	scoped_restore.
---
 gdb/ChangeLog         | 22 ++++++++++++++++++++++
 gdb/arm-tdep.c        |  8 ++------
 gdb/cli/cli-script.c  | 18 ++++--------------
 gdb/compile/compile.c | 23 ++++++++---------------
 gdb/infrun.c          | 16 ++--------------
 gdb/linux-fork.c      | 11 ++++++-----
 gdb/mi/mi-cmd-var.c   |  7 +------
 gdb/mi/mi-main.c      |  3 +--
 gdb/printcmd.c        |  5 +----
 gdb/python/python.c   | 12 +++---------
 gdb/top.c             |  3 +--
 gdb/utils.h           | 44 ++++++++++++++++++++++++++++++++++++++++++++
 12 files changed, 95 insertions(+), 77 deletions(-)

Comments

Trevor Saunders Sept. 27, 2016, 8:59 a.m. UTC | #1
On Mon, Sep 26, 2016 at 10:08:30PM -0600, Tom Tromey wrote:
> This patch replaces many (but not all) uses of
> make_cleanup_restore_integer with a simple RAII-based template class.
> It also removes the similar restore_execution_direction cleanup in
> favor of this new class.  Subsequent patches will replace other
> similar cleanups with this class.
> 
> I chose the name "scoped_restore" for this class.  I considered that
> perhaps "auto_restore" might be clearer.  Or maybe something else;
> it's simple enough to change.

Its worth noting dmalcolm wants to add something similar to gcc for his
rtl front end work.

> I had hoped that template parameter deduction would work here, but it
> did not, and so the patch uses explicit template parameters
> everywhere.

yeah, you can't deduce template args from a constructor in C++

> +  // Create a new scoped_restore object that saves the current value
> +  // of *VAR, and then sets *VAR to VALUE.  *VAR will be restored when
> +  // this scoped_restore object is destroyed.
> +  scoped_restore (T *var, T value)
> +    : saved_var (var),
> +      saved_value (*var)
> +  {
> +    *var = value;

I wonder if it isn't clearer to create the scoped_restore and then set
the variable yourself.

> +  // The saved variable.
> +  T *saved_var;
> +
> +  // The saved value.
> +  T saved_value;

 I think you could use const T, but I doubt it matters, these objects
 really shouldn't have there address taken so the compiler should see
 everything about them.

Trev
Pedro Alves Sept. 30, 2016, 1:17 a.m. UTC | #2
On 09/27/2016 09:59 AM, Trevor Saunders wrote:

>> I had hoped that template parameter deduction would work here, but it
>> did not, and so the patch uses explicit template parameters
>> everywhere.
> 
> yeah, you can't deduce template args from a constructor in C++

I don't know whether the trick below is usual, but you can work
around that and end up with no-template-args-in-sight usage
like this:

void
foo ()
{
  scoped_restore scope = make_scoped_restore (&some_global);

  ...
}



You'd do something like this:


struct scoped_restore_base {};

template<typename T>
class scoped_restore_ : public scoped_restore_base
{
 public:
...
  explicit scoped_restore (T *var)
    : saved_var (var),
      saved_value (*var)
  {}

  ~scoped_restore ()
  {
    *saved_var = saved_value;
  }

 private:
  scoped_restore &operator= (const scoped_restore &rhs);

  // The saved variable.
  T *saved_var;

  // The saved value.
  T saved_value;
};

typedef const scoped_restore_base &scoped_restore;

template<class T>
scoped_restore_<T> make_scoped_restore (T *var)
{
  return scoped_restore_<T> (var);
}

The trick is making scoped_restore_ inherit a non-template
class, and make "scoped_restore" a typedef for a const reference
to the base class, since a const reference can bind to the
temporary that is returned by make_scoped_restore, and it
extends the temporary's lifetime.

While copy elision / RVO makes it so that the temporary returned by
value from make_scoped_restore is constructed directly in the caller
instead of constructed and then copied out, you still have to
provide the copy constructor, instead of "deleting" it as in your
original patch.  Copy elision was optional until C++17, but I
believe all compilers worth their salt implement it.  And if a compiler
doesn't implement it, it's not a real issue, anyway, the code
still works.

The downside is that printing a scoped_restore object from
gdb will just print the reference using the base type, so you
won't see stored fields.

(gdb) p scope
$1 = (const scoped_restore) @0x7fffffffd910: {<No data fields>}

To see the object's fields you'd have to downcast it manually, like:

(gdb) p (scoped_restore_<ui_out *>) scope
$1 = {<scoped_restore_base> = {<No data fields>}, saved_var = 0x601050 <current_uiout>, saved_value = 0x0}

Not sure whether that's an issue.

Thanks,
Pedro Alves
Pedro Alves Sept. 30, 2016, 2:45 a.m. UTC | #3
On 09/30/2016 02:17 AM, Pedro Alves wrote:
> On 09/27/2016 09:59 AM, Trevor Saunders wrote:
> 
>>> I had hoped that template parameter deduction would work here, but it
>>> did not, and so the patch uses explicit template parameters
>>> everywhere.
>>
>> yeah, you can't deduce template args from a constructor in C++
> 
> I don't know whether the trick below is usual, 

Looks like it is -- Andrei Alexandrescu's ScopeGuard uses the
same trick:

  http://www.drdobbs.com/cpp/generic-change-the-way-you-write-excepti/184403758

Thanks,
Pedro Alves

> but you can work
> around that and end up with no-template-args-in-sight usage
> like this:
> 
> void
> foo ()
> {
>   scoped_restore scope = make_scoped_restore (&some_global);
> 
>   ...
> }
> 
> You'd do something like this:
> 
> 
> struct scoped_restore_base {};
> 
> template<typename T>
> class scoped_restore_ : public scoped_restore_base
> {
>  public:
> ...
>   explicit scoped_restore (T *var)
>     : saved_var (var),
>       saved_value (*var)
>   {}
> 
>   ~scoped_restore ()
>   {
>     *saved_var = saved_value;
>   }
> 
>  private:
>   scoped_restore &operator= (const scoped_restore &rhs);
> 
>   // The saved variable.
>   T *saved_var;
> 
>   // The saved value.
>   T saved_value;
> };
> 
> typedef const scoped_restore_base &scoped_restore;
> 
> template<class T>
> scoped_restore_<T> make_scoped_restore (T *var)
> {
>   return scoped_restore_<T> (var);
> }
> 
> The trick is making scoped_restore_ inherit a non-template
> class, and make "scoped_restore" a typedef for a const reference
> to the base class, since a const reference can bind to the
> temporary that is returned by make_scoped_restore, and it
> extends the temporary's lifetime.
> 
> While copy elision / RVO makes it so that the temporary returned by
> value from make_scoped_restore is constructed directly in the caller
> instead of constructed and then copied out, you still have to
> provide the copy constructor, instead of "deleting" it as in your
> original patch.  Copy elision was optional until C++17, but I
> believe all compilers worth their salt implement it.  And if a compiler
> doesn't implement it, it's not a real issue, anyway, the code
> still works.
> 
> The downside is that printing a scoped_restore object from
> gdb will just print the reference using the base type, so you
> won't see stored fields.
> 
> (gdb) p scope
> $1 = (const scoped_restore) @0x7fffffffd910: {<No data fields>}
> 
> To see the object's fields you'd have to downcast it manually, like:
> 
> (gdb) p (scoped_restore_<ui_out *>) scope
> $1 = {<scoped_restore_base> = {<No data fields>}, saved_var = 0x601050 <current_uiout>, saved_value = 0x0}
> 
> Not sure whether that's an issue.
> 
> Thanks,
> Pedro Alves
>
Tom Tromey Sept. 30, 2016, 2:02 p.m. UTC | #4
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> template<class T>
Pedro> scoped_restore_<T> make_scoped_restore (T *var)
Pedro> {
Pedro>   return scoped_restore_<T> (var);
Pedro> }

Yeah, I'll do this.  I don't know why I didn't remember this when
writing the patches.

I'd also appreciate it if you could say whether you want "m_" member
naming.

Tom
Tom Tromey Sept. 30, 2016, 3:17 p.m. UTC | #5
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro>   scoped_restore scope = make_scoped_restore (&some_global);

I made this change.  One minor oddity is that the 2-argument form of
make_scoped_restore required a copy constructor in the
scoped_restore_tmpl subclass.  I think that's harmless though.

Tom
Pedro Alves Sept. 30, 2016, 11:11 p.m. UTC | #6
On 09/30/2016 03:02 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> template<class T>
> Pedro> scoped_restore_<T> make_scoped_restore (T *var)
> Pedro> {
> Pedro>   return scoped_restore_<T> (var);
> Pedro> }
> 
> Yeah, I'll do this.  I don't know why I didn't remember this when
> writing the patches.

Great, thanks.

> 
> I'd also appreciate it if you could say whether you want "m_" member
> naming.
> 

Unless there's a good reason to deviate, I think it's a good idea
to follow gcc's coding conventions.  For that reason, my preference
is to use "m_" for private data members.

Thanks,
Pedro Alves
Pedro Alves Sept. 30, 2016, 11:20 p.m. UTC | #7
On 09/30/2016 04:17 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro>   scoped_restore scope = make_scoped_restore (&some_global);
> 
> I made this change.  One minor oddity is that the 2-argument form of
> make_scoped_restore required a copy constructor in the
> scoped_restore_tmpl subclass.  I think that's harmless though.

Yeah.  We just need to make sure that if a copy constructor does run,
the code behaves as if it hadn't.  My earlier comment on that
not mattering may have been incorrect - if the copied-from scoped_restore
is destructed before the make_scoped_restore function returns, then the
global will revert back to the original value too soon.  Guess we could
have the copy constructor actually move instead of copy, setting the
copied-from's pointer member to NULL, and then have the
destructor do nothing if the pointer is NULL.

Thanks,
Pedro Alves
Tom Tromey Oct. 1, 2016, 3:55 a.m. UTC | #8
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Guess we could have the copy constructor actually move instead of
Pedro> copy, setting the copied-from's pointer member to NULL, and then
Pedro> have the destructor do nothing if the pointer is NULL.

Yeah. I took this approach.  I'm not super thrilled with it, but at the
same time it's not *too* bad.

Tom
Tom Tromey Oct. 1, 2016, 4:23 a.m. UTC | #9
Pedro> Guess we could have the copy constructor actually move instead of
Pedro> copy, setting the copied-from's pointer member to NULL, and then
Pedro> have the destructor do nothing if the pointer is NULL.

Tom> Yeah. I took this approach.  I'm not super thrilled with it, but at the
Tom> same time it's not *too* bad.

Scratch that, I spoke too soon.  I couldn't get this to work.

I don't understand why the 2-argument form of make_scoped_restore
requires a copy constructor while the 1-argument form does not.

However, once you need a copy constructor, it doesn't seem possible
without a move constructor.  The issue is that a copy constructor take a
const reference, so you can't modify the fields of the original.

Removing the 2-argument form seems to work.  Though like I said, I don't
know why.


Also, as an aside, I found I was using nullptr in my patches, but this
isn't C++03.  I think -std=c++03 is going to be needed or else it will
be too easy to slip in C++11-isms.  Now, I do think C++11 is really much
better, but my understanding is that C++03 is what gdb decided on.

Tom
Pedro Alves Oct. 1, 2016, 10:33 a.m. UTC | #10
On 10/01/2016 05:23 AM, Tom Tromey wrote:
> Pedro> Guess we could have the copy constructor actually move instead of
> Pedro> copy, setting the copied-from's pointer member to NULL, and then
> Pedro> have the destructor do nothing if the pointer is NULL.
> 
> Tom> Yeah. I took this approach.  I'm not super thrilled with it, but at the
> Tom> same time it's not *too* bad.
> 
> Scratch that, I spoke too soon.  I couldn't get this to work.
> 
> I don't understand why the 2-argument form of make_scoped_restore
> requires a copy constructor while the 1-argument form does not.
> 
> However, once you need a copy constructor, it doesn't seem possible
> without a move constructor.  The issue is that a copy constructor take a
> const reference, so you can't modify the fields of the original.

You can -- make the fields mutable.  That's what ScopeGuard does too.

> 
> Removing the 2-argument form seems to work.  Though like I said, I don't
> know why.

I can't tell off hand.  Maybe seeing the code would suggest something.

> 
> 
> Also, as an aside, I found I was using nullptr in my patches, but this
> isn't C++03.  I think -std=c++03 is going to be needed or else it will
> be too easy to slip in C++11-isms.  Now, I do think C++11 is really much
> better, but my understanding is that C++03 is what gdb decided on.

I'm very much against forcing -std=c++03.

My plan is to allow compiling with C++11 too, and use
some of its features, for extra safety and extra efficiency,
as long as we have C++03 fallbacks in place.  If we force -std=c++03,
that becomes impossible.  

I recently elaborated on this here:
 https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01624.html

The gist is that I think that the features supported by compilers that
people are using in practice to build gdb with is what matters
in the end, not really C++03 vs C++11.

The only reason we're not requiring C++11 is that some people
still wanted to build gdb with older compilers that don't support it.
That was last revisited a year ago when we discussed whether to
go C++ or not in the first place.  It may be that the baseline might have 
advanced meanwhile.

Thanks,
Pedro Alves
Tom Tromey Oct. 2, 2016, 5:11 p.m. UTC | #11
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> However, once you need a copy constructor, it doesn't seem possible
>> without a move constructor.  The issue is that a copy constructor take a
>> const reference, so you can't modify the fields of the original.

Pedro> You can -- make the fields mutable.

Ok, yeah, I had thought of this, but it seemed pretty ugly to me.

Instead I removed all the 2 argument forms in favor of a second explicit
assignment.  This solves the problem as well.

I can go back and do the mutable thing if you really prefer it though.


Pedro> The gist is that I think that the features supported by compilers that
Pedro> people are using in practice to build gdb with is what matters
Pedro> in the end, not really C++03 vs C++11.

The issue I see is that it's just too easy otherwise for C++11isms to
slip in, especially considering that everybody doing development is
likely to be using a compiler with all the C++11 features.  Maybe
buildbot can do C++03 builds to check.

Tom
Pedro Alves Oct. 5, 2016, 12:05 a.m. UTC | #12
On 10/02/2016 06:11 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> However, once you need a copy constructor, it doesn't seem possible
>>> without a move constructor.  The issue is that a copy constructor take a
>>> const reference, so you can't modify the fields of the original.
> 
> Pedro> You can -- make the fields mutable.
> 
> Ok, yeah, I had thought of this, but it seemed pretty ugly to me.

It's hidden in the "library", so it'd be super fine with me.
I think of it as just part of the idiom.

> 
> Instead I removed all the 2 argument forms in favor of a second explicit
> assignment.  This solves the problem as well.

OK, I thought a little bit more about my worry with the implicit
copy constructor, and I think I was originally right -- it doesn't
make a difference; things still work if the compiler doesn't elide,
because the single-argument form doesn't change the global.  Guess
you could confirm things still build and run properly
with -fno-elide-constructors.

> I can go back and do the mutable thing if you really prefer it though.

I don't have a strong preference.  Maybe client code using the 1-argument form
ends up looking cleaner, in any case, not sure.

> 
> 
> Pedro> The gist is that I think that the features supported by compilers that
> Pedro> people are using in practice to build gdb with is what matters
> Pedro> in the end, not really C++03 vs C++11.
> 
> The issue I see is that it's just too easy otherwise for C++11isms to
> slip in, especially considering that everybody doing development is
> likely to be using a compiler with all the C++11 features.  Maybe
> buildbot can do C++03 builds to check.

I still disagree to an extent.  Even if we were claiming to
require "C++11", we probably wouldn't be able to use _all_ of C++11.
E.g. if we wanted to require full C++11 support, then we're need to
require gcc 4.8, which was released in 2013.  I'd be super fine with
requiring it personally, and I look forward to the day, tbc!
I think it's present in all current versions of major distros, I
believe,  I wouldn't be surprised to find opposition, though.

As compromise, I think we could be much better at documenting the older gcc version
that we actually support compiling with.  That oldest-supported version, I think
should be decided based on what's the oldest version people really care about,
and are willing to routinely test, vs how bad we'd want the C++11 features
not supported by that old compiler.  Last this was discussed, in context of
deciding whether to move forward with the C++ conversion, we settled on gcc 4.2
at least, since that's what some BSDs were still using.  But we have nothing in
place to enforce that, and I suspect that we still build correctly with
older gcc's even.  Or not, I don't know, since no one is testing it, for all I
know (i.e., no one is sending test reports to gdb-testers@).  I think all BSDs have
llvm/clang in their base systems now, so those are probably no longer really a concern.

Because, if we could require say, gcc 4.3 at least, then we
could make use of rvalue references.  If we required gcc 4.4, then we
could use "auto" and other goodies.  Etc.  IOW, if we took an incremental
approach, we'd be bound by the level of C++11 support in the oldest
compiler we supported.  

  https://gcc.gnu.org/projects/cxx-status.html#cxx11

And _that_ version is what I think should be tested by the buildbot,
irrespective of C++98/C++03/C++0x/C++11/... instead of being artificially
blocked by -std=c++03 or some such.

Thanks,
Pedro Alves
Tom Tromey Oct. 12, 2016, 10:35 p.m. UTC | #13
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> You can -- make the fields mutable.

>> Ok, yeah, I had thought of this, but it seemed pretty ugly to me.

Pedro> It's hidden in the "library", so it'd be super fine with me.
Pedro> I think of it as just part of the idiom.

I did this and restored the 2-argument form.

Tom
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2f28e54..104048f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,27 @@ 
 2016-09-26  Tom Tromey  <tom@tromey.com>
 
+	* utils.h (class scoped_restore): New class.
+	* top.c (execute_command_to_string): Use scoped_restore.
+	* python/python.c (python_interactive_command): Use
+	scoped_restore.
+	(python_command, execute_gdb_command): Likewise.
+	* printcmd.c (do_one_display): Use scoped_restore.
+	* mi/mi-main.c (exec_continue): Use scoped_restore.
+	* mi/mi-cmd-var.c (mi_cmd_var_assign): Use scoped_restore.
+	* linux-fork.c (checkpoint_command): Use scoped_restore.
+	* infrun.c (restore_execution_direction): Remove.
+	(fetch_inferior_event): Use scoped_restore.
+	* compile/compile.c (compile_file_command): Use
+	scoped_restore.
+	(compile_code_command, compile_print_command): Likewise.
+	* cli/cli-script.c (execute_user_command): Use
+	scoped_restore.
+	(while_command, if_command, script_from_file): Likewise.
+	* arm-tdep.c (arm_insert_single_step_breakpoint): Use
+	scoped_restore.
+
+2016-09-26  Tom Tromey  <tom@tromey.com>
+
 	* selftest.c: Include <vector>, not "vec.h".
 	(self_test_function_ptr): Remove.
 	(tests): Now a std::vector.
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 4dfd76b..d7a8799 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -4205,15 +4205,11 @@  arm_insert_single_step_breakpoint (struct gdbarch *gdbarch,
 				   struct address_space *aspace,
 				   CORE_ADDR pc)
 {
-  struct cleanup *old_chain
-    = make_cleanup_restore_integer (&arm_override_mode);
-
-  arm_override_mode = IS_THUMB_ADDR (pc);
+  scoped_restore<int> save_override_mode (&arm_override_mode,
+					  IS_THUMB_ADDR (pc));
   pc = gdbarch_addr_bits_remove (gdbarch, pc);
 
   insert_single_step_breakpoint (gdbarch, aspace, pc);
-
-  do_cleanups (old_chain);
 }
 
 /* Given BUF, which is OLD_LEN bytes ending at ENDADDR, expand
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 579d0a4..bf18226 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -380,8 +380,7 @@  execute_user_command (struct cmd_list_element *c, char *args)
      not confused with Insight.  */
   in_user_command = 1;
 
-  make_cleanup_restore_integer (&current_ui->async);
-  current_ui->async = 0;
+  scoped_restore<int> save_async (&current_ui->async, 0);
 
   command_nest_depth++;
   while (cmdlines)
@@ -654,7 +653,6 @@  static void
 while_command (char *arg, int from_tty)
 {
   struct command_line *command = NULL;
-  struct cleanup *old_chain;
 
   control_level = 1;
   command = get_command_line (while_control, arg);
@@ -662,13 +660,10 @@  while_command (char *arg, int from_tty)
   if (command == NULL)
     return;
 
-  old_chain = make_cleanup_restore_integer (&current_ui->async);
-  current_ui->async = 0;
+  scoped_restore<int> save_async (&current_ui->async, 0);
 
   execute_control_command_untraced (command);
   free_command_lines (&command);
-
-  do_cleanups (old_chain);
 }
 
 /* "if" command support.  Execute either the true or false arm depending
@@ -686,13 +681,10 @@  if_command (char *arg, int from_tty)
   if (command == NULL)
     return;
 
-  old_chain = make_cleanup_restore_integer (&current_ui->async);
-  current_ui->async = 0;
+  scoped_restore<int> save_async (&current_ui->async, 0);
 
   execute_control_command_untraced (command);
   free_command_lines (&command);
-
-  do_cleanups (old_chain);
 }
 
 /* Cleanup */
@@ -1693,10 +1685,8 @@  script_from_file (FILE *stream, const char *file)
   source_line_number = 0;
   source_file_name = file;
 
-  make_cleanup_restore_integer (&current_ui->async);
-  current_ui->async = 0;
-
   {
+    scoped_restore<int> save_async (&current_ui->async, 0);
 
     TRY
       {
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 0c4a738..e2be115 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -91,8 +91,7 @@  compile_file_command (char *arg, int from_tty)
   char *buffer;
   struct cleanup *cleanup;
 
-  cleanup = make_cleanup_restore_integer (&current_ui->async);
-  current_ui->async = 0;
+  scoped_restore<int> save_async (&current_ui->async, 0);
 
   /* Check the user did not just <enter> after command.  */
   if (arg == NULL)
@@ -115,7 +114,7 @@  compile_file_command (char *arg, int from_tty)
 
   arg = skip_spaces (arg);
   arg = gdb_abspath (arg);
-  make_cleanup (xfree, arg);
+  cleanup = make_cleanup (xfree, arg);
   buffer = xstrprintf ("#include \"%s\"\n", arg);
   make_cleanup (xfree, buffer);
   eval_compile_command (NULL, buffer, scope, NULL);
@@ -130,11 +129,9 @@  compile_file_command (char *arg, int from_tty)
 static void
 compile_code_command (char *arg, int from_tty)
 {
-  struct cleanup *cleanup;
   enum compile_i_scope_types scope = COMPILE_I_SIMPLE_SCOPE;
 
-  cleanup = make_cleanup_restore_integer (&current_ui->async);
-  current_ui->async = 0;
+  scoped_restore<int> save_async (&current_ui->async, 0);
 
   if (arg != NULL && check_raw_argument (&arg))
     {
@@ -155,13 +152,12 @@  compile_code_command (char *arg, int from_tty)
   else
     {
       struct command_line *l = get_command_line (compile_control, "");
+      struct cleanup *cleanup = make_cleanup_free_command_lines (&l);
 
-      make_cleanup_free_command_lines (&l);
       l->control_u.compile.scope = scope;
       execute_control_command_untraced (l);
+      do_cleanups (cleanup);
     }
-
-  do_cleanups (cleanup);
 }
 
 /* Callback for compile_print_command.  */
@@ -183,12 +179,10 @@  static void
 compile_print_command (char *arg_param, int from_tty)
 {
   const char *arg = arg_param;
-  struct cleanup *cleanup;
   enum compile_i_scope_types scope = COMPILE_I_PRINT_ADDRESS_SCOPE;
   struct format_data fmt;
 
-  cleanup = make_cleanup_restore_integer (&current_ui->async);
-  current_ui->async = 0;
+  scoped_restore<int> save_async (&current_ui->async, 0);
 
   /* Passing &FMT as SCOPE_DATA is safe as do_module_cleanup will not
      touch the stale pointer if compile_object_run has already quit.  */
@@ -199,14 +193,13 @@  compile_print_command (char *arg_param, int from_tty)
   else
     {
       struct command_line *l = get_command_line (compile_control, "");
+      struct cleanup *cleanup = make_cleanup_free_command_lines (&l);
 
-      make_cleanup_free_command_lines (&l);
       l->control_u.compile.scope = scope;
       l->control_u.compile.scope_data = &fmt;
       execute_control_command_untraced (l);
+      do_cleanups (cleanup);
     }
-
-  do_cleanups (cleanup);
 }
 
 /* A cleanup function to remove a directory and all its contents.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 2636a19..7832a5d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3879,17 +3879,6 @@  all_uis_on_sync_execution_starting (void)
     }
 }
 
-/* A cleanup that restores the execution direction to the value saved
-   in *ARG.  */
-
-static void
-restore_execution_direction (void *arg)
-{
-  enum exec_direction_kind *save_exec_dir = (enum exec_direction_kind *) arg;
-
-  execution_direction = *save_exec_dir;
-}
-
 /* Asynchronous version of wait_for_inferior.  It is called by the
    event loop whenever a change of state is detected on the file
    descriptor corresponding to the target.  It can be called more than
@@ -3906,7 +3895,6 @@  fetch_inferior_event (void *client_data)
   struct execution_control_state *ecs = &ecss;
   struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
   struct cleanup *ts_old_chain;
-  enum exec_direction_kind save_exec_dir = execution_direction;
   int cmd_done = 0;
   ptid_t waiton_ptid = minus_one_ptid;
 
@@ -3945,8 +3933,8 @@  fetch_inferior_event (void *client_data)
      event.  */
   target_dcache_invalidate ();
 
-  make_cleanup (restore_execution_direction, &save_exec_dir);
-  execution_direction = target_execution_direction ();
+  scoped_restore<enum exec_direction_kind>
+    save_exec_dir (&execution_direction, target_execution_direction ());
 
   ecs->ptid = do_target_wait (waiton_ptid, &ecs->ws,
 			      target_can_async_p () ? TARGET_WNOHANG : 0);
diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index e7202dd..9c2cf8a 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -680,7 +680,6 @@  checkpoint_command (char *args, int from_tty)
   struct value *fork_fn = NULL, *ret;
   struct fork_info *fp;
   pid_t retpid;
-  struct cleanup *old_chain;
 
   if (!target_has_execution) 
     error (_("The program is not being run."));
@@ -704,11 +703,13 @@  checkpoint_command (char *args, int from_tty)
   ret = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
 
   /* Tell linux-nat.c that we're checkpointing this inferior.  */
-  old_chain = make_cleanup_restore_integer (&checkpointing_pid);
-  checkpointing_pid = ptid_get_pid (inferior_ptid);
+  {
+    scoped_restore<int> save_pid (&checkpointing_pid,
+				  ptid_get_pid (inferior_ptid));
+
+    ret = call_function_by_hand (fork_fn, 0, &ret);
+  }
 
-  ret = call_function_by_hand (fork_fn, 0, &ret);
-  do_cleanups (old_chain);
   if (!ret)	/* Probably can't happen.  */
     error (_("checkpoint: call_function_by_hand returned null."));
 
diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index 3bfe4f0..1c78104 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -608,7 +608,6 @@  mi_cmd_var_assign (char *command, char **argv, int argc)
   struct ui_out *uiout = current_uiout;
   struct varobj *var;
   char *expression, *val;
-  struct cleanup *cleanup;
 
   if (argc != 2)
     error (_("-var-assign: Usage: NAME EXPRESSION."));
@@ -623,9 +622,7 @@  mi_cmd_var_assign (char *command, char **argv, int argc)
 
   /* MI command '-var-assign' may write memory, so suppress memory
      changed notification if it does.  */
-  cleanup
-    = make_cleanup_restore_integer (&mi_suppress_notification.memory);
-  mi_suppress_notification.memory = 1;
+  scoped_restore<int> save_suppress (&mi_suppress_notification.memory, 1);
 
   if (!varobj_set_value (var, expression))
     error (_("-var-assign: Could not assign "
@@ -634,8 +631,6 @@  mi_cmd_var_assign (char *command, char **argv, int argc)
   val = varobj_get_value (var);
   ui_out_field_string (uiout, "value", val);
   xfree (val);
-
-  do_cleanups (cleanup);
 }
 
 /* Type used for parameters passing to mi_cmd_var_update_iter.  */
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 1913157..c6c7067 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -300,7 +300,7 @@  exec_continue (char **argv, int argc)
     }
   else
     {
-      struct cleanup *back_to = make_cleanup_restore_integer (&sched_multi);
+      scoped_restore<int> save_multi (&sched_multi);
 
       if (current_context->all)
 	{
@@ -315,7 +315,6 @@  exec_continue (char **argv, int argc)
 	     same.  */
 	  continue_1 (1);
 	}
-      do_cleanups (back_to);
     }
 }
 
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index d4a4b9e..1fccd26 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1938,7 +1938,6 @@  undisplay_command (char *args, int from_tty)
 static void
 do_one_display (struct display *d)
 {
-  struct cleanup *old_chain;
   int within_current_scope;
 
   if (d->enabled_p == 0)
@@ -1990,8 +1989,7 @@  do_one_display (struct display *d)
   if (!within_current_scope)
     return;
 
-  old_chain = make_cleanup_restore_integer (&current_display_number);
-  current_display_number = d->number;
+  scoped_restore<int> save_display_number (&current_display_number, d->number);
 
   annotate_display_begin ();
   printf_filtered ("%d", d->number);
@@ -2079,7 +2077,6 @@  do_one_display (struct display *d)
   annotate_display_end ();
 
   gdb_flush (gdb_stdout);
-  do_cleanups (old_chain);
 }
 
 /* Display all of the values on the auto-display chain which can be
diff --git a/gdb/python/python.c b/gdb/python/python.c
index b00b70b..9e4d610 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -319,11 +319,9 @@  static void
 python_interactive_command (char *arg, int from_tty)
 {
   struct ui *ui = current_ui;
-  struct cleanup *cleanup;
   int err;
 
-  cleanup = make_cleanup_restore_integer (&current_ui->async);
-  current_ui->async = 0;
+  scoped_restore<int> save_async (&current_ui->async, 0);
 
   arg = skip_spaces (arg);
 
@@ -351,8 +349,6 @@  python_interactive_command (char *arg, int from_tty)
       gdbpy_print_stack ();
       error (_("Error while executing Python code."));
     }
-
-  do_cleanups (cleanup);
 }
 
 /* A wrapper around PyRun_SimpleFile.  FILE is the Python script to run
@@ -467,8 +463,7 @@  python_command (char *arg, int from_tty)
 
   cleanup = ensure_python_env (get_current_arch (), current_language);
 
-  make_cleanup_restore_integer (&current_ui->async);
-  current_ui->async = 0;
+  scoped_restore<int> save_async (&current_ui->async, 0);
 
   arg = skip_spaces (arg);
   if (arg && *arg)
@@ -651,8 +646,7 @@  execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
       struct cleanup *cleanup = make_cleanup (xfree, copy);
       struct interp *interp;
 
-      make_cleanup_restore_integer (&current_ui->async);
-      current_ui->async = 0;
+      scoped_restore<int> save_async (&current_ui->async, 0);
 
       make_cleanup_restore_current_uiout ();
 
diff --git a/gdb/top.c b/gdb/top.c
index 3cfa113..84285b2 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -701,8 +701,7 @@  execute_command_to_string (char *p, int from_tty)
      restoration callbacks.  */
   cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
 
-  make_cleanup_restore_integer (&current_ui->async);
-  current_ui->async = 0;
+  scoped_restore<int> save_async (&current_ui->async, 0);
 
   str_file = mem_fileopen ();
 
diff --git a/gdb/utils.h b/gdb/utils.h
index 8635075..63583ed 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -84,6 +84,50 @@  extern struct cleanup *make_cleanup_bfd_unref (bfd *abfd);
 struct obstack;
 extern struct cleanup *make_cleanup_obstack_free (struct obstack *obstack);
 
+// An RAII-based object that saves a variable's value, and then
+// restores it again when this object is destroyed.
+template<typename T>
+class scoped_restore
+{
+ public:
+
+  // Create a new scoped_restore object that saves the current value
+  // of *VAR, and then sets *VAR to VALUE.  *VAR will be restored when
+  // this scoped_restore object is destroyed.
+  scoped_restore (T *var, T value)
+    : saved_var (var),
+      saved_value (*var)
+  {
+    *var = value;
+  }
+
+  // Create a new scoped_restore object that saves the current value
+  // of *VAR.  *VAR will be restored when this scoped_restore object
+  // is destroyed.
+  explicit scoped_restore (T *var)
+    : saved_var (var),
+      saved_value (*var)
+  {
+  }
+
+  ~scoped_restore ()
+  {
+    *saved_var = saved_value;
+  }
+
+ private:
+
+  // No need for these.  They are intentionally not defined anywhere.
+  scoped_restore &operator= (const scoped_restore &);
+  scoped_restore (const scoped_restore &);
+
+  // The saved variable.
+  T *saved_var;
+
+  // The saved value.
+  T saved_value;
+};
+
 extern struct cleanup *make_cleanup_restore_integer (int *variable);
 extern struct cleanup *make_cleanup_restore_uinteger (unsigned int *variable);