From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22248 invoked by alias); 2 Apr 2009 14:26:50 -0000 Received: (qmail 22217 invoked by uid 22791); 2 Apr 2009 14:26:48 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 02 Apr 2009 14:26:43 +0000 Received: (qmail 22109 invoked from network); 2 Apr 2009 14:26:41 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 2 Apr 2009 14:26:41 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFA] Submit process record and replay fourth time, 0/8 Date: Thu, 02 Apr 2009 14:26:00 -0000 User-Agent: KMail/1.9.10 Cc: Hui Zhu , Marc Khouzam , Michael Snyder , Thiago Jung Bauermann , Eli Zaretskii , paawan1982@yahoo.com References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200904021526.53212.pedro@codesourcery.com> 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: 2009-04/txt/msg00031.txt.bz2 On Wednesday 25 March 2009 07:23:45, Hui Zhu wrote: > Some update. Thanks. I'm not sure I'm looking at the most up-to-date version. Here's a few review comments. * 1-gdbarch.txt This one's OK. * 2-target_record_stratum.txt This one's OK. * 3-record_target.txt > +record_list_release (record_t * rec) ^ drop the space between '*' and rec, here and elsewhere. Is record_t supposed to be an opaque type? Throughout gdb, we tend to only "_t" struct typedef types if they're going to be value-like, passed by type, and opaque. Otherwise, we just use something like: record_list_release (struct record_entry *rec) that is, no typedef. > +/* Add a record_end type record_t to record_arch_list. */ > +int > +record_arch_list_add_end (void) Add a blank line between comment and function. > + q = yquery (_("Do you want to auto delete previous execution " > + "log entries when record/replay buffer becomes " > + "full (record-stop-at-limit)?")); What do you mean here by "when"? Didn't the user just hit the limit *now*, and that is why you're asking what to do? > +/* Before inferior step (when GDB record the running message, inferior > + only can step), GDB will call this function to record the values to > + record_list. This function will call gdbarch_process_record to > + record the running message of inferior and set them to > + record_arch_list, and add it to record_list. */ > + (...) > + > +static int > +record_message (void *args) > +{ > + int ret; > + struct gdbarch *gdbarch = args; > + struct cleanup *old_cleanups = make_cleanup (record_message_cleanups, 0); > + > + record_arch_list_head = NULL; > + record_arch_list_tail = NULL; > + > + /* Check record_insn_num. */ > + record_check_insn_num (1); > + > + record_regcache = get_current_regcache (); - When I read the first time, my question is: 'What is a "record message"'? - ARGS is a `struct gdbarch', yet you access the current regcache. I see you pass current_gdbarch to do_record_message. We're trying to eliminate the current_gdbarch global. > + if (do_record_message (current_gdbarch)) You can get at the correct gdbarch with get_regcache_arch. While we're there, what is the point of the record_regcache global? We *are* trying to move away from global state, but I don't see how this new global is any better than accessing get_current_regcache directly. The alternative would be to pass the regcache as parameter instead. > +static void > +record_sig_handler (int signo) > +{ > + if (record_debug) > + fprintf_unfiltered (gdb_stdlog, "Process record: get a signal\n"); > + > + /* It will break the running inferior in replay mode. */ > + record_resume_step = 1; I still don't understand this comment. Please explain *why* you need to set this here. > + > + /* It will let record_wait set inferior status to get the signal > + SIGINT. */ > + record_get_sig = 1; > +} > +struct cleanup * > +record_gdb_operation_disable_set (void) > +{ > + struct cleanup *old_cleanups = NULL; > + > + if (!record_gdb_operation_disable) > + { > + old_cleanups = > + make_cleanup_restore_integer (&record_gdb_operation_disable); > + record_gdb_operation_disable = 1; > + } > + > + return old_cleanups; > +} This returns a NULL cleanup if record_gdb_operation_disable is already set, but make_cleanup also returns a legitimate NULL, and it is not an error. It returns NULL when for the first cleanup put in the chain. See make_my_cleanup2. You could make use of "make_cleanup (null_cleanup, NULL)", but, simply: struct cleanup * record_gdb_operation_disable_set (void) { struct cleanup *old_cleanups = NULL; old_cleanups = make_cleanup_restore_integer (&record_gdb_operation_disable); record_gdb_operation_disable = 1; return old_cleanups; } ... should do. make_cleanup_restore_integer restores the previous value, so nested calls are safe. > +static int > +record_insert_breakpoint (struct bp_target_info *bp_tgt) > +{ > + if (!RECORD_IS_REPLAY) > + { > + struct cleanup *old_cleanups = record_gdb_operation_disable_set (); > + int ret = record_beneath_to_insert_breakpoint (bp_tgt); > + > + if (old_cleanups) > + do_cleanups (old_cleanups); So here, this cleanup handling is wrong. Don't check for NULL old_cleanups. Call do_cleanups unconditionally. Here and and elsewhere you used this idiom. > +static void > +record_wait_cleanups (void *ignore) > +{ > + if (execution_direction == EXEC_REVERSE) > + { > + if (record_list->next) > + record_list = record_list->next; > + } > + else > + record_list = record_list->prev; > +} > +static ptid_t > +record_wait (struct target_ops *ops, > + ptid_t ptid, struct target_waitstatus *status) (...) > +struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0); This cleanup looks suspiciously broken to me. It appears that is will do weird things depending on when an exception is thrown. But then again, record_wait's nesting and use of goto's makes my eyes bleed. :P Please delete this comment: > + /* XXX: I try to use some simple commands such as "disconnect" and > + "detach" to support this functions. But these commands all have > + other affect to GDB such as call function "no_shared_libraries". > + So I add special commands to GDB. */ I was looking at the commands record, delrecord, stoprecord, record-stop-at-limit, record-insn-number-max, record-insn-number, and wondering what would you think if we made all record commands under a "record" prefix? record record stop record delete set record stop-at-limit set record insn-number-max info record insn-number * 4-linux-record.txt > + Copyright (C) 2008 Free Software Foundation, Inc. 2009. > +#include Remove this. defs.h includes stdint.h. + uint32_t addr, count; + regcache_raw_read (record_regcache, tdep->arg2, (gdb_byte *) & addr); This (and many more similar instances) is assuming host-endianness == target-endianess, that the registers are 32-bit, and probably that signed<->unsigned bit representation is equal. Is this what you want? When you get to 64-bit, most of this will break, it seems. * 5-infrun.txt infun.c: > + #include "record.h" > + && current_target.to_stratum != record_stratum); Sigh, I've spent to much time trying to explain why this was wrong. Let's at least leave this behind the macro you used to have. -- Pedro Alves