From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16301 invoked by alias); 23 Oct 2008 23:28:58 -0000 Received: (qmail 16288 invoked by uid 22791); 23 Oct 2008 23:28:57 -0000 X-Spam-Check-By: sourceware.org Received: from smtp-outbound-1.vmware.com (HELO smtp-outbound-1.vmware.com) (65.115.85.69) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 23 Oct 2008 23:28:22 +0000 Received: from mailhost3.vmware.com (mailhost3.vmware.com [10.16.27.45]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id 5E61125000; Thu, 23 Oct 2008 16:28:19 -0700 (PDT) Received: from [10.20.92.59] (promb-2s-dhcp59.eng.vmware.com [10.20.92.59]) by mailhost3.vmware.com (Postfix) with ESMTP id 52B3FC9A10; Thu, 23 Oct 2008 16:28:19 -0700 (PDT) Message-ID: <4901076C.8040104@vmware.com> Date: Thu, 23 Oct 2008 23:28: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> <48FCC413.3040506@vmware.com> <200810210121.07914.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/msg00591.txt.bz2 Hui, can you hold off on this change? What I'm finding is that I can reproduce the bad behavior that Pedro demonstrated on the 20080930 branch, but ONLY if I omit Pedro's change in adjust_pc_after_break. If I add Pedro's change, I can no longer reproduce any bad behavior. I've added Pedro's change to the branch now -- why don't you temporarily take out all these changes, and then see if you can make it manifest a problem. teawater wrote: > Sorry for understand your mean so later Pedro. I made a new patch that > Set pc if forward execute and gdbarch_decr_pc_after_break is not 0 in > replay mode. How do you think about it? > > > And I think 20080930 branch is need your "adjust_pc_reverse.diff". Do > you mind I check it in? > > On Tue, Oct 21, 2008 at 08:21, Pedro Alves wrote: >> On Tuesday 21 October 2008 00:36:12, teawater wrote: >>> I think your mean is check breakpoint in address >>> read_pc()+gdbarch_decr_pc_after_break (gdbarch) in record_wait, right? >> Taking x86 as an example, when you're doing normal debugging and you >> hit a breakpoint (SIGTRAP), the first read_pc GDB does to check where >> what breakpoint was hit, will read back `breakpoint_PC + 1' --- GDB takes care >> getting rid of that `+ 1' offset in infrun.c:adjust_pc_after_break. The >> idea is for you to do the same as the kernel/hardware would --- still >> check for breakpoints at read_pc, but increment PC by 1 before reporting the >> breakpoint to GDB's core. E.g., see the `pc += gdbarch...' line from >> the patch I posted previously, something like: >> >> record.c:record_wait () >> { >> ... >> + /* Check for breakpoint hits in forward execution. */ >> + pc = read_pc (); >> + if (execution_direction == EXEC_FORWARD >> + && regular_breakpoint_inserted_here_p (pc) >> + /* && !single-stepping */) >> + { >> + status->kind = TARGET_WAITKIND_STOPPED; >> + status->value.sig = TARGET_SIGNAL_TRAP; >> + if (software_breakpoint_inserted_here_p (pc)) >> + { >> + pc += gdbarch_decr_pc_after_break (gdbarch); >> + write_pc (pc); >> + } >> + >> >> -- >> Pedro Alves >> >> >> ------------------------------------------------------------------------ >> >> --- a/record.c >> +++ b/record.c >> @@ -497,6 +497,30 @@ 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; >> + >> + /* 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))) >> + { >> + 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; >> @@ -588,10 +612,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 +652,24 @@ 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) >> { >> - 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 +700,7 @@ next: >> perror_with_name (_("Process record: sigaction")); >> } >> >> +replay_out: >> if (record_get_sig) >> { >> status->value.sig = TARGET_SIGNAL_INT;