From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8338 invoked by alias); 11 Nov 2008 03:00:02 -0000 Received: (qmail 8241 invoked by uid 22791); 11 Nov 2008 03:00:00 -0000 X-Spam-Check-By: sourceware.org Received: from ti-out-0910.google.com (HELO ti-out-0910.google.com) (209.85.142.184) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 11 Nov 2008 02:59:24 +0000 Received: by ti-out-0910.google.com with SMTP id d10so1771871tib.12 for ; Mon, 10 Nov 2008 18:59:21 -0800 (PST) Received: by 10.110.43.20 with SMTP id q20mr8749059tiq.25.1226372361162; Mon, 10 Nov 2008 18:59:21 -0800 (PST) Received: by 10.110.103.3 with HTTP; Mon, 10 Nov 2008 18:59:21 -0800 (PST) Message-ID: Date: Tue, 11 Nov 2008 04:20:00 -0000 From: teawater To: "Eli Zaretskii" Subject: Re: [RFA] Process record and replay, 3/10 Cc: gdb-patches@sourceware.org In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline 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/msg00204.txt.bz2 Thanks Eli. On Sat, Nov 8, 2008 at 00:00, Eli Zaretskii wrote: >> 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. OK. I will let it 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. > OK. I will fix 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? What about change it to "Do you want to auto delete first execute log entry when record/replay buffer becomes full(record-stop-at-limit)?" > >> + error (_("Process record: record stop the program.")); > > Do you mean > > Process record: program recording stopped. > > ? What about "Process record stop inferior." > >> + if (ret > 0) >> + error (_("Process record pause the program.")); > ^^^^^ > Do you mean "paused"? > Yes, I will fix it. >> + 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? > What about "Process record execute log error"? >> + 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. > OK. I will. >> + if (RECORD_IS_USED) >> + { >> + if (!nquery >> + (_("Process record target already running, do you want delete the old record log?"))) > ^^^^^^^^^^^ > "want to delete" I will fix it. > >> + 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.) OK. I will try to fix all of them. > >> + 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. Could you tell me how to output this message clear? > >> + 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.) Change it to "record execute log"? > >> + 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? What about change it to "destory the execute log"? > >> + error (_("Process record cancel the operation.")); > ^^^^^^ > "canceled", I think. I will fix it. > >> +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. I will remove them. > >> +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. I think maybe we need let user know when delete some log entries. > >> + if (sigfillset (&record_maskall) == -1) >> + { >> + perror_with_name (_("Process record: sigfillset")); > > Unclear error message. > I will fix it. >> + 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. > What about this: _("When process record target running in replay mode,\n\ delete the next running messages and begin to record\n\ the execute log at current address.") >> + 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" > I will fix them.