Re: [RFA/RFC] Add dump and load command to process record and replay

[Michael Snyder <msnyder@vmware.com>]

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);
                   }
               }