[PATCH] Change bookmark allocation

[PATCH] Change bookmark allocation

[Tom Tromey]

This changes how bookmarks are allocated and stored, replacing a
linked list with a vector and removing some ALL_* iterator macros.
Regression tested on x86-64 Fedora 34.
---
 gdb/reverse.c | 137 +++++++++++++++++---------------------------------
 1 file changed, 46 insertions(+), 91 deletions(-)

diff --git a/gdb/reverse.c b/gdb/reverse.c
index dc417497b7a..fc94b8d1825 100644
--- a/gdb/reverse.c
+++ b/gdb/reverse.c
@@ -92,23 +92,15 @@ reverse_finish (const char *args, int from_tty)
 /* Data structures for a bookmark list.  */
 
 struct bookmark {
-  struct bookmark *next;
-  int number;
-  CORE_ADDR pc;
-  struct symtab_and_line sal;
-  gdb_byte *opaque_data;
+  int number = 0;
+  CORE_ADDR pc = 0;
+  struct symtab_and_line sal {};
+  gdb::unique_xmalloc_ptr<gdb_byte> opaque_data;
 };
 
-static struct bookmark *bookmark_chain;
+static std::vector<struct bookmark> all_bookmarks;
 static int bookmark_count;
 
-#define ALL_BOOKMARKS(B) for ((B) = bookmark_chain; (B); (B) = (B)->next)
-
-#define ALL_BOOKMARKS_SAFE(B,TMP)           \
-     for ((B) = bookmark_chain;             \
-	  (B) ? ((TMP) = (B)->next, 1) : 0; \
-	  (B) = (TMP))
-
 /* save_bookmark_command -- implement "bookmark" command.
    Call target method to get a bookmark identifier.
    Insert bookmark identifier into list.
@@ -130,80 +122,46 @@ save_bookmark_command (const char *args, int from_tty)
     error (_("target_get_bookmark failed."));
 
   /* Set up a bookmark struct.  */
-  bookmark *b = new bookmark ();
-  b->number = ++bookmark_count;
-  b->pc = regcache_read_pc (get_current_regcache ());
-  b->sal = find_pc_line (b->pc, 0);
-  b->sal.pspace = get_frame_program_space (get_current_frame ());
-  b->opaque_data = bookmark_id;
-  b->next = NULL;
-
-  /* Add this bookmark to the end of the chain, so that a list
-     of bookmarks will come out in order of increasing numbers.  */
-
-  bookmark *b1 = bookmark_chain;
-  if (b1 == 0)
-    bookmark_chain = b;
-  else
-    {
-      while (b1->next)
-	b1 = b1->next;
-      b1->next = b;
-    }
-  gdb_printf (_("Saved bookmark %d at %s\n"), b->number,
-	      paddress (gdbarch, b->sal.pc));
+  bookmark &b = all_bookmarks.emplace_back ();
+  b.number = ++bookmark_count;
+  b.pc = regcache_read_pc (get_current_regcache ());
+  b.sal = find_pc_line (b.pc, 0);
+  b.sal.pspace = get_frame_program_space (get_current_frame ());
+  b.opaque_data.reset (bookmark_id);
+
+  gdb_printf (_("Saved bookmark %d at %s\n"), b.number,
+	      paddress (gdbarch, b.sal.pc));
 }
 
 /* Implement "delete bookmark" command.  */
 
-static int
+static bool
 delete_one_bookmark (int num)
 {
-  struct bookmark *b1, *b;
-
   /* Find bookmark with corresponding number.  */
-  ALL_BOOKMARKS (b)
-    if (b->number == num)
-      break;
-
-  /* Special case, first item in list.  */
-  if (b == bookmark_chain)
-    bookmark_chain = b->next;
-
-  /* Find bookmark preceding "marked" one, so we can unlink.  */
-  if (b)
+  for (auto iter = all_bookmarks.begin ();
+       iter != all_bookmarks.end ();
+       ++iter)
     {
-      ALL_BOOKMARKS (b1)
-	if (b1->next == b)
-	  {
-	    /* Found designated bookmark.  Unlink and delete.  */
-	    b1->next = b->next;
-	    break;
-	  }
-      xfree (b->opaque_data);
-      delete b;
-      return 1;		/* success */
+      if (iter->number == num)
+	{
+	  all_bookmarks.erase (iter);
+	  return true;
+	}
     }
-  return 0;		/* failure */
+  return false;
 }
 
 static void
-delete_all_bookmarks (void)
+delete_all_bookmarks ()
 {
-  struct bookmark *b, *b1;
-
-  ALL_BOOKMARKS_SAFE (b, b1)
-    {
-      xfree (b->opaque_data);
-      xfree (b);
-    }
-  bookmark_chain = NULL;
+  all_bookmarks.clear ();
 }
 
 static void
 delete_bookmark_command (const char *args, int from_tty)
 {
-  if (bookmark_chain == NULL)
+  if (all_bookmarks.empty ())
     {
       warning (_("No bookmarks."));
       return;
@@ -232,7 +190,6 @@ delete_bookmark_command (const char *args, int from_tty)
 static void
 goto_bookmark_command (const char *args, int from_tty)
 {
-  struct bookmark *b;
   unsigned long num;
   const char *p = args;
 
@@ -263,15 +220,14 @@ goto_bookmark_command (const char *args, int from_tty)
   if (num == 0)
     error (_("goto-bookmark: invalid bookmark number '%s'."), p);
 
-  ALL_BOOKMARKS (b)
-    if (b->number == num)
-      break;
-
-  if (b)
+  for (const bookmark &iter : all_bookmarks)
     {
-      /* Found.  Send to target method.  */
-      target_goto_bookmark (b->opaque_data, from_tty);
-      return;
+      if (iter.number == num)
+	{
+	  /* Found.  Send to target method.  */
+	  target_goto_bookmark (iter.opaque_data.get (), from_tty);
+	  return;
+	}
     }
   /* Not found.  */
   error (_("goto-bookmark: no bookmark found for '%s'."), p);
@@ -281,20 +237,19 @@ static int
 bookmark_1 (int bnum)
 {
   struct gdbarch *gdbarch = get_current_regcache ()->arch ();
-  struct bookmark *b;
   int matched = 0;
 
-  ALL_BOOKMARKS (b)
-  {
-    if (bnum == -1 || bnum == b->number)
-      {
-	gdb_printf ("   %d       %s    '%s'\n",
-		    b->number,
-		    paddress (gdbarch, b->pc),
-		    b->opaque_data);
-	matched++;
-      }
-  }
+  for (const bookmark &iter : all_bookmarks)
+    {
+      if (bnum == -1 || bnum == iter.number)
+	{
+	  gdb_printf ("   %d       %s    '%s'\n",
+		      iter.number,
+		      paddress (gdbarch, iter.pc),
+		      iter.opaque_data.get ());
+	  matched++;
+	}
+    }
 
   if (bnum > 0 && matched == 0)
     gdb_printf ("No bookmark #%d\n", bnum);
@@ -307,7 +262,7 @@ bookmark_1 (int bnum)
 static void
 info_bookmarks_command (const char *args, int from_tty)
 {
-  if (!bookmark_chain)
+  if (all_bookmarks.empty ())
     gdb_printf (_("No bookmarks.\n"));
   else if (args == NULL || *args == '\0')
     bookmark_1 (-1);
-- 
2.34.1

Re: [PATCH] Change bookmark allocation

[Simon Marchi]

On 2022-08-15 19:10, Tom Tromey wrote:
> This changes how bookmarks are allocated and stored, replacing a
> linked list with a vector and removing some ALL_* iterator macros.
> Regression tested on x86-64 Fedora 34.
> ---
>  gdb/reverse.c | 137 +++++++++++++++++---------------------------------
>  1 file changed, 46 insertions(+), 91 deletions(-)
> 
> diff --git a/gdb/reverse.c b/gdb/reverse.c
> index dc417497b7a..fc94b8d1825 100644
> --- a/gdb/reverse.c
> +++ b/gdb/reverse.c
> @@ -92,23 +92,15 @@ reverse_finish (const char *args, int from_tty)
>  /* Data structures for a bookmark list.  */
>  
>  struct bookmark {
> -  struct bookmark *next;
> -  int number;
> -  CORE_ADDR pc;
> -  struct symtab_and_line sal;
> -  gdb_byte *opaque_data;
> +  int number = 0;
> +  CORE_ADDR pc = 0;
> +  struct symtab_and_line sal {};

{} is unnecessary here, symtab_and_line already initializes its fields.

> +  gdb::unique_xmalloc_ptr<gdb_byte> opaque_data;
>  };
>  
> -static struct bookmark *bookmark_chain;
> +static std::vector<struct bookmark> all_bookmarks;
>  static int bookmark_count;
>  
> -#define ALL_BOOKMARKS(B) for ((B) = bookmark_chain; (B); (B) = (B)->next)
> -
> -#define ALL_BOOKMARKS_SAFE(B,TMP)           \
> -     for ((B) = bookmark_chain;             \
> -	  (B) ? ((TMP) = (B)->next, 1) : 0; \
> -	  (B) = (TMP))
> -
>  /* save_bookmark_command -- implement "bookmark" command.
>     Call target method to get a bookmark identifier.
>     Insert bookmark identifier into list.
> @@ -130,80 +122,46 @@ save_bookmark_command (const char *args, int from_tty)
>      error (_("target_get_bookmark failed."));
>  
>    /* Set up a bookmark struct.  */
> -  bookmark *b = new bookmark ();
> -  b->number = ++bookmark_count;
> -  b->pc = regcache_read_pc (get_current_regcache ());
> -  b->sal = find_pc_line (b->pc, 0);
> -  b->sal.pspace = get_frame_program_space (get_current_frame ());
> -  b->opaque_data = bookmark_id;
> -  b->next = NULL;
> -
> -  /* Add this bookmark to the end of the chain, so that a list
> -     of bookmarks will come out in order of increasing numbers.  */
> -
> -  bookmark *b1 = bookmark_chain;
> -  if (b1 == 0)
> -    bookmark_chain = b;
> -  else
> -    {
> -      while (b1->next)
> -	b1 = b1->next;
> -      b1->next = b;
> -    }
> -  gdb_printf (_("Saved bookmark %d at %s\n"), b->number,
> -	      paddress (gdbarch, b->sal.pc));
> +  bookmark &b = all_bookmarks.emplace_back ();

I don't think this works in C++11, emplace_back returns void.

Otherwise, LGTM.

Simon

Re: [PATCH] Change bookmark allocation

[Tom Tromey]

>> +  struct symtab_and_line sal {};

Simon> {} is unnecessary here, symtab_and_line already initializes its fields.

Thanks, I did this.

>> +  bookmark &b = all_bookmarks.emplace_back ();

Simon> I don't think this works in C++11, emplace_back returns void.

I never remember this one since it seems so implausible to me :)

Simon> Otherwise, LGTM.

I made the changes and I'm going to check it in.

Tom