This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On Sun, Aug 2, 2009 at 12:10, Hui Zhu<teawater@gmail.com> wrote: > I think this patch is very good. > > I a new changelog for it: > 2009-08-02 ?Michael Snyder <msnyder@vmware.com> > ? ? ? ? ? ?Hui Zhu ?<teawater@gmail.com> > > ? ? ? ?* 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<msnyder@vmware.com> wrote: >> Hui Zhu wrote: >>> >>> On Sun, Jul 26, 2009 at 04:41, Michael Snyder<msnyder@vmware.com> wrote: >>>> >>>> Hui Zhu wrote: >>>>> >>>>> On Fri, Jul 24, 2009 at 10:10, Michael Snyder<msnyder@vmware.com> 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 <msnyder@vmware.com> Hui Zhu <teawater@gmail.com> * 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,
Attachment:
prec-mem-not-replay.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |