[03/28] Refactor delete_program_space as a destructor

Message ID 20200414175434.8047-4-palves@redhat.com
State New
Headers show
Series
  • Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)
Related show

Commit Message

Simon Marchi via Gdb-patches April 14, 2020, 5:54 p.m.
Currently, while the program_space's ctor adds the new pspace to the
pspaces list, the destructor doesn't remove the pspace from the
pspace list.  Instead, you're supposed to use delete_program_space, to
both remove the pspace from the list, and deleting the pspace.

This patch eliminates delete_program_space, and makes the pspace dtor
remove the deleted pspace from the pspace list itself, i.e., makes the
dtor do the mirror opposite of the ctor.

I found this helps with a following patch that will allocate a mock
program_space on the stack.  It's easier to just let the regular dtor
remove the mock pspace from the pspace list than arrange to call
delete_program_space instead of the pspace dtor in that situation.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* inferior.c (delete_inferior): Use delete operator directly
	instead of delete_program_space.
	* progspace.c (add_program_space): New, factored out from
	program_space::program_space.
	(remove_program_space): New, factored out from
	delete_program_space.
	(program_space::program_space): Rewrite.
	(program_space::~program_space): Call remove_program_space.
	(delete_program_space): Delete.
	* progspace.h (delete_program_space): Remove.
---
 gdb/inferior.c  |  2 +-
 gdb/progspace.c | 79 +++++++++++++++++++++++++++++++--------------------------
 gdb/progspace.h |  4 ---
 3 files changed, 44 insertions(+), 41 deletions(-)

-- 
2.14.5

Comments

Simon Marchi April 15, 2020, 3:54 p.m. | #1
On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
> +/* Adds a new empty program space to the program space list, and binds

> +   it to ASPACE.  Returns the pointer to the new object.  */


You could take the opportunity to update this comment.  At least, the "Return the pointer"
part is not really relevant for a constructor.

Otherwise, this LGTM.

Simon
Simon Marchi via Gdb-patches April 16, 2020, 2:47 p.m. | #2
On 4/15/20 4:54 PM, Simon Marchi wrote:
> On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:

>> +/* Adds a new empty program space to the program space list, and binds

>> +   it to ASPACE.  Returns the pointer to the new object.  */

> 

> You could take the opportunity to update this comment.  At least, the "Return the pointer"

> part is not really relevant for a constructor.

> 

> Otherwise, this LGTM.

> 


Thanks.  Here's what I'm merging.


From 381ce63f2f010ef5c246be720ef177cf46a19179 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Thu, 16 Apr 2020 14:50:07 +0100
Subject: [PATCH] Refactor delete_program_space as a destructor

Currently, while the program_space's ctor adds the new pspace to the
pspaces list, the destructor doesn't remove the pspace from the pspace
list.  Instead, you're supposed to use delete_program_space, to both
remove the pspace from the list, and deleting the pspace.

This patch eliminates delete_program_space, and makes the pspace dtor
remove the deleted pspace from the pspace list itself, i.e., makes the
dtor do the mirror opposite of the ctor.

I found this helps with a following patch that will allocate a mock
program_space on the stack.  It's easier to just let the regular dtor
remove the mock pspace from the pspace list than arrange to call
delete_program_space instead of the pspace dtor in that situation.

While at it, move the ctor/dtor intro comments to the header file, and
make the ctor explicit.

gdb/ChangeLog:
2020-04-16  Pedro Alves  <palves@redhat.com>

	* inferior.c (delete_inferior): Use delete operator directly
	instead of delete_program_space.
	* progspace.c (add_program_space): New, factored out from
	program_space::program_space.
	(remove_program_space): New, factored out from
	delete_program_space.
	(program_space::program_space): Remove intro comment.  Rewrite.
	(program_space::~program_space): Remove intro comment.  Call
	remove_program_space.
	(delete_program_space): Delete.
	* progspace.h (program_space::program_space): Make explicit.  Move
	intro comment here, adjusted.
	(program_space::~program_space): Move intro comment here,
	adjusted.
	(delete_program_space): Remove.
---
 gdb/ChangeLog   | 18 +++++++++++++
 gdb/inferior.c  |  2 +-
 gdb/progspace.c | 84 +++++++++++++++++++++++++++++----------------------------
 gdb/progspace.h | 14 ++++++----
 4 files changed, 71 insertions(+), 47 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7ba862edd3..6c280e3f49 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,21 @@
+2020-04-16  Pedro Alves  <palves@redhat.com>
+
+	* inferior.c (delete_inferior): Use delete operator directly
+	instead of delete_program_space.
+	* progspace.c (add_program_space): New, factored out from
+	program_space::program_space.
+	(remove_program_space): New, factored out from
+	delete_program_space.
+	(program_space::program_space): Remove intro comment.  Rewrite.
+	(program_space::~program_space): Remove intro comment.  Call
+	remove_program_space.
+	(delete_program_space): Delete.
+	* progspace.h (program_space::program_space): Make explicit.  Move
+	intro comment here, adjusted.
+	(program_space::~program_space): Move intro comment here,
+	adjusted.
+	(delete_program_space): Remove.
+
 2020-04-16  Tom Tromey  <tromey@adacore.com>
 
 	* windows-nat.c (windows_nat::handle_access_violation): New
diff --git a/gdb/inferior.c b/gdb/inferior.c
index c2e9da3d3d..ceee00e9ee 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -162,7 +162,7 @@ delete_inferior (struct inferior *todel)
 
   /* If this program space is rendered useless, remove it. */
   if (program_space_empty_p (inf->pspace))
-    delete_program_space (inf->pspace);
+    delete inf->pspace;
 
   delete inf;
 }
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 1361040347..6419f01770 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -109,36 +109,65 @@ init_address_spaces (void)
 
 
 
-/* Adds a new empty program space to the program space list, and binds
-   it to ASPACE.  Returns the pointer to the new object.  */
+/* Add a program space from the program spaces list.  */
 
-program_space::program_space (address_space *aspace_)
-: num (++last_program_space_num), aspace (aspace_)
+static void
+add_program_space (program_space *pspace)
 {
-  program_space_alloc_data (this);
-
   if (program_spaces == NULL)
-    program_spaces = this;
+    program_spaces = pspace;
   else
     {
-      struct program_space *last;
+      program_space *last;
 
       for (last = program_spaces; last->next != NULL; last = last->next)
 	;
-      last->next = this;
+      last->next = pspace;
     }
 }
 
-/* Releases program space PSPACE, and all its contents (shared
-   libraries, objfiles, and any other references to the PSPACE in
-   other modules).  It is an internal error to call this when PSPACE
-   is the current program space, since there should always be a
-   program space.  */
+/* Remove a program space from the program spaces list.  */
+
+static void
+remove_program_space (program_space *pspace)
+{
+  program_space *ss, **ss_link;
+  gdb_assert (pspace != NULL);
+
+  ss = program_spaces;
+  ss_link = &program_spaces;
+  while (ss != NULL)
+    {
+      if (ss == pspace)
+	{
+	  *ss_link = ss->next;
+	  return;
+	}
+
+      ss_link = &ss->next;
+      ss = *ss_link;
+    }
+}
+
+/* See progspace.h.  */
+
+program_space::program_space (address_space *aspace_)
+  : num (++last_program_space_num),
+    aspace (aspace_)
+{
+  program_space_alloc_data (this);
+
+  add_program_space (this);
+}
+
+/* See progspace.h.  */
 
 program_space::~program_space ()
 {
   gdb_assert (this != current_program_space);
 
+  remove_program_space (this);
+
   scoped_restore_current_program_space restore_pspace;
 
   set_current_program_space (this);
@@ -259,33 +288,6 @@ program_space_empty_p (struct program_space *pspace)
   return 1;
 }
 
-/* Remove a program space from the program spaces list and release it.  It is
-   an error to call this function while PSPACE is the current program space. */
-
-void
-delete_program_space (struct program_space *pspace)
-{
-  struct program_space *ss, **ss_link;
-  gdb_assert (pspace != NULL);
-  gdb_assert (pspace != current_program_space);
-
-  ss = program_spaces;
-  ss_link = &program_spaces;
-  while (ss != NULL)
-    {
-      if (ss == pspace)
-	{
-	  *ss_link = ss->next;
-	  break;
-	}
-
-      ss_link = &ss->next;
-      ss = *ss_link;
-    }
-
-  delete pspace;
-}
-
 /* Prints the list of program spaces and their details on UIOUT.  If
    REQUESTED is not -1, it's the ID of the pspace that should be
    printed.  Otherwise, all spaces are printed.  */
diff --git a/gdb/progspace.h b/gdb/progspace.h
index 71a6f2841e..2b887847e1 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -209,7 +209,15 @@ struct unwrapping_objfile_range
 
 struct program_space
 {
-  program_space (address_space *aspace_);
+  /* Constructs a new empty program space, binds it to ASPACE, and
+     adds it to the program space list.  */
+  explicit program_space (address_space *aspace);
+
+  /* Releases a program space, and all its contents (shared libraries,
+     objfiles, and any other references to the program space in other
+     modules).  It is an internal error to call this when the program
+     space is the current program space, since there should always be
+     a program space.  */
   ~program_space ();
 
   typedef unwrapping_objfile_range objfiles_range;
@@ -362,10 +370,6 @@ extern struct program_space *current_program_space;
 #define ALL_PSPACES(pspace) \
   for ((pspace) = program_spaces; (pspace) != NULL; (pspace) = (pspace)->next)
 
-/* Remove a program space from the program spaces list and release it.  It is
-   an error to call this function while PSPACE is the current program space. */
-extern void delete_program_space (struct program_space *pspace);
-
 /* Returns the number of program spaces listed.  */
 extern int number_of_program_spaces (void);
 

base-commit: a010605fef0eba73c564c3dd22e0a6ecbc26b10e
-- 
2.14.5

Patch

diff --git a/gdb/inferior.c b/gdb/inferior.c
index c2e9da3d3d..ceee00e9ee 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -162,7 +162,7 @@  delete_inferior (struct inferior *todel)
 
   /* If this program space is rendered useless, remove it. */
   if (program_space_empty_p (inf->pspace))
-    delete_program_space (inf->pspace);
+    delete inf->pspace;
 
   delete inf;
 }
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 1361040347..55648e4929 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -109,26 +109,58 @@  init_address_spaces (void)
 
 
 
-/* Adds a new empty program space to the program space list, and binds
-   it to ASPACE.  Returns the pointer to the new object.  */
+/* Add a program space from the program spaces list.  */
 
-program_space::program_space (address_space *aspace_)
-: num (++last_program_space_num), aspace (aspace_)
+static void
+add_program_space (program_space *pspace)
 {
-  program_space_alloc_data (this);
-
   if (program_spaces == NULL)
-    program_spaces = this;
+    program_spaces = pspace;
   else
     {
-      struct program_space *last;
+      program_space *last;
 
       for (last = program_spaces; last->next != NULL; last = last->next)
 	;
-      last->next = this;
+      last->next = pspace;
+    }
+}
+
+/* Remove a program space from the program spaces list.  */
+
+static void
+remove_program_space (program_space *pspace)
+{
+  program_space *ss, **ss_link;
+  gdb_assert (pspace != NULL);
+
+  ss = program_spaces;
+  ss_link = &program_spaces;
+  while (ss != NULL)
+    {
+      if (ss == pspace)
+	{
+	  *ss_link = ss->next;
+	  return;
+	}
+
+      ss_link = &ss->next;
+      ss = *ss_link;
     }
 }
 
+/* Adds a new empty program space to the program space list, and binds
+   it to ASPACE.  Returns the pointer to the new object.  */
+
+program_space::program_space (address_space *aspace_)
+  : num (++last_program_space_num),
+    aspace (aspace_)
+{
+  program_space_alloc_data (this);
+
+  add_program_space (this);
+}
+
 /* Releases program space PSPACE, and all its contents (shared
    libraries, objfiles, and any other references to the PSPACE in
    other modules).  It is an internal error to call this when PSPACE
@@ -139,6 +171,8 @@  program_space::~program_space ()
 {
   gdb_assert (this != current_program_space);
 
+  remove_program_space (this);
+
   scoped_restore_current_program_space restore_pspace;
 
   set_current_program_space (this);
@@ -259,33 +293,6 @@  program_space_empty_p (struct program_space *pspace)
   return 1;
 }
 
-/* Remove a program space from the program spaces list and release it.  It is
-   an error to call this function while PSPACE is the current program space. */
-
-void
-delete_program_space (struct program_space *pspace)
-{
-  struct program_space *ss, **ss_link;
-  gdb_assert (pspace != NULL);
-  gdb_assert (pspace != current_program_space);
-
-  ss = program_spaces;
-  ss_link = &program_spaces;
-  while (ss != NULL)
-    {
-      if (ss == pspace)
-	{
-	  *ss_link = ss->next;
-	  break;
-	}
-
-      ss_link = &ss->next;
-      ss = *ss_link;
-    }
-
-  delete pspace;
-}
-
 /* Prints the list of program spaces and their details on UIOUT.  If
    REQUESTED is not -1, it's the ID of the pspace that should be
    printed.  Otherwise, all spaces are printed.  */
diff --git a/gdb/progspace.h b/gdb/progspace.h
index 71a6f2841e..a0e9e009c0 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -362,10 +362,6 @@  extern struct program_space *current_program_space;
 #define ALL_PSPACES(pspace) \
   for ((pspace) = program_spaces; (pspace) != NULL; (pspace) = (pspace)->next)
 
-/* Remove a program space from the program spaces list and release it.  It is
-   an error to call this function while PSPACE is the current program space. */
-extern void delete_program_space (struct program_space *pspace);
-
 /* Returns the number of program spaces listed.  */
 extern int number_of_program_spaces (void);