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

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

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 8451 bytes --]

Hi,

Now, in replay reverse mode, if get error with memory entry, it will
output error.  But it just affect the operation with this part of
memory.
So I make a patch to let it set the not_replay in memory entry to 1 in
this status.  When replay forward mode, this memory entry will not
really set to memory.

For example:
#include <sys/types.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdint.h>
#include <sys/mman.h>

int
main(int argc,char *argv[],char *envp[])
{
	char	*buf;

	buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
MAP_ANONYMOUS, -1, 0);
	if (buf == (caddr_t) -1) {
		perror ("mmap");
		return (-errno);
	}

	buf[0] = 1;

	munmap (buf, 1024);

	return (0);
}

Before patch:
gdb ~/gdb/a.out
GNU gdb (GDB) 6.8.50.20090720-cvs
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Setting up the environment for debugging gdb.
Function "internal_error" not defined.
Make breakpoint pending on future shared library load? (y or [n])
[answered N; input not from terminal]
Function "info_command" not defined.
Make breakpoint pending on future shared library load? (y or [n])
[answered N; input not from terminal]
/home/teawater/gdb/bgdbno/gdb/.gdbinit:8: Error in sourced command file:
No breakpoint number 0.
(gdb) start
During symbol reading, DW_AT_name missing from DW_TAG_base_type.
Temporary breakpoint 1 at 0x8048425: file 1.c, line 17.
Starting program: /home/teawater/gdb/a.out

Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value
optimized out>, envp=<value optimized out>)
    at 1.c:17
17		buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
MAP_ANONYMOUS, -1, 0);
(gdb) record
(gdb) n
During symbol reading, incomplete CFI data; unspecified registers
(e.g., eax) at 0x8048422.
18		if (buf == (caddr_t) -1) {
(gdb)
23		buf[0] = 1;
(gdb)
25		munmap (buf, 1024);
(gdb)
The next instruction is syscall munmap.  It will free the memory addr
= 0xb7fe0000 len = 1024.  It will make record target get error.  Do
you want to stop the program?([y] or n) n
27		return (0);
(gdb) rc
Continuing.
Process record: error reading memory at addr = 0xb7fe0000 len = 1.
(gdb)




After patch:
./gdb ~/gdb/a.out
GNU gdb (GDB) 6.8.50.20090721-cvs
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Setting up the environment for debugging gdb.
Function "internal_error" not defined.
Make breakpoint pending on future shared library load? (y or [n])
[answered N; input not from terminal]
Function "info_command" not defined.
Make breakpoint pending on future shared library load? (y or [n])
[answered N; input not from terminal]
/home/teawater/gdb/bgdbno/gdb/.gdbinit:8: Error in sourced command file:
No breakpoint number 0.
(gdb) start
During symbol reading, DW_AT_name missing from DW_TAG_base_type.
Temporary breakpoint 1 at 0x8048425: file 1.c, line 17.
Starting program: /home/teawater/gdb/a.out

Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value
optimized out>, envp=<value optimized out>)
    at 1.c:17
17		buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
MAP_ANONYMOUS, -1, 0);
(gdb) record
(gdb) n
During symbol reading, incomplete CFI data; unspecified registers
(e.g., eax) at 0x8048422.
18		if (buf == (caddr_t) -1) {
(gdb)
23		buf[0] = 1;
(gdb)
25		munmap (buf, 1024);
(gdb)
The next instruction is syscall munmap.  It will free the memory addr
= 0xb7fe0000 len = 1024.  It will make record target get error.  Do
you want to stop the program?([y] or n) n
27		return (0);
(gdb) rc
Continuing.

No more reverse-execution history.
main (argc=<value optimized out>, argv=<value optimized out>,
envp=<value optimized out>) at 1.c:17
17		buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
MAP_ANONYMOUS, -1, 0);
(gdb) n
18		if (buf == (caddr_t) -1) {
(gdb)
23		buf[0] = 1;
(gdb)
25		munmap (buf, 1024);
(gdb) rn
23		buf[0] = 1;



Please help me review it.

Thanks,
Hui

2009-07-21  Hui Zhu  <teawater@gmail.com>
	
	* record.c (record_mem_entry): Add not_replay.  If this the
	memory entry doesn't replay, it will set to 1.
	(record_arch_list_add_mem): Initialize not_replay to 0.
	(record_wait): In replay reverse mode, if get error with
	memory entry, not output error, just set not_replay to 1.
	In replay forward mode, if not_replay is set, don't set
	memory entry to memory.

---
 record.c |   75 ++++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 25 deletions(-)

--- a/record.c
+++ b/record.c
@@ -51,6 +51,7 @@ struct record_mem_entry
 {
   CORE_ADDR addr;
   int len;
+  int not_replay;
   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.not_replay = 0;

   if (target_read_memory (addr, rec->u.mem.val, len))
     {
@@ -727,32 +729,55 @@ 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);
+	      if (record_list->u.mem.not_replay
+		  && execution_direction != EXEC_REVERSE)
+		record_list->u.mem.not_replay = 0;
+	      else
+		{
+		  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);

-	      memcpy (record_list->u.mem.val, mem, 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.not_replay = 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.not_replay = 1;
+			    }
+			}
+		      else
+		        {
+			  memcpy (record_list->u.mem.val, mem,
+				  record_list->u.mem.len);
+			}
+		    }
+		}
 	    }
 	  else
 	    {

[-- Attachment #2: prec-mem-not-replay.txt --]
[-- Type: text/plain, Size: 3315 bytes --]

---
 record.c |   75 ++++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 25 deletions(-)

--- a/record.c
+++ b/record.c
@@ -51,6 +51,7 @@ struct record_mem_entry
 {
   CORE_ADDR addr;
   int len;
+  int not_replay;
   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.not_replay = 0;
 
   if (target_read_memory (addr, rec->u.mem.val, len))
     {
@@ -727,32 +729,55 @@ 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);
+	      if (record_list->u.mem.not_replay
+		  && execution_direction != EXEC_REVERSE)
+		record_list->u.mem.not_replay = 0;
+	      else
+		{
+		  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);
 
-	      memcpy (record_list->u.mem.val, mem, 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.not_replay = 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.not_replay = 1;
+			    }
+			}
+		      else
+		        {
+			  memcpy (record_list->u.mem.val, mem,
+				  record_list->u.mem.len);
+			}
+		    }
+		}
 	    }
 	  else
 	    {

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

[Michael Snyder]

Hui Zhu wrote:
> Hi,
> 
> Now, in replay reverse mode, if get error with memory entry, it will
> output error.  But it just affect the operation with this part of
> memory.
> So I make a patch to let it set the not_replay in memory entry to 1 in
> this status.  When replay forward mode, this memory entry will not
> really set to memory.

I like the idea.  I like it a lot!
Can this solve the problem with sbrk() too?
(code comments below)

> For example:
> #include <sys/types.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <errno.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <stdint.h>
> #include <sys/mman.h>
> 
> int
> main(int argc,char *argv[],char *envp[])
> {
> 	char	*buf;
> 
> 	buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
> MAP_ANONYMOUS, -1, 0);
> 	if (buf == (caddr_t) -1) {
> 		perror ("mmap");
> 		return (-errno);
> 	}
> 
> 	buf[0] = 1;
> 
> 	munmap (buf, 1024);
> 
> 	return (0);
> }
> 
> Before patch:
> gdb ~/gdb/a.out
> GNU gdb (GDB) 6.8.50.20090720-cvs
> Copyright (C) 2009 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "i686-pc-linux-gnu".
> For bug reporting instructions, please see:
> <http://www.gnu.org/software/gdb/bugs/>...
> Setting up the environment for debugging gdb.
> Function "internal_error" not defined.
> Make breakpoint pending on future shared library load? (y or [n])
> [answered N; input not from terminal]
> Function "info_command" not defined.
> Make breakpoint pending on future shared library load? (y or [n])
> [answered N; input not from terminal]
> /home/teawater/gdb/bgdbno/gdb/.gdbinit:8: Error in sourced command file:
> No breakpoint number 0.
> (gdb) start
> During symbol reading, DW_AT_name missing from DW_TAG_base_type.
> Temporary breakpoint 1 at 0x8048425: file 1.c, line 17.
> Starting program: /home/teawater/gdb/a.out
> 
> Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value
> optimized out>, envp=<value optimized out>)
>     at 1.c:17
> 17		buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
> MAP_ANONYMOUS, -1, 0);
> (gdb) record
> (gdb) n
> During symbol reading, incomplete CFI data; unspecified registers
> (e.g., eax) at 0x8048422.
> 18		if (buf == (caddr_t) -1) {
> (gdb)
> 23		buf[0] = 1;
> (gdb)
> 25		munmap (buf, 1024);
> (gdb)
> The next instruction is syscall munmap.  It will free the memory addr
> = 0xb7fe0000 len = 1024.  It will make record target get error.  Do
> you want to stop the program?([y] or n) n
> 27		return (0);
> (gdb) rc
> Continuing.
> Process record: error reading memory at addr = 0xb7fe0000 len = 1.
> (gdb)
> 
> 
> 
> 
> After patch:
> ./gdb ~/gdb/a.out
> GNU gdb (GDB) 6.8.50.20090721-cvs
> Copyright (C) 2009 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "i686-pc-linux-gnu".
> For bug reporting instructions, please see:
> <http://www.gnu.org/software/gdb/bugs/>...
> Setting up the environment for debugging gdb.
> Function "internal_error" not defined.
> Make breakpoint pending on future shared library load? (y or [n])
> [answered N; input not from terminal]
> Function "info_command" not defined.
> Make breakpoint pending on future shared library load? (y or [n])
> [answered N; input not from terminal]
> /home/teawater/gdb/bgdbno/gdb/.gdbinit:8: Error in sourced command file:
> No breakpoint number 0.
> (gdb) start
> During symbol reading, DW_AT_name missing from DW_TAG_base_type.
> Temporary breakpoint 1 at 0x8048425: file 1.c, line 17.
> Starting program: /home/teawater/gdb/a.out
> 
> Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value
> optimized out>, envp=<value optimized out>)
>     at 1.c:17
> 17		buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
> MAP_ANONYMOUS, -1, 0);
> (gdb) record
> (gdb) n
> During symbol reading, incomplete CFI data; unspecified registers
> (e.g., eax) at 0x8048422.
> 18		if (buf == (caddr_t) -1) {
> (gdb)
> 23		buf[0] = 1;
> (gdb)
> 25		munmap (buf, 1024);
> (gdb)
> The next instruction is syscall munmap.  It will free the memory addr
> = 0xb7fe0000 len = 1024.  It will make record target get error.  Do
> you want to stop the program?([y] or n) n
> 27		return (0);
> (gdb) rc
> Continuing.
> 
> No more reverse-execution history.
> main (argc=<value optimized out>, argv=<value optimized out>,
> envp=<value optimized out>) at 1.c:17
> 17		buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
> MAP_ANONYMOUS, -1, 0);
> (gdb) n
> 18		if (buf == (caddr_t) -1) {
> (gdb)
> 23		buf[0] = 1;
> (gdb)
> 25		munmap (buf, 1024);
> (gdb) rn
> 23		buf[0] = 1;
> 
> 
> 
> Please help me review it.
> 
> Thanks,
> Hui
> 
> 2009-07-21  Hui Zhu  <teawater@gmail.com>
> 	
> 	* record.c (record_mem_entry): Add not_replay.  If this the
> 	memory entry doesn't replay, it will set to 1.
> 	(record_arch_list_add_mem): Initialize not_replay to 0.
> 	(record_wait): In replay reverse mode, if get error with
> 	memory entry, not output error, just set not_replay to 1.
> 	In replay forward mode, if not_replay is set, don't set
> 	memory entry to memory.

A changelog entry doesn't need to explain so much.
This much explanation belongs in comments in the code.

Change log entries explain more "what was changed",
and not so much "why".

This should say something like:

	* record.c (record_mem_entry): New field 'not_replay'.
	(record_arch_list_add_mem): Initialize not_replay to 0.
	(record_wait): Set 'not_replay' flag if target memory
	not readable.  Don't try to change target memory if
	'not_replay' is set.



> ---
>  record.c |   75 ++++++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 50 insertions(+), 25 deletions(-)
> 
> --- a/record.c
> +++ b/record.c
> @@ -51,6 +51,7 @@ struct record_mem_entry
>  {
>    CORE_ADDR addr;
>    int len;
> +  int not_replay;

This needs a more descriptive name (and some comments!)

How about something like:

     /* Set this flag if target memory for this entry
        can no longer be accessed.  */
     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.not_replay = 0;
> 
>    if (target_read_memory (addr, rec->u.mem.val, len))
>      {
> @@ -727,32 +729,55 @@ 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);
> +	      if (record_list->u.mem.not_replay
> +		  && execution_direction != EXEC_REVERSE)
> +		record_list->u.mem.not_replay = 0;

First of all, a comment to explain what's going on, please.

Now -- if I understand correctly (and please tell if I'm wrong),

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?

> +	      else
> +		{
> +		  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);
> 
> -	      memcpy (record_list->u.mem.val, mem, 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.not_replay = 1;

A comment to explain what's happening, please.

> +		    }
> +		  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.not_replay = 1;

And again, a comment.  This whole section needs more comments,
but let's start with this, before it becomes even more difficult
to understand.

> +			    }
> +			}
> +		      else
> +		        {
> +			  memcpy (record_list->u.mem.val, mem,
> +				  record_list->u.mem.len);
> +			}
> +		    }
> +		}
>  	    }
>  	  else
>  	    {

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

[Hui Zhu]

On Fri, Jul 24, 2009 at 10:10, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> Hi,
>>
>> Now, in replay reverse mode, if get error with memory entry, it will
>> output error.  But it just affect the operation with this part of
>> memory.
>> So I make a patch to let it set the not_replay in memory entry to 1 in
>> this status.  When replay forward mode, this memory entry will not
>> really set to memory.
>
> I like the idea.  I like it a lot!
> Can this solve the problem with sbrk() too?
> (code comments below)
>

Yes.  After this patch, I think we can begin to make sbrk and munmap
choice don't stop inferior.
Maybe we can add a switch like:
set record memoryxxx auto     query to user
set record memoryxxx yes       stop inferior
set record memoryxxx no        not stop inferior

Please give me your comments with it.   And please help me choice a
fit name for it.  :)


>> For example:
>> #include <sys/types.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <string.h>
>> #include <unistd.h>
>> #include <errno.h>
>> #include <sys/stat.h>
>> #include <fcntl.h>
>> #include <stdint.h>
>> #include <sys/mman.h>
>>
>> int
>> main(int argc,char *argv[],char *envp[])
>> {
>>        char    *buf;
>>
>>        buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
>> MAP_ANONYMOUS, -1, 0);
>>        if (buf == (caddr_t) -1) {
>>                perror ("mmap");
>>                return (-errno);
>>        }
>>
>>        buf[0] = 1;
>>
>>        munmap (buf, 1024);
>>
>>        return (0);
>> }
>>
>> Before patch:
>> gdb ~/gdb/a.out
>> GNU gdb (GDB) 6.8.50.20090720-cvs
>> Copyright (C) 2009 Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later
>> <http://gnu.org/licenses/gpl.html>
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>> and "show warranty" for details.
>> This GDB was configured as "i686-pc-linux-gnu".
>> For bug reporting instructions, please see:
>> <http://www.gnu.org/software/gdb/bugs/>...
>> Setting up the environment for debugging gdb.
>> Function "internal_error" not defined.
>> Make breakpoint pending on future shared library load? (y or [n])
>> [answered N; input not from terminal]
>> Function "info_command" not defined.
>> Make breakpoint pending on future shared library load? (y or [n])
>> [answered N; input not from terminal]
>> /home/teawater/gdb/bgdbno/gdb/.gdbinit:8: Error in sourced command file:
>> No breakpoint number 0.
>> (gdb) start
>> During symbol reading, DW_AT_name missing from DW_TAG_base_type.
>> Temporary breakpoint 1 at 0x8048425: file 1.c, line 17.
>> Starting program: /home/teawater/gdb/a.out
>>
>> Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value
>> optimized out>, envp=<value optimized out>)
>>    at 1.c:17
>> 17              buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
>> MAP_ANONYMOUS, -1, 0);
>> (gdb) record
>> (gdb) n
>> During symbol reading, incomplete CFI data; unspecified registers
>> (e.g., eax) at 0x8048422.
>> 18              if (buf == (caddr_t) -1) {
>> (gdb)
>> 23              buf[0] = 1;
>> (gdb)
>> 25              munmap (buf, 1024);
>> (gdb)
>> The next instruction is syscall munmap.  It will free the memory addr
>> = 0xb7fe0000 len = 1024.  It will make record target get error.  Do
>> you want to stop the program?([y] or n) n
>> 27              return (0);
>> (gdb) rc
>> Continuing.
>> Process record: error reading memory at addr = 0xb7fe0000 len = 1.
>> (gdb)
>>
>>
>>
>>
>> After patch:
>> ./gdb ~/gdb/a.out
>> GNU gdb (GDB) 6.8.50.20090721-cvs
>> Copyright (C) 2009 Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later
>> <http://gnu.org/licenses/gpl.html>
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>> and "show warranty" for details.
>> This GDB was configured as "i686-pc-linux-gnu".
>> For bug reporting instructions, please see:
>> <http://www.gnu.org/software/gdb/bugs/>...
>> Setting up the environment for debugging gdb.
>> Function "internal_error" not defined.
>> Make breakpoint pending on future shared library load? (y or [n])
>> [answered N; input not from terminal]
>> Function "info_command" not defined.
>> Make breakpoint pending on future shared library load? (y or [n])
>> [answered N; input not from terminal]
>> /home/teawater/gdb/bgdbno/gdb/.gdbinit:8: Error in sourced command file:
>> No breakpoint number 0.
>> (gdb) start
>> During symbol reading, DW_AT_name missing from DW_TAG_base_type.
>> Temporary breakpoint 1 at 0x8048425: file 1.c, line 17.
>> Starting program: /home/teawater/gdb/a.out
>>
>> Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value
>> optimized out>, envp=<value optimized out>)
>>    at 1.c:17
>> 17              buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
>> MAP_ANONYMOUS, -1, 0);
>> (gdb) record
>> (gdb) n
>> During symbol reading, incomplete CFI data; unspecified registers
>> (e.g., eax) at 0x8048422.
>> 18              if (buf == (caddr_t) -1) {
>> (gdb)
>> 23              buf[0] = 1;
>> (gdb)
>> 25              munmap (buf, 1024);
>> (gdb)
>> The next instruction is syscall munmap.  It will free the memory addr
>> = 0xb7fe0000 len = 1024.  It will make record target get error.  Do
>> you want to stop the program?([y] or n) n
>> 27              return (0);
>> (gdb) rc
>> Continuing.
>>
>> No more reverse-execution history.
>> main (argc=<value optimized out>, argv=<value optimized out>,
>> envp=<value optimized out>) at 1.c:17
>> 17              buf = mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVATE |
>> MAP_ANONYMOUS, -1, 0);
>> (gdb) n
>> 18              if (buf == (caddr_t) -1) {
>> (gdb)
>> 23              buf[0] = 1;
>> (gdb)
>> 25              munmap (buf, 1024);
>> (gdb) rn
>> 23              buf[0] = 1;
>>
>>
>>
>> Please help me review it.
>>
>> Thanks,
>> Hui
>>
>> 2009-07-21  Hui Zhu  <teawater@gmail.com>
>>
>>        * record.c (record_mem_entry): Add not_replay.  If this the
>>        memory entry doesn't replay, it will set to 1.
>>        (record_arch_list_add_mem): Initialize not_replay to 0.
>>        (record_wait): In replay reverse mode, if get error with
>>        memory entry, not output error, just set not_replay to 1.
>>        In replay forward mode, if not_replay is set, don't set
>>        memory entry to memory.
>
> A changelog entry doesn't need to explain so much.
> This much explanation belongs in comments in the code.
>
> Change log entries explain more "what was changed",
> and not so much "why".
>
> This should say something like:
>
>        * record.c (record_mem_entry): New field 'not_replay'.
>        (record_arch_list_add_mem): Initialize not_replay to 0.
>        (record_wait): Set 'not_replay' flag if target memory
>        not readable.  Don't try to change target memory if
>        'not_replay' is set.
>

OK.  I will change it.

>
>
>> ---
>>  record.c |   75
>> ++++++++++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 50 insertions(+), 25 deletions(-)
>>
>> --- a/record.c
>> +++ b/record.c
>> @@ -51,6 +51,7 @@ struct record_mem_entry
>>  {
>>   CORE_ADDR addr;
>>   int len;
>> +  int not_replay;
>
> This needs a more descriptive name (and some comments!)

OK.  I will add more comments.

>
> How about something like:
>
>    /* Set this flag if target memory for this entry
>       can no longer be accessed.  */
>    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.not_replay = 0;
>>
>>   if (target_read_memory (addr, rec->u.mem.val, len))
>>     {
>> @@ -727,32 +729,55 @@ 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);
>> +             if (record_list->u.mem.not_replay
>> +                 && execution_direction != EXEC_REVERSE)
>> +               record_list->u.mem.not_replay = 0;
>
> First of all, a comment to explain what's going on, please.
>
> Now -- if I understand correctly (and please tell if I'm wrong),
>
> 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?

>
>> +             else
>> +               {
>> +                 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);
>>
>> -             memcpy (record_list->u.mem.val, mem,
>> 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.not_replay = 1;
>
> A comment to explain what's happening, please.
>
>> +                   }
>> +                 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.not_replay = 1;

Do you think we need a warning in this part?

>
> And again, a comment.  This whole section needs more comments,
> but let's start with this, before it becomes even more difficult
> to understand.
>
>> +                           }
>> +                       }
>> +                     else
>> +                       {
>> +                         memcpy (record_list->u.mem.val, mem,
>> +                                 record_list->u.mem.len);
>> +                       }
>> +                   }
>> +               }
>>            }
>>          else
>>            {
>
>

Thanks,
Hui

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

[Michael Snyder]

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.

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

[Hui Zhu]

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.


Thanks,
Hui

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 3269 bytes --]

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.




[-- Attachment #2: memaccess.txt --]
[-- Type: text/plain, Size: 3385 bytes --]

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
 	    {

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

[Hui Zhu]

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

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

[Hui Zhu]

[-- 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,

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

[Michael Snyder]

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,

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

[Hui Zhu]

The record_exec_entry don't have big contact with core dump patch.

I think the new version is more clear.

On the other hand, there is not multithread work, core dump need
record_exec_entry, it need "mem_entry_not_accessible" too.
Because some lib address when work with core, cannot really access.

Thanks,
Hui

On Mon, Aug 3, 2009 at 13:01, Michael Snyder<msnyder@vmware.com> wrote:
> 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,
>
>

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

[Michael Snyder]

Hui Zhu wrote:
> The record_exec_entry don't have big contact with core dump patch.
> 
> I think the new version is more clear.

But we had this patch all worked out, and now you've added
a bunch of new stuff to it.  It's not a "new version" of the
"add not_replay" patch, it's a whole new change.

That's not the way we do it.  If you want to add new stuff,
you start a new thread, you don't add it into a thread that's
already in review.

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

[Hui Zhu]

On Tue, Aug 4, 2009 at 01:30, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> The record_exec_entry don't have big contact with core dump patch.
>>
>> I think the new version is more clear.
>
> But we had this patch all worked out, and now you've added
> a bunch of new stuff to it.  It's not a "new version" of the
> "add not_replay" patch, it's a whole new change.
>
> That's not the way we do it.  If you want to add new stuff,
> you start a new thread, you don't add it into a thread that's
> already in review.
>
>

OK.  I got it.

So the patch http://sourceware.org/ml/gdb-patches/2009-08/msg00006.html
looks OK.
Could you check it in?

I will update dump patch after this patch in.

Thanks,
Hui

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

Hui Zhu wrote:
> On Tue, Aug 4, 2009 at 01:30, Michael Snyder<msnyder@vmware.com> wrote:
>> Hui Zhu wrote:
>>> The record_exec_entry don't have big contact with core dump patch.
>>>
>>> I think the new version is more clear.
>> But we had this patch all worked out, and now you've added
>> a bunch of new stuff to it.  It's not a "new version" of the
>> "add not_replay" patch, it's a whole new change.
>>
>> That's not the way we do it.  If you want to add new stuff,
>> you start a new thread, you don't add it into a thread that's
>> already in review.
>>
>>
> 
> OK.  I got it.
> 
> So the patch http://sourceware.org/ml/gdb-patches/2009-08/msg00006.html
> looks OK.
> Could you check it in?
> 
> I will update dump patch after this patch in.
> 
> Thanks,
> Hui

Thanks.  Added some comments, and it's in as attached here.



[-- Attachment #2: accessible.txt --]
[-- Type: text/plain, Size: 4036 bytes --]

2009-08-04  Hui Zhu  <teawater@gmail.com>
	    Michael Snyder  <msnyder@vmware.com>

	* record.c (record_mem_entry): New field 'mem_entry_not_accessible'.
	(record_arch_list_add_mem): Initialize 'mem_entry_not_accessible'.
	(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.

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	4 Aug 2009 18:17:15 -0000
@@ -51,6 +51,9 @@ struct record_mem_entry
 {
   CORE_ADDR addr;
   int len;
+  /* Set this flag if target memory for this entry
+     can no longer be accessed.  */
+  int mem_entry_not_accessible;
   gdb_byte *val;
 };
 
@@ -275,6 +278,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 +731,55 @@ 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);
+	      /* Nothing to do if the entry is flagged not_accessible.  */
+	      if (!record_list->u.mem.mem_entry_not_accessible)
+		{
+		  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
+			/* Read failed -- 
+			   flag entry as not_accessible.  */
+		        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
+			    /* Write failed -- 
+			       flag entry as not_accessible.  */
+			    record_list->u.mem.mem_entry_not_accessible = 1;
+			}
+		      else
+		        {
+			  memcpy (record_list->u.mem.val, mem,
+				  record_list->u.mem.len);
+			}
+		    }
+		}
 	    }
 	  else
 	    {

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

[Hui Zhu]

Thanks.

Hui

On Wed, Aug 5, 2009 at 02:18, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> On Tue, Aug 4, 2009 at 01:30, Michael Snyder<msnyder@vmware.com> wrote:
>>>
>>> Hui Zhu wrote:
>>>>
>>>> The record_exec_entry don't have big contact with core dump patch.
>>>>
>>>> I think the new version is more clear.
>>>
>>> But we had this patch all worked out, and now you've added
>>> a bunch of new stuff to it.  It's not a "new version" of the
>>> "add not_replay" patch, it's a whole new change.
>>>
>>> That's not the way we do it.  If you want to add new stuff,
>>> you start a new thread, you don't add it into a thread that's
>>> already in review.
>>>
>>>
>>
>> OK.  I got it.
>>
>> So the patch http://sourceware.org/ml/gdb-patches/2009-08/msg00006.html
>> looks OK.
>> Could you check it in?
>>
>> I will update dump patch after this patch in.
>>
>> Thanks,
>> Hui
>
> Thanks.  Added some comments, and it's in as attached here.
>
>
>
> 2009-08-04  Hui Zhu  <teawater@gmail.com>
>            Michael Snyder  <msnyder@vmware.com>
>
>        * record.c (record_mem_entry): New field 'mem_entry_not_accessible'.
>        (record_arch_list_add_mem): Initialize 'mem_entry_not_accessible'.
>        (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.
>
> 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    4 Aug 2009 18:17:15 -0000
> @@ -51,6 +51,9 @@ struct record_mem_entry
>  {
>   CORE_ADDR addr;
>   int len;
> +  /* Set this flag if target memory for this entry
> +     can no longer be accessed.  */
> +  int mem_entry_not_accessible;
>   gdb_byte *val;
>  };
>
> @@ -275,6 +278,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 +731,55 @@ 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);
> +             /* Nothing to do if the entry is flagged not_accessible.  */
> +             if (!record_list->u.mem.mem_entry_not_accessible)
> +               {
> +                 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
> +                       /* Read failed --
> +                          flag entry as not_accessible.  */
> +                       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
> +                           /* Write failed --
> +                              flag entry as not_accessible.  */
> +                           record_list->u.mem.mem_entry_not_accessible = 1;
> +                       }
> +                     else
> +                       {
> +                         memcpy (record_list->u.mem.val, mem,
> +                                 record_list->u.mem.len);
> +                       }
> +                   }
> +               }
>            }
>          else
>            {
>
>