[RFA] Structure and simplify record log in record.c

[RFA] Structure and simplify record log in record.c

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 124 bytes --]

Hi Hui,

This patch abstracts and simplifies the allocation and destruction
of records in the process record log.

Michael


[-- Attachment #2: record_log.txt --]
[-- Type: text/plain, Size: 9577 bytes --]

2009-10-12  Michael Snyder  <msnyder@vmware.com>

	* record.c (record_reg_alloc): New function.
	(record_reg_release): New function.
	(record_mem_alloc): New function.
	(record_mem_release): New function.
	(record_end_alloc): New function.
	(record_end_release): New function.
	(record_entry_release): New function.
	(record_list_release): Simplify, call record_entry_release.
	(record_list_release_next): Rename to record_list_release_following.
	Simplify and call record_entry_release.
	(record_list_release_first): Simplify, comment, and use
	record_entry_release.
	(record_arch_list_add_reg): Simplify, call record_reg_alloc.
	(record_arch_list_add_mem): Simplify, call record_mem_alloc.
	(record_arch_list_add_end): Simplify, call record_end_alloc.

Index: record.c
===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.20
diff -u -p -r1.20 record.c
--- record.c	27 Sep 2009 02:49:34 -0000	1.20
+++ record.c	12 Oct 2009 17:25:22 -0000
@@ -129,88 +129,181 @@ static int (*record_beneath_to_insert_br
 static int (*record_beneath_to_remove_breakpoint) (struct gdbarch *,
 						   struct bp_target_info *);
 
+/*
+ * Alloc and free functions for record_reg, record_mem, and record_end 
+ * entries.
+ */
+
+/* Alloc a record_reg record entry.  */
+
+static inline struct record_entry *
+record_reg_alloc (struct regcache *regcache, int regnum)
+{
+  struct record_entry *rec;
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+
+  rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry));
+  rec->type = record_reg;
+  rec->u.reg.num = regnum;
+  rec->u.reg.val = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
+
+  return rec;
+}
+
+/* Free a record_reg record entry.  */
+
+static inline void
+record_reg_release (struct record_entry *rec)
+{
+  gdb_assert (rec->type == record_reg);
+  xfree (rec->u.reg.val);
+  xfree (rec);
+}
+
+/* Alloc a record_mem record entry.  */
+
+static inline struct record_entry *
+record_mem_alloc (CORE_ADDR addr, int len)
+{
+  struct record_entry *rec;
+
+  rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry));
+  rec->type = record_mem;
+  rec->u.mem.addr = addr;
+  rec->u.mem.len = len;
+  rec->u.mem.val = (gdb_byte *) xmalloc (len);
+
+  return rec;
+}
+
+/* Free a record_mem record entry.  */
+
+static inline void
+record_mem_release (struct record_entry *rec)
+{
+  gdb_assert (rec->type == record_mem);
+  xfree (rec->u.mem.val);
+  xfree (rec);
+}
+
+/* Alloc a record_mem record entry. */
+
+static inline struct record_entry *
+record_end_alloc (void)
+{
+  struct record_entry *rec;
+
+  rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry));
+  rec->type = record_end;
+
+  return rec;
+}
+
+/* Free a record_end record entry.  */
+
+static inline void
+record_end_release (struct record_entry *rec)
+{
+  xfree (rec);
+}
+
+/* Free one record entry, any type.
+   Return entry->type, in case caller wants to know.  */
+
+static inline enum record_type
+record_entry_release (struct record_entry *rec)
+{
+  enum record_type type = rec->type;
+
+  switch (type) {
+  case record_reg:
+    record_reg_release (rec);
+    break;
+  case record_mem:
+    record_mem_release (rec);
+    break;
+  case record_end:
+    record_end_release (rec);
+    break;
+  }
+  return type;
+}
+
+/* Free all record entries in list pointed to by REC.  */
+
 static void
 record_list_release (struct record_entry *rec)
 {
-  struct record_entry *tmp;
-
   if (!rec)
     return;
 
   while (rec->next)
-    {
-      rec = rec->next;
-    }
+    rec = rec->next;
 
   while (rec->prev)
     {
-      tmp = rec;
       rec = rec->prev;
-      if (tmp->type == record_reg)
-	xfree (tmp->u.reg.val);
-      else if (tmp->type == record_mem)
-	xfree (tmp->u.mem.val);
-      xfree (tmp);
+      record_entry_release (rec->next);
     }
 
-  if (rec != &record_first)
-    xfree (rec);
+  if (rec == &record_first)
+    {
+      record_insn_num = 0;
+      record_first.next = NULL;
+    }
+  else
+    record_entry_release (rec);
 }
 
+/* Free all record entries forward of the given list position.  */
+
 static void
-record_list_release_next (void)
+record_list_release_following (struct record_entry *rec)
 {
-  struct record_entry *rec = record_list;
   struct record_entry *tmp = rec->next;
+
   rec->next = NULL;
   while (tmp)
     {
       rec = tmp->next;
-      if (tmp->type == record_end)
+      if (record_entry_release (tmp) == record_end)
 	record_insn_num--;
-      else if (tmp->type == record_reg)
-	xfree (tmp->u.reg.val);
-      else if (tmp->type == record_mem)
-	xfree (tmp->u.mem.val);
-      xfree (tmp);
       tmp = rec;
     }
 }
 
+/* Free the first set of entries belonging to one instruction,
+   at the start of the main list (record_first).  */
+
 static void
 record_list_release_first (void)
 {
-  struct record_entry *tmp = NULL;
-  enum record_type type;
+  struct record_entry *tmp;
 
   if (!record_first.next)
     return;
 
+  /* Loop until a record_end.  */
   while (1)
     {
-      type = record_first.next->type;
-
-      if (type == record_reg)
-	xfree (record_first.next->u.reg.val);
-      else if (type == record_mem)
-	xfree (record_first.next->u.mem.val);
+      /* Cut record_first.next out of the linked list.  */
       tmp = record_first.next;
       record_first.next = tmp->next;
-      xfree (tmp);
+      tmp->next->prev = &record_first;
+
+      /* tmp is now isolated, and can be deleted.  */
+      if (record_entry_release (tmp) == record_end)
+	{
+	  record_insn_num--;
+	  break;	/* End loop at first record_end.  */
+	}
 
       if (!record_first.next)
 	{
 	  gdb_assert (record_insn_num == 1);
-	  break;
+	  break;	/* End loop when list is empty.  */
 	}
-
-      record_first.next->prev = &record_first;
-
-      if (type == record_end)
-	break;
     }
-
-  record_insn_num--;
 }
 
 /* Add a struct record_entry to record_arch_list.  */
@@ -236,10 +329,10 @@ record_arch_list_add (struct record_entr
     }
 }
 
-/* Record the value of a register NUM to record_arch_list.  */
+/* Record the value of a register REGNUM to record_arch_list.  */
 
 int
-record_arch_list_add_reg (struct regcache *regcache, int num)
+record_arch_list_add_reg (struct regcache *regcache, int regnum)
 {
   struct record_entry *rec;
 
@@ -247,16 +340,11 @@ record_arch_list_add_reg (struct regcach
     fprintf_unfiltered (gdb_stdlog,
 			"Process record: add register num = %d to "
 			"record list.\n",
-			num);
+			regnum);
 
-  rec = (struct record_entry *) xmalloc (sizeof (struct record_entry));
-  rec->u.reg.val = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
-  rec->prev = NULL;
-  rec->next = NULL;
-  rec->type = record_reg;
-  rec->u.reg.num = num;
+  rec = record_reg_alloc (regcache, regnum);
 
-  regcache_raw_read (regcache, num, rec->u.reg.val);
+  regcache_raw_read (regcache, regnum, rec->u.reg.val);
 
   record_arch_list_add (rec);
 
@@ -277,17 +365,10 @@ record_arch_list_add_mem (CORE_ADDR addr
 			"record list.\n",
 			paddress (target_gdbarch, addr), len);
 
-  if (!addr)
+  if (!addr)	/* FIXME: Why?  Some arch must permit it... */
     return 0;
 
-  rec = (struct record_entry *) xmalloc (sizeof (struct record_entry));
-  rec->u.mem.val = (gdb_byte *) xmalloc (len);
-  rec->prev = NULL;
-  rec->next = NULL;
-  rec->type = record_mem;
-  rec->u.mem.addr = addr;
-  rec->u.mem.len = len;
-  rec->u.mem.mem_entry_not_accessible = 0;
+  rec = record_mem_alloc (addr, len);
 
   if (target_read_memory (addr, rec->u.mem.val, len))
     {
@@ -296,8 +377,7 @@ record_arch_list_add_mem (CORE_ADDR addr
 			    "Process record: error reading memory at "
 			    "addr = %s len = %d.\n",
 			    paddress (target_gdbarch, addr), len);
-      xfree (rec->u.mem.val);
-      xfree (rec);
+      record_mem_release (rec);
       return -1;
     }
 
@@ -317,10 +397,7 @@ record_arch_list_add_end (void)
     fprintf_unfiltered (gdb_stdlog,
 			"Process record: add end to arch list.\n");
 
-  rec = (struct record_entry *) xmalloc (sizeof (struct record_entry));
-  rec->prev = NULL;
-  rec->next = NULL;
-  rec->type = record_end;
+  rec = record_end_alloc ();
   rec->u.end.sigval = TARGET_SIGNAL_0;
 
   record_arch_list_add (rec);
@@ -815,7 +892,7 @@ record_wait (struct target_ops *ops,
 		  else
 		    {
 		      if (target_write_memory (record_list->u.mem.addr,
-			                       record_list->u.mem.val,
+					       record_list->u.mem.val,
 		                               record_list->u.mem.len))
 	                {
 			  if (execution_direction != EXEC_REVERSE)
@@ -1056,7 +1133,7 @@ record_store_registers (struct target_op
 	    }
 
 	  /* Destroy the record from here forward.  */
-	  record_list_release_next ();
+	  record_list_release_following (record_list);
 	}
 
       record_registers_change (regcache, regno);
@@ -1088,7 +1165,7 @@ record_xfer_partial (struct target_ops *
 	    error (_("Process record canceled the operation."));
 
 	  /* Destroy the record from here forward.  */
-	  record_list_release_next ();
+	  record_list_release_following (record_list);
 	}
 
       /* Check record_insn_num */
@@ -1228,7 +1305,7 @@ cmd_record_delete (char *args, int from_
 	  if (!from_tty || query (_("Delete the log from this point forward "
 		                    "and begin to record the running message "
 		                    "at current PC?")))
-	    record_list_release_next ();
+	    record_list_release_following (record_list);
 	}
       else
 	  printf_unfiltered (_("Already at end of record list.\n"));

Re: [RFA] Structure and simplify record log in record.c

[Joel Brobecker]

> +/*
> + * Alloc and free functions for record_reg, record_mem, and record_end 
> + * entries.
> + */

Can you reformat this to be:

/* Alloc and free functions for record_reg, record_mem, and record_end
   entries.  */

?

This is really nit-picking, but I decided to comment on it because
I'd rather not see two different styles being used in GDB...

> +/* Alloc a record_mem record entry. */

I alos just noticed a missing space here.

-- 
Joel

Re: [RFA] Structure and simplify record log in record.c

[Hui Zhu]

Thanks Michaael and Joel,

I think the other part of this patch is OK with me.

But you still has 2 patches "[RFA] Fix off-by-one error in record.c
(record_list_release_first)" and "[RFA] Expand "info record" will
change the code of record list.
So maybe this patch need some update according to these patches.

Thanks,
Hui

On Tue, Oct 13, 2009 at 07:12, Joel Brobecker <brobecker@adacore.com> wrote:
>> +/*
>> + * Alloc and free functions for record_reg, record_mem, and record_end
>> + * entries.
>> + */
>
> Can you reformat this to be:
>
> /* Alloc and free functions for record_reg, record_mem, and record_end
>   entries.  */
>
> ?
>
> This is really nit-picking, but I decided to comment on it because
> I'd rather not see two different styles being used in GDB...
>
>> +/* Alloc a record_mem record entry. */
>
> I alos just noticed a missing space here.
>
> --
> Joel
>

Re: [RFA] Structure and simplify record log in record.c

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

Hui Zhu wrote:
> Thanks Michaael and Joel,
> 
> I think the other part of this patch is OK with me.
> 
> But you still has 2 patches "[RFA] Fix off-by-one error in record.c
> (record_list_release_first)" and "[RFA] Expand "info record" will
> change the code of record list.
> So maybe this patch need some update according to these patches.

Don't worry -- it's my responsibility to keep it in sync.
Committed as below, with Joel's suggestions and sync
(test suites passed on x86).



[-- Attachment #2: tmp.txt --]
[-- Type: text/plain, Size: 9426 bytes --]

2009-10-15  Michael Snyder  <msnyder@vmware.com>

	* record.c (record_reg_alloc): New function.
	(record_reg_release): New function.
	(record_mem_alloc): New function.
	(record_mem_release): New function.
	(record_end_alloc): New function.
	(record_end_release): New function.
	(record_entry_release): New function.
	(record_list_release): Simplify, call record_entry_release.
	(record_list_release_next): Rename to record_list_release_following.
	Simplify and call record_entry_release.
	(record_list_release_first): Simplify, comment, and use
	record_entry_release.
	(record_arch_list_add_reg): Simplify, call record_reg_alloc.
	(record_arch_list_add_mem): Simplify, call record_mem_alloc.
	(record_arch_list_add_end): Simplify, call record_end_alloc.

Index: record.c
===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.21
diff -u -p -r1.21 record.c
--- record.c	15 Oct 2009 16:57:36 -0000	1.21
+++ record.c	15 Oct 2009 17:14:30 -0000
@@ -129,50 +129,143 @@ static int (*record_beneath_to_insert_br
 static int (*record_beneath_to_remove_breakpoint) (struct gdbarch *,
 						   struct bp_target_info *);
 
+/* Alloc and free functions for record_reg, record_mem, and record_end 
+   entries.  */
+
+/* Alloc a record_reg record entry.  */
+
+static inline struct record_entry *
+record_reg_alloc (struct regcache *regcache, int regnum)
+{
+  struct record_entry *rec;
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+
+  rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry));
+  rec->type = record_reg;
+  rec->u.reg.num = regnum;
+  rec->u.reg.val = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
+
+  return rec;
+}
+
+/* Free a record_reg record entry.  */
+
+static inline void
+record_reg_release (struct record_entry *rec)
+{
+  gdb_assert (rec->type == record_reg);
+  xfree (rec->u.reg.val);
+  xfree (rec);
+}
+
+/* Alloc a record_mem record entry.  */
+
+static inline struct record_entry *
+record_mem_alloc (CORE_ADDR addr, int len)
+{
+  struct record_entry *rec;
+
+  rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry));
+  rec->type = record_mem;
+  rec->u.mem.addr = addr;
+  rec->u.mem.len = len;
+  rec->u.mem.val = (gdb_byte *) xmalloc (len);
+
+  return rec;
+}
+
+/* Free a record_mem record entry.  */
+
+static inline void
+record_mem_release (struct record_entry *rec)
+{
+  gdb_assert (rec->type == record_mem);
+  xfree (rec->u.mem.val);
+  xfree (rec);
+}
+
+/* Alloc a record_end record entry.  */
+
+static inline struct record_entry *
+record_end_alloc (void)
+{
+  struct record_entry *rec;
+
+  rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry));
+  rec->type = record_end;
+
+  return rec;
+}
+
+/* Free a record_end record entry.  */
+
+static inline void
+record_end_release (struct record_entry *rec)
+{
+  xfree (rec);
+}
+
+/* Free one record entry, any type.
+   Return entry->type, in case caller wants to know.  */
+
+static inline enum record_type
+record_entry_release (struct record_entry *rec)
+{
+  enum record_type type = rec->type;
+
+  switch (type) {
+  case record_reg:
+    record_reg_release (rec);
+    break;
+  case record_mem:
+    record_mem_release (rec);
+    break;
+  case record_end:
+    record_end_release (rec);
+    break;
+  }
+  return type;
+}
+
+/* Free all record entries in list pointed to by REC.  */
+
 static void
 record_list_release (struct record_entry *rec)
 {
-  struct record_entry *tmp;
-
   if (!rec)
     return;
 
   while (rec->next)
-    {
-      rec = rec->next;
-    }
+    rec = rec->next;
 
   while (rec->prev)
     {
-      tmp = rec;
       rec = rec->prev;
-      if (tmp->type == record_reg)
-	xfree (tmp->u.reg.val);
-      else if (tmp->type == record_mem)
-	xfree (tmp->u.mem.val);
-      xfree (tmp);
+      record_entry_release (rec->next);
     }
 
-  if (rec != &record_first)
-    xfree (rec);
+  if (rec == &record_first)
+    {
+      record_insn_num = 0;
+      record_first.next = NULL;
+    }
+  else
+    record_entry_release (rec);
 }
 
+/* Free all record entries forward of the given list position.  */
+
 static void
-record_list_release_next (void)
+record_list_release_following (struct record_entry *rec)
 {
-  struct record_entry *rec = record_list;
   struct record_entry *tmp = rec->next;
+
   rec->next = NULL;
   while (tmp)
     {
       rec = tmp->next;
-      if (tmp->type == record_end)
+      if (record_entry_release (tmp) == record_end)
 	record_insn_num--;
-      else if (tmp->type == record_reg)
-	xfree (tmp->u.reg.val);
-      else if (tmp->type == record_mem)
-	xfree (tmp->u.mem.val);
-      xfree (tmp);
       tmp = rec;
     }
 }
@@ -185,34 +278,31 @@ record_list_release_next (void)
 static void
 record_list_release_first (void)
 {
-  struct record_entry *tmp = NULL;
-  enum record_type type;
+  struct record_entry *tmp;
 
   if (!record_first.next)
     return;
 
+  /* Loop until a record_end.  */
   while (1)
     {
-      type = record_first.next->type;
-
-      if (type == record_reg)
-	xfree (record_first.next->u.reg.val);
-      else if (type == record_mem)
-	xfree (record_first.next->u.mem.val);
+      /* Cut record_first.next out of the linked list.  */
       tmp = record_first.next;
       record_first.next = tmp->next;
-      xfree (tmp);
+      tmp->next->prev = &record_first;
+
+      /* tmp is now isolated, and can be deleted.  */
+      if (record_entry_release (tmp) == record_end)
+	{
+	  record_insn_num--;
+	  break;	/* End loop at first record_end.  */
+	}
 
       if (!record_first.next)
 	{
 	  gdb_assert (record_insn_num == 1);
-	  break;
+	  break;	/* End loop when list is empty.  */
 	}
-
-      record_first.next->prev = &record_first;
-
-      if (type == record_end)
-	break;
     }
 }
 
@@ -239,10 +329,10 @@ record_arch_list_add (struct record_entr
     }
 }
 
-/* Record the value of a register NUM to record_arch_list.  */
+/* Record the value of a register REGNUM to record_arch_list.  */
 
 int
-record_arch_list_add_reg (struct regcache *regcache, int num)
+record_arch_list_add_reg (struct regcache *regcache, int regnum)
 {
   struct record_entry *rec;
 
@@ -250,16 +340,11 @@ record_arch_list_add_reg (struct regcach
     fprintf_unfiltered (gdb_stdlog,
 			"Process record: add register num = %d to "
 			"record list.\n",
-			num);
+			regnum);
 
-  rec = (struct record_entry *) xmalloc (sizeof (struct record_entry));
-  rec->u.reg.val = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
-  rec->prev = NULL;
-  rec->next = NULL;
-  rec->type = record_reg;
-  rec->u.reg.num = num;
+  rec = record_reg_alloc (regcache, regnum);
 
-  regcache_raw_read (regcache, num, rec->u.reg.val);
+  regcache_raw_read (regcache, regnum, rec->u.reg.val);
 
   record_arch_list_add (rec);
 
@@ -280,17 +365,10 @@ record_arch_list_add_mem (CORE_ADDR addr
 			"record list.\n",
 			paddress (target_gdbarch, addr), len);
 
-  if (!addr)
+  if (!addr)	/* FIXME: Why?  Some arch must permit it... */
     return 0;
 
-  rec = (struct record_entry *) xmalloc (sizeof (struct record_entry));
-  rec->u.mem.val = (gdb_byte *) xmalloc (len);
-  rec->prev = NULL;
-  rec->next = NULL;
-  rec->type = record_mem;
-  rec->u.mem.addr = addr;
-  rec->u.mem.len = len;
-  rec->u.mem.mem_entry_not_accessible = 0;
+  rec = record_mem_alloc (addr, len);
 
   if (target_read_memory (addr, rec->u.mem.val, len))
     {
@@ -299,8 +377,7 @@ record_arch_list_add_mem (CORE_ADDR addr
 			    "Process record: error reading memory at "
 			    "addr = %s len = %d.\n",
 			    paddress (target_gdbarch, addr), len);
-      xfree (rec->u.mem.val);
-      xfree (rec);
+      record_mem_release (rec);
       return -1;
     }
 
@@ -320,10 +397,7 @@ record_arch_list_add_end (void)
     fprintf_unfiltered (gdb_stdlog,
 			"Process record: add end to arch list.\n");
 
-  rec = (struct record_entry *) xmalloc (sizeof (struct record_entry));
-  rec->prev = NULL;
-  rec->next = NULL;
-  rec->type = record_end;
+  rec = record_end_alloc ();
   rec->u.end.sigval = TARGET_SIGNAL_0;
 
   record_arch_list_add (rec);
@@ -818,7 +892,7 @@ record_wait (struct target_ops *ops,
 		  else
 		    {
 		      if (target_write_memory (record_list->u.mem.addr,
-			                       record_list->u.mem.val,
+					       record_list->u.mem.val,
 		                               record_list->u.mem.len))
 	                {
 			  if (execution_direction != EXEC_REVERSE)
@@ -1059,7 +1133,7 @@ record_store_registers (struct target_op
 	    }
 
 	  /* Destroy the record from here forward.  */
-	  record_list_release_next ();
+	  record_list_release_following (record_list);
 	}
 
       record_registers_change (regcache, regno);
@@ -1091,7 +1165,7 @@ record_xfer_partial (struct target_ops *
 	    error (_("Process record canceled the operation."));
 
 	  /* Destroy the record from here forward.  */
-	  record_list_release_next ();
+	  record_list_release_following (record_list);
 	}
 
       /* Check record_insn_num */
@@ -1231,7 +1305,7 @@ cmd_record_delete (char *args, int from_
 	  if (!from_tty || query (_("Delete the log from this point forward "
 		                    "and begin to record the running message "
 		                    "at current PC?")))
-	    record_list_release_next ();
+	    record_list_release_following (record_list);
 	}
       else
 	  printf_unfiltered (_("Already at end of record list.\n"));

Re: [RFA] Structure and simplify record log in record.c

[Hui Zhu]

Thanks Michael.

Hui

On Fri, Oct 16, 2009 at 01:10, Michael Snyder <msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> Thanks Michaael and Joel,
>>
>> I think the other part of this patch is OK with me.
>>
>> But you still has 2 patches "[RFA] Fix off-by-one error in record.c
>> (record_list_release_first)" and "[RFA] Expand "info record" will
>> change the code of record list.
>> So maybe this patch need some update according to these patches.
>
> Don't worry -- it's my responsibility to keep it in sync.
> Committed as below, with Joel's suggestions and sync
> (test suites passed on x86).
>
>
>
> 2009-10-15  Michael Snyder  <msnyder@vmware.com>
>
>        * record.c (record_reg_alloc): New function.
>        (record_reg_release): New function.
>        (record_mem_alloc): New function.
>        (record_mem_release): New function.
>        (record_end_alloc): New function.
>        (record_end_release): New function.
>        (record_entry_release): New function.
>        (record_list_release): Simplify, call record_entry_release.
>        (record_list_release_next): Rename to record_list_release_following.
>        Simplify and call record_entry_release.
>        (record_list_release_first): Simplify, comment, and use
>        record_entry_release.
>        (record_arch_list_add_reg): Simplify, call record_reg_alloc.
>        (record_arch_list_add_mem): Simplify, call record_mem_alloc.
>        (record_arch_list_add_end): Simplify, call record_end_alloc.
>
> Index: record.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/record.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 record.c
> --- record.c    15 Oct 2009 16:57:36 -0000      1.21
> +++ record.c    15 Oct 2009 17:14:30 -0000
> @@ -129,50 +129,143 @@ static int (*record_beneath_to_insert_br
>  static int (*record_beneath_to_remove_breakpoint) (struct gdbarch *,
>                                                   struct bp_target_info *);
>
> +/* Alloc and free functions for record_reg, record_mem, and record_end
> +   entries.  */
> +
> +/* Alloc a record_reg record entry.  */
> +
> +static inline struct record_entry *
> +record_reg_alloc (struct regcache *regcache, int regnum)
> +{
> +  struct record_entry *rec;
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +
> +  rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry));
> +  rec->type = record_reg;
> +  rec->u.reg.num = regnum;
> +  rec->u.reg.val = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
> +
> +  return rec;
> +}
> +
> +/* Free a record_reg record entry.  */
> +
> +static inline void
> +record_reg_release (struct record_entry *rec)
> +{
> +  gdb_assert (rec->type == record_reg);
> +  xfree (rec->u.reg.val);
> +  xfree (rec);
> +}
> +
> +/* Alloc a record_mem record entry.  */
> +
> +static inline struct record_entry *
> +record_mem_alloc (CORE_ADDR addr, int len)
> +{
> +  struct record_entry *rec;
> +
> +  rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry));
> +  rec->type = record_mem;
> +  rec->u.mem.addr = addr;
> +  rec->u.mem.len = len;
> +  rec->u.mem.val = (gdb_byte *) xmalloc (len);
> +
> +  return rec;
> +}
> +
> +/* Free a record_mem record entry.  */
> +
> +static inline void
> +record_mem_release (struct record_entry *rec)
> +{
> +  gdb_assert (rec->type == record_mem);
> +  xfree (rec->u.mem.val);
> +  xfree (rec);
> +}
> +
> +/* Alloc a record_end record entry.  */
> +
> +static inline struct record_entry *
> +record_end_alloc (void)
> +{
> +  struct record_entry *rec;
> +
> +  rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry));
> +  rec->type = record_end;
> +
> +  return rec;
> +}
> +
> +/* Free a record_end record entry.  */
> +
> +static inline void
> +record_end_release (struct record_entry *rec)
> +{
> +  xfree (rec);
> +}
> +
> +/* Free one record entry, any type.
> +   Return entry->type, in case caller wants to know.  */
> +
> +static inline enum record_type
> +record_entry_release (struct record_entry *rec)
> +{
> +  enum record_type type = rec->type;
> +
> +  switch (type) {
> +  case record_reg:
> +    record_reg_release (rec);
> +    break;
> +  case record_mem:
> +    record_mem_release (rec);
> +    break;
> +  case record_end:
> +    record_end_release (rec);
> +    break;
> +  }
> +  return type;
> +}
> +
> +/* Free all record entries in list pointed to by REC.  */
> +
>  static void
>  record_list_release (struct record_entry *rec)
>  {
> -  struct record_entry *tmp;
> -
>   if (!rec)
>     return;
>
>   while (rec->next)
> -    {
> -      rec = rec->next;
> -    }
> +    rec = rec->next;
>
>   while (rec->prev)
>     {
> -      tmp = rec;
>       rec = rec->prev;
> -      if (tmp->type == record_reg)
> -       xfree (tmp->u.reg.val);
> -      else if (tmp->type == record_mem)
> -       xfree (tmp->u.mem.val);
> -      xfree (tmp);
> +      record_entry_release (rec->next);
>     }
>
> -  if (rec != &record_first)
> -    xfree (rec);
> +  if (rec == &record_first)
> +    {
> +      record_insn_num = 0;
> +      record_first.next = NULL;
> +    }
> +  else
> +    record_entry_release (rec);
>  }
>
> +/* Free all record entries forward of the given list position.  */
> +
>  static void
> -record_list_release_next (void)
> +record_list_release_following (struct record_entry *rec)
>  {
> -  struct record_entry *rec = record_list;
>   struct record_entry *tmp = rec->next;
> +
>   rec->next = NULL;
>   while (tmp)
>     {
>       rec = tmp->next;
> -      if (tmp->type == record_end)
> +      if (record_entry_release (tmp) == record_end)
>        record_insn_num--;
> -      else if (tmp->type == record_reg)
> -       xfree (tmp->u.reg.val);
> -      else if (tmp->type == record_mem)
> -       xfree (tmp->u.mem.val);
> -      xfree (tmp);
>       tmp = rec;
>     }
>  }
> @@ -185,34 +278,31 @@ record_list_release_next (void)
>  static void
>  record_list_release_first (void)
>  {
> -  struct record_entry *tmp = NULL;
> -  enum record_type type;
> +  struct record_entry *tmp;
>
>   if (!record_first.next)
>     return;
>
> +  /* Loop until a record_end.  */
>   while (1)
>     {
> -      type = record_first.next->type;
> -
> -      if (type == record_reg)
> -       xfree (record_first.next->u.reg.val);
> -      else if (type == record_mem)
> -       xfree (record_first.next->u.mem.val);
> +      /* Cut record_first.next out of the linked list.  */
>       tmp = record_first.next;
>       record_first.next = tmp->next;
> -      xfree (tmp);
> +      tmp->next->prev = &record_first;
> +
> +      /* tmp is now isolated, and can be deleted.  */
> +      if (record_entry_release (tmp) == record_end)
> +       {
> +         record_insn_num--;
> +         break;        /* End loop at first record_end.  */
> +       }
>
>       if (!record_first.next)
>        {
>          gdb_assert (record_insn_num == 1);
> -         break;
> +         break;        /* End loop when list is empty.  */
>        }
> -
> -      record_first.next->prev = &record_first;
> -
> -      if (type == record_end)
> -       break;
>     }
>  }
>
> @@ -239,10 +329,10 @@ record_arch_list_add (struct record_entr
>     }
>  }
>
> -/* Record the value of a register NUM to record_arch_list.  */
> +/* Record the value of a register REGNUM to record_arch_list.  */
>
>  int
> -record_arch_list_add_reg (struct regcache *regcache, int num)
> +record_arch_list_add_reg (struct regcache *regcache, int regnum)
>  {
>   struct record_entry *rec;
>
> @@ -250,16 +340,11 @@ record_arch_list_add_reg (struct regcach
>     fprintf_unfiltered (gdb_stdlog,
>                        "Process record: add register num = %d to "
>                        "record list.\n",
> -                       num);
> +                       regnum);
>
> -  rec = (struct record_entry *) xmalloc (sizeof (struct record_entry));
> -  rec->u.reg.val = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
> -  rec->prev = NULL;
> -  rec->next = NULL;
> -  rec->type = record_reg;
> -  rec->u.reg.num = num;
> +  rec = record_reg_alloc (regcache, regnum);
>
> -  regcache_raw_read (regcache, num, rec->u.reg.val);
> +  regcache_raw_read (regcache, regnum, rec->u.reg.val);
>
>   record_arch_list_add (rec);
>
> @@ -280,17 +365,10 @@ record_arch_list_add_mem (CORE_ADDR addr
>                        "record list.\n",
>                        paddress (target_gdbarch, addr), len);
>
> -  if (!addr)
> +  if (!addr)   /* FIXME: Why?  Some arch must permit it... */
>     return 0;
>
> -  rec = (struct record_entry *) xmalloc (sizeof (struct record_entry));
> -  rec->u.mem.val = (gdb_byte *) xmalloc (len);
> -  rec->prev = NULL;
> -  rec->next = NULL;
> -  rec->type = record_mem;
> -  rec->u.mem.addr = addr;
> -  rec->u.mem.len = len;
> -  rec->u.mem.mem_entry_not_accessible = 0;
> +  rec = record_mem_alloc (addr, len);
>
>   if (target_read_memory (addr, rec->u.mem.val, len))
>     {
> @@ -299,8 +377,7 @@ record_arch_list_add_mem (CORE_ADDR addr
>                            "Process record: error reading memory at "
>                            "addr = %s len = %d.\n",
>                            paddress (target_gdbarch, addr), len);
> -      xfree (rec->u.mem.val);
> -      xfree (rec);
> +      record_mem_release (rec);
>       return -1;
>     }
>
> @@ -320,10 +397,7 @@ record_arch_list_add_end (void)
>     fprintf_unfiltered (gdb_stdlog,
>                        "Process record: add end to arch list.\n");
>
> -  rec = (struct record_entry *) xmalloc (sizeof (struct record_entry));
> -  rec->prev = NULL;
> -  rec->next = NULL;
> -  rec->type = record_end;
> +  rec = record_end_alloc ();
>   rec->u.end.sigval = TARGET_SIGNAL_0;
>
>   record_arch_list_add (rec);
> @@ -818,7 +892,7 @@ record_wait (struct target_ops *ops,
>                  else
>                    {
>                      if (target_write_memory (record_list->u.mem.addr,
> -                                              record_list->u.mem.val,
> +                                              record_list->u.mem.val,
>                                               record_list->u.mem.len))
>                        {
>                          if (execution_direction != EXEC_REVERSE)
> @@ -1059,7 +1133,7 @@ record_store_registers (struct target_op
>            }
>
>          /* Destroy the record from here forward.  */
> -         record_list_release_next ();
> +         record_list_release_following (record_list);
>        }
>
>       record_registers_change (regcache, regno);
> @@ -1091,7 +1165,7 @@ record_xfer_partial (struct target_ops *
>            error (_("Process record canceled the operation."));
>
>          /* Destroy the record from here forward.  */
> -         record_list_release_next ();
> +         record_list_release_following (record_list);
>        }
>
>       /* Check record_insn_num */
> @@ -1231,7 +1305,7 @@ cmd_record_delete (char *args, int from_
>          if (!from_tty || query (_("Delete the log from this point forward "
>                                    "and begin to record the running message
> "
>                                    "at current PC?")))
> -           record_list_release_next ();
> +           record_list_release_following (record_list);
>        }
>       else
>          printf_unfiltered (_("Already at end of record list.\n"));
>
>