Re: record-btrace

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Mon, 04 Feb 2013 15:31:10 +0100, Metzger, Markus T wrote:
> I added a new function add_deprecated_target_alias to do this. Patch attached.

Yes, that looks nice.


> Please let me know if you'd rather want it inline in a separate email.

Official way is to put it into the mail text.  Just some mailers wrap the
lines or corrupt it some other way - unaware if your mailer will or will not.

Therefore it is also accepted to send it as attachments.  But in such case:

* only one patch/attachment per mail, so that they can be reviewed
  separately during mail reply
* Content-Disposition: inline (not attachment as yours), so that reply will
  automatically quote it.
* Content-Transfer-Encoding: 7bit or quoted-printable (not base64 as yours) as
  reportedly some people still uses MIME non-compliant mailers.

git diff --check also helps to catch various whitespacing issues (sometimes
violated in GDB...).


> > > On a similar matter, I renamed the existing "set/show record" subcommands by
> > > prepending "full-", i.e. "insn-number-max" becomes "full-insn-number-max".
> > 
> > Could it be a prefix command? "set record full insn-number-max" etc.
> > 
> > To match "record full" (vs. "record btrace").
> 
> Done.

Use --enable-targets=all (and possibly --enable-64-bit-bfd), currently your
patch:
	moxie-tdep.c:575:3: error: implicit declaration of function ‘record_arch_list_add_reg’ [-Werror=implicit-function-declaration]
	arm-tdep.c:12603:7: error: implicit declaration of function ‘record_arch_list_add_reg’ [-Werror=implicit-function-declaration]


> When I use add_alias_cmd, I do not get the deprecated warning. Am I doing something
> wrong (patch attached)?

deprecate_cmd does it, I get the warning with your patch:
	(gdb) target record
	Warning: command 'target record' is deprecated.
	Use 'target record-full'.
	[...]


> > > For record-btrace and record-full, to_record_list will be quite different. For all
> > > other targets, it will be NULL.
> > >
> > > We could share to_disconnect, to_detach, to_kill, and to_mourn_inferior. They
> > > are just forwarding the request after unpushing the record target.
> > >
> > > I made the "record stop" command generic. It searches for a record target
> > > beneath the current and unpushes the first it finds. I could do something
> > > similar for the above.
> > 
> > It looks OK to me, it would be better to see the code.
> 
> Please find the patch attached.

I find (and I do) such changes better to be multi-part, first really just some
moving of code and in another patch adjustments of the content.

It does not have to be buildable separately, it can be checked-in as a single
commit.


> @@ -567,7 +568,7 @@ record_check_insn_num (int set_terminal)
>  		target_terminal_ours ();
>  	      q = yquery (_("Do you want to auto delete previous execution "
>  			    "log entries when record/replay buffer becomes "
> -			    "full (record stop-at-limit)?"));
> +			    "full (record full-stop-at-limit)?"));

record full stop-at-limit


>  	      if (set_terminal)
>  		target_terminal_inferior ();
>  	      if (q)
> @@ -1915,11 +1917,15 @@ record_goto_bookmark (gdb_byte *bookmark, int from_tty)
>        bookmark[strlen (bookmark) - 1] = '\0';
>        /* Strip leading quote.  */
>        bookmark++;
> -      /* Pass along to cmd_record_goto.  */
> +      /* Pass along to "record goto".  */
>      }
>  
> -  cmd_record_goto ((char *) bookmark, from_tty);
> -  return;
> +  cmd = xstrprintf ("record goto %s", bookmark);
> +  cleanup = make_cleanup (xfree, cmd);
> +
> +  execute_command (cmd, from_tty);
> +
> +  do_cleanups (cleanup);

Do you have a specific reason for this execute_command call?  It could just
call target_goto_record.

(It was being done in record.c, I do not understand why, it is OK to keep such
code as is but it should not be newly introduced; I understand it is difficult
to find out which part of GDB code should be mimicked and which not.)


>  }
>  
>  static void
> @@ -1953,10 +1959,171 @@ record_execution_direction (void)
>    return record_execution_dir;
>  }
>  
> +/* The "to_info_record" method.  */
> +
> +static void record_info (char *args, int from_tty)

Formatting is:
	static void
	record_info (char *args, int from_tty)


The functions got reordered, which makes their diff with "git diff -B -M"
a bit difficult to follow, I have to reorder them myself to see their changes,
I have attached the reorder when I already did it.


> --- /dev/null
> +++ b/gdb/record-full.h
> @@ -0,0 +1,38 @@
> +/* Process record and replay target for GDB, the GNU debugger.
> +
> +   Copyright (C) 2008-2013 Free Software Foundation, Inc.

This is a new file, 2013 is enough.

> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef _RECORD_FULL_H_

GDB uses just "RECORD_FULL_H".

> +#define _RECORD_FULL_H_
> +
> +#include "record.h"

I do not see a need for this #include here.


> +
> +extern int record_memory_query;
> +
> +extern int record_arch_list_add_reg (struct regcache *regcache, int num);
> +extern int record_arch_list_add_mem (CORE_ADDR addr, int len);
> +extern int record_arch_list_add_end (void);
> +extern struct cleanup *record_gdb_operation_disable_set (void);
> +
> +/* Wrapper for target_read_memory that prints a debug message if
> +   reading memory fails.  */
> +extern int record_read_memory (struct gdbarch *gdbarch,
> +			       CORE_ADDR memaddr, gdb_byte *myaddr,
> +			       ssize_t len);
> +
> +#endif /* _RECORD_FULL_H_ */
[...]
> +/* Truncate the record log from the present point
> +   of replay until the end.  */
> +
> +static void
> +cmd_record_delete (char *args, int from_tty)
> +{
> +  require_record ();
> +
> +  target_delete_record (args, from_tty);

I do not think the to_delete_record (and other new to_*_record methods) should
be really at such high level.  target_* (to_*) methods should be at API level,
not at user command level.

Communication with user should be done in cmd_record_delete and only backend
specific data handling should be in target_delete_record.

Therefore there could be:

static void
cmd_record_delete (char *args, int from_tty)
{
  require_record ();

  if (target_record_at_end ())
    printf_unfiltered (_("Already at end of record list.\n"));
  else if (!from_tty || query (_("Delete the log from this point forward "
				 "and begin to record the running message "
				 "at current PC?")))
    target_record_delete_to_end ();
}

Typically "from_tty" should never be passed to an API method.  This will also
lead to unification of the user interface across record backends.


Sorry if I was not clear enough before, I was also offering this record.c
refactorization to do myself as it sidetracks you a lot from the btrace
implementation.


Thanks,
Jan

diff --git a/gdb/record-full.c b/gdb/record-full.c
index 3b49296..32c6558 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1959,167 +1959,6 @@ record_execution_direction (void)
   return record_execution_dir;
 }
 
-/* The "to_info_record" method.  */
-
-static void record_info (char *args, int from_tty)
-{
-  struct record_entry *p;
-
-  if (current_target.to_stratum == record_stratum)
-    {
-      if (RECORD_IS_REPLAY)
-	printf_filtered (_("Replay mode:\n"));
-      else
-	printf_filtered (_("Record mode:\n"));
-
-      /* Find entry for first actual instruction in the log.  */
-      for (p = record_first.next;
-	   p != NULL && p->type != record_end;
-	   p = p->next)
-	;
-
-      /* Do we have a log at all?  */
-      if (p != NULL && p->type == record_end)
-	{
-	  /* Display instruction number for first instruction in the log.  */
-	  printf_filtered (_("Lowest recorded instruction number is %s.\n"),
-			   pulongest (p->u.end.insn_num));
-
-	  /* If in replay mode, display where we are in the log.  */
-	  if (RECORD_IS_REPLAY)
-	    printf_filtered (_("Current instruction number is %s.\n"),
-			     pulongest (record_list->u.end.insn_num));
-
-	  /* Display instruction number for last instruction in the log.  */
-	  printf_filtered (_("Highest recorded instruction number is %s.\n"), 
-			   pulongest (record_insn_count));
-
-	  /* Display log count.  */
-	  printf_filtered (_("Log contains %d instructions.\n"), 
-			   record_insn_num);
-	}
-      else
-	{
-	  printf_filtered (_("No instructions have been logged.\n"));
-	}
-    }
-  else
-    {
-      printf_filtered (_("target record is not active.\n"));
-    }
-
-  /* Display max log size.  */
-  printf_filtered (_("Max logged instructions is %d.\n"),
-		   record_insn_max_num);
-}
-
-/* record_goto_insn -- rewind the record log (forward or backward,
-   depending on DIR) to the given entry, changing the program state
-   correspondingly.  */
-
-static void
-record_goto_insn (struct record_entry *entry,
-		  enum exec_direction_kind dir)
-{
-  struct cleanup *set_cleanups = record_gdb_operation_disable_set ();
-  struct regcache *regcache = get_current_regcache ();
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
-  /* Assume everything is valid: we will hit the entry,
-     and we will not hit the end of the recording.  */
-
-  if (dir == EXEC_FORWARD)
-    record_list = record_list->next;
-
-  do
-    {
-      record_exec_insn (regcache, gdbarch, record_list);
-      if (dir == EXEC_REVERSE)
-	record_list = record_list->prev;
-      else
-	record_list = record_list->next;
-    } while (record_list != entry);
-  do_cleanups (set_cleanups);
-}
-
-/* The "to_goto_record" method.  */
-
-static void record_goto (char *arg, int from_tty)
-{
-  struct record_entry *p = NULL;
-  ULONGEST target_insn = 0;
-
-  if (arg == NULL || *arg == '\0')
-    error (_("Command requires an argument (insn number to go to)."));
-
-  if (strncmp (arg, "start", strlen ("start")) == 0
-      || strncmp (arg, "begin", strlen ("begin")) == 0)
-    {
-      /* Special case.  Find first insn.  */
-      for (p = &record_first; p != NULL; p = p->next)
-	if (p->type == record_end)
-	  break;
-      if (p)
-	target_insn = p->u.end.insn_num;
-    }
-  else if (strncmp (arg, "end", strlen ("end")) == 0)
-    {
-      /* Special case.  Find last insn.  */
-      for (p = record_list; p->next != NULL; p = p->next)
-	;
-      for (; p!= NULL; p = p->prev)
-	if (p->type == record_end)
-	  break;
-      if (p)
-	target_insn = p->u.end.insn_num;
-    }
-  else
-    {
-      /* General case.  Find designated insn.  */
-      target_insn = parse_and_eval_long (arg);
-
-      for (p = &record_first; p != NULL; p = p->next)
-	if (p->type == record_end && p->u.end.insn_num == target_insn)
-	  break;
-    }
-
-  if (p == NULL)
-    error (_("Target insn '%s' not found."), arg);
-  else if (p == record_list)
-    error (_("Already at insn '%s'."), arg);
-  else if (p->u.end.insn_num > record_list->u.end.insn_num)
-    {
-      printf_filtered (_("Go forward to insn number %s\n"),
-		       pulongest (target_insn));
-      record_goto_insn (p, EXEC_FORWARD);
-    }
-  else
-    {
-      printf_filtered (_("Go backward to insn number %s\n"),
-		       pulongest (target_insn));
-      record_goto_insn (p, EXEC_REVERSE);
-    }
-  registers_changed ();
-  reinit_frame_cache ();
-  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
-}
-
-/* The "to_delete_record" method of target record-full.  */
-
-static void
-record_delete (char *args, int from_tty)
-{
-  if (RECORD_IS_REPLAY)
-    {
-      if (!from_tty || query (_("Delete the log from this point forward "
-				"and begin to record the running message "
-				"at current PC?")))
-	record_list_release_following (record_list);
-    }
-  else
-    printf_unfiltered (_("Already at end of record list.\n"));
-}
-
 static void
 init_record_ops (void)
 {
@@ -2387,6 +2226,33 @@ init_record_core_ops (void)
   record_core_ops.to_magic = OPS_MAGIC;
 }
 
+/* Alias for "target record".  */
+
+static void
+cmd_record_start (char *args, int from_tty)
+{
+  if (args != NULL && *args != 0)
+    error (_("Invalid argument."));
+
+  execute_command ("target record-full", from_tty);
+}
+
+/* The "to_delete_record" method of target record-full.  */
+
+static void
+record_delete (char *args, int from_tty)
+{
+  if (RECORD_IS_REPLAY)
+    {
+      if (!from_tty || query (_("Delete the log from this point forward "
+				"and begin to record the running message "
+				"at current PC?")))
+	record_list_release_following (record_list);
+    }
+  else
+    printf_unfiltered (_("Already at end of record list.\n"));
+}
+
 /* Set upper limit of record log size.  */
 
 static void
@@ -2403,6 +2269,60 @@ set_record_insn_max_num (char *args, int from_tty, struct cmd_list_element *c)
     }
 }
 
+/* The "to_info_record" method.  */
+
+static void record_info (char *args, int from_tty)
+{
+  struct record_entry *p;
+
+  if (current_target.to_stratum == record_stratum)
+    {
+      if (RECORD_IS_REPLAY)
+	printf_filtered (_("Replay mode:\n"));
+      else
+	printf_filtered (_("Record mode:\n"));
+
+      /* Find entry for first actual instruction in the log.  */
+      for (p = record_first.next;
+	   p != NULL && p->type != record_end;
+	   p = p->next)
+	;
+
+      /* Do we have a log at all?  */
+      if (p != NULL && p->type == record_end)
+	{
+	  /* Display instruction number for first instruction in the log.  */
+	  printf_filtered (_("Lowest recorded instruction number is %s.\n"),
+			   pulongest (p->u.end.insn_num));
+
+	  /* If in replay mode, display where we are in the log.  */
+	  if (RECORD_IS_REPLAY)
+	    printf_filtered (_("Current instruction number is %s.\n"),
+			     pulongest (record_list->u.end.insn_num));
+
+	  /* Display instruction number for last instruction in the log.  */
+	  printf_filtered (_("Highest recorded instruction number is %s.\n"), 
+			   pulongest (record_insn_count));
+
+	  /* Display log count.  */
+	  printf_filtered (_("Log contains %d instructions.\n"), 
+			   record_insn_num);
+	}
+      else
+	{
+	  printf_filtered (_("No instructions have been logged.\n"));
+	}
+    }
+  else
+    {
+      printf_filtered (_("target record is not active.\n"));
+    }
+
+  /* Display max log size.  */
+  printf_filtered (_("Max logged instructions is %d.\n"),
+		   record_insn_max_num);
+}
+
 /* Record log save-file format
    Version 1 (never released)
 
@@ -2664,27 +2584,6 @@ record_restore (void)
   print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
 }
 
-/* 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);
-}
-
-/* Alias for "target record".  */
-
-static void
-cmd_record_start (char *args, int from_tty)
-{
-  if (args != NULL && *args != 0)
-    error (_("Invalid argument."));
-
-  execute_command ("target record-full", from_tty);
-}
-
 /* bfdcore_write -- write bytes into a core file section.  */
 
 static inline void
@@ -2700,6 +2599,16 @@ bfdcore_write (bfd *obfd, asection *osec, void *buf, int len, int *offset)
 	   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)
 {
@@ -2915,6 +2824,97 @@ record_save (char *recfilename, int from_tty)
 		   recfilename);
 }
 
+/* record_goto_insn -- rewind the record log (forward or backward,
+   depending on DIR) to the given entry, changing the program state
+   correspondingly.  */
+
+static void
+record_goto_insn (struct record_entry *entry,
+		  enum exec_direction_kind dir)
+{
+  struct cleanup *set_cleanups = record_gdb_operation_disable_set ();
+  struct regcache *regcache = get_current_regcache ();
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+
+  /* Assume everything is valid: we will hit the entry,
+     and we will not hit the end of the recording.  */
+
+  if (dir == EXEC_FORWARD)
+    record_list = record_list->next;
+
+  do
+    {
+      record_exec_insn (regcache, gdbarch, record_list);
+      if (dir == EXEC_REVERSE)
+	record_list = record_list->prev;
+      else
+	record_list = record_list->next;
+    } while (record_list != entry);
+  do_cleanups (set_cleanups);
+}
+
+/* The "to_goto_record" method.  */
+
+static void record_goto (char *arg, int from_tty)
+{
+  struct record_entry *p = NULL;
+  ULONGEST target_insn = 0;
+
+  if (arg == NULL || *arg == '\0')
+    error (_("Command requires an argument (insn number to go to)."));
+
+  if (strncmp (arg, "start", strlen ("start")) == 0
+      || strncmp (arg, "begin", strlen ("begin")) == 0)
+    {
+      /* Special case.  Find first insn.  */
+      for (p = &record_first; p != NULL; p = p->next)
+	if (p->type == record_end)
+	  break;
+      if (p)
+	target_insn = p->u.end.insn_num;
+    }
+  else if (strncmp (arg, "end", strlen ("end")) == 0)
+    {
+      /* Special case.  Find last insn.  */
+      for (p = record_list; p->next != NULL; p = p->next)
+	;
+      for (; p!= NULL; p = p->prev)
+	if (p->type == record_end)
+	  break;
+      if (p)
+	target_insn = p->u.end.insn_num;
+    }
+  else
+    {
+      /* General case.  Find designated insn.  */
+      target_insn = parse_and_eval_long (arg);
+
+      for (p = &record_first; p != NULL; p = p->next)
+	if (p->type == record_end && p->u.end.insn_num == target_insn)
+	  break;
+    }
+
+  if (p == NULL)
+    error (_("Target insn '%s' not found."), arg);
+  else if (p == record_list)
+    error (_("Already at insn '%s'."), arg);
+  else if (p->u.end.insn_num > record_list->u.end.insn_num)
+    {
+      printf_filtered (_("Go forward to insn number %s\n"),
+		       pulongest (target_insn));
+      record_goto_insn (p, EXEC_FORWARD);
+    }
+  else
+    {
+      printf_filtered (_("Go backward to insn number %s\n"),
+		       pulongest (target_insn));
+      record_goto_insn (p, EXEC_REVERSE);
+    }
+  registers_changed ();
+  reinit_frame_cache ();
+  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
+}
+
 /* The "set record full" command.  */
 
 static void