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