From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32041 invoked by alias); 24 Jul 2009 03:11:29 -0000 Received: (qmail 32030 invoked by uid 22791); 24 Jul 2009 03:11:27 -0000 X-SWARE-Spam-Status: No, hits=-1.1 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_43,J_CHICKENPOX_44,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail-pz0-f203.google.com (HELO mail-pz0-f203.google.com) (209.85.222.203) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 24 Jul 2009 03:11:20 +0000 Received: by pzk41 with SMTP id 41so963838pzk.12 for ; Thu, 23 Jul 2009 20:11:18 -0700 (PDT) MIME-Version: 1.0 Received: by 10.142.164.10 with SMTP id m10mr534759wfe.140.1248405077978; Thu, 23 Jul 2009 20:11:17 -0700 (PDT) In-Reply-To: <4A69182D.9090004@vmware.com> References: <4A69182D.9090004@vmware.com> From: Hui Zhu Date: Fri, 24 Jul 2009 13:00:00 -0000 Message-ID: Subject: Re: [PREC/RFA] Add not_replay to make precord support release memory better To: Michael Snyder Cc: gdb-patches ml Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-07/txt/msg00593.txt.bz2 On Fri, Jul 24, 2009 at 10:10, Michael Snyder wrote: > Hui Zhu wrote: >> >> Hi, >> >> Now, in replay reverse mode, if get error with memory entry, it will >> output error. =A0But 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. =A0When replay forward mode, this memory entry will not >> really set to memory. > > I like the idea. =A0I 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 >> #include >> #include >> #include >> #include >> #include >> #include >> #include >> #include >> #include >> >> int >> main(int argc,char *argv[],char *envp[]) >> { >> =A0 =A0 =A0 =A0char =A0 =A0*buf; >> >> =A0 =A0 =A0 =A0buf =3D mmap (0, 1024, PROT_READ | PROT_WRITE, MAP_PRIVAT= E | >> MAP_ANONYMOUS, -1, 0); >> =A0 =A0 =A0 =A0if (buf =3D=3D (caddr_t) -1) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0perror ("mmap"); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (-errno); >> =A0 =A0 =A0 =A0} >> >> =A0 =A0 =A0 =A0buf[0] =3D 1; >> >> =A0 =A0 =A0 =A0munmap (buf, 1024); >> >> =A0 =A0 =A0 =A0return (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 >> >> This is free software: you are free to change and redistribute it. >> There is NO WARRANTY, to the extent permitted by law. =A0Type "show copy= ing" >> and "show warranty" for details. >> This GDB was configured as "i686-pc-linux-gnu". >> For bug reporting instructions, please see: >> ... >> 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=3D, argv=3D> optimized out>, envp=3D) >> =A0 =A0at 1.c:17 >> 17 =A0 =A0 =A0 =A0 =A0 =A0 =A0buf =3D mmap (0, 1024, PROT_READ | PROT_WR= ITE, MAP_PRIVATE | >> MAP_ANONYMOUS, -1, 0); >> (gdb) record >> (gdb) n >> During symbol reading, incomplete CFI data; unspecified registers >> (e.g., eax) at 0x8048422. >> 18 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (buf =3D=3D (caddr_t) -1) { >> (gdb) >> 23 =A0 =A0 =A0 =A0 =A0 =A0 =A0buf[0] =3D 1; >> (gdb) >> 25 =A0 =A0 =A0 =A0 =A0 =A0 =A0munmap (buf, 1024); >> (gdb) >> The next instruction is syscall munmap. =A0It will free the memory addr >> =3D 0xb7fe0000 len =3D 1024. =A0It will make record target get error. = =A0Do >> you want to stop the program?([y] or n) n >> 27 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (0); >> (gdb) rc >> Continuing. >> Process record: error reading memory at addr =3D 0xb7fe0000 len =3D 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 >> >> This is free software: you are free to change and redistribute it. >> There is NO WARRANTY, to the extent permitted by law. =A0Type "show copy= ing" >> and "show warranty" for details. >> This GDB was configured as "i686-pc-linux-gnu". >> For bug reporting instructions, please see: >> ... >> 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=3D, argv=3D> optimized out>, envp=3D) >> =A0 =A0at 1.c:17 >> 17 =A0 =A0 =A0 =A0 =A0 =A0 =A0buf =3D mmap (0, 1024, PROT_READ | PROT_WR= ITE, MAP_PRIVATE | >> MAP_ANONYMOUS, -1, 0); >> (gdb) record >> (gdb) n >> During symbol reading, incomplete CFI data; unspecified registers >> (e.g., eax) at 0x8048422. >> 18 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (buf =3D=3D (caddr_t) -1) { >> (gdb) >> 23 =A0 =A0 =A0 =A0 =A0 =A0 =A0buf[0] =3D 1; >> (gdb) >> 25 =A0 =A0 =A0 =A0 =A0 =A0 =A0munmap (buf, 1024); >> (gdb) >> The next instruction is syscall munmap. =A0It will free the memory addr >> =3D 0xb7fe0000 len =3D 1024. =A0It will make record target get error. = =A0Do >> you want to stop the program?([y] or n) n >> 27 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (0); >> (gdb) rc >> Continuing. >> >> No more reverse-execution history. >> main (argc=3D, argv=3D, >> envp=3D) at 1.c:17 >> 17 =A0 =A0 =A0 =A0 =A0 =A0 =A0buf =3D mmap (0, 1024, PROT_READ | PROT_WR= ITE, MAP_PRIVATE | >> MAP_ANONYMOUS, -1, 0); >> (gdb) n >> 18 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (buf =3D=3D (caddr_t) -1) { >> (gdb) >> 23 =A0 =A0 =A0 =A0 =A0 =A0 =A0buf[0] =3D 1; >> (gdb) >> 25 =A0 =A0 =A0 =A0 =A0 =A0 =A0munmap (buf, 1024); >> (gdb) rn >> 23 =A0 =A0 =A0 =A0 =A0 =A0 =A0buf[0] =3D 1; >> >> >> >> Please help me review it. >> >> Thanks, >> Hui >> >> 2009-07-21 =A0Hui Zhu =A0 >> >> =A0 =A0 =A0 =A0* record.c (record_mem_entry): Add not_replay. =A0If this= the >> =A0 =A0 =A0 =A0memory entry doesn't replay, it will set to 1. >> =A0 =A0 =A0 =A0(record_arch_list_add_mem): Initialize not_replay to 0. >> =A0 =A0 =A0 =A0(record_wait): In replay reverse mode, if get error with >> =A0 =A0 =A0 =A0memory entry, not output error, just set not_replay to 1. >> =A0 =A0 =A0 =A0In replay forward mode, if not_replay is set, don't set >> =A0 =A0 =A0 =A0memory 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: > > =A0 =A0 =A0 =A0* record.c (record_mem_entry): New field 'not_replay'. > =A0 =A0 =A0 =A0(record_arch_list_add_mem): Initialize not_replay to 0. > =A0 =A0 =A0 =A0(record_wait): Set 'not_replay' flag if target memory > =A0 =A0 =A0 =A0not readable. =A0Don't try to change target memory if > =A0 =A0 =A0 =A0'not_replay' is set. > OK. I will change it. > > >> --- >> =A0record.c | =A0 75 >> ++++++++++++++++++++++++++++++++++++++++++--------------------- >> =A01 file changed, 50 insertions(+), 25 deletions(-) >> >> --- a/record.c >> +++ b/record.c >> @@ -51,6 +51,7 @@ struct record_mem_entry >> =A0{ >> =A0 CORE_ADDR addr; >> =A0 int len; >> + =A0int not_replay; > > This needs a more descriptive name (and some comments!) OK. I will add more comments. > > How about something like: > > =A0 =A0/* Set this flag if target memory for this entry > =A0 =A0 =A0 can no longer be accessed. =A0*/ > =A0 =A0int mem_entry_not_accessible; > >> =A0 gdb_byte *val; >> =A0}; >> >> @@ -275,6 +276,7 @@ record_arch_list_add_mem (CORE_ADDR addr >> =A0 rec->type =3D record_mem; >> =A0 rec->u.mem.addr =3D addr; >> =A0 rec->u.mem.len =3D len; >> + =A0rec->u.mem.not_replay =3D 0; >> >> =A0 if (target_read_memory (addr, rec->u.mem.val, len)) >> =A0 =A0 { >> @@ -727,32 +729,55 @@ record_wait (struct target_ops *ops, >> =A0 =A0 =A0 =A0 =A0else if (record_list->type =3D=3D record_mem) >> =A0 =A0 =A0 =A0 =A0 =A0{ >> =A0 =A0 =A0 =A0 =A0 =A0 =A0/* mem */ >> - =A0 =A0 =A0 =A0 =A0 =A0 gdb_byte *mem =3D alloca (record_list->u.mem.l= en); >> - =A0 =A0 =A0 =A0 =A0 =A0 if (record_debug > 1) >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 fprintf_unfiltered (gdb_stdlog, >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "P= rocess record: record_mem %s to " >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "i= nferior addr =3D %s len =3D %d.\n", >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ho= st_address_to_string (record_list), >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pa= ddress (gdbarch, >> record_list->u.mem.addr), >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 re= cord_list->u.mem.len); >> - >> - =A0 =A0 =A0 =A0 =A0 =A0 if (target_read_memory >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (record_list->u.mem.addr, mem, record_= list->u.mem.len)) >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 error (_("Process record: error reading me= mory at " >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"addr =3D %s len =3D %d= ."), >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0paddress (gdbarch, record_l= ist->u.mem.addr), >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0record_list->u.mem.len); >> - >> - =A0 =A0 =A0 =A0 =A0 =A0 if (target_write_memory >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (record_list->u.mem.addr, record_list-= >u.mem.val, >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0record_list->u.mem.len)) >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 error (_ >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0("Process record: error wri= ting memory at " >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "addr =3D %s len =3D %d."), >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0paddress (gdbarch, record_l= ist->u.mem.addr), >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0record_list->u.mem.len); >> + =A0 =A0 =A0 =A0 =A0 =A0 if (record_list->u.mem.not_replay >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 && execution_direction !=3D EXEC_REVER= SE) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 record_list->u.mem.not_replay =3D 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? > >> + =A0 =A0 =A0 =A0 =A0 =A0 else >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gdb_byte *mem =3D alloca (record_list-= >u.mem.len); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (record_debug > 1) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fprintf_unfiltered (gdb_stdlog, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 "Process record: record_mem %s to >> " >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 "inferior addr =3D %s len =3D %d.\n", >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 host_address_to_string >> (record_list), >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 paddress (gdbarch, >> + >> record_list->u.mem.addr), >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 record_list->u.mem.len); >> >> - =A0 =A0 =A0 =A0 =A0 =A0 memcpy (record_list->u.mem.val, mem, >> record_list->u.mem.len); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (target_read_memory (record_list->u= .mem.addr, mem, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 record_list->u.mem.len)) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (execution_direction !=3D E= XEC_REVERSE) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 error (_("Process record: = error reading memory at >> " >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"addr = =3D %s len =3D %d."), >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0paddress (g= dbarch, >> record_list->u.mem.addr), >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0record_list= ->u.mem.len); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 else >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 record_list->u.mem.not_rep= lay =3D 1; > > A comment to explain what's happening, please. > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 else >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (target_write_memory (recor= d_list->u.mem.addr, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0record_list->u.mem.val, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0record_list->u.mem.len)) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (execution_directio= n !=3D EXEC_REVERSE) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 error (_("Process = record: error writing memory >> at " >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0"addr =3D %s len =3D %d."), >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pad= dress (gdbarch, >> record_list->u.mem.addr), >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0rec= ord_list->u.mem.len); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 else >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 record_list->u= .mem.not_replay =3D 1; Do you think we need a warning in this part? > > And again, a comment. =A0This whole section needs more comments, > but let's start with this, before it becomes even more difficult > to understand. > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 else >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 memcpy (record_list->u= .mem.val, mem, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 record= _list->u.mem.len); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 =A0 =A0} >> =A0 =A0 =A0 =A0 =A0else >> =A0 =A0 =A0 =A0 =A0 =A0{ > > Thanks, Hui