From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30435 invoked by alias); 5 Feb 2013 19:45:41 -0000 Received: (qmail 30427 invoked by uid 22791); 5 Feb 2013 19:45:40 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_EG X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 05 Feb 2013 19:45:24 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r15JjKjg018868 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 5 Feb 2013 14:45:21 -0500 Received: from host2.jankratochvil.net (ovpn-116-18.ams2.redhat.com [10.36.116.18]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r15Jj84t002195 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Tue, 5 Feb 2013 14:45:12 -0500 Date: Tue, 05 Feb 2013 19:45:00 -0000 From: Jan Kratochvil To: "Metzger, Markus T" Cc: "markus.t.metzger@gmail.com" , "gdb-patches@sourceware.org" Subject: Re: record-btrace Message-ID: <20130205194508.GA9091@host2.jankratochvil.net> References: <20130129095303.GA12614@host2.jankratochvil.net> <20130129153617.GA28655@host2.jankratochvil.net> <20130201141829.GA24703@host2.jankratochvil.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="wac7ysb48OaltWcw" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-02/txt/msg00125.txt.bz2 --wac7ysb48OaltWcw Content-Type: text/plain; charset=iso-2022-jp Content-Disposition: inline Content-length: 7441 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 . */ > + > +#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 --wac7ysb48OaltWcw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline; filename="reorder.patch" Content-length: 11475 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 --wac7ysb48OaltWcw--