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]

Re: [PREC/RFA] Add not_replay to make precord support release memory better


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]