[binutils-gdb] Make inferior::detaching a bool, and introduce scoped_restore::release()

[Pedro Alves <palves at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9bcb1f1630b05594fa86bfd017639cfcc966b11c

commit 9bcb1f1630b05594fa86bfd017639cfcc966b11c
Author: Pedro Alves <palves@redhat.com>
Date:   Wed Apr 19 13:12:23 2017 +0100

    Make inferior::detaching a bool, and introduce scoped_restore::release()
    
    I left making inferior::detaching a bool to a separate patch, because
    doing that makes a make_cleanup_restore_integer call in
    infrun.c:prepare_for_detach no longer compile (passing a 'bool *' when
    an 'int *' is expected).  Since we want to get rid of cleanups anyway,
    I looked at converting that to a scoped_restore.  However,
    prepare_for_detach wants to discard the cleanup on success, and
    scoped_restore doesn't have an equivalent for that.  So I added one --
    I called it "release()" because it seems like a natural fit in the way
    standard components call similarly-spirited methods, and, it's also
    what the proposal for a generic scope guard calls it too, AFAICS:
    
      http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4189.pdf
    
    I've added some scoped_guard unit tests, while at it.
    
    gdb/ChangeLog:
    2017-04-19  Pedro Alves  <palves@redhat.com>
    
    	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
    	unittests/scoped_restore-selftests.c.
    	(SUBDIR_UNITTESTS_OBS): Add scoped_restore-selftests.o.
    	* common/scoped_restore.h (scoped_restore_base): Make "class".
    	(scoped_restore_base::release): New public method.
    	(scoped_restore_base::scoped_restore_base): New protected ctor.
    	(scoped_restore_base::m_saved_var): New protected field.
    	(scoped_restore_tmpl::scoped_restore_tmpl(T*)): Initialize the
    	scoped_restore_base base class instead of m_saved_var directly.
    	(scoped_restore_tmpl::scoped_restore_tmpl(T*, T2)): Likewise.
    	(scoped_restore_tmpl::scoped_restore_tmpl(const
    	scoped_restore_tmpl<T>&)): Likewise.
    	(scoped_restore_tmpl::~scoped_restore_tmpl): Use the saved_var
    	method.
    	(scoped_restore_tmpl::saved_var): New method.
    	(scoped_restore_tmpl::m_saved_var): Delete.
    	* inferior.h (inferior::detaching): Now a bool.
    	* infrun.c (prepare_for_detach): Use a scoped_restore instead of a
    	cleanup.
    	* unittests/scoped_restore-selftests.c: New file.

Diff:
---
 gdb/ChangeLog                            |  23 +++++++
 gdb/Makefile.in                          |   6 +-
 gdb/common/scoped_restore.h              |  36 +++++++---
 gdb/inferior.h                           |   2 +-
 gdb/infrun.c                             |   8 +--
 gdb/unittests/scoped_restore-selftests.c | 110 +++++++++++++++++++++++++++++++
 6 files changed, 167 insertions(+), 18 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8c7710c..68c3c91 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,28 @@
 2017-04-19  Pedro Alves  <palves@redhat.com>
 
+	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
+	unittests/scoped_restore-selftests.c.
+	(SUBDIR_UNITTESTS_OBS): Add scoped_restore-selftests.o.
+	* common/scoped_restore.h (scoped_restore_base): Make "class".
+	(scoped_restore_base::release): New public method.
+	(scoped_restore_base::scoped_restore_base): New protected ctor.
+	(scoped_restore_base::m_saved_var): New protected field.
+	(scoped_restore_tmpl::scoped_restore_tmpl(T*)): Initialize the
+	scoped_restore_base base class instead of m_saved_var directly.
+	(scoped_restore_tmpl::scoped_restore_tmpl(T*, T2)): Likewise.
+	(scoped_restore_tmpl::scoped_restore_tmpl(const
+	scoped_restore_tmpl<T>&)): Likewise.
+	(scoped_restore_tmpl::~scoped_restore_tmpl): Use the saved_var
+	method.
+	(scoped_restore_tmpl::saved_var): New method.
+	(scoped_restore_tmpl::m_saved_var): Delete.
+	* inferior.h (inferior::detaching): Now a bool.
+	* infrun.c (prepare_for_detach): Use a scoped_restore instead of a
+	cleanup.
+	* unittests/scoped_restore-selftests.c: New file.
+
+2017-04-19  Pedro Alves  <palves@redhat.com>
+
 	* Makefile.in (SUBDIR_UNITTESTS_SRCS, SUBDIR_UNITTESTS_OBS):
 	Re-sort in alphabetic order.
 
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 255e694..b865b7c 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -527,13 +527,15 @@ SUBDIR_UNITTESTS_SRCS = \
 	unittests/function-view-selftests.c \
 	unittests/offset-type-selftests.c \
 	unittests/optional-selftests.c \
-	unittests/ptid-selftests.c
+	unittests/ptid-selftests.c \
+	unittests/scoped_restore-selftests.c
 
 SUBDIR_UNITTESTS_OBS = \
 	function-view-selftests.o \
 	offset-type-selftests.o \
 	optional-selftests.o \
-	ptid-selftests.o
+	ptid-selftests.o \
+	scoped_restore-selftests.o
 
 # Opcodes currently live in one of two places.  Either they are in the
 # opcode library, typically ../opcodes, or they are in a header file
diff --git a/gdb/common/scoped_restore.h b/gdb/common/scoped_restore.h
index ae7a49f..e7dbca9 100644
--- a/gdb/common/scoped_restore.h
+++ b/gdb/common/scoped_restore.h
@@ -21,8 +21,23 @@
 #define SCOPED_RESTORE_H
 
 /* Base class for scoped_restore_tmpl.  */
-struct scoped_restore_base
+class scoped_restore_base
 {
+public:
+  /* This informs the (scoped_restore_tmpl<T>) dtor that you no longer
+     want the original value restored.  */
+  void release () const
+  { m_saved_var = NULL; }
+
+protected:
+  scoped_restore_base (void *saved_var)
+    : m_saved_var (saved_var)
+  {}
+
+  /* The type-erased saved variable.  This is here so that clients can
+     call release() on a "scoped_restore" local, which is a typedef to
+     a scoped_restore_base.  See below.  */
+  mutable void *m_saved_var;
 };
 
 /* A convenience typedef.  Users of make_scoped_restore declare the
@@ -40,7 +55,7 @@ class scoped_restore_tmpl : public scoped_restore_base
      of *VAR.  *VAR will be restored when this scoped_restore object
      is destroyed.  */
   scoped_restore_tmpl (T *var)
-    : m_saved_var (var),
+    : scoped_restore_base (var),
       m_saved_value (*var)
   {
   }
@@ -52,14 +67,14 @@ class scoped_restore_tmpl : public scoped_restore_base
      E.g.: T='base'; T2='derived'.  */
   template <typename T2>
   scoped_restore_tmpl (T *var, T2 value)
-    : m_saved_var (var),
+    : scoped_restore_base (var),
       m_saved_value (*var)
   {
     *var = value;
   }
 
   scoped_restore_tmpl (const scoped_restore_tmpl<T> &other)
-    : m_saved_var (other.m_saved_var),
+    : scoped_restore_base {other.m_saved_var},
       m_saved_value (other.m_saved_value)
   {
     other.m_saved_var = NULL;
@@ -67,18 +82,19 @@ class scoped_restore_tmpl : public scoped_restore_base
 
   ~scoped_restore_tmpl ()
   {
-    if (m_saved_var != NULL)
-      *m_saved_var = m_saved_value;
+    if (saved_var () != NULL)
+      *saved_var () = m_saved_value;
   }
 
- private:
+private:
+  /* Return a pointer to the saved variable with its type
+     restored.  */
+  T *saved_var ()
+  { return static_cast<T *> (m_saved_var); }
 
   /* No need for this.  It is intentionally not defined anywhere.  */
   scoped_restore_tmpl &operator= (const scoped_restore_tmpl &);
 
-  /* The saved variable.  */
-  mutable T *m_saved_var;
-
   /* The saved value.  */
   const T m_saved_value;
 };
diff --git a/gdb/inferior.h b/gdb/inferior.h
index b5eb3d1..e39a80c 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -387,7 +387,7 @@ public:
   bool waiting_for_vfork_done = false;
 
   /* True if we're in the process of detaching from this inferior.  */
-  int detaching = 0;
+  bool detaching = false;
 
   /* What is left to do for an execution command after any thread of
      this inferior stops.  For continuations associated with a
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c7298a3..fcafdc1 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3630,7 +3630,6 @@ prepare_for_detach (void)
 {
   struct inferior *inf = current_inferior ();
   ptid_t pid_ptid = pid_to_ptid (inf->pid);
-  struct cleanup *old_chain_1;
   struct displaced_step_inferior_state *displaced;
 
   displaced = get_displaced_stepping_state (inf->pid);
@@ -3644,8 +3643,7 @@ prepare_for_detach (void)
     fprintf_unfiltered (gdb_stdlog,
 			"displaced-stepping in-process while detaching");
 
-  old_chain_1 = make_cleanup_restore_integer (&inf->detaching);
-  inf->detaching = 1;
+  scoped_restore restore_detaching = make_scoped_restore (&inf->detaching, true);
 
   while (!ptid_equal (displaced->step_ptid, null_ptid))
     {
@@ -3685,12 +3683,12 @@ prepare_for_detach (void)
 	 inferior, so this must mean the process is gone.  */
       if (!ecs->wait_some_more)
 	{
-	  discard_cleanups (old_chain_1);
+	  restore_detaching.release ();
 	  error (_("Program exited while detaching"));
 	}
     }
 
-  discard_cleanups (old_chain_1);
+  restore_detaching.release ();
 }
 
 /* Wait for control to return from inferior to debugger.
diff --git a/gdb/unittests/scoped_restore-selftests.c b/gdb/unittests/scoped_restore-selftests.c
new file mode 100644
index 0000000..e97d622
--- /dev/null
+++ b/gdb/unittests/scoped_restore-selftests.c
@@ -0,0 +1,110 @@
+/* Self tests for scoped_restore for GDB, the GNU debugger.
+
+   Copyright (C) 2017 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "selftest.h"
+#include "common/scoped_restore.h"
+
+namespace selftests {
+namespace scoped_restore_tests {
+
+struct Base {};
+struct Derived : Base {};
+
+static int global;
+
+/* Check that we can return a scoped_restore from a function.  Below
+   we'll make sure this does the right thing.  */
+static scoped_restore_tmpl<int>
+make_scoped_restore_global (int value)
+{
+  return make_scoped_restore (&global, value);
+}
+
+static void
+run_tests ()
+{
+  /* Test that single-argument make_scoped_restore restores the
+     original value on scope exit.  */
+  {
+    int integer = 0;
+    {
+      scoped_restore restore = make_scoped_restore (&integer);
+      SELF_CHECK (integer == 0);
+      integer = 1;
+    }
+    SELF_CHECK (integer == 0);
+  }
+
+  /* Same, with two-argument make_scoped_restore.  */
+  {
+    int integer = 0;
+    {
+      scoped_restore restore = make_scoped_restore (&integer, 1);
+      SELF_CHECK (integer == 1);
+    }
+    SELF_CHECK (integer == 0);
+  }
+
+  /* Test releasing a scoped_restore.  */
+  {
+    int integer = 0;
+    {
+      scoped_restore restore = make_scoped_restore (&integer, 1);
+      SELF_CHECK (integer == 1);
+      restore.release ();
+    }
+    /* The overridden value should persist.  */
+    SELF_CHECK (integer == 1);
+  }
+
+  /* Test creating a scoped_restore with a value of a type convertible
+     to T.  */
+  {
+    Base *base = nullptr;
+    Derived derived;
+    {
+      scoped_restore restore = make_scoped_restore (&base, &derived);
+
+      SELF_CHECK (base == &derived);
+    }
+    SELF_CHECK (base == nullptr);
+  }
+
+  /* Test calling a function that returns a scoped restore.  Makes
+     sure that if the compiler emits a call to the copy ctor, that we
+     still do the right thing.  */
+  {
+    {
+      SELF_CHECK (global == 0);
+      scoped_restore restore = make_scoped_restore_global (1);
+      SELF_CHECK (global == 1);
+    }
+    SELF_CHECK (global == 0);
+  }
+}
+
+} /* namespace scoped_restore_tests */
+} /* namespace selftests */
+
+void
+_initialize_scoped_restore_selftests ()
+{
+  register_self_test (selftests::scoped_restore_tests::run_tests);
+}