From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31481 invoked by alias); 30 Oct 2008 01:51:21 -0000 Received: (qmail 31460 invoked by uid 22791); 30 Oct 2008 01:51:19 -0000 X-Spam-Check-By: sourceware.org Received: from ti-out-0910.google.com (HELO ti-out-0910.google.com) (209.85.142.186) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 30 Oct 2008 01:51:14 +0000 Received: by ti-out-0910.google.com with SMTP id d10so114561tib.12 for ; Wed, 29 Oct 2008 18:51:10 -0700 (PDT) Received: by 10.110.53.14 with SMTP id b14mr6581767tia.45.1225331470790; Wed, 29 Oct 2008 18:51:10 -0700 (PDT) Received: by 10.110.42.9 with HTTP; Wed, 29 Oct 2008 18:51:10 -0700 (PDT) Message-ID: Date: Thu, 30 Oct 2008 03:01:00 -0000 From: teawater To: "Michael Snyder" Subject: Re: [reverse/record] adjust_pc_after_break in reverse execution mode? Cc: "Pedro Alves" , "gdb-patches@sourceware.org" In-Reply-To: <49079788.6000702@vmware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200810180210.16346.pedro@codesourcery.com> <200810240045.52818.pedro@codesourcery.com> <490118CB.5000500@vmware.com> <200810240250.20238.pedro@codesourcery.com> <49079788.6000702@vmware.com> 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/msg00695.txt.bz2 Maybe I can checked it in first. :) On Wed, Oct 29, 2008 at 06:51, Michael Snyder wrote: > 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; > >