Re: [RFA] Structure and simplify record log in record.c

[Hui Zhu <teawater at gmail dot com>]

Thanks Michael.

Hui

On Fri, Oct 16, 2009 at 01:10, Michael Snyder <msnyder@vmware.com> wrote:
> 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).
>
>
>
> 2009-10-15 ?Michael Snyder ?<msnyder@vmware.com>
>
> ? ? ? ?* 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"));
>
>