[RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

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

This is part 3 of the sync/update of process record save/restore patch.

This part actually adds the save and restore commands, and cleans
up a few loose ends.

If I need to resubmit the docs, I'll do that separately.
(part 4 of 3, I guess...)


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

2009-10-16  Hui Zhu  <teawater@gmail.com>
	    Michael Snyder  <msnyder@msnyder-server.eng.vmware.com>

	* record.c (RECORD_FILE_MAGIC): New constant.
	(record_arch_list_cleanups): Renamed from record_message_cleanups.
	(bfdcore_read): New function.
	(netorder64): New function.
	(netorder32): New function.
	(netorder16): New function.
	(record_restore): New function.  Restore a saved record log.
	(bfdcore_write): New function.
	(cmd_record_restore): New function.
	(cmd_record_save): New function.  Save a record log to a file.
	(_initialize_record): Set up commands for save and restore.

--- record.2.c	2009-10-16 15:09:49.000000000 -0700
+++ record.9.c	2009-10-16 17:43:40.000000000 -0700
@@ -23,17 +23,25 @@
 #include "gdbthread.h"
 #include "event-top.h"
 #include "exceptions.h"
+#include "completer.h"
+#include "arch-utils.h"
 #include "gdbcore.h"
 #include "exec.h"
 #include "record.h"
+#include "elf-bfd.h"
+#include "gcore.h"
 
+#include <byteswap.h>
 #include <signal.h>
+#include <netinet/in.h>
 
 #define DEFAULT_RECORD_INSN_MAX_NUM	200000
 
 #define RECORD_IS_REPLAY \
      (record_list->next || execution_direction == EXEC_REVERSE)
 
+#define RECORD_FILE_MAGIC	htonl(0x20091016)
+
 /* These are the core structs of the process record functionality.
 
    A record_entry is a record of the value change of a register
@@ -482,24 +490,24 @@ record_check_insn_num (int set_terminal)
 	      if (q)
 		record_stop_at_limit = 0;
 	      else
-		error (_("Process record: inferior program stopped."));
+		error (_("Process record: stopped by user."));
 	    }
 	}
     }
 }
 
+static void
+record_arch_list_cleanups (void *ignore)
+{
+  record_list_release (record_arch_list_tail);
+}
+
 /* Before inferior step (when GDB record the running message, inferior
    only can step), GDB will call this function to record the values to
    record_list.  This function will call gdbarch_process_record to
    record the running message of inferior and set them to
    record_arch_list, and add it to record_list.  */
 
-static void
-record_message_cleanups (void *ignore)
-{
-  record_list_release (record_arch_list_tail);
-}
-
 struct record_message_args {
   struct regcache *regcache;
   enum target_signal signal;
@@ -511,7 +519,7 @@ record_message (void *args)
   int ret;
   struct record_message_args *myargs = args;
   struct gdbarch *gdbarch = get_regcache_arch (myargs->regcache);
-  struct cleanup *old_cleanups = make_cleanup (record_message_cleanups, 0);
+  struct cleanup *old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
 
   record_arch_list_head = NULL;
   record_arch_list_tail = NULL;
@@ -676,6 +684,211 @@ record_exec_entry (struct regcache *regc
     }
 }
 
+/* bfdcore_read -- read bytes from a core file section.  */
+
+static void
+bfdcore_read (bfd *obfd, asection *osec, void *buf, int len, int *offset)
+{
+  int ret = bfd_get_section_contents (obfd, osec, buf, *offset, len);
+
+  if (ret)
+    *offset += len;
+  else
+    error (_("Failed to read %d bytes from core file ('%s').\n"),
+	   len, bfd_errmsg (bfd_get_error ()));
+}
+
+static uint64_t
+netorder64 (uint64_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_64 (fromfile) 
+    : fromfile;
+}
+
+static uint32_t
+netorder32 (uint32_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_32 (fromfile) 
+    : fromfile;
+}
+
+static uint16_t
+netorder16 (uint16_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_16 (fromfile) 
+    : fromfile;
+}
+
+/* Restore the execution log from a core_bfd file.  */
+
+static void
+record_restore (void)
+{
+  uint32_t magic;
+  struct cleanup *old_cleanups;
+  struct record_entry *rec;
+  asection *osec;
+  uint32_t osec_size;
+  int bfd_offset = 0;
+  struct regcache *regcache;
+
+  /* We restore the execution log from the open core bfd,
+     if there is one.  */
+  if (core_bfd == NULL)
+    return;
+
+  /* "record_restore" can only be called when record list is empty.  */
+  /* FIXME mvs */
+  gdb_assert (record_first.next == NULL);
+
+  if (record_debug)
+    printf_filtered (_("Restoring recording from core file.\n"));
+
+  /* Now need to find our special note section.  */
+  osec = bfd_get_section_by_name (core_bfd, "null0");
+  osec_size = bfd_section_size (core_bfd, osec);
+  printf_filtered ("Find precord section %s.\n",
+		   osec ? "succeeded" : "failed");
+  if (!osec)
+    return;
+  if (record_debug)
+    printf_filtered ("%s", bfd_section_name (core_bfd, osec));
+
+  /* Check the magic code.  */
+  if (record_debug)
+    printf_filtered (_("\
+  Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n"),
+		     magic);
+  bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
+  if (magic != RECORD_FILE_MAGIC)
+    error (_("Version mis-match or file format error."));
+
+  /* Restore the entries in recfd into record_arch_list_head and
+     record_arch_list_tail.  */
+  record_arch_list_head = NULL;
+  record_arch_list_tail = NULL;
+  record_insn_num = 0;
+  old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
+  regcache = get_current_regcache ();
+
+  while (1)
+    {
+      int ret;
+      uint8_t tmpu8;
+      uint32_t regnum, len, signal, count;
+      uint64_t addr;
+
+      /* We are finished when offset reaches osec_size.  */
+      if (bfd_offset >= osec_size)
+	break;
+      bfdcore_read (core_bfd, osec, &tmpu8, sizeof (tmpu8), &bfd_offset);
+
+      switch (tmpu8)
+        {
+        case record_reg: /* reg */
+          if (record_debug)
+            printf_filtered (_("\
+  Reading register %d (1 plus %d plus %d bytes)\n"),
+			     rec->u.reg.num,
+			     sizeof (regnum),
+			     rec->u.reg.len);
+          /* Get register number to regnum.  */
+          bfdcore_read (core_bfd, osec, &regnum,
+			sizeof (regnum), &bfd_offset);
+	  regnum = netorder32 (regnum);
+
+          rec = record_reg_alloc (regcache, regnum);
+
+          /* Get val.  */
+          bfdcore_read (core_bfd, osec, record_get_loc (rec),
+			rec->u.reg.len, &bfd_offset);
+          break;
+
+        case record_mem: /* mem */
+          if (record_debug)
+            printf_filtered (_("\
+  Reading memory %s (1 plus %d plus %d plus %d bytes)\n"),
+			     paddress (get_current_arch (),
+				       rec->u.mem.addr),
+			     sizeof (addr),
+			     sizeof (len),
+			     rec->u.mem.len);
+          /* Get len.  */
+          bfdcore_read (core_bfd, osec, &len, 
+			sizeof (len), &bfd_offset);
+	  len = netorder32 (len);
+
+          /* Get addr.  */
+          bfdcore_read (core_bfd, osec, &addr,
+			sizeof (addr), &bfd_offset);
+	  addr = netorder64 (addr);
+
+          rec = record_mem_alloc (addr, len);
+
+          /* Get val.  */
+          bfdcore_read (core_bfd, osec, record_get_loc (rec),
+			rec->u.mem.len, &bfd_offset);
+          break;
+
+        case record_end: /* end */
+          if (record_debug)
+            printf_filtered (_("\
+  Reading record_end (1 + %d + %d bytes), offset == %s\n"), 
+			     sizeof (signal),
+			     sizeof (count),
+			     paddress (get_current_arch (),
+				       bfd_offset));
+          rec = record_end_alloc ();
+          record_insn_num ++;
+
+	  /* Get signal value.  */
+	  bfdcore_read (core_bfd, osec, &signal, 
+			sizeof (signal), &bfd_offset);
+	  signal = netorder32 (signal);
+	  rec->u.end.sigval = signal;
+
+	  /* Get insn count.  */
+	  bfdcore_read (core_bfd, osec, &count, 
+			sizeof (count), &bfd_offset);
+	  count = netorder32 (count);
+	  rec->u.end.insn_num = count;
+	  record_insn_count = count + 1;
+          break;
+
+        default:
+          error (_("Format of core file is not right."));
+          break;
+        }
+
+      /* Add rec to record arch list.  */
+      record_arch_list_add (rec);
+    }
+
+  discard_cleanups (old_cleanups);
+
+  /* Add record_arch_list_head to the end of record list.  */
+  record_first.next = record_arch_list_head;
+  record_arch_list_head->prev = &record_first;
+  record_arch_list_tail->next = NULL;
+  record_list = &record_first;
+
+  /* Update record_insn_max_num.  */
+  if (record_insn_num > record_insn_max_num)
+    {
+      record_insn_max_num = record_insn_num;
+      warning (_("Auto increase record/replay buffer limit to %d."),
+               record_insn_max_num);
+    }
+
+  /* Succeeded.  */
+  printf_filtered ("Restored records from core file.\n");
+
+  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
+}
+
 static struct target_ops *tmp_to_resume_ops;
 static void (*tmp_to_resume) (struct target_ops *, ptid_t, int,
 			      enum target_signal);
@@ -733,9 +946,6 @@ record_open_1 (char *name, int from_tty)
 {
   struct target_ops *t;
 
-  if (record_debug)
-    fprintf_unfiltered (gdb_stdlog, "Process record: record_open\n");
-
   /* check exec */
   if (!target_has_execution)
     error (_("Process record: the program is not being run."));
@@ -842,9 +1052,9 @@ record_open (char *name, int from_tty)
   record_beneath_to_xfer_partial = tmp_to_xfer_partial;
   record_beneath_to_insert_breakpoint = tmp_to_insert_breakpoint;
   record_beneath_to_remove_breakpoint = tmp_to_remove_breakpoint;
-
-  //if (strcmp (current_target.to_shortname, "record_core") == 0)
-  //record_load ();
+  /* FIXME why doesn't this go in record_core_open?  */
+  if (strcmp (current_target.to_shortname, "record_core") == 0)
+    record_restore ();
 }
 
 static void
@@ -1643,6 +1853,283 @@ cmd_record_start (char *args, int from_t
   execute_command ("target record", from_tty);
 }
 
+/* Record log save-file format
+   Version 1 (never released)
+
+   Header:
+     4 bytes: magic number htonl(0x20090829).
+       NOTE: be sure to change whenever this file format changes!
+
+   Records:
+     record_end:
+       1 byte:  record type (record_end).
+     record_reg:
+       1 byte:  record type (record_reg).
+       8 bytes: register id (network byte order).
+       MAX_REGISTER_SIZE bytes: register value.
+     record_mem:
+       1 byte:  record type (record_mem).
+       8 bytes: memory length (network byte order).
+       8 bytes: memory address (network byte order).
+       n bytes: memory value (n == memory length).
+
+   Version 2
+     4 bytes: magic number htonl(0x20091016).
+       NOTE: be sure to change whenever this file format changes!
+
+   Records:
+     record_end:
+       1 byte:  record type (record_end).
+       4 bytes: signal
+       4 bytes: instruction count
+     record_reg:
+       1 byte:  record type (record_reg).
+       4 bytes: register id (network byte order).
+       n bytes: register value (n == register size).
+     record_mem:
+       1 byte:  record type (record_mem).
+       4 bytes: memory length (network byte order).
+       8 bytes: memory address (network byte order).
+       n bytes: memory value (n == memory length).
+
+*/
+
+/* bfdcore_write -- write bytes into a core file section.  */
+
+static void
+bfdcore_write (bfd *obfd, asection *osec, void *buf, int len, int *offset)
+{
+  int ret = bfd_set_section_contents (obfd, osec, buf, *offset, len);
+
+  if (ret)
+    *offset += len;
+  else
+    error (_("Failed to write %d bytes to core file ('%s').\n"),
+	   len, bfd_errmsg (bfd_get_error ()));
+}
+
+/* Restore the execution log from a file.  We use a modified elf
+   corefile format, with an extra section for our data.  */
+
+static void
+cmd_record_restore (char *args, int from_tty)
+{
+  core_file_command (args, from_tty);
+  record_open (args, from_tty);
+}
+
+/* Save the execution log to a file.  We use a modified elf corefile
+   format, with an extra section for our data.  */
+
+static void
+cmd_record_save (char *args, int from_tty)
+{
+  char *recfilename, recfilename_buffer[40];
+  int recfd;
+  struct record_entry *cur_record_list;
+  uint32_t magic;
+  struct regcache *regcache;
+  struct gdbarch *gdbarch;
+  struct cleanup *old_cleanups;
+  struct cleanup *set_cleanups;
+  bfd *obfd;
+  int save_size = 0;
+  asection *osec = NULL;
+  int bfd_offset = 0;
+
+  if (strcmp (current_target.to_shortname, "record") != 0)
+    error (_("Process record is not started.\n"));
+
+  if (args && *args)
+    recfilename = args;
+  else
+    {
+      /* Default recfile name is "gdb_record.PID".  */
+      snprintf (recfilename_buffer, sizeof (recfilename_buffer),
+                "gdb_record.%d", PIDGET (inferior_ptid));
+      recfilename = recfilename_buffer;
+    }
+
+  /* Open the save file.  */
+  if (record_debug)
+    printf_filtered (_("Saving recording to file '%s'\n"),
+		     recfilename);
+  /* Open the output file.  */
+  obfd = create_gcore_bfd (recfilename);
+  /* Need a cleanup that will close the file (FIXME: delete it?).  */
+  old_cleanups = make_cleanup_bfd_close (obfd);
+
+  /* Save the current record entry to "cur_record_list".  */
+  cur_record_list = record_list;
+
+  /* Get the values of regcache and gdbarch.  */
+  regcache = get_current_regcache ();
+  gdbarch = get_regcache_arch (regcache);
+
+  /* Disable the GDB operation record.  */
+  set_cleanups = record_gdb_operation_disable_set ();
+
+  /* Reverse execute to the begin of record list.  */
+  while (1)
+    {
+      /* Check for beginning and end of log.  */
+      if (record_list == &record_first)
+        break;
+
+      record_exec_entry (regcache, gdbarch, record_list);
+
+      if (record_list->prev)
+        record_list = record_list->prev;
+    }
+
+  /* Compute the size needed for the extra bfd section.  */
+  save_size = 4;	/* magic cookie */
+  for (record_list = record_first.next; record_list;
+       record_list = record_list->next)
+    switch (record_list->type)
+      {
+      case record_end:
+	save_size += 1 + 4 + 4;
+	break;
+      case record_reg:
+	save_size += 1 + 4 + record_list->u.reg.len;
+	break;
+      case record_mem:
+	save_size += 1 + 4 + 8 + record_list->u.mem.len;
+	break;
+      }
+
+  /* Make the new bfd section.  */
+  osec = bfd_make_section_anyway_with_flags (obfd, "precord",
+                                             SEC_HAS_CONTENTS
+                                             | SEC_READONLY);
+  if (osec == NULL)
+    error (_("Failed to create 'precord' section for corefile: %s"),
+           bfd_errmsg (bfd_get_error ()));
+  bfd_set_section_size (obfd, osec, save_size);
+  bfd_set_section_vma (obfd, osec, 0);
+  bfd_set_section_alignment (obfd, osec, 0);
+  bfd_section_lma (obfd, osec) = 0;
+
+  /* Save corefile state.  */
+  write_gcore_file (obfd);
+
+  /* Write out the record log.  */
+  /* Write the magic code.  */
+  magic = RECORD_FILE_MAGIC;
+  if (record_debug)
+    printf_filtered (_("\
+  Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n"),
+		     magic);
+  bfdcore_write (obfd, osec, &magic, sizeof (magic), &bfd_offset);
+
+  /* Save the entries to recfd and forward execute to the end of
+     record list.  */
+  record_list = &record_first;
+  while (1)
+    {
+      /* Save entry.  */
+      if (record_list != &record_first)
+        {
+	  uint8_t type;
+	  uint32_t regnum, len, signal, count;
+          uint64_t addr;
+
+	  type = record_list->type;
+          bfdcore_write (obfd, osec, &type, sizeof (type), &bfd_offset);
+
+          switch (record_list->type)
+            {
+            case record_reg: /* reg */
+              if (record_debug)
+		printf_filtered (_("\
+  Writing register %d (1 plus %d plus %d bytes)\n"),
+				 record_list->u.reg.num,
+				 sizeof (regnum),
+				 record_list->u.reg.len);
+
+              /* Write regnum.  */
+              regnum = netorder32 (record_list->u.reg.num);
+              bfdcore_write (obfd, osec, &regnum,
+			     sizeof (regnum), &bfd_offset);
+
+              /* Write regval.  */
+              bfdcore_write (obfd, osec, record_get_loc (record_list),
+			     record_list->u.reg.len, &bfd_offset);
+              break;
+
+            case record_mem: /* mem */
+	      if (record_debug)
+		printf_filtered (_("\
+  Writing memory %s (1 plus %d plus %d plus %d bytes)\n"),
+				 paddress (gdbarch,
+					   record_list->u.mem.addr),
+				 sizeof (addr),
+				 sizeof (len),
+				 record_list->u.mem.len);
+
+	      /* Write memlen.  */
+	      len = netorder32 (record_list->u.mem.len);
+	      bfdcore_write (obfd, osec, &len, sizeof (len), &bfd_offset);
+
+	      /* Write memaddr.  */
+	      addr = netorder64 (record_list->u.mem.addr);
+	      bfdcore_write (obfd, osec, &addr, 
+			     sizeof (addr), &bfd_offset);
+
+	      /* Write memval.  */
+	      bfdcore_write (obfd, osec, record_get_loc (record_list),
+			     record_list->u.mem.len, &bfd_offset);
+              break;
+
+              case record_end:
+                if (record_debug)
+                  printf_filtered (_("\
+  Writing record_end (1 + %d + %d bytes)\n"), 
+				   sizeof (signal),
+				   sizeof (count));
+		/* Write signal value.  */
+		signal = netorder32 (record_list->u.end.sigval);
+		bfdcore_write (obfd, osec, &signal,
+			       sizeof (signal), &bfd_offset);
+
+		/* Write insn count.  */
+		count = netorder32 (record_list->u.end.insn_num);
+		bfdcore_write (obfd, osec, &count,
+			       sizeof (count), &bfd_offset);
+                break;
+            }
+        }
+
+      /* Execute entry.  */
+      record_exec_entry (regcache, gdbarch, record_list);
+
+      if (record_list->next)
+        record_list = record_list->next;
+      else
+        break;
+    }
+
+  /* Reverse execute to cur_record_list.  */
+  while (1)
+    {
+      /* Check for beginning and end of log.  */
+      if (record_list == cur_record_list)
+        break;
+
+      record_exec_entry (regcache, gdbarch, record_list);
+
+      if (record_list->prev)
+        record_list = record_list->prev;
+    }
+
+  do_cleanups (set_cleanups);
+  do_cleanups (old_cleanups);
+
+  /* Succeeded.  */
+  printf_filtered (_("Saved recfile %s.\n"), recfilename);
+}
+
 /* Truncate the record log from the present point
    of replay until the end.  */
 
@@ -1750,6 +2237,8 @@ info_record_command (char *args, int fro
 void
 _initialize_record (void)
 {
+  struct cmd_list_element *c;
+
   /* Init record_first.  */
   record_first.prev = NULL;
   record_first.next = NULL;
@@ -1768,10 +2257,12 @@ _initialize_record (void)
 			    NULL, show_record_debug, &setdebuglist,
 			    &showdebuglist);
 
-  add_prefix_cmd ("record", class_obscure, cmd_record_start,
+  c = add_prefix_cmd ("record", class_obscure, cmd_record_start,
 		  _("Abbreviated form of \"target record\" command."),
  		  &record_cmdlist, "record ", 0, &cmdlist);
+  set_cmd_completer (c, filename_completer);
   add_com_alias ("rec", "record", class_obscure, 1);
+
   add_prefix_cmd ("record", class_support, set_record_command,
 		  _("Set record options"), &set_record_cmdlist,
 		  "set record ", 0, &setlist);
@@ -1785,6 +2276,17 @@ _initialize_record (void)
 		  "info record ", 0, &infolist);
   add_alias_cmd ("rec", "record", class_obscure, 1, &infolist);
 
+  c = add_cmd ("save", class_obscure, cmd_record_save,
+	       _("Save the execution log to a file.\n\
+Argument is optional filename.\n\
+Default filename is 'gdb_record.<process_id>'."),
+	       &record_cmdlist);
+
+  c = add_cmd ("restore", class_obscure, cmd_record_restore,
+	       _("Restore the execution log from a file.\n\
+Argument is filename.  File must be created with 'record dump'."),
+	       &record_cmdlist);
+  set_cmd_completer (c, filename_completer);
 
   add_cmd ("delete", class_obscure, cmd_record_delete,
 	   _("Delete the rest of execution log and start recording it anew."),

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Eli Zaretskii]

> Date: Fri, 16 Oct 2009 18:27:14 -0700
> From: Michael Snyder <msnyder@vmware.com>
> 
> If I need to resubmit the docs, I'll do that separately.

Yes, please.  It should document "record save" and "record restore".
An entry in NEWS is also in order.

What follows are mainly usability and UI related comments.

> +  if (record_debug)
> +    printf_filtered (_("Restoring recording from core file.\n"));

Debug messages are not supposed to be marked for translation.  (You
have several more of those in the patch.)

> +  printf_filtered ("Find precord section %s.\n",
> +		   osec ? "succeeded" : "failed");

Is this also a debug printout?  If so, why isn't it conditioned by
record_debug?  If it isn't, then why not marked for translation?  (I
think this kind of message should definitely be printed only under
record_debug.)

> +  bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
> +  if (magic != RECORD_FILE_MAGIC)
> +    error (_("Version mis-match or file format error."));

It would be nice to say what file this refers to.

> +        default:
> +          error (_("Format of core file is not right."));

Suggest something like "Incorrect or unsupported format of core file."
"Not right" is too vague, IMO.

> +  printf_filtered ("Restored records from core file.\n");

This should be marked for translation.

> +  /* FIXME why doesn't this go in record_core_open?  */
> +  if (strcmp (current_target.to_shortname, "record_core") == 0)
> +    record_restore ();

Yes, why indeed?  Can this be resolved before the patch goes in?

> +     4 bytes: magic number htonl(0x20090829).
                              ^^^^^
Elsewhere in this documentation you use a more human-readable "network
byte order".

> +       NOTE: be sure to change whenever this file format changes!
> +
> +   Records:
> +     record_end:
> +       1 byte:  record type (record_end).
                                ^^^^^^^^^^
This probably has some integer value, right?  Or does this indicate
something other than an integer type?

> +     record_reg:
> +       1 byte:  record type (record_reg).
> +       4 bytes: register id (network byte order).
> +       n bytes: register value (n == register size).
                                        ^^^^^^^^^^^^^
How does one know what is the correct register size?

> +    error (_("Failed to write %d bytes to core file ('%s').\n"),
> +	   len, bfd_errmsg (bfd_get_error ()));

How about stating the name of the file?

> +  if (strcmp (current_target.to_shortname, "record") != 0)
> +    error (_("Process record is not started.\n"));

Suggest to add to the message text something that tells the user how
to remedy this situation.  E.g., "Invoke FOO command first."

> +      snprintf (recfilename_buffer, sizeof (recfilename_buffer),
> +                "gdb_record.%d", PIDGET (inferior_ptid));

What will this do in go32, where the "PID" is always 42?  Does a
target or an architecture have a way of overriding this default?

> +  if (record_debug)
> +    printf_filtered (_("Saving recording to file '%s'\n"),
> +		     recfilename);

Suggest to say "Saving execution log to core file '%s'\n".  I added
"core" because you use that freely elsewhere, and the user should
therefore know that the recorded data goes to a file formatted as a
core file.

> +  /* Need a cleanup that will close the file (FIXME: delete it?).  */

Can this be fixed before you commit?

> +  if (osec == NULL)
> +    error (_("Failed to create 'precord' section for corefile: %s"),
> +           bfd_errmsg (bfd_get_error ()));

Again, adding the name of the file will go a long way towards making
the intent clear to a user who has no clue in how precord works.

> +  printf_filtered (_("Saved recfile %s.\n"), recfilename);

"recfile"?  What's that? ;-)

> +Argument is filename.  File must be created with 'record dump'."),
                                                     ^^^^^^^^^^^
You mean, "record save", right?

Thanks.

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

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

Eli Zaretskii wrote:
>> Date: Fri, 16 Oct 2009 18:27:14 -0700
>> From: Michael Snyder <msnyder@vmware.com>
>>
>> If I need to resubmit the docs, I'll do that separately.
> 
> Yes, please.  It should document "record save" and "record restore".
> An entry in NEWS is also in order.

Sure.  I'll follow up with a fourth patch.

> What follows are mainly usability and UI related comments.
> 
>> +  if (record_debug)
>> +    printf_filtered (_("Restoring recording from core file.\n"));
> 
> Debug messages are not supposed to be marked for translation.  (You
> have several more of those in the patch.)

They're not?  OK -- I'll change them.   ;-)

>> +  printf_filtered ("Find precord section %s.\n",
>> +		   osec ? "succeeded" : "failed");
> 
> Is this also a debug printout?  If so, why isn't it conditioned by
> record_debug?  If it isn't, then why not marked for translation?  (I
> think this kind of message should definitely be printed only under
> record_debug.)

Right.

>> +  bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
>> +  if (magic != RECORD_FILE_MAGIC)
>> +    error (_("Version mis-match or file format error."));
> 
> It would be nice to say what file this refers to.

OK.  Also adding file name to several other error messages.

>> +        default:
>> +          error (_("Format of core file is not right."));
> 
> Suggest something like "Incorrect or unsupported format of core file."
> "Not right" is too vague, IMO.

OK.

>> +  printf_filtered ("Restored records from core file.\n");
> 
> This should be marked for translation.

Check.

>> +  /* FIXME why doesn't this go in record_core_open?  */
>> +  if (strcmp (current_target.to_shortname, "record_core") == 0)
>> +    record_restore ();
> 
> Yes, why indeed?  Can this be resolved before the patch goes in?

Ah, thanks for poking me on that.  It's fixed now.

>> +     4 bytes: magic number htonl(0x20090829).
>                               ^^^^^
> Elsewhere in this documentation you use a more human-readable "network
> byte order".

Hmmm.  OK, yeah, that'll work.
Can get rid of an include file now, too.


>> +       NOTE: be sure to change whenever this file format changes!
>> +
>> +   Records:
>> +     record_end:
>> +       1 byte:  record type (record_end).
>                                 ^^^^^^^^^^
> This probably has some integer value, right?  Or does this indicate
> something other than an integer type?

It's an enum.  Why?  You think I should give its numeric
value in the comment, instead of its symbolic value?


>> +     record_reg:
>> +       1 byte:  record type (record_reg).
>> +       4 bytes: register id (network byte order).
>> +       n bytes: register value (n == register size).
>                                         ^^^^^^^^^^^^^
> How does one know what is the correct register size?

We get it from the current regcache and regnum.  What can I say about
it here, that wouldn't be arch-specific?  I mean, I could say:

    This will generally be 4 bytes for x86, 8 bytes for x86_64.

but that doesn't seem very useful or scalable.  Plus it will
be different for floating point regs, once we handle them too.



>> +    error (_("Failed to write %d bytes to core file ('%s').\n"),
>> +	   len, bfd_errmsg (bfd_get_error ()));
> 
> How about stating the name of the file?

Yep.

>> +  if (strcmp (current_target.to_shortname, "record") != 0)
>> +    error (_("Process record is not started.\n"));
> 
> Suggest to add to the message text something that tells the user how
> to remedy this situation.  E.g., "Invoke FOO command first."

OK.  It's a target-specific command, that can only be used
with target 'record'.  How about this?

   error (_("This command can only be used with target 'record'.\n"));

In general, I think we need a better idea about how to handle
commands that can only be used with a specific target or backend.

>> +      snprintf (recfilename_buffer, sizeof (recfilename_buffer),
>> +                "gdb_record.%d", PIDGET (inferior_ptid));
> 
> What will this do in go32, where the "PID" is always 42?  Does a
> target or an architecture have a way of overriding this default?

Dunno, currently the only OSABI that we work with is linux.
But you're right, if and when we add more, we'll need to
handle this better.

This is the same approach that is used by the "gcore" command.
How does "gcore" work with go32, if at all?

> 
>> +  if (record_debug)
>> +    printf_filtered (_("Saving recording to file '%s'\n"),
>> +		     recfilename);
> 
> Suggest to say "Saving execution log to core file '%s'\n".  I added
> "core" because you use that freely elsewhere, and the user should
> therefore know that the recorded data goes to a file formatted as a
> core file.

OK.

>> +  /* Need a cleanup that will close the file (FIXME: delete it?).  */

Hmmm, yeah.  Done.

>> +  if (osec == NULL)
>> +    error (_("Failed to create 'precord' section for corefile: %s"),
>> +           bfd_errmsg (bfd_get_error ()));
> 
> Again, adding the name of the file will go a long way towards making
> the intent clear to a user who has no clue in how precord works.

Done.

>> +  printf_filtered (_("Saved recfile %s.\n"), recfilename);
> 
> "recfile"?  What's that? ;-)

Right.

>> +Argument is filename.  File must be created with 'record dump'."),
>                                                      ^^^^^^^^^^^
> You mean, "record save", right?

Yes.  Thanks for all your suggestions, Eli.
Please see revised patch attached.



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

--- record.2b.c	2009-10-17 11:30:35.000000000 -0700
+++ record.11.c	2009-10-17 11:30:12.000000000 -0700
@@ -23,10 +23,15 @@
 #include "gdbthread.h"
 #include "event-top.h"
 #include "exceptions.h"
+#include "completer.h"
+#include "arch-utils.h"
 #include "gdbcore.h"
 #include "exec.h"
 #include "record.h"
+#include "elf-bfd.h"
+#include "gcore.h"
 
+#include <byteswap.h>
 #include <signal.h>
 
 #define DEFAULT_RECORD_INSN_MAX_NUM	200000
@@ -34,6 +39,8 @@
 #define RECORD_IS_REPLAY \
      (record_list->next || execution_direction == EXEC_REVERSE)
 
+#define RECORD_FILE_MAGIC	netorder32(0x20091016)
+
 /* These are the core structs of the process record functionality.
 
    A record_entry is a record of the value change of a register
@@ -482,24 +489,24 @@ record_check_insn_num (int set_terminal)
 	      if (q)
 		record_stop_at_limit = 0;
 	      else
-		error (_("Process record: inferior program stopped."));
+		error (_("Process record: stopped by user."));
 	    }
 	}
     }
 }
 
+static void
+record_arch_list_cleanups (void *ignore)
+{
+  record_list_release (record_arch_list_tail);
+}
+
 /* Before inferior step (when GDB record the running message, inferior
    only can step), GDB will call this function to record the values to
    record_list.  This function will call gdbarch_process_record to
    record the running message of inferior and set them to
    record_arch_list, and add it to record_list.  */
 
-static void
-record_message_cleanups (void *ignore)
-{
-  record_list_release (record_arch_list_tail);
-}
-
 struct record_message_args {
   struct regcache *regcache;
   enum target_signal signal;
@@ -511,7 +518,7 @@ record_message (void *args)
   int ret;
   struct record_message_args *myargs = args;
   struct gdbarch *gdbarch = get_regcache_arch (myargs->regcache);
-  struct cleanup *old_cleanups = make_cleanup (record_message_cleanups, 0);
+  struct cleanup *old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
 
   record_arch_list_head = NULL;
   record_arch_list_tail = NULL;
@@ -651,8 +658,8 @@ record_exec_insn (struct regcache *regca
               {
                 entry->u.mem.mem_entry_not_accessible = 1;
                 if (record_debug)
-                  warning (_("Process record: error reading memory at "
-                             "addr = %s len = %d."),
+                  warning ("Process record: error reading memory at "
+			   "addr = %s len = %d.",
 			   paddress (gdbarch, entry->u.mem.addr),
                            entry->u.mem.len);
               }
@@ -664,8 +671,8 @@ record_exec_insn (struct regcache *regca
                   {
                     entry->u.mem.mem_entry_not_accessible = 1;
                     if (record_debug)
-                      warning (_("Process record: error writing memory at "
-                                 "addr = %s len = %d."),
+                      warning ("Process record: error writing memory at "
+			       "addr = %s len = %d.",
                                paddress (gdbarch, entry->u.mem.addr),
                                entry->u.mem.len);
                   }
@@ -678,6 +685,215 @@ record_exec_insn (struct regcache *regca
     }
 }
 
+/* bfdcore_read -- read bytes from a core file section.  */
+
+static void
+bfdcore_read (bfd *obfd, asection *osec, void *buf, int len, int *offset)
+{
+  int ret = bfd_get_section_contents (obfd, osec, buf, *offset, len);
+
+  if (ret)
+    *offset += len;
+  else
+    error (_("Failed to read %d bytes from core file %s ('%s').\n"),
+	   len, bfd_get_filename (obfd),
+	   bfd_errmsg (bfd_get_error ()));
+}
+
+static uint64_t
+netorder64 (uint64_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_64 (fromfile) 
+    : fromfile;
+}
+
+static uint32_t
+netorder32 (uint32_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_32 (fromfile) 
+    : fromfile;
+}
+
+static uint16_t
+netorder16 (uint16_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_16 (fromfile) 
+    : fromfile;
+}
+
+/* Restore the execution log from a core_bfd file.  */
+
+static void
+record_restore (void)
+{
+  uint32_t magic;
+  struct cleanup *old_cleanups;
+  struct record_entry *rec;
+  asection *osec;
+  uint32_t osec_size;
+  int bfd_offset = 0;
+  struct regcache *regcache;
+
+  /* We restore the execution log from the open core bfd,
+     if there is one.  */
+  if (core_bfd == NULL)
+    return;
+
+  /* "record_restore" can only be called when record list is empty.  */
+  gdb_assert (record_first.next == NULL);
+
+  if (record_debug)
+    printf_filtered ("Restoring recording from core file.\n");
+
+  /* Now need to find our special note section.  */
+  osec = bfd_get_section_by_name (core_bfd, "null0");
+  osec_size = bfd_section_size (core_bfd, osec);
+  if (record_debug)
+    printf_filtered ("Find precord section %s.\n",
+		     osec ? "succeeded" : "failed");
+  if (!osec)
+    return;
+  if (record_debug)
+    printf_filtered ("%s", bfd_section_name (core_bfd, osec));
+
+  /* Check the magic code.  */
+  if (record_debug)
+    printf_filtered ("\
+  Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
+		     magic);
+  bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
+  if (magic != RECORD_FILE_MAGIC)
+    error (_("Version mis-match or file format error in core file %s."),
+	   bfd_get_filename (core_bfd));
+
+  /* Restore the entries in recfd into record_arch_list_head and
+     record_arch_list_tail.  */
+  record_arch_list_head = NULL;
+  record_arch_list_tail = NULL;
+  record_insn_num = 0;
+  old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
+  regcache = get_current_regcache ();
+
+  while (1)
+    {
+      int ret;
+      uint8_t tmpu8;
+      uint32_t regnum, len, signal, count;
+      uint64_t addr;
+
+      /* We are finished when offset reaches osec_size.  */
+      if (bfd_offset >= osec_size)
+	break;
+      bfdcore_read (core_bfd, osec, &tmpu8, sizeof (tmpu8), &bfd_offset);
+
+      switch (tmpu8)
+        {
+        case record_reg: /* reg */
+          if (record_debug)
+            printf_filtered ("\
+  Reading register %d (1 plus %d plus %d bytes)\n",
+			     rec->u.reg.num,
+			     sizeof (regnum),
+			     rec->u.reg.len);
+          /* Get register number to regnum.  */
+          bfdcore_read (core_bfd, osec, &regnum,
+			sizeof (regnum), &bfd_offset);
+	  regnum = netorder32 (regnum);
+
+          rec = record_reg_alloc (regcache, regnum);
+
+          /* Get val.  */
+          bfdcore_read (core_bfd, osec, record_get_loc (rec),
+			rec->u.reg.len, &bfd_offset);
+          break;
+
+        case record_mem: /* mem */
+          if (record_debug)
+            printf_filtered ("\
+  Reading memory %s (1 plus %d plus %d plus %d bytes)\n",
+			     paddress (get_current_arch (),
+				       rec->u.mem.addr),
+			     sizeof (addr),
+			     sizeof (len),
+			     rec->u.mem.len);
+          /* Get len.  */
+          bfdcore_read (core_bfd, osec, &len, 
+			sizeof (len), &bfd_offset);
+	  len = netorder32 (len);
+
+          /* Get addr.  */
+          bfdcore_read (core_bfd, osec, &addr,
+			sizeof (addr), &bfd_offset);
+	  addr = netorder64 (addr);
+
+          rec = record_mem_alloc (addr, len);
+
+          /* Get val.  */
+          bfdcore_read (core_bfd, osec, record_get_loc (rec),
+			rec->u.mem.len, &bfd_offset);
+          break;
+
+        case record_end: /* end */
+          if (record_debug)
+            printf_filtered ("\
+  Reading record_end (1 + %d + %d bytes), offset == %s\n",
+			     sizeof (signal),
+			     sizeof (count),
+			     paddress (get_current_arch (),
+				       bfd_offset));
+          rec = record_end_alloc ();
+          record_insn_num ++;
+
+	  /* Get signal value.  */
+	  bfdcore_read (core_bfd, osec, &signal, 
+			sizeof (signal), &bfd_offset);
+	  signal = netorder32 (signal);
+	  rec->u.end.sigval = signal;
+
+	  /* Get insn count.  */
+	  bfdcore_read (core_bfd, osec, &count, 
+			sizeof (count), &bfd_offset);
+	  count = netorder32 (count);
+	  rec->u.end.insn_num = count;
+	  record_insn_count = count + 1;
+          break;
+
+        default:
+          error (_("Bad entry type in core file %s."),
+		 bfd_get_filename (core_bfd));
+          break;
+        }
+
+      /* Add rec to record arch list.  */
+      record_arch_list_add (rec);
+    }
+
+  discard_cleanups (old_cleanups);
+
+  /* Add record_arch_list_head to the end of record list.  */
+  record_first.next = record_arch_list_head;
+  record_arch_list_head->prev = &record_first;
+  record_arch_list_tail->next = NULL;
+  record_list = &record_first;
+
+  /* Update record_insn_max_num.  */
+  if (record_insn_num > record_insn_max_num)
+    {
+      record_insn_max_num = record_insn_num;
+      warning (_("Auto increase record/replay buffer limit to %d."),
+               record_insn_max_num);
+    }
+
+  /* Succeeded.  */
+  printf_filtered (_("Restored records from core file %s.\n"),
+		   bfd_get_filename (core_bfd));
+
+  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
+}
+
 static struct target_ops *tmp_to_resume_ops;
 static void (*tmp_to_resume) (struct target_ops *, ptid_t, int,
 			      enum target_signal);
@@ -728,6 +944,7 @@ record_core_open_1 (char *name, int from
     }
 
   push_target (&record_core_ops);
+  record_restore ();
 }
 
 static void
@@ -735,9 +952,6 @@ record_open_1 (char *name, int from_tty)
 {
   struct target_ops *t;
 
-  if (record_debug)
-    fprintf_unfiltered (gdb_stdlog, "Process record: record_open\n");
-
   /* check exec */
   if (!target_has_execution)
     error (_("Process record: the program is not being run."));
@@ -779,6 +993,12 @@ record_open (char *name, int from_tty)
     error (_("Process record target already running.  Use \"record stop\" to "
              "stop record target first."));
 
+  /* Reset */
+  record_insn_num = 0;
+  record_insn_count = 0;
+  record_list = &record_first;
+  record_list->next = NULL;
+
   /* Reset the tmp beneath pointers.  */
   tmp_to_resume_ops = NULL;
   tmp_to_resume = NULL;
@@ -822,17 +1042,6 @@ record_open (char *name, int from_tty)
   if (!tmp_to_xfer_partial)
     error (_("Could not find 'to_xfer_partial' method on the target stack."));
 
-  if (current_target.to_stratum == core_stratum)
-    record_core_open_1 (name, from_tty);
-  else
-    record_open_1 (name, from_tty);
-
-  /* Reset */
-  record_insn_num = 0;
-  record_insn_count = 0;
-  record_list = &record_first;
-  record_list->next = NULL;
-
   /* Set the tmp beneath pointers to beneath pointers.  */
   record_beneath_to_resume_ops = tmp_to_resume_ops;
   record_beneath_to_resume = tmp_to_resume;
@@ -844,6 +1053,11 @@ record_open (char *name, int from_tty)
   record_beneath_to_xfer_partial = tmp_to_xfer_partial;
   record_beneath_to_insert_breakpoint = tmp_to_insert_breakpoint;
   record_beneath_to_remove_breakpoint = tmp_to_remove_breakpoint;
+
+  if (current_target.to_stratum == core_stratum)
+    record_core_open_1 (name, from_tty);
+  else
+    record_open_1 (name, from_tty);
 }
 
 static void
@@ -1114,8 +1328,7 @@ record_wait (struct target_ops *ops,
 		    {
 		      if (record_debug)
 			fprintf_unfiltered (gdb_stdlog,
-					    "Process record: break "
-					    "at %s.\n",
+					    "Process record: break at %s.\n",
 					    paddress (gdbarch, tmp_pc));
 		      if (gdbarch_decr_pc_after_break (gdbarch)
 			  && execution_direction == EXEC_FORWARD
@@ -1190,8 +1403,7 @@ static void
 record_mourn_inferior (struct target_ops *ops)
 {
   if (record_debug)
-    fprintf_unfiltered (gdb_stdlog, "Process record: "
-			            "record_mourn_inferior\n");
+    fprintf_unfiltered (gdb_stdlog, "Process record: record_mourn_inferior\n");
 
   unpush_target (&record_ops);
   target_mourn_inferior ();
@@ -1345,8 +1557,8 @@ record_xfer_partial (struct target_ops *
 	  record_list_release (record_arch_list_tail);
 	  if (record_debug)
 	    fprintf_unfiltered (gdb_stdlog,
-				_("Process record: failed to record "
-				  "execution log."));
+				"Process record: failed to record "
+				"execution log.");
 	  return -1;
 	}
       if (record_arch_list_add_end ())
@@ -1354,8 +1566,8 @@ record_xfer_partial (struct target_ops *
 	  record_list_release (record_arch_list_tail);
 	  if (record_debug)
 	    fprintf_unfiltered (gdb_stdlog,
-				_("Process record: failed to record "
-				  "execution log."));
+				"Process record: failed to record "
+				"execution log.");
 	  return -1;
 	}
       record_list->next = record_arch_list_head;
@@ -1642,6 +1854,295 @@ cmd_record_start (char *args, int from_t
   execute_command ("target record", from_tty);
 }
 
+/* Record log save-file format
+   Version 1 (never released)
+
+   Header:
+     4 bytes: magic number htonl(0x20090829).
+       NOTE: be sure to change whenever this file format changes!
+
+   Records:
+     record_end:
+       1 byte:  record type (record_end).
+     record_reg:
+       1 byte:  record type (record_reg).
+       8 bytes: register id (network byte order).
+       MAX_REGISTER_SIZE bytes: register value.
+     record_mem:
+       1 byte:  record type (record_mem).
+       8 bytes: memory length (network byte order).
+       8 bytes: memory address (network byte order).
+       n bytes: memory value (n == memory length).
+
+   Version 2
+     4 bytes: magic number htonl(0x20091016).
+       NOTE: be sure to change whenever this file format changes!
+
+   Records:
+     record_end:
+       1 byte:  record type (record_end).
+       4 bytes: signal
+       4 bytes: instruction count
+     record_reg:
+       1 byte:  record type (record_reg).
+       4 bytes: register id (network byte order).
+       n bytes: register value (n == register size).
+     record_mem:
+       1 byte:  record type (record_mem).
+       4 bytes: memory length (network byte order).
+       8 bytes: memory address (network byte order).
+       n bytes: memory value (n == memory length).
+
+*/
+
+/* bfdcore_write -- write bytes into a core file section.  */
+
+static void
+bfdcore_write (bfd *obfd, asection *osec, void *buf, int len, int *offset)
+{
+  int ret = bfd_set_section_contents (obfd, osec, buf, *offset, len);
+
+  if (ret)
+    *offset += len;
+  else
+    error (_("Failed to write %d bytes to core file %s ('%s').\n"),
+	   len, bfd_get_filename (obfd),
+	   bfd_errmsg (bfd_get_error ()));
+}
+
+/* Restore the execution log from a file.  We use a modified elf
+   corefile format, with an extra section for our data.  */
+
+static void
+cmd_record_restore (char *args, int from_tty)
+{
+  core_file_command (args, from_tty);
+  record_open (args, from_tty);
+}
+
+static void
+record_save_cleanups (void *data)
+{
+  bfd *obfd = data;
+  char *pathname = xstrdup (bfd_get_filename (obfd));
+  bfd_close (obfd);
+  unlink (pathname);
+  xfree (pathname);
+}
+
+/* Save the execution log to a file.  We use a modified elf corefile
+   format, with an extra section for our data.  */
+
+static void
+cmd_record_save (char *args, int from_tty)
+{
+  char *recfilename, recfilename_buffer[40];
+  int recfd;
+  struct record_entry *cur_record_list;
+  uint32_t magic;
+  struct regcache *regcache;
+  struct gdbarch *gdbarch;
+  struct cleanup *old_cleanups;
+  struct cleanup *set_cleanups;
+  bfd *obfd;
+  int save_size = 0;
+  asection *osec = NULL;
+  int bfd_offset = 0;
+
+  if (strcmp (current_target.to_shortname, "record") != 0)
+    error (_("This command can only be used with target 'record'.\n"));
+
+  if (args && *args)
+    recfilename = args;
+  else
+    {
+      /* Default recfile name is "gdb_record.PID".  */
+      snprintf (recfilename_buffer, sizeof (recfilename_buffer),
+                "gdb_record.%d", PIDGET (inferior_ptid));
+      recfilename = recfilename_buffer;
+    }
+
+  /* Open the save file.  */
+  if (record_debug)
+    printf_filtered ("Saving execution log to core file '%s'\n", recfilename);
+
+  /* Open the output file.  */
+  obfd = create_gcore_bfd (recfilename);
+  old_cleanups = make_cleanup (record_save_cleanups, obfd);
+
+  /* Save the current record entry to "cur_record_list".  */
+  cur_record_list = record_list;
+
+  /* Get the values of regcache and gdbarch.  */
+  regcache = get_current_regcache ();
+  gdbarch = get_regcache_arch (regcache);
+
+  /* Disable the GDB operation record.  */
+  set_cleanups = record_gdb_operation_disable_set ();
+
+  /* Reverse execute to the begin of record list.  */
+  while (1)
+    {
+      /* Check for beginning and end of log.  */
+      if (record_list == &record_first)
+        break;
+
+      record_exec_insn (regcache, gdbarch, record_list);
+
+      if (record_list->prev)
+        record_list = record_list->prev;
+    }
+
+  /* Compute the size needed for the extra bfd section.  */
+  save_size = 4;	/* magic cookie */
+  for (record_list = record_first.next; record_list;
+       record_list = record_list->next)
+    switch (record_list->type)
+      {
+      case record_end:
+	save_size += 1 + 4 + 4;
+	break;
+      case record_reg:
+	save_size += 1 + 4 + record_list->u.reg.len;
+	break;
+      case record_mem:
+	save_size += 1 + 4 + 8 + record_list->u.mem.len;
+	break;
+      }
+
+  /* Make the new bfd section.  */
+  osec = bfd_make_section_anyway_with_flags (obfd, "precord",
+                                             SEC_HAS_CONTENTS
+                                             | SEC_READONLY);
+  if (osec == NULL)
+    error (_("Failed to create 'precord' section for corefile %s: %s"),
+	   recfilename,
+           bfd_errmsg (bfd_get_error ()));
+  bfd_set_section_size (obfd, osec, save_size);
+  bfd_set_section_vma (obfd, osec, 0);
+  bfd_set_section_alignment (obfd, osec, 0);
+  bfd_section_lma (obfd, osec) = 0;
+
+  /* Save corefile state.  */
+  write_gcore_file (obfd);
+
+  /* Write out the record log.  */
+  /* Write the magic code.  */
+  magic = RECORD_FILE_MAGIC;
+  if (record_debug)
+    printf_filtered ("\
+  Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
+		     magic);
+  bfdcore_write (obfd, osec, &magic, sizeof (magic), &bfd_offset);
+
+  /* Save the entries to recfd and forward execute to the end of
+     record list.  */
+  record_list = &record_first;
+  while (1)
+    {
+      /* Save entry.  */
+      if (record_list != &record_first)
+        {
+	  uint8_t type;
+	  uint32_t regnum, len, signal, count;
+          uint64_t addr;
+
+	  type = record_list->type;
+          bfdcore_write (obfd, osec, &type, sizeof (type), &bfd_offset);
+
+          switch (record_list->type)
+            {
+            case record_reg: /* reg */
+              if (record_debug)
+		printf_filtered ("\
+  Writing register %d (1 plus %d plus %d bytes)\n",
+				 record_list->u.reg.num,
+				 sizeof (regnum),
+				 record_list->u.reg.len);
+
+              /* Write regnum.  */
+              regnum = netorder32 (record_list->u.reg.num);
+              bfdcore_write (obfd, osec, &regnum,
+			     sizeof (regnum), &bfd_offset);
+
+              /* Write regval.  */
+              bfdcore_write (obfd, osec, record_get_loc (record_list),
+			     record_list->u.reg.len, &bfd_offset);
+              break;
+
+            case record_mem: /* mem */
+	      if (record_debug)
+		printf_filtered ("\
+  Writing memory %s (1 plus %d plus %d plus %d bytes)\n",
+				 paddress (gdbarch,
+					   record_list->u.mem.addr),
+				 sizeof (addr),
+				 sizeof (len),
+				 record_list->u.mem.len);
+
+	      /* Write memlen.  */
+	      len = netorder32 (record_list->u.mem.len);
+	      bfdcore_write (obfd, osec, &len, sizeof (len), &bfd_offset);
+
+	      /* Write memaddr.  */
+	      addr = netorder64 (record_list->u.mem.addr);
+	      bfdcore_write (obfd, osec, &addr, 
+			     sizeof (addr), &bfd_offset);
+
+	      /* Write memval.  */
+	      bfdcore_write (obfd, osec, record_get_loc (record_list),
+			     record_list->u.mem.len, &bfd_offset);
+              break;
+
+              case record_end:
+                if (record_debug)
+                  printf_filtered ("\
+  Writing record_end (1 + %d + %d bytes)\n", 
+				   sizeof (signal),
+				   sizeof (count));
+		/* Write signal value.  */
+		signal = netorder32 (record_list->u.end.sigval);
+		bfdcore_write (obfd, osec, &signal,
+			       sizeof (signal), &bfd_offset);
+
+		/* Write insn count.  */
+		count = netorder32 (record_list->u.end.insn_num);
+		bfdcore_write (obfd, osec, &count,
+			       sizeof (count), &bfd_offset);
+                break;
+            }
+        }
+
+      /* Execute entry.  */
+      record_exec_insn (regcache, gdbarch, record_list);
+
+      if (record_list->next)
+        record_list = record_list->next;
+      else
+        break;
+    }
+
+  /* Reverse execute to cur_record_list.  */
+  while (1)
+    {
+      /* Check for beginning and end of log.  */
+      if (record_list == cur_record_list)
+        break;
+
+      record_exec_insn (regcache, gdbarch, record_list);
+
+      if (record_list->prev)
+        record_list = record_list->prev;
+    }
+
+  do_cleanups (set_cleanups);
+  do_cleanups (old_cleanups);
+
+  /* Succeeded.  */
+  printf_filtered (_("Saved core file %s with execution log.\n"),
+		   recfilename);
+}
+
 /* Truncate the record log from the present point
    of replay until the end.  */
 
@@ -1749,6 +2250,8 @@ info_record_command (char *args, int fro
 void
 _initialize_record (void)
 {
+  struct cmd_list_element *c;
+
   /* Init record_first.  */
   record_first.prev = NULL;
   record_first.next = NULL;
@@ -1767,10 +2270,12 @@ _initialize_record (void)
 			    NULL, show_record_debug, &setdebuglist,
 			    &showdebuglist);
 
-  add_prefix_cmd ("record", class_obscure, cmd_record_start,
+  c = add_prefix_cmd ("record", class_obscure, cmd_record_start,
 		  _("Abbreviated form of \"target record\" command."),
  		  &record_cmdlist, "record ", 0, &cmdlist);
+  set_cmd_completer (c, filename_completer);
   add_com_alias ("rec", "record", class_obscure, 1);
+
   add_prefix_cmd ("record", class_support, set_record_command,
 		  _("Set record options"), &set_record_cmdlist,
 		  "set record ", 0, &setlist);
@@ -1784,6 +2289,17 @@ _initialize_record (void)
 		  "info record ", 0, &infolist);
   add_alias_cmd ("rec", "record", class_obscure, 1, &infolist);
 
+  c = add_cmd ("save", class_obscure, cmd_record_save,
+	       _("Save the execution log to a file.\n\
+Argument is optional filename.\n\
+Default filename is 'gdb_record.<process_id>'."),
+	       &record_cmdlist);
+
+  c = add_cmd ("restore", class_obscure, cmd_record_restore,
+	       _("Restore the execution log from a file.\n\
+Argument is filename.  File must be created with 'record save'."),
+	       &record_cmdlist);
+  set_cmd_completer (c, filename_completer);
 
   add_cmd ("delete", class_obscure, cmd_record_delete,
 	   _("Delete the rest of execution log and start recording it anew."),

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Eli Zaretskii]

> Date: Sat, 17 Oct 2009 11:36:35 -0700
> From: Michael Snyder <msnyder@vmware.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, 
>  "teawater@gmail.com" <teawater@gmail.com>
> 
> >> +       NOTE: be sure to change whenever this file format changes!
> >> +
> >> +   Records:
> >> +     record_end:
> >> +       1 byte:  record type (record_end).
> >                                 ^^^^^^^^^^
> > This probably has some integer value, right?  Or does this indicate
> > something other than an integer type?
> 
> It's an enum.  Why?  You think I should give its numeric
> value in the comment, instead of its symbolic value?

Either the numeric value or a more explicit reference to the enum
(e.g., say that it's an enum defined on such-and-such file).

IOW, imagine that someone reads these comments because they want to
write a utility that reads these files, and ask yourself what would
they be missing in this text.

> >> +     record_reg:
> >> +       1 byte:  record type (record_reg).
> >> +       4 bytes: register id (network byte order).
> >> +       n bytes: register value (n == register size).
> >                                         ^^^^^^^^^^^^^
> > How does one know what is the correct register size?
> 
> We get it from the current regcache and regnum.  What can I say about
> it here, that wouldn't be arch-specific?

Well, saying it's the exact size of the register being recorded works
for me.

> I mean, I could say:
> 
>     This will generally be 4 bytes for x86, 8 bytes for x86_64.

That's good as an example.

> but that doesn't seem very useful or scalable.  Plus it will
> be different for floating point regs, once we handle them too.

Would it make sense to use here some size that can accommodate all the
known registers, to make the file format truly architecture
independent?  Or would it be more trouble than it's worth?

> >> +  if (strcmp (current_target.to_shortname, "record") != 0)
> >> +    error (_("Process record is not started.\n"));
> > 
> > Suggest to add to the message text something that tells the user how
> > to remedy this situation.  E.g., "Invoke FOO command first."
> 
> OK.  It's a target-specific command, that can only be used
> with target 'record'.  How about this?
> 
>    error (_("This command can only be used with target 'record'.\n"));

That's good, but maybe adding "Use 'record start' first." will be
better?

> This is the same approach that is used by the "gcore" command.
> How does "gcore" work with go32, if at all?

It doesn't.  DJGPP cannot generate core files.

> Thanks for all your suggestions, Eli.
> Please see revised patch attached.

It's fine, except for this:

> +   Header:
> +     4 bytes: magic number htonl(0x20090829).

I thought you agreed to replace this with "0x20090829 (network byte
order)"?

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

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

Eli Zaretskii wrote:
>> Date: Sat, 17 Oct 2009 11:36:35 -0700
>> From: Michael Snyder <msnyder@vmware.com>
>> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, 
>>  "teawater@gmail.com" <teawater@gmail.com>
>>
>>>> +       NOTE: be sure to change whenever this file format changes!
>>>> +
>>>> +   Records:
>>>> +     record_end:
>>>> +       1 byte:  record type (record_end).
>>>                                 ^^^^^^^^^^
>>> This probably has some integer value, right?  Or does this indicate
>>> something other than an integer type?
>> It's an enum.  Why?  You think I should give its numeric
>> value in the comment, instead of its symbolic value?
> 
> Either the numeric value or a more explicit reference to the enum
> (e.g., say that it's an enum defined on such-and-such file).
> 
> IOW, imagine that someone reads these comments because they want to
> write a utility that reads these files, and ask yourself what would
> they be missing in this text.

OK, see revised patch.

>>>> +     record_reg:
>>>> +       1 byte:  record type (record_reg).
>>>> +       4 bytes: register id (network byte order).
>>>> +       n bytes: register value (n == register size).
>>>                                         ^^^^^^^^^^^^^
>>> How does one know what is the correct register size?
>> We get it from the current regcache and regnum.  What can I say about
>> it here, that wouldn't be arch-specific?
> 
> Well, saying it's the exact size of the register being recorded works
> for me.

OK.

> 
>> I mean, I could say:
>>
>>     This will generally be 4 bytes for x86, 8 bytes for x86_64.
> 
> That's good as an example.
> 
>> but that doesn't seem very useful or scalable.  Plus it will
>> be different for floating point regs, once we handle them too.
> 
> Would it make sense to use here some size that can accommodate all the
> known registers, to make the file format truly architecture
> independent?  Or would it be more trouble than it's worth?

That's what version 1 of the file format did.
I found it wasteful, hence this change.

> 
>>>> +  if (strcmp (current_target.to_shortname, "record") != 0)
>>>> +    error (_("Process record is not started.\n"));
>>> Suggest to add to the message text something that tells the user how
>>> to remedy this situation.  E.g., "Invoke FOO command first."
>> OK.  It's a target-specific command, that can only be used
>> with target 'record'.  How about this?
>>
>>    error (_("This command can only be used with target 'record'.\n"));
> 
> That's good, but maybe adding "Use 'record start' first." will be
> better?

OK.

> 
>> This is the same approach that is used by the "gcore" command.
>> How does "gcore" work with go32, if at all?
> 
> It doesn't.  DJGPP cannot generate core files.

Well, save/restore depends on core files, so I guess
it won't work in go32.


>> Thanks for all your suggestions, Eli.
>> Please see revised patch attached.
> 
> It's fine, except for this:
> 
>> +   Header:
>> +     4 bytes: magic number htonl(0x20090829).
> 
> I thought you agreed to replace this with "0x20090829 (network byte
> order)"?

Ah.  Yes -- you're looking at the version 1 header, but I do need
to change the comment for the version 2 header.

See revised patch below.



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

2009-10-16  Hui Zhu  <teawater@gmail.com>
	    Michael Snyder  <msnyder@msnyder-server.eng.vmware.com>

	* record.c (RECORD_FILE_MAGIC): New constant.
	(record_arch_list_cleanups): Renamed from record_message_cleanups.
	(bfdcore_read): New function.
	(netorder64): New function.
	(netorder32): New function.
	(netorder16): New function.
	(record_restore): New function.  Restore a saved record log.
	(bfdcore_write): New function.
	(cmd_record_restore): New function.
	(cmd_record_save): New function.  Save a record log to a file.
	(_initialize_record): Set up commands for save and restore.

--- record.2b.c	2009-10-17 11:30:35.000000000 -0700
+++ record.12.c	2009-10-17 15:09:48.000000000 -0700
@@ -23,10 +23,15 @@
 #include "gdbthread.h"
 #include "event-top.h"
 #include "exceptions.h"
+#include "completer.h"
+#include "arch-utils.h"
 #include "gdbcore.h"
 #include "exec.h"
 #include "record.h"
+#include "elf-bfd.h"
+#include "gcore.h"
 
+#include <byteswap.h>
 #include <signal.h>
 
 #define DEFAULT_RECORD_INSN_MAX_NUM	200000
@@ -34,6 +39,8 @@
 #define RECORD_IS_REPLAY \
      (record_list->next || execution_direction == EXEC_REVERSE)
 
+#define RECORD_FILE_MAGIC	netorder32(0x20091016)
+
 /* These are the core structs of the process record functionality.
 
    A record_entry is a record of the value change of a register
@@ -482,24 +489,24 @@ record_check_insn_num (int set_terminal)
 	      if (q)
 		record_stop_at_limit = 0;
 	      else
-		error (_("Process record: inferior program stopped."));
+		error (_("Process record: stopped by user."));
 	    }
 	}
     }
 }
 
+static void
+record_arch_list_cleanups (void *ignore)
+{
+  record_list_release (record_arch_list_tail);
+}
+
 /* Before inferior step (when GDB record the running message, inferior
    only can step), GDB will call this function to record the values to
    record_list.  This function will call gdbarch_process_record to
    record the running message of inferior and set them to
    record_arch_list, and add it to record_list.  */
 
-static void
-record_message_cleanups (void *ignore)
-{
-  record_list_release (record_arch_list_tail);
-}
-
 struct record_message_args {
   struct regcache *regcache;
   enum target_signal signal;
@@ -511,7 +518,7 @@ record_message (void *args)
   int ret;
   struct record_message_args *myargs = args;
   struct gdbarch *gdbarch = get_regcache_arch (myargs->regcache);
-  struct cleanup *old_cleanups = make_cleanup (record_message_cleanups, 0);
+  struct cleanup *old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
 
   record_arch_list_head = NULL;
   record_arch_list_tail = NULL;
@@ -651,8 +658,8 @@ record_exec_insn (struct regcache *regca
               {
                 entry->u.mem.mem_entry_not_accessible = 1;
                 if (record_debug)
-                  warning (_("Process record: error reading memory at "
-                             "addr = %s len = %d."),
+                  warning ("Process record: error reading memory at "
+			   "addr = %s len = %d.",
 			   paddress (gdbarch, entry->u.mem.addr),
                            entry->u.mem.len);
               }
@@ -664,8 +671,8 @@ record_exec_insn (struct regcache *regca
                   {
                     entry->u.mem.mem_entry_not_accessible = 1;
                     if (record_debug)
-                      warning (_("Process record: error writing memory at "
-                                 "addr = %s len = %d."),
+                      warning ("Process record: error writing memory at "
+			       "addr = %s len = %d.",
                                paddress (gdbarch, entry->u.mem.addr),
                                entry->u.mem.len);
                   }
@@ -678,6 +685,215 @@ record_exec_insn (struct regcache *regca
     }
 }
 
+/* bfdcore_read -- read bytes from a core file section.  */
+
+static void
+bfdcore_read (bfd *obfd, asection *osec, void *buf, int len, int *offset)
+{
+  int ret = bfd_get_section_contents (obfd, osec, buf, *offset, len);
+
+  if (ret)
+    *offset += len;
+  else
+    error (_("Failed to read %d bytes from core file %s ('%s').\n"),
+	   len, bfd_get_filename (obfd),
+	   bfd_errmsg (bfd_get_error ()));
+}
+
+static uint64_t
+netorder64 (uint64_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_64 (fromfile) 
+    : fromfile;
+}
+
+static uint32_t
+netorder32 (uint32_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_32 (fromfile) 
+    : fromfile;
+}
+
+static uint16_t
+netorder16 (uint16_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_16 (fromfile) 
+    : fromfile;
+}
+
+/* Restore the execution log from a core_bfd file.  */
+
+static void
+record_restore (void)
+{
+  uint32_t magic;
+  struct cleanup *old_cleanups;
+  struct record_entry *rec;
+  asection *osec;
+  uint32_t osec_size;
+  int bfd_offset = 0;
+  struct regcache *regcache;
+
+  /* We restore the execution log from the open core bfd,
+     if there is one.  */
+  if (core_bfd == NULL)
+    return;
+
+  /* "record_restore" can only be called when record list is empty.  */
+  gdb_assert (record_first.next == NULL);
+
+  if (record_debug)
+    printf_filtered ("Restoring recording from core file.\n");
+
+  /* Now need to find our special note section.  */
+  osec = bfd_get_section_by_name (core_bfd, "null0");
+  osec_size = bfd_section_size (core_bfd, osec);
+  if (record_debug)
+    printf_filtered ("Find precord section %s.\n",
+		     osec ? "succeeded" : "failed");
+  if (!osec)
+    return;
+  if (record_debug)
+    printf_filtered ("%s", bfd_section_name (core_bfd, osec));
+
+  /* Check the magic code.  */
+  if (record_debug)
+    printf_filtered ("\
+  Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
+		     magic);
+  bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
+  if (magic != RECORD_FILE_MAGIC)
+    error (_("Version mis-match or file format error in core file %s."),
+	   bfd_get_filename (core_bfd));
+
+  /* Restore the entries in recfd into record_arch_list_head and
+     record_arch_list_tail.  */
+  record_arch_list_head = NULL;
+  record_arch_list_tail = NULL;
+  record_insn_num = 0;
+  old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
+  regcache = get_current_regcache ();
+
+  while (1)
+    {
+      int ret;
+      uint8_t tmpu8;
+      uint32_t regnum, len, signal, count;
+      uint64_t addr;
+
+      /* We are finished when offset reaches osec_size.  */
+      if (bfd_offset >= osec_size)
+	break;
+      bfdcore_read (core_bfd, osec, &tmpu8, sizeof (tmpu8), &bfd_offset);
+
+      switch (tmpu8)
+        {
+        case record_reg: /* reg */
+          if (record_debug)
+            printf_filtered ("\
+  Reading register %d (1 plus %d plus %d bytes)\n",
+			     rec->u.reg.num,
+			     sizeof (regnum),
+			     rec->u.reg.len);
+          /* Get register number to regnum.  */
+          bfdcore_read (core_bfd, osec, &regnum,
+			sizeof (regnum), &bfd_offset);
+	  regnum = netorder32 (regnum);
+
+          rec = record_reg_alloc (regcache, regnum);
+
+          /* Get val.  */
+          bfdcore_read (core_bfd, osec, record_get_loc (rec),
+			rec->u.reg.len, &bfd_offset);
+          break;
+
+        case record_mem: /* mem */
+          if (record_debug)
+            printf_filtered ("\
+  Reading memory %s (1 plus %d plus %d plus %d bytes)\n",
+			     paddress (get_current_arch (),
+				       rec->u.mem.addr),
+			     sizeof (addr),
+			     sizeof (len),
+			     rec->u.mem.len);
+          /* Get len.  */
+          bfdcore_read (core_bfd, osec, &len, 
+			sizeof (len), &bfd_offset);
+	  len = netorder32 (len);
+
+          /* Get addr.  */
+          bfdcore_read (core_bfd, osec, &addr,
+			sizeof (addr), &bfd_offset);
+	  addr = netorder64 (addr);
+
+          rec = record_mem_alloc (addr, len);
+
+          /* Get val.  */
+          bfdcore_read (core_bfd, osec, record_get_loc (rec),
+			rec->u.mem.len, &bfd_offset);
+          break;
+
+        case record_end: /* end */
+          if (record_debug)
+            printf_filtered ("\
+  Reading record_end (1 + %d + %d bytes), offset == %s\n",
+			     sizeof (signal),
+			     sizeof (count),
+			     paddress (get_current_arch (),
+				       bfd_offset));
+          rec = record_end_alloc ();
+          record_insn_num ++;
+
+	  /* Get signal value.  */
+	  bfdcore_read (core_bfd, osec, &signal, 
+			sizeof (signal), &bfd_offset);
+	  signal = netorder32 (signal);
+	  rec->u.end.sigval = signal;
+
+	  /* Get insn count.  */
+	  bfdcore_read (core_bfd, osec, &count, 
+			sizeof (count), &bfd_offset);
+	  count = netorder32 (count);
+	  rec->u.end.insn_num = count;
+	  record_insn_count = count + 1;
+          break;
+
+        default:
+          error (_("Bad entry type in core file %s."),
+		 bfd_get_filename (core_bfd));
+          break;
+        }
+
+      /* Add rec to record arch list.  */
+      record_arch_list_add (rec);
+    }
+
+  discard_cleanups (old_cleanups);
+
+  /* Add record_arch_list_head to the end of record list.  */
+  record_first.next = record_arch_list_head;
+  record_arch_list_head->prev = &record_first;
+  record_arch_list_tail->next = NULL;
+  record_list = &record_first;
+
+  /* Update record_insn_max_num.  */
+  if (record_insn_num > record_insn_max_num)
+    {
+      record_insn_max_num = record_insn_num;
+      warning (_("Auto increase record/replay buffer limit to %d."),
+               record_insn_max_num);
+    }
+
+  /* Succeeded.  */
+  printf_filtered (_("Restored records from core file %s.\n"),
+		   bfd_get_filename (core_bfd));
+
+  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
+}
+
 static struct target_ops *tmp_to_resume_ops;
 static void (*tmp_to_resume) (struct target_ops *, ptid_t, int,
 			      enum target_signal);
@@ -728,6 +944,7 @@ record_core_open_1 (char *name, int from
     }
 
   push_target (&record_core_ops);
+  record_restore ();
 }
 
 static void
@@ -735,9 +952,6 @@ record_open_1 (char *name, int from_tty)
 {
   struct target_ops *t;
 
-  if (record_debug)
-    fprintf_unfiltered (gdb_stdlog, "Process record: record_open\n");
-
   /* check exec */
   if (!target_has_execution)
     error (_("Process record: the program is not being run."));
@@ -779,6 +993,12 @@ record_open (char *name, int from_tty)
     error (_("Process record target already running.  Use \"record stop\" to "
              "stop record target first."));
 
+  /* Reset */
+  record_insn_num = 0;
+  record_insn_count = 0;
+  record_list = &record_first;
+  record_list->next = NULL;
+
   /* Reset the tmp beneath pointers.  */
   tmp_to_resume_ops = NULL;
   tmp_to_resume = NULL;
@@ -822,17 +1042,6 @@ record_open (char *name, int from_tty)
   if (!tmp_to_xfer_partial)
     error (_("Could not find 'to_xfer_partial' method on the target stack."));
 
-  if (current_target.to_stratum == core_stratum)
-    record_core_open_1 (name, from_tty);
-  else
-    record_open_1 (name, from_tty);
-
-  /* Reset */
-  record_insn_num = 0;
-  record_insn_count = 0;
-  record_list = &record_first;
-  record_list->next = NULL;
-
   /* Set the tmp beneath pointers to beneath pointers.  */
   record_beneath_to_resume_ops = tmp_to_resume_ops;
   record_beneath_to_resume = tmp_to_resume;
@@ -844,6 +1053,11 @@ record_open (char *name, int from_tty)
   record_beneath_to_xfer_partial = tmp_to_xfer_partial;
   record_beneath_to_insert_breakpoint = tmp_to_insert_breakpoint;
   record_beneath_to_remove_breakpoint = tmp_to_remove_breakpoint;
+
+  if (current_target.to_stratum == core_stratum)
+    record_core_open_1 (name, from_tty);
+  else
+    record_open_1 (name, from_tty);
 }
 
 static void
@@ -1114,8 +1328,7 @@ record_wait (struct target_ops *ops,
 		    {
 		      if (record_debug)
 			fprintf_unfiltered (gdb_stdlog,
-					    "Process record: break "
-					    "at %s.\n",
+					    "Process record: break at %s.\n",
 					    paddress (gdbarch, tmp_pc));
 		      if (gdbarch_decr_pc_after_break (gdbarch)
 			  && execution_direction == EXEC_FORWARD
@@ -1190,8 +1403,7 @@ static void
 record_mourn_inferior (struct target_ops *ops)
 {
   if (record_debug)
-    fprintf_unfiltered (gdb_stdlog, "Process record: "
-			            "record_mourn_inferior\n");
+    fprintf_unfiltered (gdb_stdlog, "Process record: record_mourn_inferior\n");
 
   unpush_target (&record_ops);
   target_mourn_inferior ();
@@ -1345,8 +1557,8 @@ record_xfer_partial (struct target_ops *
 	  record_list_release (record_arch_list_tail);
 	  if (record_debug)
 	    fprintf_unfiltered (gdb_stdlog,
-				_("Process record: failed to record "
-				  "execution log."));
+				"Process record: failed to record "
+				"execution log.");
 	  return -1;
 	}
       if (record_arch_list_add_end ())
@@ -1354,8 +1566,8 @@ record_xfer_partial (struct target_ops *
 	  record_list_release (record_arch_list_tail);
 	  if (record_debug)
 	    fprintf_unfiltered (gdb_stdlog,
-				_("Process record: failed to record "
-				  "execution log."));
+				"Process record: failed to record "
+				"execution log.");
 	  return -1;
 	}
       record_list->next = record_arch_list_head;
@@ -1642,6 +1854,297 @@ cmd_record_start (char *args, int from_t
   execute_command ("target record", from_tty);
 }
 
+/* Record log save-file format
+   Version 1 (never released)
+
+   Header:
+     4 bytes: magic number htonl(0x20090829).
+       NOTE: be sure to change whenever this file format changes!
+
+   Records:
+     record_end:
+       1 byte:  record type (record_end, see enum record_type).
+     record_reg:
+       1 byte:  record type (record_reg, see enum record_type).
+       8 bytes: register id (network byte order).
+       MAX_REGISTER_SIZE bytes: register value.
+     record_mem:
+       1 byte:  record type (record_mem, see enum record_type).
+       8 bytes: memory length (network byte order).
+       8 bytes: memory address (network byte order).
+       n bytes: memory value (n == memory length).
+
+   Version 2
+     4 bytes: magic number netorder32(0x20091016).
+       NOTE: be sure to change whenever this file format changes!
+
+   Records:
+     record_end:
+       1 byte:  record type (record_end, see enum record_type).
+       4 bytes: signal
+       4 bytes: instruction count
+     record_reg:
+       1 byte:  record type (record_reg, see enum record_type).
+       4 bytes: register id (network byte order).
+       n bytes: register value (n == actual register size).
+                (eg. 4 bytes for x86 general registers).
+     record_mem:
+       1 byte:  record type (record_mem, see enum record_type).
+       4 bytes: memory length (network byte order).
+       8 bytes: memory address (network byte order).
+       n bytes: memory value (n == memory length).
+
+*/
+
+/* bfdcore_write -- write bytes into a core file section.  */
+
+static void
+bfdcore_write (bfd *obfd, asection *osec, void *buf, int len, int *offset)
+{
+  int ret = bfd_set_section_contents (obfd, osec, buf, *offset, len);
+
+  if (ret)
+    *offset += len;
+  else
+    error (_("Failed to write %d bytes to core file %s ('%s').\n"),
+	   len, bfd_get_filename (obfd),
+	   bfd_errmsg (bfd_get_error ()));
+}
+
+/* Restore the execution log from a file.  We use a modified elf
+   corefile format, with an extra section for our data.  */
+
+static void
+cmd_record_restore (char *args, int from_tty)
+{
+  core_file_command (args, from_tty);
+  record_open (args, from_tty);
+}
+
+static void
+record_save_cleanups (void *data)
+{
+  bfd *obfd = data;
+  char *pathname = xstrdup (bfd_get_filename (obfd));
+  bfd_close (obfd);
+  unlink (pathname);
+  xfree (pathname);
+}
+
+/* Save the execution log to a file.  We use a modified elf corefile
+   format, with an extra section for our data.  */
+
+static void
+cmd_record_save (char *args, int from_tty)
+{
+  char *recfilename, recfilename_buffer[40];
+  int recfd;
+  struct record_entry *cur_record_list;
+  uint32_t magic;
+  struct regcache *regcache;
+  struct gdbarch *gdbarch;
+  struct cleanup *old_cleanups;
+  struct cleanup *set_cleanups;
+  bfd *obfd;
+  int save_size = 0;
+  asection *osec = NULL;
+  int bfd_offset = 0;
+
+  if (strcmp (current_target.to_shortname, "record") != 0)
+    error (_("This command can only be used with target 'record'.\n"
+	     "Use 'target record' first.\n"));
+
+  if (args && *args)
+    recfilename = args;
+  else
+    {
+      /* Default recfile name is "gdb_record.PID".  */
+      snprintf (recfilename_buffer, sizeof (recfilename_buffer),
+                "gdb_record.%d", PIDGET (inferior_ptid));
+      recfilename = recfilename_buffer;
+    }
+
+  /* Open the save file.  */
+  if (record_debug)
+    printf_filtered ("Saving execution log to core file '%s'\n", recfilename);
+
+  /* Open the output file.  */
+  obfd = create_gcore_bfd (recfilename);
+  old_cleanups = make_cleanup (record_save_cleanups, obfd);
+
+  /* Save the current record entry to "cur_record_list".  */
+  cur_record_list = record_list;
+
+  /* Get the values of regcache and gdbarch.  */
+  regcache = get_current_regcache ();
+  gdbarch = get_regcache_arch (regcache);
+
+  /* Disable the GDB operation record.  */
+  set_cleanups = record_gdb_operation_disable_set ();
+
+  /* Reverse execute to the begin of record list.  */
+  while (1)
+    {
+      /* Check for beginning and end of log.  */
+      if (record_list == &record_first)
+        break;
+
+      record_exec_insn (regcache, gdbarch, record_list);
+
+      if (record_list->prev)
+        record_list = record_list->prev;
+    }
+
+  /* Compute the size needed for the extra bfd section.  */
+  save_size = 4;	/* magic cookie */
+  for (record_list = record_first.next; record_list;
+       record_list = record_list->next)
+    switch (record_list->type)
+      {
+      case record_end:
+	save_size += 1 + 4 + 4;
+	break;
+      case record_reg:
+	save_size += 1 + 4 + record_list->u.reg.len;
+	break;
+      case record_mem:
+	save_size += 1 + 4 + 8 + record_list->u.mem.len;
+	break;
+      }
+
+  /* Make the new bfd section.  */
+  osec = bfd_make_section_anyway_with_flags (obfd, "precord",
+                                             SEC_HAS_CONTENTS
+                                             | SEC_READONLY);
+  if (osec == NULL)
+    error (_("Failed to create 'precord' section for corefile %s: %s"),
+	   recfilename,
+           bfd_errmsg (bfd_get_error ()));
+  bfd_set_section_size (obfd, osec, save_size);
+  bfd_set_section_vma (obfd, osec, 0);
+  bfd_set_section_alignment (obfd, osec, 0);
+  bfd_section_lma (obfd, osec) = 0;
+
+  /* Save corefile state.  */
+  write_gcore_file (obfd);
+
+  /* Write out the record log.  */
+  /* Write the magic code.  */
+  magic = RECORD_FILE_MAGIC;
+  if (record_debug)
+    printf_filtered ("\
+  Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
+		     magic);
+  bfdcore_write (obfd, osec, &magic, sizeof (magic), &bfd_offset);
+
+  /* Save the entries to recfd and forward execute to the end of
+     record list.  */
+  record_list = &record_first;
+  while (1)
+    {
+      /* Save entry.  */
+      if (record_list != &record_first)
+        {
+	  uint8_t type;
+	  uint32_t regnum, len, signal, count;
+          uint64_t addr;
+
+	  type = record_list->type;
+          bfdcore_write (obfd, osec, &type, sizeof (type), &bfd_offset);
+
+          switch (record_list->type)
+            {
+            case record_reg: /* reg */
+              if (record_debug)
+		printf_filtered ("\
+  Writing register %d (1 plus %d plus %d bytes)\n",
+				 record_list->u.reg.num,
+				 sizeof (regnum),
+				 record_list->u.reg.len);
+
+              /* Write regnum.  */
+              regnum = netorder32 (record_list->u.reg.num);
+              bfdcore_write (obfd, osec, &regnum,
+			     sizeof (regnum), &bfd_offset);
+
+              /* Write regval.  */
+              bfdcore_write (obfd, osec, record_get_loc (record_list),
+			     record_list->u.reg.len, &bfd_offset);
+              break;
+
+            case record_mem: /* mem */
+	      if (record_debug)
+		printf_filtered ("\
+  Writing memory %s (1 plus %d plus %d plus %d bytes)\n",
+				 paddress (gdbarch,
+					   record_list->u.mem.addr),
+				 sizeof (addr),
+				 sizeof (len),
+				 record_list->u.mem.len);
+
+	      /* Write memlen.  */
+	      len = netorder32 (record_list->u.mem.len);
+	      bfdcore_write (obfd, osec, &len, sizeof (len), &bfd_offset);
+
+	      /* Write memaddr.  */
+	      addr = netorder64 (record_list->u.mem.addr);
+	      bfdcore_write (obfd, osec, &addr, 
+			     sizeof (addr), &bfd_offset);
+
+	      /* Write memval.  */
+	      bfdcore_write (obfd, osec, record_get_loc (record_list),
+			     record_list->u.mem.len, &bfd_offset);
+              break;
+
+              case record_end:
+                if (record_debug)
+                  printf_filtered ("\
+  Writing record_end (1 + %d + %d bytes)\n", 
+				   sizeof (signal),
+				   sizeof (count));
+		/* Write signal value.  */
+		signal = netorder32 (record_list->u.end.sigval);
+		bfdcore_write (obfd, osec, &signal,
+			       sizeof (signal), &bfd_offset);
+
+		/* Write insn count.  */
+		count = netorder32 (record_list->u.end.insn_num);
+		bfdcore_write (obfd, osec, &count,
+			       sizeof (count), &bfd_offset);
+                break;
+            }
+        }
+
+      /* Execute entry.  */
+      record_exec_insn (regcache, gdbarch, record_list);
+
+      if (record_list->next)
+        record_list = record_list->next;
+      else
+        break;
+    }
+
+  /* Reverse execute to cur_record_list.  */
+  while (1)
+    {
+      /* Check for beginning and end of log.  */
+      if (record_list == cur_record_list)
+        break;
+
+      record_exec_insn (regcache, gdbarch, record_list);
+
+      if (record_list->prev)
+        record_list = record_list->prev;
+    }
+
+  do_cleanups (set_cleanups);
+  do_cleanups (old_cleanups);
+
+  /* Succeeded.  */
+  printf_filtered (_("Saved core file %s with execution log.\n"),
+		   recfilename);
+}
+
 /* Truncate the record log from the present point
    of replay until the end.  */
 
@@ -1749,6 +2252,8 @@ info_record_command (char *args, int fro
 void
 _initialize_record (void)
 {
+  struct cmd_list_element *c;
+
   /* Init record_first.  */
   record_first.prev = NULL;
   record_first.next = NULL;
@@ -1767,10 +2272,12 @@ _initialize_record (void)
 			    NULL, show_record_debug, &setdebuglist,
 			    &showdebuglist);
 
-  add_prefix_cmd ("record", class_obscure, cmd_record_start,
+  c = add_prefix_cmd ("record", class_obscure, cmd_record_start,
 		  _("Abbreviated form of \"target record\" command."),
  		  &record_cmdlist, "record ", 0, &cmdlist);
+  set_cmd_completer (c, filename_completer);
   add_com_alias ("rec", "record", class_obscure, 1);
+
   add_prefix_cmd ("record", class_support, set_record_command,
 		  _("Set record options"), &set_record_cmdlist,
 		  "set record ", 0, &setlist);
@@ -1784,6 +2291,17 @@ _initialize_record (void)
 		  "info record ", 0, &infolist);
   add_alias_cmd ("rec", "record", class_obscure, 1, &infolist);
 
+  c = add_cmd ("save", class_obscure, cmd_record_save,
+	       _("Save the execution log to a file.\n\
+Argument is optional filename.\n\
+Default filename is 'gdb_record.<process_id>'."),
+	       &record_cmdlist);
+
+  c = add_cmd ("restore", class_obscure, cmd_record_restore,
+	       _("Restore the execution log from a file.\n\
+Argument is filename.  File must be created with 'record save'."),
+	       &record_cmdlist);
+  set_cmd_completer (c, filename_completer);
 
   add_cmd ("delete", class_obscure, cmd_record_delete,
 	   _("Delete the rest of execution log and start recording it anew."),

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Hui Zhu]

Hi Michael,

Thanks for your patch.

I cannot find the diff for the gcore.c file.

       (netorder64): New function.
       (netorder32): New function.
       (netorder16): New function.

I suggest inline them.

Thanks,
Hui

On Sun, Oct 18, 2009 at 06:13, Michael Snyder <msnyder@vmware.com> wrote:
> Eli Zaretskii wrote:
>>>
>>> Date: Sat, 17 Oct 2009 11:36:35 -0700
>>> From: Michael Snyder <msnyder@vmware.com>
>>> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
>>>  "teawater@gmail.com" <teawater@gmail.com>
>>>
>>>>> +       NOTE: be sure to change whenever this file format changes!
>>>>> +
>>>>> +   Records:
>>>>> +     record_end:
>>>>> +       1 byte:  record type (record_end).
>>>>
>>>>                                ^^^^^^^^^^
>>>> This probably has some integer value, right?  Or does this indicate
>>>> something other than an integer type?
>>>
>>> It's an enum.  Why?  You think I should give its numeric
>>> value in the comment, instead of its symbolic value?
>>
>> Either the numeric value or a more explicit reference to the enum
>> (e.g., say that it's an enum defined on such-and-such file).
>>
>> IOW, imagine that someone reads these comments because they want to
>> write a utility that reads these files, and ask yourself what would
>> they be missing in this text.
>
> OK, see revised patch.
>
>>>>> +     record_reg:
>>>>> +       1 byte:  record type (record_reg).
>>>>> +       4 bytes: register id (network byte order).
>>>>> +       n bytes: register value (n == register size).
>>>>
>>>>                                        ^^^^^^^^^^^^^
>>>> How does one know what is the correct register size?
>>>
>>> We get it from the current regcache and regnum.  What can I say about
>>> it here, that wouldn't be arch-specific?
>>
>> Well, saying it's the exact size of the register being recorded works
>> for me.
>
> OK.
>
>>
>>> I mean, I could say:
>>>
>>>    This will generally be 4 bytes for x86, 8 bytes for x86_64.
>>
>> That's good as an example.
>>
>>> but that doesn't seem very useful or scalable.  Plus it will
>>> be different for floating point regs, once we handle them too.
>>
>> Would it make sense to use here some size that can accommodate all the
>> known registers, to make the file format truly architecture
>> independent?  Or would it be more trouble than it's worth?
>
> That's what version 1 of the file format did.
> I found it wasteful, hence this change.
>
>>
>>>>> +  if (strcmp (current_target.to_shortname, "record") != 0)
>>>>> +    error (_("Process record is not started.\n"));
>>>>
>>>> Suggest to add to the message text something that tells the user how
>>>> to remedy this situation.  E.g., "Invoke FOO command first."
>>>
>>> OK.  It's a target-specific command, that can only be used
>>> with target 'record'.  How about this?
>>>
>>>   error (_("This command can only be used with target 'record'.\n"));
>>
>> That's good, but maybe adding "Use 'record start' first." will be
>> better?
>
> OK.
>
>>
>>> This is the same approach that is used by the "gcore" command.
>>> How does "gcore" work with go32, if at all?
>>
>> It doesn't.  DJGPP cannot generate core files.
>
> Well, save/restore depends on core files, so I guess
> it won't work in go32.
>
>
>>> Thanks for all your suggestions, Eli.
>>> Please see revised patch attached.
>>
>> It's fine, except for this:
>>
>>> +   Header:
>>> +     4 bytes: magic number htonl(0x20090829).
>>
>> I thought you agreed to replace this with "0x20090829 (network byte
>> order)"?
>
> Ah.  Yes -- you're looking at the version 1 header, but I do need
> to change the comment for the version 2 header.
>
> See revised patch below.
>
>
>
> 2009-10-16  Hui Zhu  <teawater@gmail.com>
>            Michael Snyder  <msnyder@msnyder-server.eng.vmware.com>
>
>        * record.c (RECORD_FILE_MAGIC): New constant.
>        (record_arch_list_cleanups): Renamed from record_message_cleanups.
>        (bfdcore_read): New function.
>        (netorder64): New function.
>        (netorder32): New function.
>        (netorder16): New function.
>        (record_restore): New function.  Restore a saved record log.
>        (bfdcore_write): New function.
>        (cmd_record_restore): New function.
>        (cmd_record_save): New function.  Save a record log to a file.
>        (_initialize_record): Set up commands for save and restore.
>
> --- record.2b.c 2009-10-17 11:30:35.000000000 -0700
> +++ record.12.c 2009-10-17 15:09:48.000000000 -0700
> @@ -23,10 +23,15 @@
>  #include "gdbthread.h"
>  #include "event-top.h"
>  #include "exceptions.h"
> +#include "completer.h"
> +#include "arch-utils.h"
>  #include "gdbcore.h"
>  #include "exec.h"
>  #include "record.h"
> +#include "elf-bfd.h"
> +#include "gcore.h"
>
> +#include <byteswap.h>
>  #include <signal.h>
>
>  #define DEFAULT_RECORD_INSN_MAX_NUM    200000
> @@ -34,6 +39,8 @@
>  #define RECORD_IS_REPLAY \
>      (record_list->next || execution_direction == EXEC_REVERSE)
>
> +#define RECORD_FILE_MAGIC      netorder32(0x20091016)
> +
>  /* These are the core structs of the process record functionality.
>
>    A record_entry is a record of the value change of a register
> @@ -482,24 +489,24 @@ record_check_insn_num (int set_terminal)
>              if (q)
>                record_stop_at_limit = 0;
>              else
> -               error (_("Process record: inferior program stopped."));
> +               error (_("Process record: stopped by user."));
>            }
>        }
>     }
>  }
>
> +static void
> +record_arch_list_cleanups (void *ignore)
> +{
> +  record_list_release (record_arch_list_tail);
> +}
> +
>  /* Before inferior step (when GDB record the running message, inferior
>    only can step), GDB will call this function to record the values to
>    record_list.  This function will call gdbarch_process_record to
>    record the running message of inferior and set them to
>    record_arch_list, and add it to record_list.  */
>
> -static void
> -record_message_cleanups (void *ignore)
> -{
> -  record_list_release (record_arch_list_tail);
> -}
> -
>  struct record_message_args {
>   struct regcache *regcache;
>   enum target_signal signal;
> @@ -511,7 +518,7 @@ record_message (void *args)
>   int ret;
>   struct record_message_args *myargs = args;
>   struct gdbarch *gdbarch = get_regcache_arch (myargs->regcache);
> -  struct cleanup *old_cleanups = make_cleanup (record_message_cleanups, 0);
> +  struct cleanup *old_cleanups = make_cleanup (record_arch_list_cleanups,
> 0);
>
>   record_arch_list_head = NULL;
>   record_arch_list_tail = NULL;
> @@ -651,8 +658,8 @@ record_exec_insn (struct regcache *regca
>               {
>                 entry->u.mem.mem_entry_not_accessible = 1;
>                 if (record_debug)
> -                  warning (_("Process record: error reading memory at "
> -                             "addr = %s len = %d."),
> +                  warning ("Process record: error reading memory at "
> +                          "addr = %s len = %d.",
>                           paddress (gdbarch, entry->u.mem.addr),
>                            entry->u.mem.len);
>               }
> @@ -664,8 +671,8 @@ record_exec_insn (struct regcache *regca
>                   {
>                     entry->u.mem.mem_entry_not_accessible = 1;
>                     if (record_debug)
> -                      warning (_("Process record: error writing memory at "
> -                                 "addr = %s len = %d."),
> +                      warning ("Process record: error writing memory at "
> +                              "addr = %s len = %d.",
>                                paddress (gdbarch, entry->u.mem.addr),
>                                entry->u.mem.len);
>                   }
> @@ -678,6 +685,215 @@ record_exec_insn (struct regcache *regca
>     }
>  }
>
> +/* bfdcore_read -- read bytes from a core file section.  */
> +
> +static void
> +bfdcore_read (bfd *obfd, asection *osec, void *buf, int len, int *offset)
> +{
> +  int ret = bfd_get_section_contents (obfd, osec, buf, *offset, len);
> +
> +  if (ret)
> +    *offset += len;
> +  else
> +    error (_("Failed to read %d bytes from core file %s ('%s').\n"),
> +          len, bfd_get_filename (obfd),
> +          bfd_errmsg (bfd_get_error ()));
> +}
> +
> +static uint64_t
> +netorder64 (uint64_t fromfile)
> +{
> +  return (BYTE_ORDER == LITTLE_ENDIAN)
> +    ? bswap_64 (fromfile)
> +    : fromfile;
> +}
> +
> +static uint32_t
> +netorder32 (uint32_t fromfile)
> +{
> +  return (BYTE_ORDER == LITTLE_ENDIAN)
> +    ? bswap_32 (fromfile)
> +    : fromfile;
> +}
> +
> +static uint16_t
> +netorder16 (uint16_t fromfile)
> +{
> +  return (BYTE_ORDER == LITTLE_ENDIAN)
> +    ? bswap_16 (fromfile)
> +    : fromfile;
> +}
> +
> +/* Restore the execution log from a core_bfd file.  */
> +
> +static void
> +record_restore (void)
> +{
> +  uint32_t magic;
> +  struct cleanup *old_cleanups;
> +  struct record_entry *rec;
> +  asection *osec;
> +  uint32_t osec_size;
> +  int bfd_offset = 0;
> +  struct regcache *regcache;
> +
> +  /* We restore the execution log from the open core bfd,
> +     if there is one.  */
> +  if (core_bfd == NULL)
> +    return;
> +
> +  /* "record_restore" can only be called when record list is empty.  */
> +  gdb_assert (record_first.next == NULL);
> +
> +  if (record_debug)
> +    printf_filtered ("Restoring recording from core file.\n");
> +
> +  /* Now need to find our special note section.  */
> +  osec = bfd_get_section_by_name (core_bfd, "null0");
> +  osec_size = bfd_section_size (core_bfd, osec);
> +  if (record_debug)
> +    printf_filtered ("Find precord section %s.\n",
> +                    osec ? "succeeded" : "failed");
> +  if (!osec)
> +    return;
> +  if (record_debug)
> +    printf_filtered ("%s", bfd_section_name (core_bfd, osec));
> +
> +  /* Check the magic code.  */
> +  if (record_debug)
> +    printf_filtered ("\
> +  Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
> +                    magic);
> +  bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
> +  if (magic != RECORD_FILE_MAGIC)
> +    error (_("Version mis-match or file format error in core file %s."),
> +          bfd_get_filename (core_bfd));
> +
> +  /* Restore the entries in recfd into record_arch_list_head and
> +     record_arch_list_tail.  */
> +  record_arch_list_head = NULL;
> +  record_arch_list_tail = NULL;
> +  record_insn_num = 0;
> +  old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
> +  regcache = get_current_regcache ();
> +
> +  while (1)
> +    {
> +      int ret;
> +      uint8_t tmpu8;
> +      uint32_t regnum, len, signal, count;
> +      uint64_t addr;
> +
> +      /* We are finished when offset reaches osec_size.  */
> +      if (bfd_offset >= osec_size)
> +       break;
> +      bfdcore_read (core_bfd, osec, &tmpu8, sizeof (tmpu8), &bfd_offset);
> +
> +      switch (tmpu8)
> +        {
> +        case record_reg: /* reg */
> +          if (record_debug)
> +            printf_filtered ("\
> +  Reading register %d (1 plus %d plus %d bytes)\n",
> +                            rec->u.reg.num,
> +                            sizeof (regnum),
> +                            rec->u.reg.len);
> +          /* Get register number to regnum.  */
> +          bfdcore_read (core_bfd, osec, &regnum,
> +                       sizeof (regnum), &bfd_offset);
> +         regnum = netorder32 (regnum);
> +
> +          rec = record_reg_alloc (regcache, regnum);
> +
> +          /* Get val.  */
> +          bfdcore_read (core_bfd, osec, record_get_loc (rec),
> +                       rec->u.reg.len, &bfd_offset);
> +          break;
> +
> +        case record_mem: /* mem */
> +          if (record_debug)
> +            printf_filtered ("\
> +  Reading memory %s (1 plus %d plus %d plus %d bytes)\n",
> +                            paddress (get_current_arch (),
> +                                      rec->u.mem.addr),
> +                            sizeof (addr),
> +                            sizeof (len),
> +                            rec->u.mem.len);
> +          /* Get len.  */
> +          bfdcore_read (core_bfd, osec, &len,
> +                       sizeof (len), &bfd_offset);
> +         len = netorder32 (len);
> +
> +          /* Get addr.  */
> +          bfdcore_read (core_bfd, osec, &addr,
> +                       sizeof (addr), &bfd_offset);
> +         addr = netorder64 (addr);
> +
> +          rec = record_mem_alloc (addr, len);
> +
> +          /* Get val.  */
> +          bfdcore_read (core_bfd, osec, record_get_loc (rec),
> +                       rec->u.mem.len, &bfd_offset);
> +          break;
> +
> +        case record_end: /* end */
> +          if (record_debug)
> +            printf_filtered ("\
> +  Reading record_end (1 + %d + %d bytes), offset == %s\n",
> +                            sizeof (signal),
> +                            sizeof (count),
> +                            paddress (get_current_arch (),
> +                                      bfd_offset));
> +          rec = record_end_alloc ();
> +          record_insn_num ++;
> +
> +         /* Get signal value.  */
> +         bfdcore_read (core_bfd, osec, &signal,
> +                       sizeof (signal), &bfd_offset);
> +         signal = netorder32 (signal);
> +         rec->u.end.sigval = signal;
> +
> +         /* Get insn count.  */
> +         bfdcore_read (core_bfd, osec, &count,
> +                       sizeof (count), &bfd_offset);
> +         count = netorder32 (count);
> +         rec->u.end.insn_num = count;
> +         record_insn_count = count + 1;
> +          break;
> +
> +        default:
> +          error (_("Bad entry type in core file %s."),
> +                bfd_get_filename (core_bfd));
> +          break;
> +        }
> +
> +      /* Add rec to record arch list.  */
> +      record_arch_list_add (rec);
> +    }
> +
> +  discard_cleanups (old_cleanups);
> +
> +  /* Add record_arch_list_head to the end of record list.  */
> +  record_first.next = record_arch_list_head;
> +  record_arch_list_head->prev = &record_first;
> +  record_arch_list_tail->next = NULL;
> +  record_list = &record_first;
> +
> +  /* Update record_insn_max_num.  */
> +  if (record_insn_num > record_insn_max_num)
> +    {
> +      record_insn_max_num = record_insn_num;
> +      warning (_("Auto increase record/replay buffer limit to %d."),
> +               record_insn_max_num);
> +    }
> +
> +  /* Succeeded.  */
> +  printf_filtered (_("Restored records from core file %s.\n"),
> +                  bfd_get_filename (core_bfd));
> +
> +  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
> +}
> +
>  static struct target_ops *tmp_to_resume_ops;
>  static void (*tmp_to_resume) (struct target_ops *, ptid_t, int,
>                              enum target_signal);
> @@ -728,6 +944,7 @@ record_core_open_1 (char *name, int from
>     }
>
>   push_target (&record_core_ops);
> +  record_restore ();
>  }
>
>  static void
> @@ -735,9 +952,6 @@ record_open_1 (char *name, int from_tty)
>  {
>   struct target_ops *t;
>
> -  if (record_debug)
> -    fprintf_unfiltered (gdb_stdlog, "Process record: record_open\n");
> -
>   /* check exec */
>   if (!target_has_execution)
>     error (_("Process record: the program is not being run."));
> @@ -779,6 +993,12 @@ record_open (char *name, int from_tty)
>     error (_("Process record target already running.  Use \"record stop\" to
> "
>              "stop record target first."));
>
> +  /* Reset */
> +  record_insn_num = 0;
> +  record_insn_count = 0;
> +  record_list = &record_first;
> +  record_list->next = NULL;
> +
>   /* Reset the tmp beneath pointers.  */
>   tmp_to_resume_ops = NULL;
>   tmp_to_resume = NULL;
> @@ -822,17 +1042,6 @@ record_open (char *name, int from_tty)
>   if (!tmp_to_xfer_partial)
>     error (_("Could not find 'to_xfer_partial' method on the target
> stack."));
>
> -  if (current_target.to_stratum == core_stratum)
> -    record_core_open_1 (name, from_tty);
> -  else
> -    record_open_1 (name, from_tty);
> -
> -  /* Reset */
> -  record_insn_num = 0;
> -  record_insn_count = 0;
> -  record_list = &record_first;
> -  record_list->next = NULL;
> -
>   /* Set the tmp beneath pointers to beneath pointers.  */
>   record_beneath_to_resume_ops = tmp_to_resume_ops;
>   record_beneath_to_resume = tmp_to_resume;
> @@ -844,6 +1053,11 @@ record_open (char *name, int from_tty)
>   record_beneath_to_xfer_partial = tmp_to_xfer_partial;
>   record_beneath_to_insert_breakpoint = tmp_to_insert_breakpoint;
>   record_beneath_to_remove_breakpoint = tmp_to_remove_breakpoint;
> +
> +  if (current_target.to_stratum == core_stratum)
> +    record_core_open_1 (name, from_tty);
> +  else
> +    record_open_1 (name, from_tty);
>  }
>
>  static void
> @@ -1114,8 +1328,7 @@ record_wait (struct target_ops *ops,
>                    {
>                      if (record_debug)
>                        fprintf_unfiltered (gdb_stdlog,
> -                                           "Process record: break "
> -                                           "at %s.\n",
> +                                           "Process record: break at
> %s.\n",
>                                            paddress (gdbarch, tmp_pc));
>                      if (gdbarch_decr_pc_after_break (gdbarch)
>                          && execution_direction == EXEC_FORWARD
> @@ -1190,8 +1403,7 @@ static void
>  record_mourn_inferior (struct target_ops *ops)
>  {
>   if (record_debug)
> -    fprintf_unfiltered (gdb_stdlog, "Process record: "
> -                                   "record_mourn_inferior\n");
> +    fprintf_unfiltered (gdb_stdlog, "Process record:
> record_mourn_inferior\n");
>
>   unpush_target (&record_ops);
>   target_mourn_inferior ();
> @@ -1345,8 +1557,8 @@ record_xfer_partial (struct target_ops *
>          record_list_release (record_arch_list_tail);
>          if (record_debug)
>            fprintf_unfiltered (gdb_stdlog,
> -                               _("Process record: failed to record "
> -                                 "execution log."));
> +                               "Process record: failed to record "
> +                               "execution log.");
>          return -1;
>        }
>       if (record_arch_list_add_end ())
> @@ -1354,8 +1566,8 @@ record_xfer_partial (struct target_ops *
>          record_list_release (record_arch_list_tail);
>          if (record_debug)
>            fprintf_unfiltered (gdb_stdlog,
> -                               _("Process record: failed to record "
> -                                 "execution log."));
> +                               "Process record: failed to record "
> +                               "execution log.");
>          return -1;
>        }
>       record_list->next = record_arch_list_head;
> @@ -1642,6 +1854,297 @@ cmd_record_start (char *args, int from_t
>   execute_command ("target record", from_tty);
>  }
>
> +/* Record log save-file format
> +   Version 1 (never released)
> +
> +   Header:
> +     4 bytes: magic number htonl(0x20090829).
> +       NOTE: be sure to change whenever this file format changes!
> +
> +   Records:
> +     record_end:
> +       1 byte:  record type (record_end, see enum record_type).
> +     record_reg:
> +       1 byte:  record type (record_reg, see enum record_type).
> +       8 bytes: register id (network byte order).
> +       MAX_REGISTER_SIZE bytes: register value.
> +     record_mem:
> +       1 byte:  record type (record_mem, see enum record_type).
> +       8 bytes: memory length (network byte order).
> +       8 bytes: memory address (network byte order).
> +       n bytes: memory value (n == memory length).
> +
> +   Version 2
> +     4 bytes: magic number netorder32(0x20091016).
> +       NOTE: be sure to change whenever this file format changes!
> +
> +   Records:
> +     record_end:
> +       1 byte:  record type (record_end, see enum record_type).
> +       4 bytes: signal
> +       4 bytes: instruction count
> +     record_reg:
> +       1 byte:  record type (record_reg, see enum record_type).
> +       4 bytes: register id (network byte order).
> +       n bytes: register value (n == actual register size).
> +                (eg. 4 bytes for x86 general registers).
> +     record_mem:
> +       1 byte:  record type (record_mem, see enum record_type).
> +       4 bytes: memory length (network byte order).
> +       8 bytes: memory address (network byte order).
> +       n bytes: memory value (n == memory length).
> +
> +*/
> +
> +/* bfdcore_write -- write bytes into a core file section.  */
> +
> +static void
> +bfdcore_write (bfd *obfd, asection *osec, void *buf, int len, int *offset)
> +{
> +  int ret = bfd_set_section_contents (obfd, osec, buf, *offset, len);
> +
> +  if (ret)
> +    *offset += len;
> +  else
> +    error (_("Failed to write %d bytes to core file %s ('%s').\n"),
> +          len, bfd_get_filename (obfd),
> +          bfd_errmsg (bfd_get_error ()));
> +}
> +
> +/* Restore the execution log from a file.  We use a modified elf
> +   corefile format, with an extra section for our data.  */
> +
> +static void
> +cmd_record_restore (char *args, int from_tty)
> +{
> +  core_file_command (args, from_tty);
> +  record_open (args, from_tty);
> +}
> +
> +static void
> +record_save_cleanups (void *data)
> +{
> +  bfd *obfd = data;
> +  char *pathname = xstrdup (bfd_get_filename (obfd));
> +  bfd_close (obfd);
> +  unlink (pathname);
> +  xfree (pathname);
> +}
> +
> +/* Save the execution log to a file.  We use a modified elf corefile
> +   format, with an extra section for our data.  */
> +
> +static void
> +cmd_record_save (char *args, int from_tty)
> +{
> +  char *recfilename, recfilename_buffer[40];
> +  int recfd;
> +  struct record_entry *cur_record_list;
> +  uint32_t magic;
> +  struct regcache *regcache;
> +  struct gdbarch *gdbarch;
> +  struct cleanup *old_cleanups;
> +  struct cleanup *set_cleanups;
> +  bfd *obfd;
> +  int save_size = 0;
> +  asection *osec = NULL;
> +  int bfd_offset = 0;
> +
> +  if (strcmp (current_target.to_shortname, "record") != 0)
> +    error (_("This command can only be used with target 'record'.\n"
> +            "Use 'target record' first.\n"));
> +
> +  if (args && *args)
> +    recfilename = args;
> +  else
> +    {
> +      /* Default recfile name is "gdb_record.PID".  */
> +      snprintf (recfilename_buffer, sizeof (recfilename_buffer),
> +                "gdb_record.%d", PIDGET (inferior_ptid));
> +      recfilename = recfilename_buffer;
> +    }
> +
> +  /* Open the save file.  */
> +  if (record_debug)
> +    printf_filtered ("Saving execution log to core file '%s'\n",
> recfilename);
> +
> +  /* Open the output file.  */
> +  obfd = create_gcore_bfd (recfilename);
> +  old_cleanups = make_cleanup (record_save_cleanups, obfd);
> +
> +  /* Save the current record entry to "cur_record_list".  */
> +  cur_record_list = record_list;
> +
> +  /* Get the values of regcache and gdbarch.  */
> +  regcache = get_current_regcache ();
> +  gdbarch = get_regcache_arch (regcache);
> +
> +  /* Disable the GDB operation record.  */
> +  set_cleanups = record_gdb_operation_disable_set ();
> +
> +  /* Reverse execute to the begin of record list.  */
> +  while (1)
> +    {
> +      /* Check for beginning and end of log.  */
> +      if (record_list == &record_first)
> +        break;
> +
> +      record_exec_insn (regcache, gdbarch, record_list);
> +
> +      if (record_list->prev)
> +        record_list = record_list->prev;
> +    }
> +
> +  /* Compute the size needed for the extra bfd section.  */
> +  save_size = 4;       /* magic cookie */
> +  for (record_list = record_first.next; record_list;
> +       record_list = record_list->next)
> +    switch (record_list->type)
> +      {
> +      case record_end:
> +       save_size += 1 + 4 + 4;
> +       break;
> +      case record_reg:
> +       save_size += 1 + 4 + record_list->u.reg.len;
> +       break;
> +      case record_mem:
> +       save_size += 1 + 4 + 8 + record_list->u.mem.len;
> +       break;
> +      }
> +
> +  /* Make the new bfd section.  */
> +  osec = bfd_make_section_anyway_with_flags (obfd, "precord",
> +                                             SEC_HAS_CONTENTS
> +                                             | SEC_READONLY);
> +  if (osec == NULL)
> +    error (_("Failed to create 'precord' section for corefile %s: %s"),
> +          recfilename,
> +           bfd_errmsg (bfd_get_error ()));
> +  bfd_set_section_size (obfd, osec, save_size);
> +  bfd_set_section_vma (obfd, osec, 0);
> +  bfd_set_section_alignment (obfd, osec, 0);
> +  bfd_section_lma (obfd, osec) = 0;
> +
> +  /* Save corefile state.  */
> +  write_gcore_file (obfd);
> +
> +  /* Write out the record log.  */
> +  /* Write the magic code.  */
> +  magic = RECORD_FILE_MAGIC;
> +  if (record_debug)
> +    printf_filtered ("\
> +  Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
> +                    magic);
> +  bfdcore_write (obfd, osec, &magic, sizeof (magic), &bfd_offset);
> +
> +  /* Save the entries to recfd and forward execute to the end of
> +     record list.  */
> +  record_list = &record_first;
> +  while (1)
> +    {
> +      /* Save entry.  */
> +      if (record_list != &record_first)
> +        {
> +         uint8_t type;
> +         uint32_t regnum, len, signal, count;
> +          uint64_t addr;
> +
> +         type = record_list->type;
> +          bfdcore_write (obfd, osec, &type, sizeof (type), &bfd_offset);
> +
> +          switch (record_list->type)
> +            {
> +            case record_reg: /* reg */
> +              if (record_debug)
> +               printf_filtered ("\
> +  Writing register %d (1 plus %d plus %d bytes)\n",
> +                                record_list->u.reg.num,
> +                                sizeof (regnum),
> +                                record_list->u.reg.len);
> +
> +              /* Write regnum.  */
> +              regnum = netorder32 (record_list->u.reg.num);
> +              bfdcore_write (obfd, osec, &regnum,
> +                            sizeof (regnum), &bfd_offset);
> +
> +              /* Write regval.  */
> +              bfdcore_write (obfd, osec, record_get_loc (record_list),
> +                            record_list->u.reg.len, &bfd_offset);
> +              break;
> +
> +            case record_mem: /* mem */
> +             if (record_debug)
> +               printf_filtered ("\
> +  Writing memory %s (1 plus %d plus %d plus %d bytes)\n",
> +                                paddress (gdbarch,
> +                                          record_list->u.mem.addr),
> +                                sizeof (addr),
> +                                sizeof (len),
> +                                record_list->u.mem.len);
> +
> +             /* Write memlen.  */
> +             len = netorder32 (record_list->u.mem.len);
> +             bfdcore_write (obfd, osec, &len, sizeof (len), &bfd_offset);
> +
> +             /* Write memaddr.  */
> +             addr = netorder64 (record_list->u.mem.addr);
> +             bfdcore_write (obfd, osec, &addr,
> +                            sizeof (addr), &bfd_offset);
> +
> +             /* Write memval.  */
> +             bfdcore_write (obfd, osec, record_get_loc (record_list),
> +                            record_list->u.mem.len, &bfd_offset);
> +              break;
> +
> +              case record_end:
> +                if (record_debug)
> +                  printf_filtered ("\
> +  Writing record_end (1 + %d + %d bytes)\n",
> +                                  sizeof (signal),
> +                                  sizeof (count));
> +               /* Write signal value.  */
> +               signal = netorder32 (record_list->u.end.sigval);
> +               bfdcore_write (obfd, osec, &signal,
> +                              sizeof (signal), &bfd_offset);
> +
> +               /* Write insn count.  */
> +               count = netorder32 (record_list->u.end.insn_num);
> +               bfdcore_write (obfd, osec, &count,
> +                              sizeof (count), &bfd_offset);
> +                break;
> +            }
> +        }
> +
> +      /* Execute entry.  */
> +      record_exec_insn (regcache, gdbarch, record_list);
> +
> +      if (record_list->next)
> +        record_list = record_list->next;
> +      else
> +        break;
> +    }
> +
> +  /* Reverse execute to cur_record_list.  */
> +  while (1)
> +    {
> +      /* Check for beginning and end of log.  */
> +      if (record_list == cur_record_list)
> +        break;
> +
> +      record_exec_insn (regcache, gdbarch, record_list);
> +
> +      if (record_list->prev)
> +        record_list = record_list->prev;
> +    }
> +
> +  do_cleanups (set_cleanups);
> +  do_cleanups (old_cleanups);
> +
> +  /* Succeeded.  */
> +  printf_filtered (_("Saved core file %s with execution log.\n"),
> +                  recfilename);
> +}
> +
>  /* Truncate the record log from the present point
>    of replay until the end.  */
>
> @@ -1749,6 +2252,8 @@ info_record_command (char *args, int fro
>  void
>  _initialize_record (void)
>  {
> +  struct cmd_list_element *c;
> +
>   /* Init record_first.  */
>   record_first.prev = NULL;
>   record_first.next = NULL;
> @@ -1767,10 +2272,12 @@ _initialize_record (void)
>                            NULL, show_record_debug, &setdebuglist,
>                            &showdebuglist);
>
> -  add_prefix_cmd ("record", class_obscure, cmd_record_start,
> +  c = add_prefix_cmd ("record", class_obscure, cmd_record_start,
>                  _("Abbreviated form of \"target record\" command."),
>                  &record_cmdlist, "record ", 0, &cmdlist);
> +  set_cmd_completer (c, filename_completer);
>   add_com_alias ("rec", "record", class_obscure, 1);
> +
>   add_prefix_cmd ("record", class_support, set_record_command,
>                  _("Set record options"), &set_record_cmdlist,
>                  "set record ", 0, &setlist);
> @@ -1784,6 +2291,17 @@ _initialize_record (void)
>                  "info record ", 0, &infolist);
>   add_alias_cmd ("rec", "record", class_obscure, 1, &infolist);
>
> +  c = add_cmd ("save", class_obscure, cmd_record_save,
> +              _("Save the execution log to a file.\n\
> +Argument is optional filename.\n\
> +Default filename is 'gdb_record.<process_id>'."),
> +              &record_cmdlist);
> +
> +  c = add_cmd ("restore", class_obscure, cmd_record_restore,
> +              _("Restore the execution log from a file.\n\
> +Argument is filename.  File must be created with 'record save'."),
> +              &record_cmdlist);
> +  set_cmd_completer (c, filename_completer);
>
>   add_cmd ("delete", class_obscure, cmd_record_delete,
>           _("Delete the rest of execution log and start recording it
> anew."),
>
>

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

Hui Zhu wrote:
> Hi Michael,
> 
> Thanks for your patch.
> 
> I cannot find the diff for the gcore.c file.

Oh no -- that's because I forgot to include it.  ;-(

Guess I'll have to submit a "part 5 of 3".

>        (netorder64): New function.
>        (netorder32): New function.
>        (netorder16): New function.

Ah, good idea, maybe bfdcore_read and write too.

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Eli Zaretskii]

> Date: Sat, 17 Oct 2009 15:13:37 -0700
> From: Michael Snyder <msnyder@vmware.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, 
>  "teawater@gmail.com" <teawater@gmail.com>
> 
> >> This is the same approach that is used by the "gcore" command.
> >> How does "gcore" work with go32, if at all?
> > 
> > It doesn't.  DJGPP cannot generate core files.
> 
> Well, save/restore depends on core files, so I guess
> it won't work in go32.

DJGPP does not support core files created from a memory image of a
running process, but I don't see any reason why bfdcore_write won't
work for it.

> See revised patch below.

Thanks. this is fine.

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

Eli Zaretskii wrote:
>> Date: Sat, 17 Oct 2009 15:13:37 -0700
>> From: Michael Snyder <msnyder@vmware.com>
>> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, 
>>  "teawater@gmail.com" <teawater@gmail.com>
>>
>>>> This is the same approach that is used by the "gcore" command.
>>>> How does "gcore" work with go32, if at all?
>>> It doesn't.  DJGPP cannot generate core files.
>> Well, save/restore depends on core files, so I guess
>> it won't work in go32.
> 
> DJGPP does not support core files created from a memory image of a
> running process, but I don't see any reason why bfdcore_write won't
> work for it.

We don't just do bfdcore_write -- we actually create a core file
from the memory image, and then add an extra segment to it for
bfdcore_write to write into.  The core file is an integral part
of the execution log file.

I forgot to post the accompanying changes to gcore.c with this
patch.  I'm just about to put them up now that Hui reminded me.

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Hui Zhu]

Hi Michael,

+  do_cleanups (old_cleanups);

This line will remove the record file that we just save.

I change some code:
static void
record_save_cleanups (void *data)
{
  bfd *obfd = data;
  //char *pathname = xstrdup (bfd_get_filename (obfd));
  bfd_close (obfd);
  //unlink (pathname);
  //xfree (pathname);
}

I think you want unlink the gdb_record when save get some error.  It
maybe need "discard_cleanups" the old_cleanups and bfd_close (obfd);

After change the code, everything is OK.

I try it in i386 ubuntu.  Testsuite is OK too.

Thanks for you are working on it.  :)

Thanks,
Hui

On Sun, Oct 18, 2009 at 12:04, Michael Snyder <msnyder@vmware.com> wrote:
> Eli Zaretskii wrote:
>>>
>>> Date: Sat, 17 Oct 2009 15:13:37 -0700
>>> From: Michael Snyder <msnyder@vmware.com>
>>> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
>>>  "teawater@gmail.com" <teawater@gmail.com>
>>>
>>>>> This is the same approach that is used by the "gcore" command.
>>>>> How does "gcore" work with go32, if at all?
>>>>
>>>> It doesn't.  DJGPP cannot generate core files.
>>>
>>> Well, save/restore depends on core files, so I guess
>>> it won't work in go32.
>>
>> DJGPP does not support core files created from a memory image of a
>> running process, but I don't see any reason why bfdcore_write won't
>> work for it.
>
> We don't just do bfdcore_write -- we actually create a core file
> from the memory image, and then add an extra segment to it for
> bfdcore_write to write into.  The core file is an integral part
> of the execution log file.
>
> I forgot to post the accompanying changes to gcore.c with this
> patch.  I'm just about to put them up now that Hui reminded me.
>
>

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

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

Hui Zhu wrote:
> Hi Michael,
> 
> +  do_cleanups (old_cleanups);
> 
> This line will remove the record file that we just save.
> 
> I change some code:
> static void
> record_save_cleanups (void *data)
> {
>   bfd *obfd = data;
>   //char *pathname = xstrdup (bfd_get_filename (obfd));
>   bfd_close (obfd);
>   //unlink (pathname);
>   //xfree (pathname);
> }
> 
> I think you want unlink the gdb_record when save get some error.  It
> maybe need "discard_cleanups" the old_cleanups and bfd_close (obfd);
> 
> After change the code, everything is OK.

Yes.  Thanks.  Like this:
+      if (record_list->prev)
+        record_list = record_list->prev;
+    }
+
+  do_cleanups (set_cleanups);
+  bfd_close (obfd);
+  discard_cleanups (old_cleanups);
+
+  /* Succeeded.  */

New diff attached.


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

--- record.2b.c	2009-10-18 10:17:40.000000000 -0700
+++ record.12b.c	2009-10-19 10:52:28.000000000 -0700
@@ -23,10 +23,15 @@
 #include "gdbthread.h"
 #include "event-top.h"
 #include "exceptions.h"
+#include "completer.h"
+#include "arch-utils.h"
 #include "gdbcore.h"
 #include "exec.h"
 #include "record.h"
+#include "elf-bfd.h"
+#include "gcore.h"
 
+#include <byteswap.h>
 #include <signal.h>
 
 #define DEFAULT_RECORD_INSN_MAX_NUM	200000
@@ -34,6 +39,8 @@
 #define RECORD_IS_REPLAY \
      (record_list->next || execution_direction == EXEC_REVERSE)
 
+#define RECORD_FILE_MAGIC	netorder32(0x20091016)
+
 /* These are the core structs of the process record functionality.
 
    A record_entry is a record of the value change of a register
@@ -482,24 +489,24 @@ record_check_insn_num (int set_terminal)
 	      if (q)
 		record_stop_at_limit = 0;
 	      else
-		error (_("Process record: inferior program stopped."));
+		error (_("Process record: stopped by user."));
 	    }
 	}
     }
 }
 
+static void
+record_arch_list_cleanups (void *ignore)
+{
+  record_list_release (record_arch_list_tail);
+}
+
 /* Before inferior step (when GDB record the running message, inferior
    only can step), GDB will call this function to record the values to
    record_list.  This function will call gdbarch_process_record to
    record the running message of inferior and set them to
    record_arch_list, and add it to record_list.  */
 
-static void
-record_message_cleanups (void *ignore)
-{
-  record_list_release (record_arch_list_tail);
-}
-
 struct record_message_args {
   struct regcache *regcache;
   enum target_signal signal;
@@ -511,7 +518,7 @@ record_message (void *args)
   int ret;
   struct record_message_args *myargs = args;
   struct gdbarch *gdbarch = get_regcache_arch (myargs->regcache);
-  struct cleanup *old_cleanups = make_cleanup (record_message_cleanups, 0);
+  struct cleanup *old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
 
   record_arch_list_head = NULL;
   record_arch_list_tail = NULL;
@@ -651,8 +658,8 @@ record_exec_insn (struct regcache *regca
               {
                 entry->u.mem.mem_entry_not_accessible = 1;
                 if (record_debug)
-                  warning (_("Process record: error reading memory at "
-                             "addr = %s len = %d."),
+                  warning ("Process record: error reading memory at "
+			   "addr = %s len = %d.",
 			   paddress (gdbarch, entry->u.mem.addr),
                            entry->u.mem.len);
               }
@@ -664,8 +671,8 @@ record_exec_insn (struct regcache *regca
                   {
                     entry->u.mem.mem_entry_not_accessible = 1;
                     if (record_debug)
-                      warning (_("Process record: error writing memory at "
-                                 "addr = %s len = %d."),
+                      warning ("Process record: error writing memory at "
+			       "addr = %s len = %d.",
                                paddress (gdbarch, entry->u.mem.addr),
                                entry->u.mem.len);
                   }
@@ -678,6 +685,217 @@ record_exec_insn (struct regcache *regca
     }
 }
 
+/* bfdcore_read -- read bytes from a core file section.  */
+
+static inline void
+bfdcore_read (bfd *obfd, asection *osec, void *buf, int len, int *offset)
+{
+  int ret = bfd_get_section_contents (obfd, osec, buf, *offset, len);
+
+  if (ret)
+    *offset += len;
+  else
+    error (_("Failed to read %d bytes from core file %s ('%s').\n"),
+	   len, bfd_get_filename (obfd),
+	   bfd_errmsg (bfd_get_error ()));
+}
+
+static inline uint64_t
+netorder64 (uint64_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_64 (fromfile) 
+    : fromfile;
+}
+
+static inline uint32_t
+netorder32 (uint32_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_32 (fromfile) 
+    : fromfile;
+}
+
+static inline uint16_t
+netorder16 (uint16_t fromfile)
+{
+  return (BYTE_ORDER == LITTLE_ENDIAN) 
+    ? bswap_16 (fromfile) 
+    : fromfile;
+}
+
+/* Restore the execution log from a core_bfd file.  */
+
+static void
+record_restore (void)
+{
+  uint32_t magic;
+  struct cleanup *old_cleanups;
+  struct record_entry *rec;
+  asection *osec;
+  uint32_t osec_size;
+  int bfd_offset = 0;
+  struct regcache *regcache;
+
+  /* We restore the execution log from the open core bfd,
+     if there is one.  */
+  if (core_bfd == NULL)
+    return;
+
+  /* "record_restore" can only be called when record list is empty.  */
+  gdb_assert (record_first.next == NULL);
+
+  if (record_debug)
+    printf_filtered ("Restoring recording from core file.\n");
+
+  /* Now need to find our special note section.  */
+  osec = bfd_get_section_by_name (core_bfd, "null0");
+  osec_size = bfd_section_size (core_bfd, osec);
+  if (record_debug)
+    printf_filtered ("Find precord section %s.\n",
+		     osec ? "succeeded" : "failed");
+  if (!osec)
+    return;
+  if (record_debug)
+    printf_filtered ("%s", bfd_section_name (core_bfd, osec));
+
+  /* Check the magic code.  */
+  bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
+  if (magic != RECORD_FILE_MAGIC)
+    error (_("Version mis-match or file format error in core file %s."),
+	   bfd_get_filename (core_bfd));
+  if (record_debug)
+    printf_filtered ("\
+  Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
+		     netorder32 (magic));
+
+  /* Restore the entries in recfd into record_arch_list_head and
+     record_arch_list_tail.  */
+  record_arch_list_head = NULL;
+  record_arch_list_tail = NULL;
+  record_insn_num = 0;
+  old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
+  regcache = get_current_regcache ();
+
+  while (1)
+    {
+      int ret;
+      uint8_t tmpu8;
+      uint32_t regnum, len, signal, count;
+      uint64_t addr;
+
+      /* We are finished when offset reaches osec_size.  */
+      if (bfd_offset >= osec_size)
+	break;
+      bfdcore_read (core_bfd, osec, &tmpu8, sizeof (tmpu8), &bfd_offset);
+
+      switch (tmpu8)
+        {
+        case record_reg: /* reg */
+          /* Get register number to regnum.  */
+          bfdcore_read (core_bfd, osec, &regnum,
+			sizeof (regnum), &bfd_offset);
+	  regnum = netorder32 (regnum);
+
+          rec = record_reg_alloc (regcache, regnum);
+
+          /* Get val.  */
+          bfdcore_read (core_bfd, osec, record_get_loc (rec),
+			rec->u.reg.len, &bfd_offset);
+
+          if (record_debug)
+            printf_filtered ("\
+  Reading register %d (1 plus %d plus %d bytes)\n",
+			     rec->u.reg.num,
+			     sizeof (regnum),
+			     rec->u.reg.len);
+          break;
+
+        case record_mem: /* mem */
+          /* Get len.  */
+          bfdcore_read (core_bfd, osec, &len, 
+			sizeof (len), &bfd_offset);
+	  len = netorder32 (len);
+
+          /* Get addr.  */
+          bfdcore_read (core_bfd, osec, &addr,
+			sizeof (addr), &bfd_offset);
+	  addr = netorder64 (addr);
+
+          rec = record_mem_alloc (addr, len);
+
+          /* Get val.  */
+          bfdcore_read (core_bfd, osec, record_get_loc (rec),
+			rec->u.mem.len, &bfd_offset);
+
+          if (record_debug)
+            printf_filtered ("\
+  Reading memory %s (1 plus %d plus %d plus %d bytes)\n",
+			     paddress (get_current_arch (),
+				       rec->u.mem.addr),
+			     sizeof (addr),
+			     sizeof (len),
+			     rec->u.mem.len);
+          break;
+
+        case record_end: /* end */
+          rec = record_end_alloc ();
+          record_insn_num ++;
+
+	  /* Get signal value.  */
+	  bfdcore_read (core_bfd, osec, &signal, 
+			sizeof (signal), &bfd_offset);
+	  signal = netorder32 (signal);
+	  rec->u.end.sigval = signal;
+
+	  /* Get insn count.  */
+	  bfdcore_read (core_bfd, osec, &count, 
+			sizeof (count), &bfd_offset);
+	  count = netorder32 (count);
+	  rec->u.end.insn_num = count;
+	  record_insn_count = count + 1;
+          if (record_debug)
+            printf_filtered ("\
+  Reading record_end (1 + %d + %d bytes), offset == %s\n",
+			     sizeof (signal),
+			     sizeof (count),
+			     paddress (get_current_arch (),
+				       bfd_offset));
+          break;
+
+        default:
+          error (_("Bad entry type in core file %s."),
+		 bfd_get_filename (core_bfd));
+          break;
+        }
+
+      /* Add rec to record arch list.  */
+      record_arch_list_add (rec);
+    }
+
+  discard_cleanups (old_cleanups);
+
+  /* Add record_arch_list_head to the end of record list.  */
+  record_first.next = record_arch_list_head;
+  record_arch_list_head->prev = &record_first;
+  record_arch_list_tail->next = NULL;
+  record_list = &record_first;
+
+  /* Update record_insn_max_num.  */
+  if (record_insn_num > record_insn_max_num)
+    {
+      record_insn_max_num = record_insn_num;
+      warning (_("Auto increase record/replay buffer limit to %d."),
+               record_insn_max_num);
+    }
+
+  /* Succeeded.  */
+  printf_filtered (_("Restored records from core file %s.\n"),
+		   bfd_get_filename (core_bfd));
+
+  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
+}
+
 static struct target_ops *tmp_to_resume_ops;
 static void (*tmp_to_resume) (struct target_ops *, ptid_t, int,
 			      enum target_signal);
@@ -728,6 +946,7 @@ record_core_open_1 (char *name, int from
     }
 
   push_target (&record_core_ops);
+  record_restore ();
 }
 
 static void
@@ -735,9 +954,6 @@ record_open_1 (char *name, int from_tty)
 {
   struct target_ops *t;
 
-  if (record_debug)
-    fprintf_unfiltered (gdb_stdlog, "Process record: record_open\n");
-
   /* check exec */
   if (!target_has_execution)
     error (_("Process record: the program is not being run."));
@@ -779,6 +995,12 @@ record_open (char *name, int from_tty)
     error (_("Process record target already running.  Use \"record stop\" to "
              "stop record target first."));
 
+  /* Reset */
+  record_insn_num = 0;
+  record_insn_count = 0;
+  record_list = &record_first;
+  record_list->next = NULL;
+
   /* Reset the tmp beneath pointers.  */
   tmp_to_resume_ops = NULL;
   tmp_to_resume = NULL;
@@ -822,17 +1044,6 @@ record_open (char *name, int from_tty)
   if (!tmp_to_xfer_partial)
     error (_("Could not find 'to_xfer_partial' method on the target stack."));
 
-  if (current_target.to_stratum == core_stratum)
-    record_core_open_1 (name, from_tty);
-  else
-    record_open_1 (name, from_tty);
-
-  /* Reset */
-  record_insn_num = 0;
-  record_insn_count = 0;
-  record_list = &record_first;
-  record_list->next = NULL;
-
   /* Set the tmp beneath pointers to beneath pointers.  */
   record_beneath_to_resume_ops = tmp_to_resume_ops;
   record_beneath_to_resume = tmp_to_resume;
@@ -844,6 +1055,11 @@ record_open (char *name, int from_tty)
   record_beneath_to_xfer_partial = tmp_to_xfer_partial;
   record_beneath_to_insert_breakpoint = tmp_to_insert_breakpoint;
   record_beneath_to_remove_breakpoint = tmp_to_remove_breakpoint;
+
+  if (current_target.to_stratum == core_stratum)
+    record_core_open_1 (name, from_tty);
+  else
+    record_open_1 (name, from_tty);
 }
 
 static void
@@ -1114,8 +1330,7 @@ record_wait (struct target_ops *ops,
 		    {
 		      if (record_debug)
 			fprintf_unfiltered (gdb_stdlog,
-					    "Process record: break "
-					    "at %s.\n",
+					    "Process record: break at %s.\n",
 					    paddress (gdbarch, tmp_pc));
 		      if (gdbarch_decr_pc_after_break (gdbarch)
 			  && execution_direction == EXEC_FORWARD
@@ -1190,8 +1405,7 @@ static void
 record_mourn_inferior (struct target_ops *ops)
 {
   if (record_debug)
-    fprintf_unfiltered (gdb_stdlog, "Process record: "
-			            "record_mourn_inferior\n");
+    fprintf_unfiltered (gdb_stdlog, "Process record: record_mourn_inferior\n");
 
   unpush_target (&record_ops);
   target_mourn_inferior ();
@@ -1345,8 +1559,8 @@ record_xfer_partial (struct target_ops *
 	  record_list_release (record_arch_list_tail);
 	  if (record_debug)
 	    fprintf_unfiltered (gdb_stdlog,
-				_("Process record: failed to record "
-				  "execution log."));
+				"Process record: failed to record "
+				"execution log.");
 	  return -1;
 	}
       if (record_arch_list_add_end ())
@@ -1354,8 +1568,8 @@ record_xfer_partial (struct target_ops *
 	  record_list_release (record_arch_list_tail);
 	  if (record_debug)
 	    fprintf_unfiltered (gdb_stdlog,
-				_("Process record: failed to record "
-				  "execution log."));
+				"Process record: failed to record "
+				"execution log.");
 	  return -1;
 	}
       record_list->next = record_arch_list_head;
@@ -1642,6 +1856,298 @@ cmd_record_start (char *args, int from_t
   execute_command ("target record", from_tty);
 }
 
+/* Record log save-file format
+   Version 1 (never released)
+
+   Header:
+     4 bytes: magic number htonl(0x20090829).
+       NOTE: be sure to change whenever this file format changes!
+
+   Records:
+     record_end:
+       1 byte:  record type (record_end, see enum record_type).
+     record_reg:
+       1 byte:  record type (record_reg, see enum record_type).
+       8 bytes: register id (network byte order).
+       MAX_REGISTER_SIZE bytes: register value.
+     record_mem:
+       1 byte:  record type (record_mem, see enum record_type).
+       8 bytes: memory length (network byte order).
+       8 bytes: memory address (network byte order).
+       n bytes: memory value (n == memory length).
+
+   Version 2
+     4 bytes: magic number netorder32(0x20091016).
+       NOTE: be sure to change whenever this file format changes!
+
+   Records:
+     record_end:
+       1 byte:  record type (record_end, see enum record_type).
+       4 bytes: signal
+       4 bytes: instruction count
+     record_reg:
+       1 byte:  record type (record_reg, see enum record_type).
+       4 bytes: register id (network byte order).
+       n bytes: register value (n == actual register size).
+                (eg. 4 bytes for x86 general registers).
+     record_mem:
+       1 byte:  record type (record_mem, see enum record_type).
+       4 bytes: memory length (network byte order).
+       8 bytes: memory address (network byte order).
+       n bytes: memory value (n == memory length).
+
+*/
+
+/* bfdcore_write -- write bytes into a core file section.  */
+
+static inline void
+bfdcore_write (bfd *obfd, asection *osec, void *buf, int len, int *offset)
+{
+  int ret = bfd_set_section_contents (obfd, osec, buf, *offset, len);
+
+  if (ret)
+    *offset += len;
+  else
+    error (_("Failed to write %d bytes to core file %s ('%s').\n"),
+	   len, bfd_get_filename (obfd),
+	   bfd_errmsg (bfd_get_error ()));
+}
+
+/* Restore the execution log from a file.  We use a modified elf
+   corefile format, with an extra section for our data.  */
+
+static void
+cmd_record_restore (char *args, int from_tty)
+{
+  core_file_command (args, from_tty);
+  record_open (args, from_tty);
+}
+
+static void
+record_save_cleanups (void *data)
+{
+  bfd *obfd = data;
+  char *pathname = xstrdup (bfd_get_filename (obfd));
+  bfd_close (obfd);
+  unlink (pathname);
+  xfree (pathname);
+}
+
+/* Save the execution log to a file.  We use a modified elf corefile
+   format, with an extra section for our data.  */
+
+static void
+cmd_record_save (char *args, int from_tty)
+{
+  char *recfilename, recfilename_buffer[40];
+  int recfd;
+  struct record_entry *cur_record_list;
+  uint32_t magic;
+  struct regcache *regcache;
+  struct gdbarch *gdbarch;
+  struct cleanup *old_cleanups;
+  struct cleanup *set_cleanups;
+  bfd *obfd;
+  int save_size = 0;
+  asection *osec = NULL;
+  int bfd_offset = 0;
+
+  if (strcmp (current_target.to_shortname, "record") != 0)
+    error (_("This command can only be used with target 'record'.\n"
+	     "Use 'target record' first.\n"));
+
+  if (args && *args)
+    recfilename = args;
+  else
+    {
+      /* Default recfile name is "gdb_record.PID".  */
+      snprintf (recfilename_buffer, sizeof (recfilename_buffer),
+                "gdb_record.%d", PIDGET (inferior_ptid));
+      recfilename = recfilename_buffer;
+    }
+
+  /* Open the save file.  */
+  if (record_debug)
+    printf_filtered ("Saving execution log to core file '%s'\n", recfilename);
+
+  /* Open the output file.  */
+  obfd = create_gcore_bfd (recfilename);
+  old_cleanups = make_cleanup (record_save_cleanups, obfd);
+
+  /* Save the current record entry to "cur_record_list".  */
+  cur_record_list = record_list;
+
+  /* Get the values of regcache and gdbarch.  */
+  regcache = get_current_regcache ();
+  gdbarch = get_regcache_arch (regcache);
+
+  /* Disable the GDB operation record.  */
+  set_cleanups = record_gdb_operation_disable_set ();
+
+  /* Reverse execute to the begin of record list.  */
+  while (1)
+    {
+      /* Check for beginning and end of log.  */
+      if (record_list == &record_first)
+        break;
+
+      record_exec_insn (regcache, gdbarch, record_list);
+
+      if (record_list->prev)
+        record_list = record_list->prev;
+    }
+
+  /* Compute the size needed for the extra bfd section.  */
+  save_size = 4;	/* magic cookie */
+  for (record_list = record_first.next; record_list;
+       record_list = record_list->next)
+    switch (record_list->type)
+      {
+      case record_end:
+	save_size += 1 + 4 + 4;
+	break;
+      case record_reg:
+	save_size += 1 + 4 + record_list->u.reg.len;
+	break;
+      case record_mem:
+	save_size += 1 + 4 + 8 + record_list->u.mem.len;
+	break;
+      }
+
+  /* Make the new bfd section.  */
+  osec = bfd_make_section_anyway_with_flags (obfd, "precord",
+                                             SEC_HAS_CONTENTS
+                                             | SEC_READONLY);
+  if (osec == NULL)
+    error (_("Failed to create 'precord' section for corefile %s: %s"),
+	   recfilename,
+           bfd_errmsg (bfd_get_error ()));
+  bfd_set_section_size (obfd, osec, save_size);
+  bfd_set_section_vma (obfd, osec, 0);
+  bfd_set_section_alignment (obfd, osec, 0);
+  bfd_section_lma (obfd, osec) = 0;
+
+  /* Save corefile state.  */
+  write_gcore_file (obfd);
+
+  /* Write out the record log.  */
+  /* Write the magic code.  */
+  magic = RECORD_FILE_MAGIC;
+  if (record_debug)
+    printf_filtered ("\
+  Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
+		     magic);
+  bfdcore_write (obfd, osec, &magic, sizeof (magic), &bfd_offset);
+
+  /* Save the entries to recfd and forward execute to the end of
+     record list.  */
+  record_list = &record_first;
+  while (1)
+    {
+      /* Save entry.  */
+      if (record_list != &record_first)
+        {
+	  uint8_t type;
+	  uint32_t regnum, len, signal, count;
+          uint64_t addr;
+
+	  type = record_list->type;
+          bfdcore_write (obfd, osec, &type, sizeof (type), &bfd_offset);
+
+          switch (record_list->type)
+            {
+            case record_reg: /* reg */
+              if (record_debug)
+		printf_filtered ("\
+  Writing register %d (1 plus %d plus %d bytes)\n",
+				 record_list->u.reg.num,
+				 sizeof (regnum),
+				 record_list->u.reg.len);
+
+              /* Write regnum.  */
+              regnum = netorder32 (record_list->u.reg.num);
+              bfdcore_write (obfd, osec, &regnum,
+			     sizeof (regnum), &bfd_offset);
+
+              /* Write regval.  */
+              bfdcore_write (obfd, osec, record_get_loc (record_list),
+			     record_list->u.reg.len, &bfd_offset);
+              break;
+
+            case record_mem: /* mem */
+	      if (record_debug)
+		printf_filtered ("\
+  Writing memory %s (1 plus %d plus %d plus %d bytes)\n",
+				 paddress (gdbarch,
+					   record_list->u.mem.addr),
+				 sizeof (addr),
+				 sizeof (len),
+				 record_list->u.mem.len);
+
+	      /* Write memlen.  */
+	      len = netorder32 (record_list->u.mem.len);
+	      bfdcore_write (obfd, osec, &len, sizeof (len), &bfd_offset);
+
+	      /* Write memaddr.  */
+	      addr = netorder64 (record_list->u.mem.addr);
+	      bfdcore_write (obfd, osec, &addr, 
+			     sizeof (addr), &bfd_offset);
+
+	      /* Write memval.  */
+	      bfdcore_write (obfd, osec, record_get_loc (record_list),
+			     record_list->u.mem.len, &bfd_offset);
+              break;
+
+              case record_end:
+                if (record_debug)
+                  printf_filtered ("\
+  Writing record_end (1 + %d + %d bytes)\n", 
+				   sizeof (signal),
+				   sizeof (count));
+		/* Write signal value.  */
+		signal = netorder32 (record_list->u.end.sigval);
+		bfdcore_write (obfd, osec, &signal,
+			       sizeof (signal), &bfd_offset);
+
+		/* Write insn count.  */
+		count = netorder32 (record_list->u.end.insn_num);
+		bfdcore_write (obfd, osec, &count,
+			       sizeof (count), &bfd_offset);
+                break;
+            }
+        }
+
+      /* Execute entry.  */
+      record_exec_insn (regcache, gdbarch, record_list);
+
+      if (record_list->next)
+        record_list = record_list->next;
+      else
+        break;
+    }
+
+  /* Reverse execute to cur_record_list.  */
+  while (1)
+    {
+      /* Check for beginning and end of log.  */
+      if (record_list == cur_record_list)
+        break;
+
+      record_exec_insn (regcache, gdbarch, record_list);
+
+      if (record_list->prev)
+        record_list = record_list->prev;
+    }
+
+  do_cleanups (set_cleanups);
+  bfd_close (obfd);
+  discard_cleanups (old_cleanups);
+
+  /* Succeeded.  */
+  printf_filtered (_("Saved core file %s with execution log.\n"),
+		   recfilename);
+}
+
 /* Truncate the record log from the present point
    of replay until the end.  */
 
@@ -1749,6 +2255,8 @@ info_record_command (char *args, int fro
 void
 _initialize_record (void)
 {
+  struct cmd_list_element *c;
+
   /* Init record_first.  */
   record_first.prev = NULL;
   record_first.next = NULL;
@@ -1767,10 +2275,12 @@ _initialize_record (void)
 			    NULL, show_record_debug, &setdebuglist,
 			    &showdebuglist);
 
-  add_prefix_cmd ("record", class_obscure, cmd_record_start,
+  c = add_prefix_cmd ("record", class_obscure, cmd_record_start,
 		  _("Abbreviated form of \"target record\" command."),
  		  &record_cmdlist, "record ", 0, &cmdlist);
+  set_cmd_completer (c, filename_completer);
   add_com_alias ("rec", "record", class_obscure, 1);
+
   add_prefix_cmd ("record", class_support, set_record_command,
 		  _("Set record options"), &set_record_cmdlist,
 		  "set record ", 0, &setlist);
@@ -1784,6 +2294,17 @@ _initialize_record (void)
 		  "info record ", 0, &infolist);
   add_alias_cmd ("rec", "record", class_obscure, 1, &infolist);
 
+  c = add_cmd ("save", class_obscure, cmd_record_save,
+	       _("Save the execution log to a file.\n\
+Argument is optional filename.\n\
+Default filename is 'gdb_record.<process_id>'."),
+	       &record_cmdlist);
+
+  c = add_cmd ("restore", class_obscure, cmd_record_restore,
+	       _("Restore the execution log from a file.\n\
+Argument is filename.  File must be created with 'record save'."),
+	       &record_cmdlist);
+  set_cmd_completer (c, filename_completer);
 
   add_cmd ("delete", class_obscure, cmd_record_delete,
 	   _("Delete the rest of execution log and start recording it anew."),

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Hui Zhu]

On Tue, Oct 20, 2009 at 01:54, Michael Snyder <msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> Hi Michael,
>>
>> +  do_cleanups (old_cleanups);
>>
>> This line will remove the record file that we just save.
>>
>> I change some code:
>> static void
>> record_save_cleanups (void *data)
>> {
>>  bfd *obfd = data;
>>  //char *pathname = xstrdup (bfd_get_filename (obfd));
>>  bfd_close (obfd);
>>  //unlink (pathname);
>>  //xfree (pathname);
>> }
>>
>> I think you want unlink the gdb_record when save get some error.  It
>> maybe need "discard_cleanups" the old_cleanups and bfd_close (obfd);
>>
>> After change the code, everything is OK.
>
> Yes.  Thanks.  Like this:
> +      if (record_list->prev)
> +        record_list = record_list->prev;
> +    }
> +
> +  do_cleanups (set_cleanups);
> +  bfd_close (obfd);
> +  discard_cleanups (old_cleanups);
> +
> +  /* Succeeded.  */


I suggest:
+  discard_cleanups (old_cleanups);
+  bfd_close (obfd);

BTW, the record.c update by Pedro,
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/record.c.diff?r1=1.24&r2=1.25&cvsroot=src

Maybe the record save patch need some update.

Thanks,
Hui

>
> New diff attached.
>
>
> --- record.2b.c 2009-10-18 10:17:40.000000000 -0700
> +++ record.12b.c        2009-10-19 10:52:28.000000000 -0700
> @@ -23,10 +23,15 @@
>  #include "gdbthread.h"
>  #include "event-top.h"
>  #include "exceptions.h"
> +#include "completer.h"
> +#include "arch-utils.h"
>  #include "gdbcore.h"
>  #include "exec.h"
>  #include "record.h"
> +#include "elf-bfd.h"
> +#include "gcore.h"
>
> +#include <byteswap.h>
>  #include <signal.h>
>
>  #define DEFAULT_RECORD_INSN_MAX_NUM    200000
> @@ -34,6 +39,8 @@
>  #define RECORD_IS_REPLAY \
>      (record_list->next || execution_direction == EXEC_REVERSE)
>
> +#define RECORD_FILE_MAGIC      netorder32(0x20091016)
> +
>  /* These are the core structs of the process record functionality.
>
>    A record_entry is a record of the value change of a register
> @@ -482,24 +489,24 @@ record_check_insn_num (int set_terminal)
>              if (q)
>                record_stop_at_limit = 0;
>              else
> -               error (_("Process record: inferior program stopped."));
> +               error (_("Process record: stopped by user."));
>            }
>        }
>     }
>  }
>
> +static void
> +record_arch_list_cleanups (void *ignore)
> +{
> +  record_list_release (record_arch_list_tail);
> +}
> +
>  /* Before inferior step (when GDB record the running message, inferior
>    only can step), GDB will call this function to record the values to
>    record_list.  This function will call gdbarch_process_record to
>    record the running message of inferior and set them to
>    record_arch_list, and add it to record_list.  */
>
> -static void
> -record_message_cleanups (void *ignore)
> -{
> -  record_list_release (record_arch_list_tail);
> -}
> -
>  struct record_message_args {
>   struct regcache *regcache;
>   enum target_signal signal;
> @@ -511,7 +518,7 @@ record_message (void *args)
>   int ret;
>   struct record_message_args *myargs = args;
>   struct gdbarch *gdbarch = get_regcache_arch (myargs->regcache);
> -  struct cleanup *old_cleanups = make_cleanup (record_message_cleanups, 0);
> +  struct cleanup *old_cleanups = make_cleanup (record_arch_list_cleanups,
> 0);
>
>   record_arch_list_head = NULL;
>   record_arch_list_tail = NULL;
> @@ -651,8 +658,8 @@ record_exec_insn (struct regcache *regca
>               {
>                 entry->u.mem.mem_entry_not_accessible = 1;
>                 if (record_debug)
> -                  warning (_("Process record: error reading memory at "
> -                             "addr = %s len = %d."),
> +                  warning ("Process record: error reading memory at "
> +                          "addr = %s len = %d.",
>                           paddress (gdbarch, entry->u.mem.addr),
>                            entry->u.mem.len);
>               }
> @@ -664,8 +671,8 @@ record_exec_insn (struct regcache *regca
>                   {
>                     entry->u.mem.mem_entry_not_accessible = 1;
>                     if (record_debug)
> -                      warning (_("Process record: error writing memory at "
> -                                 "addr = %s len = %d."),
> +                      warning ("Process record: error writing memory at "
> +                              "addr = %s len = %d.",
>                                paddress (gdbarch, entry->u.mem.addr),
>                                entry->u.mem.len);
>                   }
> @@ -678,6 +685,217 @@ record_exec_insn (struct regcache *regca
>     }
>  }
>
> +/* bfdcore_read -- read bytes from a core file section.  */
> +
> +static inline void
> +bfdcore_read (bfd *obfd, asection *osec, void *buf, int len, int *offset)
> +{
> +  int ret = bfd_get_section_contents (obfd, osec, buf, *offset, len);
> +
> +  if (ret)
> +    *offset += len;
> +  else
> +    error (_("Failed to read %d bytes from core file %s ('%s').\n"),
> +          len, bfd_get_filename (obfd),
> +          bfd_errmsg (bfd_get_error ()));
> +}
> +
> +static inline uint64_t
> +netorder64 (uint64_t fromfile)
> +{
> +  return (BYTE_ORDER == LITTLE_ENDIAN)
> +    ? bswap_64 (fromfile)
> +    : fromfile;
> +}
> +
> +static inline uint32_t
> +netorder32 (uint32_t fromfile)
> +{
> +  return (BYTE_ORDER == LITTLE_ENDIAN)
> +    ? bswap_32 (fromfile)
> +    : fromfile;
> +}
> +
> +static inline uint16_t
> +netorder16 (uint16_t fromfile)
> +{
> +  return (BYTE_ORDER == LITTLE_ENDIAN)
> +    ? bswap_16 (fromfile)
> +    : fromfile;
> +}
> +
> +/* Restore the execution log from a core_bfd file.  */
> +
> +static void
> +record_restore (void)
> +{
> +  uint32_t magic;
> +  struct cleanup *old_cleanups;
> +  struct record_entry *rec;
> +  asection *osec;
> +  uint32_t osec_size;
> +  int bfd_offset = 0;
> +  struct regcache *regcache;
> +
> +  /* We restore the execution log from the open core bfd,
> +     if there is one.  */
> +  if (core_bfd == NULL)
> +    return;
> +
> +  /* "record_restore" can only be called when record list is empty.  */
> +  gdb_assert (record_first.next == NULL);
> +
> +  if (record_debug)
> +    printf_filtered ("Restoring recording from core file.\n");
> +
> +  /* Now need to find our special note section.  */
> +  osec = bfd_get_section_by_name (core_bfd, "null0");
> +  osec_size = bfd_section_size (core_bfd, osec);
> +  if (record_debug)
> +    printf_filtered ("Find precord section %s.\n",
> +                    osec ? "succeeded" : "failed");
> +  if (!osec)
> +    return;
> +  if (record_debug)
> +    printf_filtered ("%s", bfd_section_name (core_bfd, osec));
> +
> +  /* Check the magic code.  */
> +  bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
> +  if (magic != RECORD_FILE_MAGIC)
> +    error (_("Version mis-match or file format error in core file %s."),
> +          bfd_get_filename (core_bfd));
> +  if (record_debug)
> +    printf_filtered ("\
> +  Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
> +                    netorder32 (magic));
> +
> +  /* Restore the entries in recfd into record_arch_list_head and
> +     record_arch_list_tail.  */
> +  record_arch_list_head = NULL;
> +  record_arch_list_tail = NULL;
> +  record_insn_num = 0;
> +  old_cleanups = make_cleanup (record_arch_list_cleanups, 0);
> +  regcache = get_current_regcache ();
> +
> +  while (1)
> +    {
> +      int ret;
> +      uint8_t tmpu8;
> +      uint32_t regnum, len, signal, count;
> +      uint64_t addr;
> +
> +      /* We are finished when offset reaches osec_size.  */
> +      if (bfd_offset >= osec_size)
> +       break;
> +      bfdcore_read (core_bfd, osec, &tmpu8, sizeof (tmpu8), &bfd_offset);
> +
> +      switch (tmpu8)
> +        {
> +        case record_reg: /* reg */
> +          /* Get register number to regnum.  */
> +          bfdcore_read (core_bfd, osec, &regnum,
> +                       sizeof (regnum), &bfd_offset);
> +         regnum = netorder32 (regnum);
> +
> +          rec = record_reg_alloc (regcache, regnum);
> +
> +          /* Get val.  */
> +          bfdcore_read (core_bfd, osec, record_get_loc (rec),
> +                       rec->u.reg.len, &bfd_offset);
> +
> +          if (record_debug)
> +            printf_filtered ("\
> +  Reading register %d (1 plus %d plus %d bytes)\n",
> +                            rec->u.reg.num,
> +                            sizeof (regnum),
> +                            rec->u.reg.len);
> +          break;
> +
> +        case record_mem: /* mem */
> +          /* Get len.  */
> +          bfdcore_read (core_bfd, osec, &len,
> +                       sizeof (len), &bfd_offset);
> +         len = netorder32 (len);
> +
> +          /* Get addr.  */
> +          bfdcore_read (core_bfd, osec, &addr,
> +                       sizeof (addr), &bfd_offset);
> +         addr = netorder64 (addr);
> +
> +          rec = record_mem_alloc (addr, len);
> +
> +          /* Get val.  */
> +          bfdcore_read (core_bfd, osec, record_get_loc (rec),
> +                       rec->u.mem.len, &bfd_offset);
> +
> +          if (record_debug)
> +            printf_filtered ("\
> +  Reading memory %s (1 plus %d plus %d plus %d bytes)\n",
> +                            paddress (get_current_arch (),
> +                                      rec->u.mem.addr),
> +                            sizeof (addr),
> +                            sizeof (len),
> +                            rec->u.mem.len);
> +          break;
> +
> +        case record_end: /* end */
> +          rec = record_end_alloc ();
> +          record_insn_num ++;
> +
> +         /* Get signal value.  */
> +         bfdcore_read (core_bfd, osec, &signal,
> +                       sizeof (signal), &bfd_offset);
> +         signal = netorder32 (signal);
> +         rec->u.end.sigval = signal;
> +
> +         /* Get insn count.  */
> +         bfdcore_read (core_bfd, osec, &count,
> +                       sizeof (count), &bfd_offset);
> +         count = netorder32 (count);
> +         rec->u.end.insn_num = count;
> +         record_insn_count = count + 1;
> +          if (record_debug)
> +            printf_filtered ("\
> +  Reading record_end (1 + %d + %d bytes), offset == %s\n",
> +                            sizeof (signal),
> +                            sizeof (count),
> +                            paddress (get_current_arch (),
> +                                      bfd_offset));
> +          break;
> +
> +        default:
> +          error (_("Bad entry type in core file %s."),
> +                bfd_get_filename (core_bfd));
> +          break;
> +        }
> +
> +      /* Add rec to record arch list.  */
> +      record_arch_list_add (rec);
> +    }
> +
> +  discard_cleanups (old_cleanups);
> +
> +  /* Add record_arch_list_head to the end of record list.  */
> +  record_first.next = record_arch_list_head;
> +  record_arch_list_head->prev = &record_first;
> +  record_arch_list_tail->next = NULL;
> +  record_list = &record_first;
> +
> +  /* Update record_insn_max_num.  */
> +  if (record_insn_num > record_insn_max_num)
> +    {
> +      record_insn_max_num = record_insn_num;
> +      warning (_("Auto increase record/replay buffer limit to %d."),
> +               record_insn_max_num);
> +    }
> +
> +  /* Succeeded.  */
> +  printf_filtered (_("Restored records from core file %s.\n"),
> +                  bfd_get_filename (core_bfd));
> +
> +  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
> +}
> +
>  static struct target_ops *tmp_to_resume_ops;
>  static void (*tmp_to_resume) (struct target_ops *, ptid_t, int,
>                              enum target_signal);
> @@ -728,6 +946,7 @@ record_core_open_1 (char *name, int from
>     }
>
>   push_target (&record_core_ops);
> +  record_restore ();
>  }
>
>  static void
> @@ -735,9 +954,6 @@ record_open_1 (char *name, int from_tty)
>  {
>   struct target_ops *t;
>
> -  if (record_debug)
> -    fprintf_unfiltered (gdb_stdlog, "Process record: record_open\n");
> -
>   /* check exec */
>   if (!target_has_execution)
>     error (_("Process record: the program is not being run."));
> @@ -779,6 +995,12 @@ record_open (char *name, int from_tty)
>     error (_("Process record target already running.  Use \"record stop\" to
> "
>              "stop record target first."));
>
> +  /* Reset */
> +  record_insn_num = 0;
> +  record_insn_count = 0;
> +  record_list = &record_first;
> +  record_list->next = NULL;
> +
>   /* Reset the tmp beneath pointers.  */
>   tmp_to_resume_ops = NULL;
>   tmp_to_resume = NULL;
> @@ -822,17 +1044,6 @@ record_open (char *name, int from_tty)
>   if (!tmp_to_xfer_partial)
>     error (_("Could not find 'to_xfer_partial' method on the target
> stack."));
>
> -  if (current_target.to_stratum == core_stratum)
> -    record_core_open_1 (name, from_tty);
> -  else
> -    record_open_1 (name, from_tty);
> -
> -  /* Reset */
> -  record_insn_num = 0;
> -  record_insn_count = 0;
> -  record_list = &record_first;
> -  record_list->next = NULL;
> -
>   /* Set the tmp beneath pointers to beneath pointers.  */
>   record_beneath_to_resume_ops = tmp_to_resume_ops;
>   record_beneath_to_resume = tmp_to_resume;
> @@ -844,6 +1055,11 @@ record_open (char *name, int from_tty)
>   record_beneath_to_xfer_partial = tmp_to_xfer_partial;
>   record_beneath_to_insert_breakpoint = tmp_to_insert_breakpoint;
>   record_beneath_to_remove_breakpoint = tmp_to_remove_breakpoint;
> +
> +  if (current_target.to_stratum == core_stratum)
> +    record_core_open_1 (name, from_tty);
> +  else
> +    record_open_1 (name, from_tty);
>  }
>
>  static void
> @@ -1114,8 +1330,7 @@ record_wait (struct target_ops *ops,
>                    {
>                      if (record_debug)
>                        fprintf_unfiltered (gdb_stdlog,
> -                                           "Process record: break "
> -                                           "at %s.\n",
> +                                           "Process record: break at
> %s.\n",
>                                            paddress (gdbarch, tmp_pc));
>                      if (gdbarch_decr_pc_after_break (gdbarch)
>                          && execution_direction == EXEC_FORWARD
> @@ -1190,8 +1405,7 @@ static void
>  record_mourn_inferior (struct target_ops *ops)
>  {
>   if (record_debug)
> -    fprintf_unfiltered (gdb_stdlog, "Process record: "
> -                                   "record_mourn_inferior\n");
> +    fprintf_unfiltered (gdb_stdlog, "Process record:
> record_mourn_inferior\n");
>
>   unpush_target (&record_ops);
>   target_mourn_inferior ();
> @@ -1345,8 +1559,8 @@ record_xfer_partial (struct target_ops *
>          record_list_release (record_arch_list_tail);
>          if (record_debug)
>            fprintf_unfiltered (gdb_stdlog,
> -                               _("Process record: failed to record "
> -                                 "execution log."));
> +                               "Process record: failed to record "
> +                               "execution log.");
>          return -1;
>        }
>       if (record_arch_list_add_end ())
> @@ -1354,8 +1568,8 @@ record_xfer_partial (struct target_ops *
>          record_list_release (record_arch_list_tail);
>          if (record_debug)
>            fprintf_unfiltered (gdb_stdlog,
> -                               _("Process record: failed to record "
> -                                 "execution log."));
> +                               "Process record: failed to record "
> +                               "execution log.");
>          return -1;
>        }
>       record_list->next = record_arch_list_head;
> @@ -1642,6 +1856,298 @@ cmd_record_start (char *args, int from_t
>   execute_command ("target record", from_tty);
>  }
>
> +/* Record log save-file format
> +   Version 1 (never released)
> +
> +   Header:
> +     4 bytes: magic number htonl(0x20090829).
> +       NOTE: be sure to change whenever this file format changes!
> +
> +   Records:
> +     record_end:
> +       1 byte:  record type (record_end, see enum record_type).
> +     record_reg:
> +       1 byte:  record type (record_reg, see enum record_type).
> +       8 bytes: register id (network byte order).
> +       MAX_REGISTER_SIZE bytes: register value.
> +     record_mem:
> +       1 byte:  record type (record_mem, see enum record_type).
> +       8 bytes: memory length (network byte order).
> +       8 bytes: memory address (network byte order).
> +       n bytes: memory value (n == memory length).
> +
> +   Version 2
> +     4 bytes: magic number netorder32(0x20091016).
> +       NOTE: be sure to change whenever this file format changes!
> +
> +   Records:
> +     record_end:
> +       1 byte:  record type (record_end, see enum record_type).
> +       4 bytes: signal
> +       4 bytes: instruction count
> +     record_reg:
> +       1 byte:  record type (record_reg, see enum record_type).
> +       4 bytes: register id (network byte order).
> +       n bytes: register value (n == actual register size).
> +                (eg. 4 bytes for x86 general registers).
> +     record_mem:
> +       1 byte:  record type (record_mem, see enum record_type).
> +       4 bytes: memory length (network byte order).
> +       8 bytes: memory address (network byte order).
> +       n bytes: memory value (n == memory length).
> +
> +*/
> +
> +/* bfdcore_write -- write bytes into a core file section.  */
> +
> +static inline void
> +bfdcore_write (bfd *obfd, asection *osec, void *buf, int len, int *offset)
> +{
> +  int ret = bfd_set_section_contents (obfd, osec, buf, *offset, len);
> +
> +  if (ret)
> +    *offset += len;
> +  else
> +    error (_("Failed to write %d bytes to core file %s ('%s').\n"),
> +          len, bfd_get_filename (obfd),
> +          bfd_errmsg (bfd_get_error ()));
> +}
> +
> +/* Restore the execution log from a file.  We use a modified elf
> +   corefile format, with an extra section for our data.  */
> +
> +static void
> +cmd_record_restore (char *args, int from_tty)
> +{
> +  core_file_command (args, from_tty);
> +  record_open (args, from_tty);
> +}
> +
> +static void
> +record_save_cleanups (void *data)
> +{
> +  bfd *obfd = data;
> +  char *pathname = xstrdup (bfd_get_filename (obfd));
> +  bfd_close (obfd);
> +  unlink (pathname);
> +  xfree (pathname);
> +}
> +
> +/* Save the execution log to a file.  We use a modified elf corefile
> +   format, with an extra section for our data.  */
> +
> +static void
> +cmd_record_save (char *args, int from_tty)
> +{
> +  char *recfilename, recfilename_buffer[40];
> +  int recfd;
> +  struct record_entry *cur_record_list;
> +  uint32_t magic;
> +  struct regcache *regcache;
> +  struct gdbarch *gdbarch;
> +  struct cleanup *old_cleanups;
> +  struct cleanup *set_cleanups;
> +  bfd *obfd;
> +  int save_size = 0;
> +  asection *osec = NULL;
> +  int bfd_offset = 0;
> +
> +  if (strcmp (current_target.to_shortname, "record") != 0)
> +    error (_("This command can only be used with target 'record'.\n"
> +            "Use 'target record' first.\n"));
> +
> +  if (args && *args)
> +    recfilename = args;
> +  else
> +    {
> +      /* Default recfile name is "gdb_record.PID".  */
> +      snprintf (recfilename_buffer, sizeof (recfilename_buffer),
> +                "gdb_record.%d", PIDGET (inferior_ptid));
> +      recfilename = recfilename_buffer;
> +    }
> +
> +  /* Open the save file.  */
> +  if (record_debug)
> +    printf_filtered ("Saving execution log to core file '%s'\n",
> recfilename);
> +
> +  /* Open the output file.  */
> +  obfd = create_gcore_bfd (recfilename);
> +  old_cleanups = make_cleanup (record_save_cleanups, obfd);
> +
> +  /* Save the current record entry to "cur_record_list".  */
> +  cur_record_list = record_list;
> +
> +  /* Get the values of regcache and gdbarch.  */
> +  regcache = get_current_regcache ();
> +  gdbarch = get_regcache_arch (regcache);
> +
> +  /* Disable the GDB operation record.  */
> +  set_cleanups = record_gdb_operation_disable_set ();
> +
> +  /* Reverse execute to the begin of record list.  */
> +  while (1)
> +    {
> +      /* Check for beginning and end of log.  */
> +      if (record_list == &record_first)
> +        break;
> +
> +      record_exec_insn (regcache, gdbarch, record_list);
> +
> +      if (record_list->prev)
> +        record_list = record_list->prev;
> +    }
> +
> +  /* Compute the size needed for the extra bfd section.  */
> +  save_size = 4;       /* magic cookie */
> +  for (record_list = record_first.next; record_list;
> +       record_list = record_list->next)
> +    switch (record_list->type)
> +      {
> +      case record_end:
> +       save_size += 1 + 4 + 4;
> +       break;
> +      case record_reg:
> +       save_size += 1 + 4 + record_list->u.reg.len;
> +       break;
> +      case record_mem:
> +       save_size += 1 + 4 + 8 + record_list->u.mem.len;
> +       break;
> +      }
> +
> +  /* Make the new bfd section.  */
> +  osec = bfd_make_section_anyway_with_flags (obfd, "precord",
> +                                             SEC_HAS_CONTENTS
> +                                             | SEC_READONLY);
> +  if (osec == NULL)
> +    error (_("Failed to create 'precord' section for corefile %s: %s"),
> +          recfilename,
> +           bfd_errmsg (bfd_get_error ()));
> +  bfd_set_section_size (obfd, osec, save_size);
> +  bfd_set_section_vma (obfd, osec, 0);
> +  bfd_set_section_alignment (obfd, osec, 0);
> +  bfd_section_lma (obfd, osec) = 0;
> +
> +  /* Save corefile state.  */
> +  write_gcore_file (obfd);
> +
> +  /* Write out the record log.  */
> +  /* Write the magic code.  */
> +  magic = RECORD_FILE_MAGIC;
> +  if (record_debug)
> +    printf_filtered ("\
> +  Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
> +                    magic);
> +  bfdcore_write (obfd, osec, &magic, sizeof (magic), &bfd_offset);
> +
> +  /* Save the entries to recfd and forward execute to the end of
> +     record list.  */
> +  record_list = &record_first;
> +  while (1)
> +    {
> +      /* Save entry.  */
> +      if (record_list != &record_first)
> +        {
> +         uint8_t type;
> +         uint32_t regnum, len, signal, count;
> +          uint64_t addr;
> +
> +         type = record_list->type;
> +          bfdcore_write (obfd, osec, &type, sizeof (type), &bfd_offset);
> +
> +          switch (record_list->type)
> +            {
> +            case record_reg: /* reg */
> +              if (record_debug)
> +               printf_filtered ("\
> +  Writing register %d (1 plus %d plus %d bytes)\n",
> +                                record_list->u.reg.num,
> +                                sizeof (regnum),
> +                                record_list->u.reg.len);
> +
> +              /* Write regnum.  */
> +              regnum = netorder32 (record_list->u.reg.num);
> +              bfdcore_write (obfd, osec, &regnum,
> +                            sizeof (regnum), &bfd_offset);
> +
> +              /* Write regval.  */
> +              bfdcore_write (obfd, osec, record_get_loc (record_list),
> +                            record_list->u.reg.len, &bfd_offset);
> +              break;
> +
> +            case record_mem: /* mem */
> +             if (record_debug)
> +               printf_filtered ("\
> +  Writing memory %s (1 plus %d plus %d plus %d bytes)\n",
> +                                paddress (gdbarch,
> +                                          record_list->u.mem.addr),
> +                                sizeof (addr),
> +                                sizeof (len),
> +                                record_list->u.mem.len);
> +
> +             /* Write memlen.  */
> +             len = netorder32 (record_list->u.mem.len);
> +             bfdcore_write (obfd, osec, &len, sizeof (len), &bfd_offset);
> +
> +             /* Write memaddr.  */
> +             addr = netorder64 (record_list->u.mem.addr);
> +             bfdcore_write (obfd, osec, &addr,
> +                            sizeof (addr), &bfd_offset);
> +
> +             /* Write memval.  */
> +             bfdcore_write (obfd, osec, record_get_loc (record_list),
> +                            record_list->u.mem.len, &bfd_offset);
> +              break;
> +
> +              case record_end:
> +                if (record_debug)
> +                  printf_filtered ("\
> +  Writing record_end (1 + %d + %d bytes)\n",
> +                                  sizeof (signal),
> +                                  sizeof (count));
> +               /* Write signal value.  */
> +               signal = netorder32 (record_list->u.end.sigval);
> +               bfdcore_write (obfd, osec, &signal,
> +                              sizeof (signal), &bfd_offset);
> +
> +               /* Write insn count.  */
> +               count = netorder32 (record_list->u.end.insn_num);
> +               bfdcore_write (obfd, osec, &count,
> +                              sizeof (count), &bfd_offset);
> +                break;
> +            }
> +        }
> +
> +      /* Execute entry.  */
> +      record_exec_insn (regcache, gdbarch, record_list);
> +
> +      if (record_list->next)
> +        record_list = record_list->next;
> +      else
> +        break;
> +    }
> +
> +  /* Reverse execute to cur_record_list.  */
> +  while (1)
> +    {
> +      /* Check for beginning and end of log.  */
> +      if (record_list == cur_record_list)
> +        break;
> +
> +      record_exec_insn (regcache, gdbarch, record_list);
> +
> +      if (record_list->prev)
> +        record_list = record_list->prev;
> +    }
> +
> +  do_cleanups (set_cleanups);
> +  bfd_close (obfd);
> +  discard_cleanups (old_cleanups);
> +
> +  /* Succeeded.  */
> +  printf_filtered (_("Saved core file %s with execution log.\n"),
> +                  recfilename);
> +}
> +
>  /* Truncate the record log from the present point
>    of replay until the end.  */
>
> @@ -1749,6 +2255,8 @@ info_record_command (char *args, int fro
>  void
>  _initialize_record (void)
>  {
> +  struct cmd_list_element *c;
> +
>   /* Init record_first.  */
>   record_first.prev = NULL;
>   record_first.next = NULL;
> @@ -1767,10 +2275,12 @@ _initialize_record (void)
>                            NULL, show_record_debug, &setdebuglist,
>                            &showdebuglist);
>
> -  add_prefix_cmd ("record", class_obscure, cmd_record_start,
> +  c = add_prefix_cmd ("record", class_obscure, cmd_record_start,
>                  _("Abbreviated form of \"target record\" command."),
>                  &record_cmdlist, "record ", 0, &cmdlist);
> +  set_cmd_completer (c, filename_completer);
>   add_com_alias ("rec", "record", class_obscure, 1);
> +
>   add_prefix_cmd ("record", class_support, set_record_command,
>                  _("Set record options"), &set_record_cmdlist,
>                  "set record ", 0, &setlist);
> @@ -1784,6 +2294,17 @@ _initialize_record (void)
>                  "info record ", 0, &infolist);
>   add_alias_cmd ("rec", "record", class_obscure, 1, &infolist);
>
> +  c = add_cmd ("save", class_obscure, cmd_record_save,
> +              _("Save the execution log to a file.\n\
> +Argument is optional filename.\n\
> +Default filename is 'gdb_record.<process_id>'."),
> +              &record_cmdlist);
> +
> +  c = add_cmd ("restore", class_obscure, cmd_record_restore,
> +              _("Restore the execution log from a file.\n\
> +Argument is filename.  File must be created with 'record save'."),
> +              &record_cmdlist);
> +  set_cmd_completer (c, filename_completer);
>
>   add_cmd ("delete", class_obscure, cmd_record_delete,
>           _("Delete the rest of execution log and start recording it
> anew."),
>
>

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

Hui Zhu wrote:
> On Tue, Oct 20, 2009 at 01:54, Michael Snyder <msnyder@vmware.com> wrote:
>> Hui Zhu wrote:
>>> Hi Michael,
>>>
>>> +  do_cleanups (old_cleanups);
>>>
>>> This line will remove the record file that we just save.
>>>
>>> I change some code:
>>> static void
>>> record_save_cleanups (void *data)
>>> {
>>>  bfd *obfd = data;
>>>  //char *pathname = xstrdup (bfd_get_filename (obfd));
>>>  bfd_close (obfd);
>>>  //unlink (pathname);
>>>  //xfree (pathname);
>>> }
>>>
>>> I think you want unlink the gdb_record when save get some error.  It
>>> maybe need "discard_cleanups" the old_cleanups and bfd_close (obfd);
>>>
>>> After change the code, everything is OK.
>> Yes.  Thanks.  Like this:
>> +      if (record_list->prev)
>> +        record_list = record_list->prev;
>> +    }
>> +
>> +  do_cleanups (set_cleanups);
>> +  bfd_close (obfd);
>> +  discard_cleanups (old_cleanups);
>> +
>> +  /* Succeeded.  */
> 
> 
> I suggest:
> +  discard_cleanups (old_cleanups);
> +  bfd_close (obfd);

The reason I did the bfd_close first is because I wasn't
sure if it was safe to delete the file first.  In any way,
it seems more logical to close the file before delete it.

What do you think?

> BTW, the record.c update by Pedro,
> http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/record.c.diff?r1=1.24&r2=1.25&cvsroot=src
> 
> Maybe the record save patch need some update.

Hmmm.  Yes, I'm sure it will.   ;-)

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

Michael Snyder wrote:
> Hui Zhu wrote:
>> On Tue, Oct 20, 2009 at 01:54, Michael Snyder <msnyder@vmware.com> wrote:
>>> Hui Zhu wrote:
>>>> Hi Michael,
>>>>
>>>> I think you want unlink the gdb_record when save get some error.  It
>>>> maybe need "discard_cleanups" the old_cleanups and bfd_close (obfd);
>>>>
>>>> After change the code, everything is OK.
>>> Yes.  Thanks.  Like this:
>>> +      if (record_list->prev)
>>> +        record_list = record_list->prev;
>>> +    }
>>> +
>>> +  do_cleanups (set_cleanups);
>>> +  bfd_close (obfd);
>>> +  discard_cleanups (old_cleanups);
>>> +
>>> +  /* Succeeded.  */
>>
>> I suggest:
>> +  discard_cleanups (old_cleanups);
>> +  bfd_close (obfd);
> 
> The reason I did the bfd_close first is because I wasn't
> sure if it was safe to delete the file first.  In any way,
> it seems more logical to close the file before delete it.

Oh, oops, never mind.  My head's fuzzy today.

Why do you suggest to change the order?
Just curious, I don't have an issue with it...

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Hui Zhu]

On Wed, Oct 21, 2009 at 03:58, Michael Snyder <msnyder@vmware.com> wrote:
> Michael Snyder wrote:
>>
>> Hui Zhu wrote:
>>>
>>> On Tue, Oct 20, 2009 at 01:54, Michael Snyder <msnyder@vmware.com> wrote:
>>>>
>>>> Hui Zhu wrote:
>>>>>
>>>>> Hi Michael,
>>>>>
>>>>> I think you want unlink the gdb_record when save get some error.  It
>>>>> maybe need "discard_cleanups" the old_cleanups and bfd_close (obfd);
>>>>>
>>>>> After change the code, everything is OK.
>>>>
>>>> Yes.  Thanks.  Like this:
>>>> +      if (record_list->prev)
>>>> +        record_list = record_list->prev;
>>>> +    }
>>>> +
>>>> +  do_cleanups (set_cleanups);
>>>> +  bfd_close (obfd);
>>>> +  discard_cleanups (old_cleanups);
>>>> +
>>>> +  /* Succeeded.  */
>>>
>>> I suggest:
>>> +  discard_cleanups (old_cleanups);
>>> +  bfd_close (obfd);
>>
>> The reason I did the bfd_close first is because I wasn't
>> sure if it was safe to delete the file first.  In any way,
>> it seems more logical to close the file before delete it.
>
> Oh, oops, never mind.  My head's fuzzy today.
>
> Why do you suggest to change the order?
> Just curious, I don't have an issue with it...
>

Sorry I didn't talk the function very clear.

>>>> +  bfd_close (obfd);
>>>> +  discard_cleanups (old_cleanups);
Before "discard_cleanups (old_cleanups);", the "record_save_cleanups"
will be call sometime.  "record_save_cleanups" will call "bfd_close
(obfd)".
If the record_save_cleanups will happen after "bfd_close (obfd)".
"bfd_close (obfd)" will be call twice.

Of curse, most of time it will not happen.   So I just suggest.  :)


Thanks,
Hui

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

Hui Zhu wrote:
> On Wed, Oct 21, 2009 at 03:58, Michael Snyder <msnyder@vmware.com> wrote:
>> Michael Snyder wrote:
>>> Hui Zhu wrote:
>>>> On Tue, Oct 20, 2009 at 01:54, Michael Snyder <msnyder@vmware.com> wrote:
>>>>> Hui Zhu wrote:
>>>>>> Hi Michael,
>>>>>>
>>>>>> I think you want unlink the gdb_record when save get some error.  It
>>>>>> maybe need "discard_cleanups" the old_cleanups and bfd_close (obfd);
>>>>>>
>>>>>> After change the code, everything is OK.
>>>>> Yes.  Thanks.  Like this:
>>>>> +      if (record_list->prev)
>>>>> +        record_list = record_list->prev;
>>>>> +    }
>>>>> +
>>>>> +  do_cleanups (set_cleanups);
>>>>> +  bfd_close (obfd);
>>>>> +  discard_cleanups (old_cleanups);
>>>>> +
>>>>> +  /* Succeeded.  */
>>>> I suggest:
>>>> +  discard_cleanups (old_cleanups);
>>>> +  bfd_close (obfd);
>>> The reason I did the bfd_close first is because I wasn't
>>> sure if it was safe to delete the file first.  In any way,
>>> it seems more logical to close the file before delete it.
>> Oh, oops, never mind.  My head's fuzzy today.
>>
>> Why do you suggest to change the order?
>> Just curious, I don't have an issue with it...
>>
> 
> Sorry I didn't talk the function very clear.
> 
>>>>> +  bfd_close (obfd);
>>>>> +  discard_cleanups (old_cleanups);
> Before "discard_cleanups (old_cleanups);", the "record_save_cleanups"
> will be call sometime.  "record_save_cleanups" will call "bfd_close
> (obfd)".
> If the record_save_cleanups will happen after "bfd_close (obfd)".
> "bfd_close (obfd)" will be call twice.
> 
> Of curse, most of time it will not happen.   So I just suggest.  :)

OK, done and committed.

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Paul Pluzhnikov]

On Thu, Oct 22, 2009 at 12:35 PM, Michael Snyder <msnyder@vmware.com> wrote:

> OK, done and committed.

The ChangeLog entries are now out of sequence WRT dates:

2009-10-22  Paul Pluzhnikov  <ppluzhnikov@google.com>
...
2009-10-16  Hui Zhu  <teawater@gmail.com>
            Michael Snyder  <msnyder@msnyder-server.eng.vmware.com>
...
2009-10-17  Michael Snyder  <msnyder@vmware.com>
...
2009-10-22  Hui Zhu  <teawater@gmail.com>
            Michael Snyder  <msnyder@vmware.com>

The entry should reflect the date patch is committed, right?

Thanks,
-- 
Paul Pluzhnikov

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

Paul Pluzhnikov wrote:
> On Thu, Oct 22, 2009 at 12:35 PM, Michael Snyder <msnyder@vmware.com> wrote:
> 
>> OK, done and committed.
> 
> The ChangeLog entries are now out of sequence WRT dates:
> 
> 2009-10-22  Paul Pluzhnikov  <ppluzhnikov@google.com>
> ...
> 2009-10-16  Hui Zhu  <teawater@gmail.com>
>             Michael Snyder  <msnyder@msnyder-server.eng.vmware.com>
> ...
> 2009-10-17  Michael Snyder  <msnyder@vmware.com>
> ...
> 2009-10-22  Hui Zhu  <teawater@gmail.com>
>             Michael Snyder  <msnyder@vmware.com>
> 
> The entry should reflect the date patch is committed, right?

Sorry, fixed.

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Paul Pluzhnikov]

On Thu, Oct 22, 2009 at 1:54 PM, Michael Snyder <msnyder@vmware.com> wrote:

> Sorry, fixed.

As of Thu Oct 22 14:22:55 PDT 2009, I didn't see your fix, so I just
committed this:

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.10990
retrieving revision 1.10991
diff -u -u -r1.10990 -r1.10991
--- ChangeLog   22 Oct 2009 20:20:27 -0000      1.10990
+++ ChangeLog   22 Oct 2009 21:22:47 -0000      1.10991
@@ -5,7 +5,7 @@
        * objfiles.c (find_pc_section): Likewise.
        (update_section_map): Don't allocate empty table.

-2009-10-16  Hui Zhu  <teawater@gmail.com>
+2009-10-22  Hui Zhu  <teawater@gmail.com>
            Michael Snyder  <msnyder@msnyder-server.eng.vmware.com>

        * record.c (RECORD_FILE_MAGIC): New constant.
@@ -20,7 +20,7 @@
        (cmd_record_save): New function.  Save a record log to a file.
        (_initialize_record): Set up commands for save and restore.

-2009-10-17  Michael Snyder  <msnyder@vmware.com>
+2009-10-22  Michael Snyder  <msnyder@vmware.com>

        * gcore.h: New file.
        * gcore.c (create_gcore_bfd): New function.


-- 
Paul Pluzhnikov

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Paul Pluzhnikov]

On Thu, Oct 22, 2009 at 2:25 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Thu, Oct 22, 2009 at 1:54 PM, Michael Snyder <msnyder@vmware.com> wrote:
>
>> Sorry, fixed.

More problems: on Linux/x86_64:

gcc -g -O2   -I. -I../../src/gdb -I../../src/gdb/common
-I../../src/gdb/config -DLOCALEDIR="\"/usr/share/locale\""
-DHAVE_CONFIG_H -I../../src/gdb/../include/opcode
-I../../src/gdb/../readline/.. -I../bfd -I../../src/gdb/../bfd
-I../../src/gdb/../include -I../libdecnumber
-I../../src/gdb/../libdecnumber  -I../../src/gdb/gnulib -Ignulib
-DMI_OUT=1 -DTUI=1  -I/usr/grte/v1/k8-linux/include -Wall
-Wdeclaration-after-statement -Wpointer-arith -Wformat-nonliteral
-Wno-pointer-sign -Wno-unused -Wunused-value -Wno-switch
-Wno-char-subscripts -Werror -c -o record.o -MT record.o -MMD -MP -MF
.deps/record.Tpo ../../src/gdb/record.c
cc1: warnings being treated as errors
../../src/gdb/record.c: In function 'record_restore':
../../src/gdb/record.c:2062: warning: format '%d' expects type 'int',
but argument 3 has type 'long unsigned int'
../../src/gdb/record.c:2089: warning: format '%d' expects type 'int',
but argument 3 has type 'long unsigned int'
../../src/gdb/record.c:2089: warning: format '%d' expects type 'int',
but argument 4 has type 'long unsigned int'
../../src/gdb/record.c:2114: warning: format '%d' expects type 'int',
but argument 2 has type 'long unsigned int'
../../src/gdb/record.c:2114: warning: format '%d' expects type 'int',
but argument 3 has type 'long unsigned int'
../../src/gdb/record.c: In function 'cmd_record_save':
../../src/gdb/record.c:2314: warning: format '%d' expects type 'int',
but argument 3 has type 'long unsigned int'
../../src/gdb/record.c:2334: warning: format '%d' expects type 'int',
but argument 3 has type 'long unsigned int'
../../src/gdb/record.c:2334: warning: format '%d' expects type 'int',
but argument 4 has type 'long unsigned int'
../../src/gdb/record.c:2355: warning: format '%d' expects type 'int',
but argument 2 has type 'long unsigned int'
../../src/gdb/record.c:2355: warning: format '%d' expects type 'int',
but argument 3 has type 'long unsigned int'
make: *** [record.o] Error 1




-- 
Paul Pluzhnikov

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Paul Pluzhnikov]

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

On Thu, Oct 22, 2009 at 5:45 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> More problems: on Linux/x86_64:
>

> ../../src/gdb/record.c:2062: warning: format '%d' expects type 'int',
> but argument 3 has type 'long unsigned int'

I've committed attached patch as obvious.

Thanks,
-- 
Paul Pluzhnikov

2009-10-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* record.c (record_restore, cmd_record_save): Fix warnings.

[-- Attachment #2: gdb-fixup-20091022.txt --]
[-- Type: text/plain, Size: 3024 bytes --]

Index: record.c
===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.30
diff -u -p -u -r1.30 record.c
--- record.c	22 Oct 2009 19:36:06 -0000	1.30
+++ record.c	23 Oct 2009 00:58:23 -0000
@@ -2056,9 +2056,9 @@ record_restore (void)
 
           if (record_debug)
             printf_filtered ("\
-  Reading register %d (1 plus %d plus %d bytes)\n",
+  Reading register %d (1 plus %lu plus %d bytes)\n",
 			     rec->u.reg.num,
-			     sizeof (regnum),
+			     (unsigned long) sizeof (regnum),
 			     rec->u.reg.len);
           break;
 
@@ -2081,11 +2081,11 @@ record_restore (void)
 
           if (record_debug)
             printf_filtered ("\
-  Reading memory %s (1 plus %d plus %d plus %d bytes)\n",
+  Reading memory %s (1 plus %lu plus %lu plus %d bytes)\n",
 			     paddress (get_current_arch (),
 				       rec->u.mem.addr),
-			     sizeof (addr),
-			     sizeof (len),
+			     (unsigned long) sizeof (addr),
+			     (unsigned long) sizeof (len),
 			     rec->u.mem.len);
           break;
 
@@ -2107,9 +2107,9 @@ record_restore (void)
 	  record_insn_count = count + 1;
           if (record_debug)
             printf_filtered ("\
-  Reading record_end (1 + %d + %d bytes), offset == %s\n",
-			     sizeof (signal),
-			     sizeof (count),
+  Reading record_end (1 + %lu + %lu bytes), offset == %s\n",
+			     (unsigned long) sizeof (signal),
+			     (unsigned long) sizeof (count),
 			     paddress (get_current_arch (),
 				       bfd_offset));
           break;
@@ -2308,9 +2308,9 @@ cmd_record_save (char *args, int from_tt
             case record_reg: /* reg */
               if (record_debug)
 		printf_filtered ("\
-  Writing register %d (1 plus %d plus %d bytes)\n",
+  Writing register %d (1 plus %lu plus %d bytes)\n",
 				 record_list->u.reg.num,
-				 sizeof (regnum),
+				 (unsigned long) sizeof (regnum),
 				 record_list->u.reg.len);
 
               /* Write regnum.  */
@@ -2326,11 +2326,11 @@ cmd_record_save (char *args, int from_tt
             case record_mem: /* mem */
 	      if (record_debug)
 		printf_filtered ("\
-  Writing memory %s (1 plus %d plus %d plus %d bytes)\n",
+  Writing memory %s (1 plus %lu plus %lu plus %d bytes)\n",
 				 paddress (gdbarch,
 					   record_list->u.mem.addr),
-				 sizeof (addr),
-				 sizeof (len),
+				 (unsigned long) sizeof (addr),
+				 (unsigned long) sizeof (len),
 				 record_list->u.mem.len);
 
 	      /* Write memlen.  */
@@ -2350,9 +2350,9 @@ cmd_record_save (char *args, int from_tt
               case record_end:
                 if (record_debug)
                   printf_filtered ("\
-  Writing record_end (1 + %d + %d bytes)\n", 
-				   sizeof (signal),
-				   sizeof (count));
+  Writing record_end (1 + %lu + %lu bytes)\n", 
+				   (unsigned long) sizeof (signal),
+				   (unsigned long) sizeof (count));
 		/* Write signal value.  */
 		signal = netorder32 (record_list->u.end.sigval);
 		bfdcore_write (obfd, osec, &signal,

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Hui Zhu]

Thanks Paul.

Hui

On Fri, Oct 23, 2009 at 09:03, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Thu, Oct 22, 2009 at 5:45 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
>> More problems: on Linux/x86_64:
>>
>
>> ../../src/gdb/record.c:2062: warning: format '%d' expects type 'int',
>> but argument 3 has type 'long unsigned int'
>
> I've committed attached patch as obvious.
>
> Thanks,
> --
> Paul Pluzhnikov
>
> 2009-10-22  Paul Pluzhnikov  <ppluzhnikov@google.com>
>
>        * record.c (record_restore, cmd_record_save): Fix warnings.
>

RE: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Pierre Muller]

  I still have problems to compile
on Cygwin:

gcc -g -O2   -I. -I../../src/gdb -I../../src/gdb/common
-I../../src/gdb/config -
DLOCALEDIR="\"/usr/local/share/locale\"" -DHAVE_CONFIG_H
-I../../src/gdb/../incl
ude/opcode -I../../src/gdb/../readline/.. -I../bfd -I../../src/gdb/../bfd
-I../.
./src/gdb/../include -I../libdecnumber -I../../src/gdb/../libdecnumber
-I../../
src/gdb/gnulib -Ignulib  -DMI_OUT=1 -DTUI=1   -Wall
-Wdeclaration-after-statemen
t -Wpointer-arith -Wformat-nonliteral -Wno-unused -Wunused-value -Wno-switch
-Wn
o-char-subscripts -Werror -c -o record.o -MT record.o -MMD -MP -MF
.deps/record.
Tpo ../../src/gdb/record.c
../../src/gdb/record.c: In function `record_restore':
../../src/gdb/record.c:2021: warning: unsigned int format, uint32_t arg (arg
2)
../../src/gdb/record.c: In function `cmd_record_save':
../../src/gdb/record.c:2288: warning: unsigned int format, uint32_t arg (arg
2)
make[1]: *** [record.o] Error 1


 Furthermore, there is a '%ll' rule
in ARI that states that:
Do not use printf("%ll"), instead use printf("%s",phex()) to dump a `long
long' value
Shouldn't this also concern '%lu'?

Do we really need long for sizeof function returns?
Are there any types (use in record.c) for which 
sizeof would not fit into a regular "unsigned int" ?

Pierre Muller
as ARI "maintainer"...

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Paul Pluzhnikov
> Envoyé : Friday, October 23, 2009 3:03 AM
> À : Michael Snyder
> Cc : Hui Zhu; Eli Zaretskii; gdb-patches@sourceware.org
> Objet : Re: [RFA, 3 of 3] save/restore process record, part 3
> (save/restore)
> 
> On Thu, Oct 22, 2009 at 5:45 PM, Paul Pluzhnikov
> <ppluzhnikov@google.com> wrote:
> 
> > More problems: on Linux/x86_64:
> >
> 
> > ../../src/gdb/record.c:2062: warning: format '%d' expects type 'int',
> > but argument 3 has type 'long unsigned int'
> 
> I've committed attached patch as obvious.
> 
> Thanks,
> --
> Paul Pluzhnikov
> 
> 2009-10-22  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	* record.c (record_restore, cmd_record_save): Fix warnings.

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Eli Zaretskii]

> From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> Cc: "'Hui Zhu'" <teawater@gmail.com>, "'Eli Zaretskii'" <eliz@gnu.org>,
>         <gdb-patches@sourceware.org>
> Date: Fri, 23 Oct 2009 10:52:27 +0200
> 
>  Furthermore, there is a '%ll' rule
> in ARI that states that:
> Do not use printf("%ll"), instead use printf("%s",phex()) to dump a `long
> long' value
> Shouldn't this also concern '%lu'?

There is a difference: %lu is defined by C89, while %ll is only
codified by C9X.  GDB does not yet require a C9X compiler.

> Do we really need long for sizeof function returns?
> Are there any types (use in record.c) for which 
> sizeof would not fit into a regular "unsigned int" ?

sizeof returns a value of the type `size_t'.  On a 64-bit host, size_t
is typically a 64-bit data type, and so is `unsigned long'.  (64-bit
Windows is an exception, because it uses a different programming
model.)

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Hui Zhu]

Thanks guys,

I changed the this 0x%08x to phex_nz.
Committed following patch as obvious.

Thanks,
Hui

===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.10995
retrieving revision 1.10996
diff -u -r1.10995 -r1.10996
--- src/gdb/ChangeLog	2009/10/23 14:31:33	1.10995
+++ src/gdb/ChangeLog	2009/10/23 14:35:29	1.10996
@@ -1,3 +1,7 @@
+2009-10-23  Hui Zhu  <teawater@gmail.com>
+
+	* record.c (record_restore): Use phex_nz.
+
 2009-10-23  Tristan Gingold  <gingold@adacore.com>

 	* frame.c (frame_unwind_pc): Fix typo: remove duplicate 0x.


===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.31
retrieving revision 1.32
diff -u -r1.31 -r1.32
--- src/gdb/record.c	2009/10/23 01:00:35	1.31
+++ src/gdb/record.c	2009/10/23 14:35:30	1.32
@@ -2017,8 +2017,8 @@
 	   bfd_get_filename (core_bfd));
   if (record_debug)
     printf_filtered ("\
-  Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
-		     netorder32 (magic));
+  Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%s)\n",
+		     phex_nz (netorder32 (magic), 4));

   /* Restore the entries in recfd into record_arch_list_head and
      record_arch_list_tail.  */
@@ -2284,8 +2284,8 @@
   magic = RECORD_FILE_MAGIC;
   if (record_debug)
     printf_filtered ("\
-  Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x%08x)\n",
-		     magic);
+  Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x%s)\n",
+		     phex_nz (magic, 4));
   bfdcore_write (obfd, osec, &magic, sizeof (magic), &bfd_offset);

   /* Save the entries to recfd and forward execute to the end of

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Pedro Alves]

On Friday 23 October 2009 15:42:00, Hui Zhu wrote:
>    if (record_debug)
>      printf_filtered ("\

All debug output should go to fprintf_unfiltered (gdb_stdlog, ...
not printf_filtered.

-- 
Pedro Alves

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Pedro Alves]

On Friday 23 October 2009 15:54:11, Pedro Alves wrote:
> On Friday 23 October 2009 15:42:00, Hui Zhu wrote:
> >    if (record_debug)
> >      printf_filtered ("\
> 
> All debug output should go to fprintf_unfiltered (gdb_stdlog, ...
> not printf_filtered.
> 

Applied.

-- 
Pedro Alves

2009-10-31  Pedro Alves  <pedro@codesourcery.com>

	* record.c (record_restore, cmd_record_save): Debug output goes to
	gdb_stdlog.

---
 gdb/record.c |   87 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 44 insertions(+), 43 deletions(-)

Index: src/gdb/record.c
===================================================================
--- src.orig/gdb/record.c	2009-10-26 17:09:38.000000000 +0000
+++ src/gdb/record.c	2009-10-31 14:45:19.000000000 +0000
@@ -2005,18 +2005,18 @@ record_restore (void)
   gdb_assert (record_first.next == NULL);
  
   if (record_debug)
-    printf_filtered ("Restoring recording from core file.\n");
+    fprintf_unfiltered (gdb_stdlog, "Restoring recording from core file.\n");
 
   /* Now need to find our special note section.  */
   osec = bfd_get_section_by_name (core_bfd, "null0");
   osec_size = bfd_section_size (core_bfd, osec);
   if (record_debug)
-    printf_filtered ("Find precord section %s.\n",
-		     osec ? "succeeded" : "failed");
-  if (!osec)
+    fprintf_unfiltered (gdb_stdlog, "Find precord section %s.\n",
+			osec ? "succeeded" : "failed");
+  if (osec == NULL)
     return;
   if (record_debug)
-    printf_filtered ("%s", bfd_section_name (core_bfd, osec));
+    fprintf_unfiltered (gdb_stdlog, "%s", bfd_section_name (core_bfd, osec));
 
   /* Check the magic code.  */
   bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
@@ -2024,9 +2024,9 @@ record_restore (void)
     error (_("Version mis-match or file format error in core file %s."),
 	   bfd_get_filename (core_bfd));
   if (record_debug)
-    printf_filtered ("\
+    fprintf_unfiltered (gdb_stdlog, "\
   Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%s)\n",
-		     phex_nz (netorder32 (magic), 4));
+			phex_nz (netorder32 (magic), 4));
 
   /* Restore the entries in recfd into record_arch_list_head and
      record_arch_list_tail.  */
@@ -2062,12 +2062,12 @@ record_restore (void)
           bfdcore_read (core_bfd, osec, record_get_loc (rec),
 			rec->u.reg.len, &bfd_offset);
 
-          if (record_debug)
-            printf_filtered ("\
+	  if (record_debug)
+	    fprintf_unfiltered (gdb_stdlog, "\
   Reading register %d (1 plus %lu plus %d bytes)\n",
-			     rec->u.reg.num,
-			     (unsigned long) sizeof (regnum),
-			     rec->u.reg.len);
+				rec->u.reg.num,
+				(unsigned long) sizeof (regnum),
+				rec->u.reg.len);
           break;
 
         case record_mem: /* mem */
@@ -2087,14 +2087,14 @@ record_restore (void)
           bfdcore_read (core_bfd, osec, record_get_loc (rec),
 			rec->u.mem.len, &bfd_offset);
 
-          if (record_debug)
-            printf_filtered ("\
+	  if (record_debug)
+	    fprintf_unfiltered (gdb_stdlog, "\
   Reading memory %s (1 plus %lu plus %lu plus %d bytes)\n",
-			     paddress (get_current_arch (),
-				       rec->u.mem.addr),
-			     (unsigned long) sizeof (addr),
-			     (unsigned long) sizeof (len),
-			     rec->u.mem.len);
+				paddress (get_current_arch (),
+					  rec->u.mem.addr),
+				(unsigned long) sizeof (addr),
+				(unsigned long) sizeof (len),
+				rec->u.mem.len);
           break;
 
         case record_end: /* end */
@@ -2113,13 +2113,13 @@ record_restore (void)
 	  count = netorder32 (count);
 	  rec->u.end.insn_num = count;
 	  record_insn_count = count + 1;
-          if (record_debug)
-            printf_filtered ("\
+	  if (record_debug)
+	    fprintf_unfiltered (gdb_stdlog, "\
   Reading record_end (1 + %lu + %lu bytes), offset == %s\n",
-			     (unsigned long) sizeof (signal),
-			     (unsigned long) sizeof (count),
-			     paddress (get_current_arch (),
-				       bfd_offset));
+				(unsigned long) sizeof (signal),
+				(unsigned long) sizeof (count),
+				paddress (get_current_arch (),
+					  bfd_offset));
           break;
 
         default:
@@ -2225,7 +2225,8 @@ cmd_record_save (char *args, int from_tt
 
   /* Open the save file.  */
   if (record_debug)
-    printf_filtered ("Saving execution log to core file '%s'\n", recfilename);
+    fprintf_unfiltered (gdb_stdlog, "Saving execution log to core file '%s'\n",
+			recfilename);
 
   /* Open the output file.  */
   obfd = create_gcore_bfd (recfilename);
@@ -2291,9 +2292,9 @@ cmd_record_save (char *args, int from_tt
   /* Write the magic code.  */
   magic = RECORD_FILE_MAGIC;
   if (record_debug)
-    printf_filtered ("\
+    fprintf_unfiltered (gdb_stdlog, "\
   Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x%s)\n",
-		     phex_nz (magic, 4));
+		      phex_nz (magic, 4));
   bfdcore_write (obfd, osec, &magic, sizeof (magic), &bfd_offset);
 
   /* Save the entries to recfd and forward execute to the end of
@@ -2314,12 +2315,12 @@ cmd_record_save (char *args, int from_tt
           switch (record_list->type)
             {
             case record_reg: /* reg */
-              if (record_debug)
-		printf_filtered ("\
+	      if (record_debug)
+		fprintf_unfiltered (gdb_stdlog, "\
   Writing register %d (1 plus %lu plus %d bytes)\n",
-				 record_list->u.reg.num,
-				 (unsigned long) sizeof (regnum),
-				 record_list->u.reg.len);
+				    record_list->u.reg.num,
+				    (unsigned long) sizeof (regnum),
+				    record_list->u.reg.len);
 
               /* Write regnum.  */
               regnum = netorder32 (record_list->u.reg.num);
@@ -2333,13 +2334,13 @@ cmd_record_save (char *args, int from_tt
 
             case record_mem: /* mem */
 	      if (record_debug)
-		printf_filtered ("\
+		fprintf_unfiltered (gdb_stdlog, "\
   Writing memory %s (1 plus %lu plus %lu plus %d bytes)\n",
-				 paddress (gdbarch,
-					   record_list->u.mem.addr),
-				 (unsigned long) sizeof (addr),
-				 (unsigned long) sizeof (len),
-				 record_list->u.mem.len);
+				    paddress (gdbarch,
+					      record_list->u.mem.addr),
+				    (unsigned long) sizeof (addr),
+				    (unsigned long) sizeof (len),
+				    record_list->u.mem.len);
 
 	      /* Write memlen.  */
 	      len = netorder32 (record_list->u.mem.len);
@@ -2356,11 +2357,11 @@ cmd_record_save (char *args, int from_tt
               break;
 
               case record_end:
-                if (record_debug)
-                  printf_filtered ("\
+		if (record_debug)
+		  fprintf_unfiltered (gdb_stdlog, "\
   Writing record_end (1 + %lu + %lu bytes)\n", 
-				   (unsigned long) sizeof (signal),
-				   (unsigned long) sizeof (count));
+				      (unsigned long) sizeof (signal),
+				      (unsigned long) sizeof (count));
 		/* Write signal value.  */
 		signal = netorder32 (record_list->u.end.sigval);
 		bfdcore_write (obfd, osec, &signal,

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Hui Zhu]

Cool.  Sorry I forgot this thread.  Thanks a lot.

Hui

On Sat, Oct 31, 2009 at 22:57, Pedro Alves <pedro@codesourcery.com> wrote:
> On Friday 23 October 2009 15:54:11, Pedro Alves wrote:
>> On Friday 23 October 2009 15:42:00, Hui Zhu wrote:
>> >    if (record_debug)
>> >      printf_filtered ("\
>>
>> All debug output should go to fprintf_unfiltered (gdb_stdlog, ...
>> not printf_filtered.
>>
>
> Applied.
>
> --
> Pedro Alves
>
> 2009-10-31  Pedro Alves  <pedro@codesourcery.com>
>
>        * record.c (record_restore, cmd_record_save): Debug output goes to
>        gdb_stdlog.
>
> ---
>  gdb/record.c |   87 +++++++++++++++++++++++++++++------------------------------
>  1 file changed, 44 insertions(+), 43 deletions(-)
>
> Index: src/gdb/record.c
> ===================================================================
> --- src.orig/gdb/record.c       2009-10-26 17:09:38.000000000 +0000
> +++ src/gdb/record.c    2009-10-31 14:45:19.000000000 +0000
> @@ -2005,18 +2005,18 @@ record_restore (void)
>   gdb_assert (record_first.next == NULL);
>
>   if (record_debug)
> -    printf_filtered ("Restoring recording from core file.\n");
> +    fprintf_unfiltered (gdb_stdlog, "Restoring recording from core file.\n");
>
>   /* Now need to find our special note section.  */
>   osec = bfd_get_section_by_name (core_bfd, "null0");
>   osec_size = bfd_section_size (core_bfd, osec);
>   if (record_debug)
> -    printf_filtered ("Find precord section %s.\n",
> -                    osec ? "succeeded" : "failed");
> -  if (!osec)
> +    fprintf_unfiltered (gdb_stdlog, "Find precord section %s.\n",
> +                       osec ? "succeeded" : "failed");
> +  if (osec == NULL)
>     return;
>   if (record_debug)
> -    printf_filtered ("%s", bfd_section_name (core_bfd, osec));
> +    fprintf_unfiltered (gdb_stdlog, "%s", bfd_section_name (core_bfd, osec));
>
>   /* Check the magic code.  */
>   bfdcore_read (core_bfd, osec, &magic, sizeof (magic), &bfd_offset);
> @@ -2024,9 +2024,9 @@ record_restore (void)
>     error (_("Version mis-match or file format error in core file %s."),
>           bfd_get_filename (core_bfd));
>   if (record_debug)
> -    printf_filtered ("\
> +    fprintf_unfiltered (gdb_stdlog, "\
>   Reading 4-byte magic cookie RECORD_FILE_MAGIC (0x%s)\n",
> -                    phex_nz (netorder32 (magic), 4));
> +                       phex_nz (netorder32 (magic), 4));
>
>   /* Restore the entries in recfd into record_arch_list_head and
>      record_arch_list_tail.  */
> @@ -2062,12 +2062,12 @@ record_restore (void)
>           bfdcore_read (core_bfd, osec, record_get_loc (rec),
>                        rec->u.reg.len, &bfd_offset);
>
> -          if (record_debug)
> -            printf_filtered ("\
> +         if (record_debug)
> +           fprintf_unfiltered (gdb_stdlog, "\
>   Reading register %d (1 plus %lu plus %d bytes)\n",
> -                            rec->u.reg.num,
> -                            (unsigned long) sizeof (regnum),
> -                            rec->u.reg.len);
> +                               rec->u.reg.num,
> +                               (unsigned long) sizeof (regnum),
> +                               rec->u.reg.len);
>           break;
>
>         case record_mem: /* mem */
> @@ -2087,14 +2087,14 @@ record_restore (void)
>           bfdcore_read (core_bfd, osec, record_get_loc (rec),
>                        rec->u.mem.len, &bfd_offset);
>
> -          if (record_debug)
> -            printf_filtered ("\
> +         if (record_debug)
> +           fprintf_unfiltered (gdb_stdlog, "\
>   Reading memory %s (1 plus %lu plus %lu plus %d bytes)\n",
> -                            paddress (get_current_arch (),
> -                                      rec->u.mem.addr),
> -                            (unsigned long) sizeof (addr),
> -                            (unsigned long) sizeof (len),
> -                            rec->u.mem.len);
> +                               paddress (get_current_arch (),
> +                                         rec->u.mem.addr),
> +                               (unsigned long) sizeof (addr),
> +                               (unsigned long) sizeof (len),
> +                               rec->u.mem.len);
>           break;
>
>         case record_end: /* end */
> @@ -2113,13 +2113,13 @@ record_restore (void)
>          count = netorder32 (count);
>          rec->u.end.insn_num = count;
>          record_insn_count = count + 1;
> -          if (record_debug)
> -            printf_filtered ("\
> +         if (record_debug)
> +           fprintf_unfiltered (gdb_stdlog, "\
>   Reading record_end (1 + %lu + %lu bytes), offset == %s\n",
> -                            (unsigned long) sizeof (signal),
> -                            (unsigned long) sizeof (count),
> -                            paddress (get_current_arch (),
> -                                      bfd_offset));
> +                               (unsigned long) sizeof (signal),
> +                               (unsigned long) sizeof (count),
> +                               paddress (get_current_arch (),
> +                                         bfd_offset));
>           break;
>
>         default:
> @@ -2225,7 +2225,8 @@ cmd_record_save (char *args, int from_tt
>
>   /* Open the save file.  */
>   if (record_debug)
> -    printf_filtered ("Saving execution log to core file '%s'\n", recfilename);
> +    fprintf_unfiltered (gdb_stdlog, "Saving execution log to core file '%s'\n",
> +                       recfilename);
>
>   /* Open the output file.  */
>   obfd = create_gcore_bfd (recfilename);
> @@ -2291,9 +2292,9 @@ cmd_record_save (char *args, int from_tt
>   /* Write the magic code.  */
>   magic = RECORD_FILE_MAGIC;
>   if (record_debug)
> -    printf_filtered ("\
> +    fprintf_unfiltered (gdb_stdlog, "\
>   Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x%s)\n",
> -                    phex_nz (magic, 4));
> +                     phex_nz (magic, 4));
>   bfdcore_write (obfd, osec, &magic, sizeof (magic), &bfd_offset);
>
>   /* Save the entries to recfd and forward execute to the end of
> @@ -2314,12 +2315,12 @@ cmd_record_save (char *args, int from_tt
>           switch (record_list->type)
>             {
>             case record_reg: /* reg */
> -              if (record_debug)
> -               printf_filtered ("\
> +             if (record_debug)
> +               fprintf_unfiltered (gdb_stdlog, "\
>   Writing register %d (1 plus %lu plus %d bytes)\n",
> -                                record_list->u.reg.num,
> -                                (unsigned long) sizeof (regnum),
> -                                record_list->u.reg.len);
> +                                   record_list->u.reg.num,
> +                                   (unsigned long) sizeof (regnum),
> +                                   record_list->u.reg.len);
>
>               /* Write regnum.  */
>               regnum = netorder32 (record_list->u.reg.num);
> @@ -2333,13 +2334,13 @@ cmd_record_save (char *args, int from_tt
>
>             case record_mem: /* mem */
>              if (record_debug)
> -               printf_filtered ("\
> +               fprintf_unfiltered (gdb_stdlog, "\
>   Writing memory %s (1 plus %lu plus %lu plus %d bytes)\n",
> -                                paddress (gdbarch,
> -                                          record_list->u.mem.addr),
> -                                (unsigned long) sizeof (addr),
> -                                (unsigned long) sizeof (len),
> -                                record_list->u.mem.len);
> +                                   paddress (gdbarch,
> +                                             record_list->u.mem.addr),
> +                                   (unsigned long) sizeof (addr),
> +                                   (unsigned long) sizeof (len),
> +                                   record_list->u.mem.len);
>
>              /* Write memlen.  */
>              len = netorder32 (record_list->u.mem.len);
> @@ -2356,11 +2357,11 @@ cmd_record_save (char *args, int from_tt
>               break;
>
>               case record_end:
> -                if (record_debug)
> -                  printf_filtered ("\
> +               if (record_debug)
> +                 fprintf_unfiltered (gdb_stdlog, "\
>   Writing record_end (1 + %lu + %lu bytes)\n",
> -                                  (unsigned long) sizeof (signal),
> -                                  (unsigned long) sizeof (count));
> +                                     (unsigned long) sizeof (signal),
> +                                     (unsigned long) sizeof (count));
>                /* Write signal value.  */
>                signal = netorder32 (record_list->u.end.sigval);
>                bfdcore_write (obfd, osec, &signal,
>

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Pedro Alves]

BTW, IMO, if a patch is indeed going to be reverted, applying
small fixes on top only makes it a tad harder to revert, IMO.

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> sizeof returns a value of the type `size_t'.  On a 64-bit host, size_t
> is typically a 64-bit data type, and so is `unsigned long'.

Even though the sizeof return value has type size_t it still may fit in
an int (and most of the time it does).

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Eli Zaretskii]

> X-Spam-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,
> 	FORGED_RCVD_HELO autolearn=ham version=3.1.0
> From: Andreas Schwab <schwab@redhat.com>
> Cc: Pierre Muller <muller@ics.u-strasbg.fr>, ppluzhnikov@google.com,
>         msnyder@vmware.com, teawater@gmail.com, gdb-patches@sourceware.org
> Date: Fri, 23 Oct 2009 16:46:18 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > sizeof returns a value of the type `size_t'.  On a 64-bit host, size_t
> > is typically a 64-bit data type, and so is `unsigned long'.
> 
> Even though the sizeof return value has type size_t it still may fit in
> an int (and most of the time it does).

That's true, but printf will still complain, unless we cast
explicitly.

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> That's true, but printf will still complain, unless we cast
> explicitly.

If you want to save a cast you need to be able to use %zu.

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Eli Zaretskii]

> From: Andreas Schwab <schwab@redhat.com>
> Cc: muller@ics.u-strasbg.fr, ppluzhnikov@google.com, msnyder@vmware.com,
>         teawater@gmail.com, gdb-patches@sourceware.org
> Date: Fri, 23 Oct 2009 17:34:53 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > That's true, but printf will still complain, unless we cast
> > explicitly.
> 
> If you want to save a cast you need to be able to use %zu.

Right, but that's only available with C9X, isn't it?

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andreas Schwab <schwab@redhat.com>
>> Cc: muller@ics.u-strasbg.fr, ppluzhnikov@google.com, msnyder@vmware.com,
>>         teawater@gmail.com, gdb-patches@sourceware.org
>> Date: Fri, 23 Oct 2009 17:34:53 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > That's true, but printf will still complain, unless we cast
>> > explicitly.
>> 
>> If you want to save a cast you need to be able to use %zu.
>
> Right, but that's only available with C9X, isn't it?

If you are not able to use %zu then you need a cast, yes.

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

RE: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Eli Zaretskii
> Envoyé : Friday, October 23, 2009 12:06 PM
> À : Pierre Muller
> Cc : ppluzhnikov@google.com; msnyder@vmware.com; teawater@gmail.com;
> gdb-patches@sourceware.org
> Objet : Re: [RFA, 3 of 3] save/restore process record, part 3
> (save/restore)
> 
> > From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> > Cc: "'Hui Zhu'" <teawater@gmail.com>, "'Eli Zaretskii'"
> <eliz@gnu.org>,
> >         <gdb-patches@sourceware.org>
> > Date: Fri, 23 Oct 2009 10:52:27 +0200
> >
> >  Furthermore, there is a '%ll' rule
> > in ARI that states that:
> > Do not use printf("%ll"), instead use printf("%s",phex()) to dump a
> `long
> > long' value
> > Shouldn't this also concern '%lu'?
> 
> There is a difference: %lu is defined by C89, while %ll is only
> codified by C9X.  GDB does not yet require a C9X compiler.

  Whoops, OK, %ll is for 'long long' types,
whereas %lu is for 'unsigned long'...
  Consider this part of my email as unfounded.

Sorry, all
and thanks for the explanation, Eli.

Pierre Muller
... still a poor in C language :(

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Hui Zhu]

Cool.  Thanks, Michael.

I got some fail with testsuite:
consecutive-precsave.exp
solib-precsave.exp
until-precsave.exp

I will try to fix them.

Hui

On Fri, Oct 23, 2009 at 03:35, Michael Snyder <msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> On Wed, Oct 21, 2009 at 03:58, Michael Snyder <msnyder@vmware.com> wrote:
>>>
>>> Michael Snyder wrote:
>>>>
>>>> Hui Zhu wrote:
>>>>>
>>>>> On Tue, Oct 20, 2009 at 01:54, Michael Snyder <msnyder@vmware.com>
>>>>> wrote:
>>>>>>
>>>>>> Hui Zhu wrote:
>>>>>>>
>>>>>>> Hi Michael,
>>>>>>>
>>>>>>> I think you want unlink the gdb_record when save get some error.  It
>>>>>>> maybe need "discard_cleanups" the old_cleanups and bfd_close (obfd);
>>>>>>>
>>>>>>> After change the code, everything is OK.
>>>>>>
>>>>>> Yes.  Thanks.  Like this:
>>>>>> +      if (record_list->prev)
>>>>>> +        record_list = record_list->prev;
>>>>>> +    }
>>>>>> +
>>>>>> +  do_cleanups (set_cleanups);
>>>>>> +  bfd_close (obfd);
>>>>>> +  discard_cleanups (old_cleanups);
>>>>>> +
>>>>>> +  /* Succeeded.  */
>>>>>
>>>>> I suggest:
>>>>> +  discard_cleanups (old_cleanups);
>>>>> +  bfd_close (obfd);
>>>>
>>>> The reason I did the bfd_close first is because I wasn't
>>>> sure if it was safe to delete the file first.  In any way,
>>>> it seems more logical to close the file before delete it.
>>>
>>> Oh, oops, never mind.  My head's fuzzy today.
>>>
>>> Why do you suggest to change the order?
>>> Just curious, I don't have an issue with it...
>>>
>>
>> Sorry I didn't talk the function very clear.
>>
>>>>>> +  bfd_close (obfd);
>>>>>> +  discard_cleanups (old_cleanups);
>>
>> Before "discard_cleanups (old_cleanups);", the "record_save_cleanups"
>> will be call sometime.  "record_save_cleanups" will call "bfd_close
>> (obfd)".
>> If the record_save_cleanups will happen after "bfd_close (obfd)".
>> "bfd_close (obfd)" will be call twice.
>>
>> Of curse, most of time it will not happen.   So I just suggest.  :)
>
> OK, done and committed.
>
>

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

Hui Zhu wrote:
> Cool.  Thanks, Michael.
> 
> I got some fail with testsuite:
> consecutive-precsave.exp

This one is due to the disassembly changes.
I'm sorry -- I thought I fixed it.

> solib-precsave.exp

This one is due to the memory_xfer_partial issue that I discussed
in a separate thread.  We'll have failures until that issue is
addressed.

> until-precsave.exp

I don't think I've seen this one...

Re: [RFA, 3 of 3] save/restore process record, part 3 (save/restore)

[Michael Snyder]

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

Michael Snyder wrote:
> Hui Zhu wrote:
>> Cool.  Thanks, Michael.
>>
>> I got some fail with testsuite:
>> consecutive-precsave.exp
> 
> This one is due to the disassembly changes.
> I'm sorry -- I thought I fixed it.

Hui, here's the fix for this one.
OK?


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

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

	gdb.reverse/consecutive-precsave.exp: Change expect pattern
	to allow for new disassembly style.

Index: consecutive-precsave.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.reverse/consecutive-precsave.exp,v
retrieving revision 1.1
diff -r1.1 consecutive-precsave.exp
72c72
<     -re "($hex).*\[\r\n\]+($hex).*$gdb_prompt $" {
---
>     -re "=> ($hex).*\[\r\n\]+   ($hex).*$gdb_prompt $" {