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

[Hui Zhu <teawater at gmail dot com>]

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
> ? ? ? ? ? ?{
>
>