From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24928 invoked by alias); 23 Aug 2009 23:25:47 -0000 Received: (qmail 24918 invoked by uid 22791); 23 Aug 2009 23:25:46 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 23 Aug 2009 23:25:39 +0000 Received: from mailhost4.vmware.com (mailhost4.vmware.com [10.16.67.124]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id 051424F006; Sun, 23 Aug 2009 16:25:37 -0700 (PDT) Received: from [10.20.94.141] (msnyder-server.eng.vmware.com [10.20.94.141]) by mailhost4.vmware.com (Postfix) with ESMTP id EB8B7C9A17; Sun, 23 Aug 2009 16:25:36 -0700 (PDT) Message-ID: <4A91CEEC.5000802@vmware.com> Date: Sun, 23 Aug 2009 23:43:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Hui Zhu CC: Eli Zaretskii , "gdb-patches@sourceware.org" Subject: Re: [RFA/RFC] Add dump and load command to process record and replay References: <4A7B99B3.40407@vmware.com> <4A7B9F49.9030202@vmware.com> <83ws5gm30b.fsf@gnu.org> <4A7C625B.8080005@vmware.com> <4A7F5410.4000400@vmware.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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-08/txt/msg00374.txt.bz2 Hui Zhu wrote: > > Hi Michael, > > I make a new version patch. It has a lot of changes. > Remove record_core and add a new target record_core for core target to > make the code more clear. > Make the load together with record_open. > > Please help me review it. Hi Hui, In this review, I'm going to comment only on the parts of the patch that relate to the record_core_ops (ie. pushing the record stratum on top of the core file stratum). Those parts of the patch are much improved. I like this version a lot better. Thanks for reworking it. Here's the one major thing that still worries me: > +static inline void > +record_exec_entry (struct regcache *regcache, struct gdbarch *gdbarch, > + struct record_entry *entry, int core) > +{ > + switch (entry->type) > + { > + case record_reg: /* reg */ > + { > + gdb_byte reg[MAX_REGISTER_SIZE]; > + > + if (record_debug > 1) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: record_reg %s to " > + "inferior num = %d.\n", > + host_address_to_string (entry), > + entry->u.reg.num); > + > + regcache_cooked_read (regcache, entry->u.reg.num, reg); > + regcache_cooked_write (regcache, entry->u.reg.num, entry->u.reg.val); > + memcpy (entry->u.reg.val, reg, MAX_REGISTER_SIZE); > + } > + break; > + > + case record_mem: /* mem */ > + { > + if (!record_list->u.mem.mem_entry_not_accessible) > + { > + gdb_byte *mem = alloca (entry->u.mem.len); > + > + if (record_debug > 1) > + fprintf_unfiltered (gdb_stdlog, > + "Process record: record_mem %s to " > + "inferior addr = %s len = %d.\n", > + host_address_to_string (entry), > + paddress (gdbarch, entry->u.mem.addr), > + record_list->u.mem.len); > + > + if (target_read_memory (entry->u.mem.addr, mem, entry->u.mem.len)) > + { > + if ((execution_direction == EXEC_REVERSE && !core) > + || (execution_direction != EXEC_REVERSE && core)) > + { It troubles me that we have to do one thing if we're going forward and a different thing if we're going backward, unles it's a core file, and in that case we do the opposite. That's just not elegant. We're really going to do the same thing (swap the contents of target memory with the contents of the record log) regardless of which direction we're going. See below for my suggestion. > + record_list->u.mem.mem_entry_not_accessible = 1; > + if (record_debug) > + warning (_("Process record: error reading memory at " > + "addr = %s len = %d."), > + paddress (gdbarch, entry->u.mem.addr), > + entry->u.mem.len); > + } > + else > + error (_("Process record: error reading memory at " > + "addr = %s len = %d."), > + paddress (gdbarch, entry->u.mem.addr), > + entry->u.mem.len); > + } > + else > + { > + if (target_write_memory (entry->u.mem.addr, entry->u.mem.val, > + entry->u.mem.len)) > + { > + if ((execution_direction == EXEC_REVERSE && !core) > + || (execution_direction != EXEC_REVERSE && core)) And the same thing here. What if we just replace the above with this? What do you think? if (target_read_memory (entry->u.mem.addr, mem, entry->u.mem.len)) { record_list->u.mem.mem_entry_not_accessible = 1; if (record_debug) warning (_("Process record: error reading memory at " "addr = %s len = %d."), paddress (gdbarch, entry->u.mem.addr), entry->u.mem.len); } else { if (target_write_memory (entry->u.mem.addr, entry->u.mem.val, entry->u.mem.len)) { record_list->u.mem.mem_entry_not_accessible = 1; if (record_debug) warning (_("Process record: error writing memory at " "addr = %s len = %d."), paddress (gdbarch, entry->u.mem.addr), entry->u.mem.len); } }