From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26610 invoked by alias); 20 Oct 2008 12:10:09 -0000 Received: (qmail 26601 invoked by uid 22791); 20 Oct 2008 12:10:07 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 20 Oct 2008 12:09:27 +0000 Received: (qmail 11813 invoked from network); 20 Oct 2008 12:09:24 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 20 Oct 2008 12:09:24 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [reverse/record] adjust_pc_after_break in reverse execution mode? Date: Mon, 20 Oct 2008 12:10:00 -0000 User-Agent: KMail/1.9.9 Cc: Michael Snyder , teawater References: <200810180210.16346.pedro@codesourcery.com> <200810200109.55661.pedro@codesourcery.com> <48FBD342.5070905@vmware.com> In-Reply-To: <48FBD342.5070905@vmware.com> MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_/TH/IBU9gAyDspV" Message-Id: <200810201309.35333.pedro@codesourcery.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/msg00487.txt.bz2 --Boundary-00=_/TH/IBU9gAyDspV Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Content-length: 3002 A Monday 20 October 2008 01:39:30, Michael Snyder escreveu: > Pedro Alves wrote: > > On Sunday 19 October 2008 23:39:20, Michael Snyder wrote: > >> After codgitating for a bit (that's "thinking" when you're over 50), > >> I've decided that you're right. > >> > >> However, I have a new concern -- I'm worried about what it will do > >> when it's replaying but going forward. > >> > >> Could you possibly revisit your test and see what it does > >> if you record all the way to line 9 or 10, then back up > >> to line 6, then continue with breakpoints at 6 and 7? > >=20 > > Eh, you're right. It's broken. >=20 > Thought so. >=20 > See, the problem is that "adjust_pc_after_break" is assuming > memory-breakpoint semantics, but Process Record/Replay actually > implements hardware-breakpoint semantics. It watches the > instruction-address "bus" and stops when the PC matches the > address of a breakpoint. >=20 > I suspect this is probably a problem with other record/replay > back-ends too, but I haven't confirmed it yet. >=20 > Still, I think that the patch you committed was correct > for the reverse case.=20=20 > This is a corner case that reveals=20 > that "reverse" and "replay" are not synonymous. They certainly aren't. When replaying, I believe it's just best to behave as close as possible to when it the inferior is really running. =46rom the inferior control side, GDB be mostly as agnostic about "replay" vs normal run as possibly. IIUC from reading the code, I see two issues. 1) When going forward and in reply mode, breakpoint hits are being checked *after* a record item is replays. IIUC, we should check *before*, and report an adjusted PC. 2) Un-inserted breakpoints weren't accounted for AFAICT (GDB will un-inserted breakpoints temporarily when stepping over them). Maybe they are, I got lost. :-) There's a loop going through the bp_location_chain. Can you get rid of that and use regular_breakpoint_inserted_here_p or similars? Below is a 10 minutes hack at it, as a starting point. Replay stil isn't perfect, mainly because I got lost in the record_wait maze --- that, needs a bit of clean up. :-) >=20 > > (gdb) record > > (gdb) b 6 > > Breakpoint 2 at 0x8048352: file nop.c, line 6. > > (gdb) b 7 > > Breakpoint 3 at 0x8048353: file nop.c, line 7. > > (gdb) n > >=20 > > Breakpoint 3, main () at nop.c:7 > > 7 asm ("nop"); > > (gdb) n > > 8 asm ("nop"); > > (gdb) > > 9 asm ("nop"); > > (gdb) n > > 10 } > > (gdb) rc > > Continuing. > >=20 > > Breakpoint 3, main () at nop.c:7 > > 7 asm ("nop"); > > (gdb) rn > >=20 > > No more reverse-execution history. > > main () at nop.c:6 > > 6 asm ("nop"); > > (gdb) n > >=20 > > Breakpoint 2, main () at nop.c:6 > > 6 asm ("nop"); > > (gdb) > > 8 asm ("nop"); > > (gdb) > > 9 asm ("nop"); > > (gdb) > >=20 > >=20 > >=20 > > -- > > Pedro Alves >=20 >=20 --=20 Pedro Alves --Boundary-00=_/TH/IBU9gAyDspV Content-Type: text/x-diff; charset="utf-8"; name="record_decr_pc.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="record_decr_pc.diff" Content-length: 8073 --- gdb/record.c | 159 +++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 101 insertions(+), 58 deletions(-) Index: src/gdb/record.c =================================================================== --- src.orig/gdb/record.c 2008-10-20 00:48:50.000000000 +0100 +++ src/gdb/record.c 2008-10-20 13:02:38.000000000 +0100 @@ -497,6 +497,9 @@ 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 pc; + record_t *curr_record; + int first = 1; record_get_sig = 0; act.sa_handler = record_sig_handler; @@ -512,20 +515,13 @@ record_wait (ptid_t ptid, struct target_ Then set it to terminal_ours to make GDB get the signal. */ target_terminal_ours (); - /* In EXEC_FORWARD mode, record_list point to the tail of prev - instruction. */ - if (execution_direction == EXEC_FORWARD && record_list->next) - { - record_list = record_list->next; - } - /* 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. */ - if (execution_direction == EXEC_REVERSE + if (execution_direction == EXEC_REVERSE && record_list == &record_first) { /* Hit beginning of record log in reverse. */ @@ -539,8 +535,51 @@ record_wait (ptid_t ptid, struct target_ break; } + /* 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 (current_gdbarch); + write_pc (pc); + + if (sigaction (SIGALRM, &old_act, NULL)) + perror_with_name (_("Process record: sigaction")); + + discard_cleanups (old_cleanups); + return inferior_ptid; + } + + if (first) + { + first = 0; + /* In EXEC_FORWARD mode, record_list point to the tail of prev + instruction. */ + if (execution_direction == EXEC_FORWARD && record_list->next) + { + record_list = record_list->next; + } + } + + curr_record = record_list; + + if (execution_direction == EXEC_REVERSE) + { + if (record_list->prev) + record_list = record_list->prev; + } + else + { + if (record_list->next) + record_list = record_list->next; + } + /* set ptid, register and memory according to record_list */ - if (record_list->type == record_reg) + if (curr_record->type == record_reg) { /* reg */ gdb_byte reg[MAX_REGISTER_SIZE]; @@ -548,43 +587,43 @@ record_wait (ptid_t ptid, struct target_ { fprintf_unfiltered (gdb_stdlog, "Process record: record_reg 0x%s to inferior num = %d.\n", - paddr_nz ((CORE_ADDR)record_list), - record_list->u.reg.num); + paddr_nz ((CORE_ADDR)curr_record), + curr_record->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); + regcache_cooked_read (regcache, curr_record->u.reg.num, reg); + regcache_cooked_write (regcache, curr_record->u.reg.num, + curr_record->u.reg.val); + memcpy (curr_record->u.reg.val, reg, MAX_REGISTER_SIZE); } - else if (record_list->type == record_mem) + else if (curr_record->type == record_mem) { /* mem */ - gdb_byte *mem = alloca (record_list->u.mem.len); + gdb_byte *mem = alloca (curr_record->u.mem.len); if (record_debug > 1) { fprintf_unfiltered (gdb_stdlog, "Process record: record_mem 0x%s to inferior addr = 0x%s len = %d.\n", - paddr_nz ((CORE_ADDR)record_list), - paddr_nz (record_list->u.mem.addr), - record_list->u.mem.len); + paddr_nz ((CORE_ADDR)curr_record), + paddr_nz (curr_record->u.mem.addr), + curr_record->u.mem.len); } if (target_read_memory - (record_list->u.mem.addr, mem, record_list->u.mem.len)) + (curr_record->u.mem.addr, mem, curr_record->u.mem.len)) { error (_("Process record: read memory addr = 0x%s len = %d error."), - paddr_nz (record_list->u.mem.addr), - record_list->u.mem.len); + paddr_nz (curr_record->u.mem.addr), + curr_record->u.mem.len); } if (target_write_memory - (record_list->u.mem.addr, record_list->u.mem.val, - record_list->u.mem.len)) + (curr_record->u.mem.addr, curr_record->u.mem.val, + curr_record->u.mem.len)) { error (_ ("Process record: write memory addr = 0x%s len = %d error."), - paddr_nz (record_list->u.mem.addr), - record_list->u.mem.len); + paddr_nz (curr_record->u.mem.addr), + curr_record->u.mem.len); } - memcpy (record_list->u.mem.val, mem, record_list->u.mem.len); + memcpy (curr_record->u.mem.val, mem, curr_record->u.mem.len); } else { @@ -596,13 +635,13 @@ record_wait (ptid_t ptid, struct target_ { fprintf_unfiltered (gdb_stdlog, "Process record: record_end 0x%s to inferior need_dasm = %d.\n", - paddr_nz ((CORE_ADDR)record_list), - record_list->u.need_dasm); + paddr_nz ((CORE_ADDR)curr_record), + curr_record->u.need_dasm); } if (execution_direction == EXEC_FORWARD) { - need_dasm = record_list->u.need_dasm; + need_dasm = curr_record->u.need_dasm; } if (need_dasm) { @@ -631,45 +670,48 @@ record_wait (ptid_t ptid, struct target_ continue_flag = 0; } - /* check breakpoint */ - tmp_pc = read_pc (); - for (bl = bp_location_chain; bl; bl = bl->global_next) + if (execution_direction == EXEC_REVERSE) { - 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) - { - continue; - } - if (bl->address == tmp_pc) + /* check breakpoint */ + tmp_pc = read_pc (); + for (bl = bp_location_chain; bl; bl = bl->global_next) { - if (record_debug) + 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) + { + continue; + } + if (bl->address == tmp_pc) { - fprintf_unfiltered (gdb_stdlog, - "Process record: break at 0x%s.\n", - paddr_nz (tmp_pc)); + if (record_debug) + { + fprintf_unfiltered (gdb_stdlog, + "Process record: break at 0x%s.\n", + paddr_nz (tmp_pc)); + } + continue_flag = 0; + break; } - continue_flag = 0; - break; } } } if (execution_direction == EXEC_REVERSE) { - need_dasm = record_list->u.need_dasm; + need_dasm = curr_record->u.need_dasm; } } -next: +#if 0 if (continue_flag) { if (execution_direction == EXEC_REVERSE) @@ -683,6 +725,7 @@ next: record_list = record_list->next; } } +#endif } while (continue_flag); --Boundary-00=_/TH/IBU9gAyDspV--