From: Hui Zhu <teawater@gmail.com>
To: Michael Snyder <msnyder@vmware.com>
Cc: gdb-patches ml <gdb-patches@sourceware.org>
Subject: Re: [PREC/RFA] Add not_replay to make precord support release memory better
Date: Mon, 03 Aug 2009 03:51:00 -0000 [thread overview]
Message-ID: <daef60380908022050k1c6cc3dpab9aa434a185cc24@mail.gmail.com> (raw)
In-Reply-To: <daef60380908012110v15e4a4dcld47fee7b35706be1@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 15869 bytes --]
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 #2: prec-mem-not-replay.txt --]
[-- Type: text/plain, Size: 6026 bytes --]
---
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,
next prev parent reply other threads:[~2009-08-03 3:51 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-07-21 15:39 Hui Zhu
2009-07-24 11:31 ` Michael Snyder
2009-07-24 13:00 ` Hui Zhu
2009-07-25 20:56 ` Michael Snyder
2009-07-27 16:18 ` Hui Zhu
2009-08-01 23:01 ` Michael Snyder
2009-08-02 4:11 ` Hui Zhu
2009-08-03 3:51 ` Hui Zhu [this message]
2009-08-03 5:02 ` Michael Snyder
2009-08-03 5:19 ` Hui Zhu
2009-08-03 17:31 ` Michael Snyder
2009-08-04 1:50 ` Hui Zhu
2009-08-04 18:19 ` Michael Snyder
2009-08-05 1:26 ` Hui Zhu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=daef60380908022050k1c6cc3dpab9aa434a185cc24@mail.gmail.com \
--to=teawater@gmail.com \
--cc=gdb-patches@sourceware.org \
--cc=msnyder@vmware.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox