From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16920 invoked by alias); 15 Oct 2009 17:16:06 -0000 Received: (qmail 16783 invoked by uid 22791); 15 Oct 2009 17:16: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-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 15 Oct 2009 17:15:56 +0000 Received: from mailhost4.vmware.com (mailhost4.vmware.com [10.16.67.124]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id C7BB433014; Thu, 15 Oct 2009 10:15:54 -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 BF076C9B3B; Thu, 15 Oct 2009 10:15:54 -0700 (PDT) Message-ID: <4AD75788.9050409@vmware.com> Date: Thu, 15 Oct 2009 17:16:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: Hui Zhu CC: Joel Brobecker , "gdb-patches@sourceware.org" Subject: Re: [RFA] Structure and simplify record log in record.c References: <4AD367DC.9020901@vmware.com> <20091012231232.GH5272@adacore.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------070202000509000802080101" 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/msg00343.txt.bz2 This is a multi-part message in MIME format. --------------070202000509000802080101 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 485 Hui Zhu wrote: > Thanks Michaael and Joel, > > I think the other part of this patch is OK with me. > > But you still has 2 patches "[RFA] Fix off-by-one error in record.c > (record_list_release_first)" and "[RFA] Expand "info record" will > change the code of record list. > So maybe this patch need some update according to these patches. Don't worry -- it's my responsibility to keep it in sync. Committed as below, with Joel's suggestions and sync (test suites passed on x86). --------------070202000509000802080101 Content-Type: text/plain; name="tmp.txt" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="tmp.txt" Content-length: 9426 2009-10-15 Michael Snyder * record.c (record_reg_alloc): New function. (record_reg_release): New function. (record_mem_alloc): New function. (record_mem_release): New function. (record_end_alloc): New function. (record_end_release): New function. (record_entry_release): New function. (record_list_release): Simplify, call record_entry_release. (record_list_release_next): Rename to record_list_release_following. Simplify and call record_entry_release. (record_list_release_first): Simplify, comment, and use record_entry_release. (record_arch_list_add_reg): Simplify, call record_reg_alloc. (record_arch_list_add_mem): Simplify, call record_mem_alloc. (record_arch_list_add_end): Simplify, call record_end_alloc. Index: record.c =================================================================== RCS file: /cvs/src/src/gdb/record.c,v retrieving revision 1.21 diff -u -p -r1.21 record.c --- record.c 15 Oct 2009 16:57:36 -0000 1.21 +++ record.c 15 Oct 2009 17:14:30 -0000 @@ -129,50 +129,143 @@ static int (*record_beneath_to_insert_br static int (*record_beneath_to_remove_breakpoint) (struct gdbarch *, struct bp_target_info *); +/* Alloc and free functions for record_reg, record_mem, and record_end + entries. */ + +/* Alloc a record_reg record entry. */ + +static inline struct record_entry * +record_reg_alloc (struct regcache *regcache, int regnum) +{ + struct record_entry *rec; + struct gdbarch *gdbarch = get_regcache_arch (regcache); + + rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry)); + rec->type = record_reg; + rec->u.reg.num = regnum; + rec->u.reg.val = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE); + + return rec; +} + +/* Free a record_reg record entry. */ + +static inline void +record_reg_release (struct record_entry *rec) +{ + gdb_assert (rec->type == record_reg); + xfree (rec->u.reg.val); + xfree (rec); +} + +/* Alloc a record_mem record entry. */ + +static inline struct record_entry * +record_mem_alloc (CORE_ADDR addr, int len) +{ + struct record_entry *rec; + + rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry)); + rec->type = record_mem; + rec->u.mem.addr = addr; + rec->u.mem.len = len; + rec->u.mem.val = (gdb_byte *) xmalloc (len); + + return rec; +} + +/* Free a record_mem record entry. */ + +static inline void +record_mem_release (struct record_entry *rec) +{ + gdb_assert (rec->type == record_mem); + xfree (rec->u.mem.val); + xfree (rec); +} + +/* Alloc a record_end record entry. */ + +static inline struct record_entry * +record_end_alloc (void) +{ + struct record_entry *rec; + + rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry)); + rec->type = record_end; + + return rec; +} + +/* Free a record_end record entry. */ + +static inline void +record_end_release (struct record_entry *rec) +{ + xfree (rec); +} + +/* Free one record entry, any type. + Return entry->type, in case caller wants to know. */ + +static inline enum record_type +record_entry_release (struct record_entry *rec) +{ + enum record_type type = rec->type; + + switch (type) { + case record_reg: + record_reg_release (rec); + break; + case record_mem: + record_mem_release (rec); + break; + case record_end: + record_end_release (rec); + break; + } + return type; +} + +/* Free all record entries in list pointed to by REC. */ + static void record_list_release (struct record_entry *rec) { - struct record_entry *tmp; - if (!rec) return; while (rec->next) - { - rec = rec->next; - } + rec = rec->next; while (rec->prev) { - tmp = rec; rec = rec->prev; - if (tmp->type == record_reg) - xfree (tmp->u.reg.val); - else if (tmp->type == record_mem) - xfree (tmp->u.mem.val); - xfree (tmp); + record_entry_release (rec->next); } - if (rec != &record_first) - xfree (rec); + if (rec == &record_first) + { + record_insn_num = 0; + record_first.next = NULL; + } + else + record_entry_release (rec); } +/* Free all record entries forward of the given list position. */ + static void -record_list_release_next (void) +record_list_release_following (struct record_entry *rec) { - struct record_entry *rec = record_list; struct record_entry *tmp = rec->next; + rec->next = NULL; while (tmp) { rec = tmp->next; - if (tmp->type == record_end) + if (record_entry_release (tmp) == record_end) record_insn_num--; - else if (tmp->type == record_reg) - xfree (tmp->u.reg.val); - else if (tmp->type == record_mem) - xfree (tmp->u.mem.val); - xfree (tmp); tmp = rec; } } @@ -185,34 +278,31 @@ record_list_release_next (void) static void record_list_release_first (void) { - struct record_entry *tmp = NULL; - enum record_type type; + struct record_entry *tmp; if (!record_first.next) return; + /* Loop until a record_end. */ while (1) { - type = record_first.next->type; - - if (type == record_reg) - xfree (record_first.next->u.reg.val); - else if (type == record_mem) - xfree (record_first.next->u.mem.val); + /* Cut record_first.next out of the linked list. */ tmp = record_first.next; record_first.next = tmp->next; - xfree (tmp); + tmp->next->prev = &record_first; + + /* tmp is now isolated, and can be deleted. */ + if (record_entry_release (tmp) == record_end) + { + record_insn_num--; + break; /* End loop at first record_end. */ + } if (!record_first.next) { gdb_assert (record_insn_num == 1); - break; + break; /* End loop when list is empty. */ } - - record_first.next->prev = &record_first; - - if (type == record_end) - break; } } @@ -239,10 +329,10 @@ record_arch_list_add (struct record_entr } } -/* Record the value of a register NUM to record_arch_list. */ +/* Record the value of a register REGNUM to record_arch_list. */ int -record_arch_list_add_reg (struct regcache *regcache, int num) +record_arch_list_add_reg (struct regcache *regcache, int regnum) { struct record_entry *rec; @@ -250,16 +340,11 @@ record_arch_list_add_reg (struct regcach fprintf_unfiltered (gdb_stdlog, "Process record: add register num = %d to " "record list.\n", - num); + regnum); - rec = (struct record_entry *) xmalloc (sizeof (struct record_entry)); - rec->u.reg.val = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE); - rec->prev = NULL; - rec->next = NULL; - rec->type = record_reg; - rec->u.reg.num = num; + rec = record_reg_alloc (regcache, regnum); - regcache_raw_read (regcache, num, rec->u.reg.val); + regcache_raw_read (regcache, regnum, rec->u.reg.val); record_arch_list_add (rec); @@ -280,17 +365,10 @@ record_arch_list_add_mem (CORE_ADDR addr "record list.\n", paddress (target_gdbarch, addr), len); - if (!addr) + if (!addr) /* FIXME: Why? Some arch must permit it... */ return 0; - rec = (struct record_entry *) xmalloc (sizeof (struct record_entry)); - rec->u.mem.val = (gdb_byte *) xmalloc (len); - rec->prev = NULL; - rec->next = NULL; - rec->type = record_mem; - rec->u.mem.addr = addr; - rec->u.mem.len = len; - rec->u.mem.mem_entry_not_accessible = 0; + rec = record_mem_alloc (addr, len); if (target_read_memory (addr, rec->u.mem.val, len)) { @@ -299,8 +377,7 @@ record_arch_list_add_mem (CORE_ADDR addr "Process record: error reading memory at " "addr = %s len = %d.\n", paddress (target_gdbarch, addr), len); - xfree (rec->u.mem.val); - xfree (rec); + record_mem_release (rec); return -1; } @@ -320,10 +397,7 @@ record_arch_list_add_end (void) fprintf_unfiltered (gdb_stdlog, "Process record: add end to arch list.\n"); - rec = (struct record_entry *) xmalloc (sizeof (struct record_entry)); - rec->prev = NULL; - rec->next = NULL; - rec->type = record_end; + rec = record_end_alloc (); rec->u.end.sigval = TARGET_SIGNAL_0; record_arch_list_add (rec); @@ -818,7 +892,7 @@ record_wait (struct target_ops *ops, else { if (target_write_memory (record_list->u.mem.addr, - record_list->u.mem.val, + record_list->u.mem.val, record_list->u.mem.len)) { if (execution_direction != EXEC_REVERSE) @@ -1059,7 +1133,7 @@ record_store_registers (struct target_op } /* Destroy the record from here forward. */ - record_list_release_next (); + record_list_release_following (record_list); } record_registers_change (regcache, regno); @@ -1091,7 +1165,7 @@ record_xfer_partial (struct target_ops * error (_("Process record canceled the operation.")); /* Destroy the record from here forward. */ - record_list_release_next (); + record_list_release_following (record_list); } /* Check record_insn_num */ @@ -1231,7 +1305,7 @@ cmd_record_delete (char *args, int from_ if (!from_tty || query (_("Delete the log from this point forward " "and begin to record the running message " "at current PC?"))) - record_list_release_next (); + record_list_release_following (record_list); } else printf_unfiltered (_("Already at end of record list.\n")); --------------070202000509000802080101--