On Sun, Aug 2, 2009 at 12:10, Hui Zhu wrote: > I think this patch is very good. > > I a new changelog for it: > 2009-08-02  Michael Snyder >            Hui Zhu   > >        * record.c (record_mem_entry): New field >        'mem_entry_not_accessible'. >        (record_arch_list_add_mem): Initialize >        'mem_entry_not_accessible' to 0. >        (record_wait): Set 'mem_entry_not_accessible' flag if target >        memory not readable.  Don't try to change target memory if >        'mem_entry_not_accessible' is set. > > Do you think it's ok? > > Thanks, > Hui > > On Sun, Aug 2, 2009 at 07:00, Michael Snyder wrote: >> Hui Zhu wrote: >>> >>> On Sun, Jul 26, 2009 at 04:41, Michael Snyder wrote: >>>> >>>> Hui Zhu wrote: >>>>> >>>>> On Fri, Jul 24, 2009 at 10:10, Michael Snyder wrote: >>>>>> >>>>>> 1) During the recording "pass", there's no change. >>>>>> 2) During the reverse-replay pass, if the memory is >>>>>> not readable or writable, we will set this flag to TRUE. >>>>>> 3) Finally, during the forward-replay pass, if the flag >>>>>> has previously been set to TRUE, we will skip this entry >>>>>> (and set the flag to FALSE.) >>>>>> >>>>>> So my question is -- is there any reason to set it to FALSE here? >>>>>> Is there any way that the memory can ever become readable again? >>>>>> >>>>>> Seems to me, once it is set TRUE, we might as well just leave it TRUE. >>>>>> Am I right? >>>>> >>>>> I thought about it too.  I think if we don't need this entry.  We can >>>>> delete it directly. >>>>> But there is a special status, after release memory, inferior alloc >>>>> some memory and its address is same with the memory that released >>>>> memory.  Then the memory entry will can be use in this status.  User >>>>> can get the right value of memory before this entry.  So I think maybe >>>>> we can keep it. >>>>> >>>>> What do you think about it? >>>> >>>> Let's say a program does something like this: >>>> >>>> buf = mmap (...); >>>> munmap (...); >>>> buf = mmap (...); >>>> munmap (...); >>>> buf = mmap (...); >>>> >>>> and so on.  And suppose that, for whatever reason, mmap always >>>> returns the same address. >>>> >>>> Then it seems to me (and please correct me if I am wrong), that >>>> it all depends on where we stop the recording. >>>> >>>> If we stop the recording after mmap, but before munmap, then >>>> the memory will be readable throughout the ENTIRE recording. >>>> >>>> But if we stop the recording after munmap, but before mmap, then >>>> the memory will NOT be readable (again for the entire recording). >>>> >>>> So as you replay backward and forward through the recording, the >>>> readability state of the memory location will never change -- it >>>> will remain either readable, or unreadable, depending only on >>>> the mapped-state when the recording ended. >>>> >>>> The only way for it to change, I think, would be if we could >>>> resume the process and add some more execution to the end of >>>> the existing recording. >>>> >>> >>> Agree with you.  We can do more thing around release memory. >>> >>> But this patch make prec work OK even if inferior release some memory. >>>  Of course, the released memory we still can not access.  But other >>> memory is OK. >>> >>> This is a low cost idea for release memory.  And we still can add >>> other thing about  release memory. >> >> Yes, I like the patch, and I want to see it go in, but >> you haven't really answered my question: >> >> Once we have set this flag to TRUE in a recording entry, >> why would we ever need to set it to FALSE? >> >> The patch is simpler and easier to understand if we simply >> leave the flag TRUE.  It worries me to see it toggling back >> and forth between TRUE and FALSE every time we play through >> the recording, if there is no benefit to doing that.  Plus >> it means that we keep trying to read the target memory even >> though we know it will fail. >> >> I'm attaching a diff to show what I mean, in case it isn't clear. >> This diff gives me the same behavior with your munmap test case >> as your diff does. >> >> >> >> >> Index: record.c >> =================================================================== >> RCS file: /cvs/src/src/gdb/record.c,v >> retrieving revision 1.9 >> diff -u -p -r1.9 record.c >> --- record.c    22 Jul 2009 05:31:26 -0000      1.9 >> +++ record.c    1 Aug 2009 22:59:55 -0000 >> @@ -51,6 +51,7 @@ struct record_mem_entry >>  { >>   CORE_ADDR addr; >>   int len; >> +  int mem_entry_not_accessible; >>   gdb_byte *val; >>  }; >> >> @@ -275,6 +276,7 @@ record_arch_list_add_mem (CORE_ADDR addr >>   rec->type = record_mem; >>   rec->u.mem.addr = addr; >>   rec->u.mem.len = len; >> +  rec->u.mem.mem_entry_not_accessible = 0; >> >>   if (target_read_memory (addr, rec->u.mem.val, len)) >>     { >> @@ -727,32 +729,52 @@ record_wait (struct target_ops *ops, >>          else if (record_list->type == record_mem) >>            { >>              /* mem */ >> -             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); >> - >> -             if (target_read_memory >> -                 (record_list->u.mem.addr, mem, record_list->u.mem.len)) >> -               error (_("Process record: error reading memory at " >> -                        "addr = %s len = %d."), >> -                      paddress (gdbarch, record_list->u.mem.addr), >> -                      record_list->u.mem.len); >> - >> -             if (target_write_memory >> -                 (record_list->u.mem.addr, record_list->u.mem.val, >> -                  record_list->u.mem.len)) >> -               error (_ >> -                      ("Process record: error writing memory at " >> -                       "addr = %s len = %d."), >> -                      paddress (gdbarch, record_list->u.mem.addr), >> -                      record_list->u.mem.len); >> - >> -             memcpy (record_list->u.mem.val, mem, record_list->u.mem.len); >> +             if (record_list->u.mem.mem_entry_not_accessible == 0) >> +               { >> +                 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); >> + >> +                 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 >> +                       record_list->u.mem.mem_entry_not_accessible = 1; >> +                   } >> +                 else >> +                   { >> +                     if (target_write_memory (record_list->u.mem.addr, >> +                                              record_list->u.mem.val, >> +                                              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 >> +                           { >> +                             record_list->u.mem.mem_entry_not_accessible = >> 1; >> +                           } >> +                       } >> +                     else >> +                       { >> +                         memcpy (record_list->u.mem.val, mem, >> +                                 record_list->u.mem.len); >> +                       } >> +                   } >> +               } >>            } >>          else >>            { >> >> > Hi Michael, I make a new patch for this function. Please help me review it. Thanks, Hui 2009-08-03 Michael Snyder Hui Zhu * record.c (record_mem_entry): New field 'mem_entry_not_accessible'. (record_arch_list_add_mem): Initialize 'mem_entry_not_accessible' to 0. (record_exec_entry): New function. (record_wait): Call 'record_exec_entry'. --- record.c | 133 +++++++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 87 insertions(+), 46 deletions(-) --- a/record.c +++ b/record.c @@ -51,6 +51,7 @@ struct record_mem_entry { CORE_ADDR addr; int len; + int mem_entry_not_accessible; gdb_byte *val; }; @@ -275,6 +276,7 @@ record_arch_list_add_mem (CORE_ADDR addr rec->type = record_mem; rec->u.mem.addr = addr; rec->u.mem.len = len; + rec->u.mem.mem_entry_not_accessible = 0; if (target_read_memory (addr, rec->u.mem.val, len)) { @@ -412,6 +414,89 @@ record_gdb_operation_disable_set (void) return old_cleanups; } +static inline void +record_exec_entry (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, 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) + { + 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) + { + 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); + } + else + error (_("Process record: error writing memory at " + "addr = %s len = %d."), + paddress (gdbarch, entry->u.mem.addr), + entry->u.mem.len); + } + } + + memcpy (entry->u.mem.val, mem, entry->u.mem.len); + } + } + break; + } +} + static void record_open (char *name, int from_tty) { @@ -708,53 +793,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_list->u.reg.val); - memcpy (record_list->u.reg.val, reg, MAX_REGISTER_SIZE); - } - else if (record_list->type == record_mem) - { - /* mem */ - 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); - - if (target_read_memory - (record_list->u.mem.addr, mem, record_list->u.mem.len)) - error (_("Process record: error reading memory at " - "addr = %s len = %d."), - paddress (gdbarch, record_list->u.mem.addr), - record_list->u.mem.len); - - if (target_write_memory - (record_list->u.mem.addr, record_list->u.mem.val, - record_list->u.mem.len)) - error (_ - ("Process record: error writing memory at " - "addr = %s len = %d."), - paddress (gdbarch, record_list->u.mem.addr), - record_list->u.mem.len); + record_exec_entry (regcache, gdbarch, record_list); - memcpy (record_list->u.mem.val, mem, record_list->u.mem.len); - } - else + if (record_list->type == record_end) { if (record_debug > 1) fprintf_unfiltered (gdb_stdlog,