From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13488 invoked by alias); 17 Oct 2009 17:02:04 -0000 Received: (qmail 12965 invoked by uid 22791); 17 Oct 2009 17:02:02 -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-1.vmware.com (HELO smtp-outbound-1.vmware.com) (65.115.85.69) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 17 Oct 2009 17:01:56 +0000 Received: from jupiter.vmware.com (mailhost5.vmware.com [10.16.68.131]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id 938841303B; Sat, 17 Oct 2009 10:01:53 -0700 (PDT) Received: from [10.20.94.141] (msnyder-server.eng.vmware.com [10.20.94.141]) by jupiter.vmware.com (Postfix) with ESMTP id 88011DC056; Sat, 17 Oct 2009 10:01:53 -0700 (PDT) Message-ID: <4AD9F729.1020204@vmware.com> Date: Sat, 17 Oct 2009 17:02:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Joel Brobecker CC: "gdb-patches@sourceware.org" , Hui Zhu Subject: Re: [RFA, 1 of 3] save/restore process record, part 1 (exec_entry) References: <4AD91C32.2090900@vmware.com> <20091017040352.GI5272@adacore.com> In-Reply-To: <20091017040352.GI5272@adacore.com> Content-Type: multipart/mixed; boundary="------------070708000600040103090508" 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-10/txt/msg00390.txt.bz2 This is a multi-part message in MIME format. --------------070708000600040103090508 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2178 Joel Brobecker wrote: >> 2009-10-16 Hui Zhu >> Michael Snyder >> >> * record.c (record_exec_entry): New function. Emulate one >> instruction, forward or backward. Abstracted from record_wait. >> (record_wait) Call record_exec_entry. > > I can personnally only comment on details, since I don't know much > about process record. Not sure who from the Global Maintainers actually > know much about it except you, Michael :). I know -- do you think, if I add more comments and documentation, it will help encourage a few of the other GMs to take an interest? ;-) In the meantime, Hui and I depend on each other's review. >> +static inline void >> +record_exec_entry (struct regcache *regcache, struct gdbarch *gdbarch, >> + struct record_entry *entry) > > We're really pushing for having all functions properly documented. > Can you add a comment explaining that this function does? The function > name makes it more or less obvious, I guess, but I'd personally rather > have a consistent (mindless) approach of documenting everything rather > than having to judge on a case-by-case basis. OK, see attached revision. Thanks for the request. > Also, I can't help but wonder about the use of "inline" in this case. > I'm always reluctant to use this sort of feature until I can be proven > that this helps performance. Since this is a static function, I would > imagine that the compiler would have more knowledge of whether the > function should be inlined or not? Or is that too naive? "Inline", like "register", is just a suggestion to the compiler. The compiler will do as it pleases, based on other factors. This function gets called once per instruction in playback mode. Hence it seemed like it would benefit from inlining. It's probably too big to be inlined anyway, but there again, the compiler will make up its own mind. I don't know whether we have a policy about using "inline" in gdb code. Maybe we should. I'll be willing to take it out, but until I hear a positive request, I'll leave it in. I doubt if I can measure the performance difference, if any. Revised diff attached. --------------070708000600040103090508 Content-Type: text/plain; name="tmp1b.txt" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="tmp1b.txt" Content-length: 6123 2009-10-16 Hui Zhu Michael Snyder * record.c (record_exec_entry): New function. Emulate one instruction, forward or backward. Abstracted from record_wait. (record_wait) Call record_exec_entry. --- record.0.c 2009-10-16 14:28:07.000000000 -0700 +++ record.1.c 2009-10-17 09:50:42.000000000 -0700 @@ -588,6 +588,80 @@ record_gdb_operation_disable_set (void) return old_cleanups; } +/* Execute one instruction from the record log. Each instruction in + the log will be represented by an arbitrary sequence of register + entries and memory entries, followed by an 'end' entry. */ + +static inline void +record_exec_insn (struct regcache *regcache, struct gdbarch *gdbarch, + struct record_entry *entry) +{ + 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, + record_get_loc (entry)); + memcpy (record_get_loc (entry), reg, entry->u.reg.len); + } + break; + + case record_mem: /* mem */ + { + /* Nothing to do if the entry is flagged not_accessible. */ + if (!entry->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), + entry->u.mem.len); + + if (target_read_memory (entry->u.mem.addr, mem, entry->u.mem.len)) + { + entry->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, + record_get_loc (entry), + entry->u.mem.len)) + { + entry->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); + } + else + memcpy (record_get_loc (entry), mem, entry->u.mem.len); + } + } + } + break; + } +} + static void record_open (char *name, int from_tty) { @@ -884,77 +958,9 @@ record_wait (struct target_ops *ops, break; } - /* Set ptid, register and memory according to record_list. */ - if (record_list->type == 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 (record_list), - record_list->u.reg.num); - regcache_cooked_read (regcache, record_list->u.reg.num, reg); - regcache_cooked_write (regcache, record_list->u.reg.num, - record_get_loc (record_list)); - memcpy (record_get_loc (record_list), reg, - record_list->u.reg.len); - } - else if (record_list->type == record_mem) - { - /* mem */ - /* Nothing to do if the entry is flagged not_accessible. */ - if (!record_list->u.mem.mem_entry_not_accessible) - { - gdb_byte *mem = alloca (record_list->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 (record_list), - paddress (gdbarch, - record_list->u.mem.addr), - record_list->u.mem.len); + record_exec_insn (regcache, gdbarch, record_list); - if (target_read_memory (record_list->u.mem.addr, mem, - record_list->u.mem.len)) - { - if (execution_direction != EXEC_REVERSE) - error (_("Process record: error reading memory at " - "addr = %s len = %d."), - paddress (gdbarch, record_list->u.mem.addr), - record_list->u.mem.len); - else - /* Read failed -- - flag entry as not_accessible. */ - record_list->u.mem.mem_entry_not_accessible = 1; - } - else - { - if (target_write_memory (record_list->u.mem.addr, - record_get_loc (record_list), - record_list->u.mem.len)) - { - if (execution_direction != EXEC_REVERSE) - error (_("Process record: error writing memory at " - "addr = %s len = %d."), - paddress (gdbarch, record_list->u.mem.addr), - record_list->u.mem.len); - else - /* Write failed -- - flag entry as not_accessible. */ - record_list->u.mem.mem_entry_not_accessible = 1; - } - else - { - memcpy (record_get_loc (record_list), mem, - record_list->u.mem.len); - } - } - } - } - else + if (record_list->type == record_end) { if (record_debug > 1) fprintf_unfiltered (gdb_stdlog, --------------070708000600040103090508--