From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28248 invoked by alias); 7 Nov 2008 16:01:49 -0000 Received: (qmail 28208 invoked by uid 22791); 7 Nov 2008 16:01:47 -0000 X-Spam-Check-By: sourceware.org Received: from mtaout5.012.net.il (HELO mtaout5.012.net.il) (84.95.2.13) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 07 Nov 2008 16:00:57 +0000 Received: from conversion-daemon.i_mtaout5.012.net.il by i_mtaout5.012.net.il (HyperSendmail v2004.12) id <0K9Z00F0005NAX00@i_mtaout5.012.net.il> for gdb-patches@sourceware.org; Fri, 07 Nov 2008 18:02:39 +0200 (IST) Received: from HOME-C4E4A596F7 ([77.126.241.172]) by i_mtaout5.012.net.il (HyperSendmail v2004.12) with ESMTPA id <0K9Z00DX20JY8F61@i_mtaout5.012.net.il>; Fri, 07 Nov 2008 18:02:39 +0200 (IST) Date: Fri, 07 Nov 2008 16:01:00 -0000 From: Eli Zaretskii Subject: Re: [RFA] Process record and replay, 3/10 In-reply-to: X-012-Sender: halo1@inter.net.il To: teawater Cc: gdb-patches@sourceware.org Reply-to: Eli Zaretskii Message-id: References: 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: 2008-11/txt/msg00131.txt.bz2 > Date: Thu, 6 Nov 2008 15:46:41 +0800 > From: teawater > > This patch add the process record and replay target. This is the core > part of process record and replay. Thanks! > + if (target_read_memory (addr, rec->u.mem.val, len)) > + { > + fprintf_unfiltered (gdb_stdlog, > + "Process record: read memory addr = 0x%s len = %d error.\n", This looks like a debugging printout, but it is not conditioned on record_debug. > + /* Ask user how to do */ ^^^^^^^^^ "what to do". And please put a period at the end of the sentence, and add one more space after it. > + q = yquery (_("The record instruction number (record-insn-number) is equal to record-insn-number-max. Do you want to close record/replay stop when record/replay buffer becomes full(record-stop-at-limit) then auto delete first record_t?")); There's something wrong with this query. First, why both "close" and "stop"? Also, what is "record_t"? a typo? > + error (_("Process record: record stop the program.")); Do you mean Process record: program recording stopped. ? > + if (ret > 0) > + error (_("Process record pause the program.")); ^^^^^ Do you mean "paused"? > + if (ret < 0) > + error (_("Process record record message error.")); Do you mean something like "Process record error."? That is, does this happen when some error is encountered inside gdbarch_process_record? > + if (non_stop) > + { > + error (_("Process record target can't debug inferior in non-stop mode (non-stop).")); > + } > + if (target_async_permitted) > + { > + error (_("Process record target can't debug the GNU/Linux inferior in asynchronous mode (linux-async).")); > + } I think these limitations should also be mentioned in the manual. > + if (RECORD_IS_USED) > + { > + if (!nquery > + (_("Process record target already running, do you want delete the old record log?"))) ^^^^^^^^^^^ "want to delete" > + if (sigaction (SIGINT, &act, &old_act)) > + { > + perror_with_name (_("Process record: sigaction")); > + } Can we have a better error message here? Something like Process record: sigaction failed (There are more instances of this in the patch.) > + if (target_read_memory > + (record_list->u.mem.addr, mem, record_list->u.mem.len)) > + { > + error (_("Process record: read memory addr = 0x%s len = %d error."), Here also, the error message should be more clearly phrased. > + if (record_arch_list_add_reg (i)) > + { > + record_list_release (record_arch_list_tail); > + error (_("Process record: record message error.")); Same here. (There are more like this one.) > + nquery (_ > + ("Becuse GDB is in replay mode, changing the value of a register will destroy the record from this point forward. Change all register?")); "all registers", in plural. Also, I'm not sure I would understand what you mean by ``destroy the record''? Are you saying that process recording will effectively stop working from this point onward? > + error (_("Process record cancel the operation.")); ^^^^^^ "canceled", I think. > +static void > +cmd_record_delete (char *args, int from_tty) > +{ > + if (RECORD_IS_USED) > + { > + if (RECORD_IS_REPLAY) > + { > + if (!from_tty || query (_("Process record: delete the log from this point forward and begin to record the running message at current PC?"))) I think I'd prefer if we lose the "Process record:" prefix in this query. After all, the user knows very well that she invoked a command related to process recording and replay, so there's no need to remind her that. > + printf_unfiltered (_("Process record: already at end of record list.\n")); Same here. > +static void > +cmd_record_stop (char *args, int from_tty) > +{ > + if (RECORD_IS_USED) > + { > + if (!record_list || !from_tty || query (_("Process record: delete recorded log and stop recording?"))) And here. > +static void > +set_record_insn_max_num (char *args, int from_tty, struct cmd_list_element *c) > +{ > + if (record_insn_num > record_insn_max_num && record_insn_max_num) > + { > + printf_unfiltered (_("Process record: record instructions number is bigger than record instructions max number. Auto delete the first ones.\n")); And here. Also, we need a question mark at the end of the second sentence. > + if (sigfillset (&record_maskall) == -1) > + { > + perror_with_name (_("Process record: sigfillset")); Unclear error message. > + add_com ("delrecord", class_obscure, cmd_record_delete, > + _("When process record target running in replay mode, delete the next running messages and begin to record the running message at current address.")); This doc string should be made shorter, to fit a single terminal line, and the first line should not include comma characters, because otherwise some help commands will not display anything beyond the first comma. > + add_setshow_boolean_cmd ("record-stop-at-limit", no_class, > + &record_stop_at_limit, > + _("Set record/replay stop when record/replay buffer becomes full."), "Set whether record/replay stops when ..." > +When enable, if the record/replay buffer becomes full,\n\ ^^^^^^ "enabled" > +ask user how to do.\n\ ^^^^^^^^^ "what to do" > +When disable, if the record/replay buffer becomes full,\n\ ^^^^^^^ "disabled"