From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15125 invoked by alias); 28 Oct 2008 22:58:30 -0000 Received: (qmail 15114 invoked by uid 22791); 28 Oct 2008 22:58:29 -0000 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-2.vmware.com (HELO smtp-outbound-2.vmware.com) (65.115.85.73) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 28 Oct 2008 22:57:44 +0000 Received: from mailhost2.vmware.com (mailhost2.vmware.com [10.16.67.167]) by smtp-outbound-2.vmware.com (Postfix) with ESMTP id CEEA739068; Tue, 28 Oct 2008 15:57:41 -0700 (PDT) Received: from [10.20.92.59] (promb-2s-dhcp59.eng.vmware.com [10.20.92.59]) by mailhost2.vmware.com (Postfix) with ESMTP id BAA118E55B; Tue, 28 Oct 2008 15:57:41 -0700 (PDT) Message-ID: <49079788.6000702@vmware.com> Date: Wed, 29 Oct 2008 01:24:00 -0000 From: Michael Snyder User-Agent: Thunderbird 1.5.0.12 (X11/20080411) MIME-Version: 1.0 To: teawater CC: Pedro Alves , "gdb-patches@sourceware.org" Subject: Re: [reverse/record] adjust_pc_after_break in reverse execution mode? References: <200810180210.16346.pedro@codesourcery.com> <200810240045.52818.pedro@codesourcery.com> <490118CB.5000500@vmware.com> <200810240250.20238.pedro@codesourcery.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: 2008-10/txt/msg00687.txt.bz2 teawater wrote: > The old patch make my_waitpid_record set pc even if this is not a breakpoint. > So I make a new patch that my_waitpid_record just set pc when this is > a breakpoint. Well, before I can evaluate the patch, I need a test case to see what behavior it is fixing. Doesn't have to be a formal DEJAGNU script, just something like the printf example that you posted for the other bug. Right now, I am unable to get the reverse-20080930-branch to exhibit any bad behavior that I could attribute to this issue. It seems to work just fine... > 2008-10-24 Hui Zhu > > * record.c (record_wait): Check breakpint before forward > execute in replay mode. > Check breakpoint use function "breakpoint_inserted_here_p" > in replay mode. > Set pc if forward execute, gdbarch_decr_pc_after_break is not > 0 and this is not single step in replay mode. > > * linux-nat.c (my_waitpid_record): Add > gdbarch_decr_pc_after_break to pc if need. > > > > On Fri, Oct 24, 2008 at 17:57, teawater wrote: >> Hi buddies, >> >> This is the new patch that fix the break bug. >> >> But I think I still need to add some code to deal with signal. >> >> 2008-10-24 Hui Zhu >> >> * record.c (record_wait): Check breakpint before forward >> execute in replay mode. >> Check breakpoint use function "breakpoint_inserted_here_p" >> in replay mode. >> Set pc if forward execute, gdbarch_decr_pc_after_break is not >> 0 and this is not single step in replay mode. >> >> * linux-nat.c (my_waitpid_record): Add >> gdbarch_decr_pc_after_break to pc if need. >> >> Thanks, >> Hui >> >> On Fri, Oct 24, 2008 at 16:10, teawater wrote: >>> Thanks Pedro and Michael, >>> >>> I think the reason is P record let inferior step recycle in the >>> linux-nat target. >>> So when it break by breakpint, it will not let >>> (pc+gdbarch_decr_pc_after_break (gdbarch)). Then after >>> adjust_pc_after_break, The PC is error. >>> >>> I will try to deal with it. >>> >>> Hui >>> >>> On Fri, Oct 24, 2008 at 09:50, Pedro Alves wrote: >>>> On Friday 24 October 2008 01:37:31, Michael Snyder wrote: >>>>>> In sum, it appears that decr_pc_after_break doesn't matter when you have >>>>>> continguous breakpoints, as long as you get from from B1's address to B2's >>>>>> address by single-stepping. All is good then, it appears! >>>>> I agree, at least that is the conclusion I am leaning toward. >>>>> >>>> Not so fast! I knew I had to spend a little extra thinking about >>>> it, 'cause I knew something was broken, just couldn't find what. :-) >>>> *as long as you get from from B1's address to B2's address >>>> by single-stepping* was a restriction that doesn't always apply. >>>> >>>> Here's a test that will fail in forward record/replay mode, but not >>>> in normal "play" mode. >>>> >>>> volatile int global_foo = 0; >>>> >>>> int >>>> main (int argc, char **argv) >>>> { >>>> asm ("nop"); /* 1st insn */ >>>> asm ("nop"); /* 2nd insn */ >>>> asm ("nop"); /* 3rd insn */ >>>> asm ("nop"); /* 4th insn */ >>>> if (!global_foo) >>>> goto ahead; >>>> asm ("nop"); /* 5th insn */ >>>> asm ("nop"); /* 6th insn */ >>>> asm ("nop"); /* 7th insn */ >>>> asm ("nop"); /* 8th insn */ <<< break 1 here >>>> ahead: >>>> asm ("nop"); /* 9th insn */ <<< break 2 here >>>> end: >>>> return 0; >>>> } >>>> >>>> If you let the program reply until break 2 is hit, and assuming insn >>>> 8th and 9th are assembled as contiguous (they do on x86 -O0 for me), you'll >>>> see that adjust_pc_after_break will indeed make it appear that breakpoint >>>> 1 was hit. Now, nops are nops, but real code could have something >>>> else there... >>>> >>>> /me goes back to bed. >>>> >>>> -- >>>> Pedro Alves >>>> >> >> ------------------------------------------------------------------------ >> >> --- a/linux-nat.c >> +++ b/linux-nat.c >> @@ -514,7 +514,9 @@ my_waitpid_record (int pid, int *status, >> struct bp_location *bl; >> struct breakpoint *b; >> CORE_ADDR pc; >> + CORE_ADDR decr_pc_after_break; >> struct lwp_info *lp; >> + int is_breakpoint = 1; >> >> wait_begin: >> ret = my_waitpid (pid, status, flags); >> @@ -530,7 +532,7 @@ wait_begin: >> >> if (WIFSTOPPED (*status) && WSTOPSIG (*status) == SIGTRAP) >> { >> - /* Check if there is a breakpoint */ >> + /* Check if there is a breakpoint. */ >> pc = 0; >> registers_changed (); >> for (bl = bp_location_chain; bl; bl = bl->global_next) >> @@ -602,7 +604,26 @@ wait_begin: >> goto wait_begin; >> } >> >> + is_breakpoint = 0; >> + >> out: >> + /* Add gdbarch_decr_pc_after_break to pc because pc will be break at address >> + add gdbarch_decr_pc_after_break when inferior non-step execute. */ >> + if (is_breakpoint) >> + { >> + decr_pc_after_break = gdbarch_decr_pc_after_break >> + (get_regcache_arch (get_thread_regcache (pid_to_ptid (ret)))); >> + if (decr_pc_after_break) >> + { >> + if (!pc) >> + { >> + pc = regcache_read_pc (get_thread_regcache (pid_to_ptid (ret))); >> + } >> + regcache_write_pc (get_thread_regcache (pid_to_ptid (ret)), >> + pc + decr_pc_after_break); >> + } >> + } >> + >> return ret; >> } >> >> --- a/record.c >> +++ b/record.c >> @@ -497,6 +497,33 @@ record_wait (ptid_t ptid, struct target_ >> int continue_flag = 1; >> int first_record_end = 1; >> struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0); >> + CORE_ADDR tmp_pc; >> + >> + status->kind = TARGET_WAITKIND_STOPPED; >> + >> + /* Check breakpoint when forward execute. */ >> + if (execution_direction == EXEC_FORWARD) >> + { >> + tmp_pc = regcache_read_pc (regcache); >> + if (breakpoint_inserted_here_p (tmp_pc)) >> + { >> + if (record_debug) >> + { >> + fprintf_unfiltered (gdb_stdlog, >> + "Process record: break at 0x%s.\n", >> + paddr_nz (tmp_pc)); >> + } >> + if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache)) >> + && !record_resume_step) >> + { >> + regcache_write_pc (regcache, >> + tmp_pc + >> + gdbarch_decr_pc_after_break >> + (get_regcache_arch (regcache))); >> + } >> + goto replay_out; >> + } >> + } >> >> record_get_sig = 0; >> act.sa_handler = record_sig_handler; >> @@ -521,7 +548,6 @@ record_wait (ptid_t ptid, struct target_ >> >> /* Loop over the record_list, looking for the next place to >> stop. */ >> - status->kind = TARGET_WAITKIND_STOPPED; >> do >> { >> /* Check for beginning and end of log. */ >> @@ -588,10 +614,6 @@ record_wait (ptid_t ptid, struct target_ >> } >> else >> { >> - CORE_ADDR tmp_pc; >> - struct bp_location *bl; >> - struct breakpoint *b; >> - >> if (record_debug > 1) >> { >> fprintf_unfiltered (gdb_stdlog, >> @@ -632,35 +654,25 @@ record_wait (ptid_t ptid, struct target_ >> } >> >> /* check breakpoint */ >> - tmp_pc = read_pc (); >> - for (bl = bp_location_chain; bl; bl = bl->global_next) >> + tmp_pc = regcache_read_pc (regcache); >> + if (breakpoint_inserted_here_p (tmp_pc)) >> { >> - b = bl->owner; >> - gdb_assert (b); >> - if (b->enable_state != bp_enabled >> - && b->enable_state != bp_permanent) >> - continue; >> - >> - if (b->type == bp_watchpoint || b->type == bp_catch_fork >> - || b->type == bp_catch_vfork >> - || b->type == bp_catch_exec >> - || b->type == bp_hardware_watchpoint >> - || b->type == bp_read_watchpoint >> - || b->type == bp_access_watchpoint) >> + if (record_debug) >> { >> - continue; >> + fprintf_unfiltered (gdb_stdlog, >> + "Process record: break at 0x%s.\n", >> + paddr_nz (tmp_pc)); >> } >> - if (bl->address == tmp_pc) >> + if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache)) >> + && execution_direction == EXEC_FORWARD >> + && !record_resume_step) >> { >> - if (record_debug) >> - { >> - fprintf_unfiltered (gdb_stdlog, >> - "Process record: break at 0x%s.\n", >> - paddr_nz (tmp_pc)); >> - } >> - continue_flag = 0; >> - break; >> + regcache_write_pc (regcache, >> + tmp_pc + >> + gdbarch_decr_pc_after_break >> + (get_regcache_arch (regcache))); >> } >> + continue_flag = 0; >> } >> } >> if (execution_direction == EXEC_REVERSE) >> @@ -691,6 +703,7 @@ next: >> perror_with_name (_("Process record: sigaction")); >> } >> >> +replay_out: >> if (record_get_sig) >> { >> status->value.sig = TARGET_SIGNAL_INT;