RE: record-btrace

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Tuesday, February 05, 2013 8:45 PM


> > > > 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]

Thanks. I'll fix that.


> > 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 "target record", I added a full command using add_cmd. When I use add_alias_cmd
and then deprecate_cmd on the returned command, I do not get the warning.


> >  	      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.)

That was exactly the reason - I tried to do what record.c did. I'll remove 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.

It has been copied from record.h so I kept the copyright.


> > +/* 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.

OK.


> 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.

Sorry, I didn't get that. You can help me hook up a stack unwinder in exchange;-)


I'll send an updated version as an rfc patch series.

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052