From: Michael Snyder <msnyder@vmware.com>
To: Hui Zhu <teawater@gmail.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 05:02:00 -0000 [thread overview]
Message-ID: <4A766F30.2030000@vmware.com> (raw)
In-Reply-To: <daef60380908022050k1c6cc3dpab9aa434a185cc24@mail.gmail.com>
Oops, now this patch has parts of your newer patch from
the other thread mixed in.
I'm sorry, we've got multiple patches going back and forth.
Partly my fault, I've sent you a couple and I'm sure its
confusing.
Let's try to keep them separate, OK?
If you like the last patch that I sent you on this thread,
just say OK and I'll check it in. Then we can discuss
'record_exec_entry' etc. on the other thread.
Michael
Hui Zhu wrote:
> 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,
>
>
> ------------------------------------------------------------------------
>
> ---
> 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 5:02 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
2009-08-03 5:02 ` Michael Snyder [this message]
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=4A766F30.2030000@vmware.com \
--to=msnyder@vmware.com \
--cc=gdb-patches@sourceware.org \
--cc=teawater@gmail.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