From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24588 invoked by alias); 28 Nov 2013 21:16:38 -0000 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 Received: (qmail 24578 invoked by uid 89); 28 Nov 2013 21:16:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_50,RDNS_NONE,SPF_HELO_PASS,SPF_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 28 Nov 2013 21:16:36 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rASLGSxT032248 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 28 Nov 2013 16:16:28 -0500 Received: from host2.jankratochvil.net (ovpn-116-70.ams2.redhat.com [10.36.116.70]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id rASLGOda026776 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Thu, 28 Nov 2013 16:16:26 -0500 Date: Thu, 28 Nov 2013 22:35:00 -0000 From: Jan Kratochvil To: "Metzger, Markus T" Cc: "gdb-patches@sourceware.org" Subject: Re: [+rfc] Re: [patch v6 00/21] record-btrace: reverse Message-ID: <20131128211623.GA16695@host2.jankratochvil.net> References: <1379676639-31802-1-git-send-email-markus.t.metzger@intel.com> <20131006195913.GA2518@host2.jankratochvil.net> <20131127185727.GA18038@host2.jankratochvil.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="cNdxnHkX5QqsyA0e" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-11/txt/msg00893.txt.bz2 --cNdxnHkX5QqsyA0e Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 1329 On Thu, 28 Nov 2013 09:42:16 +0100, Metzger, Markus T wrote: > We store the frame_id of the current frame and do a single-step. > Then we try to detect stepping into a subroutine by unwinding > the stack frames and comparing the frame_id's with our stored > frame_id. OK, understood now. In fact the time you change btinfo->replay you also change register contents. Therefore the registers_changed_ptid() call is there right. For the frame_id change one could also provide mapping of old frame_id to new frame_id in frame_id_eq() but that is worse than just re-setting it. > Do you have a better idea? I agree in general. Just: Rather than get_current_frame_nocheck I find safer to just temporarily switch off the executing flag. There are many other checks which make sense which were omitted. Calling set_step_info seems needlessly intrusive to me, there is no need to re-set tp->current_symtab + tp->current_line. Despite in the moment of record_btrace_start_replaying() I see step_frame_id should be the current frame id it does not have to be. Such as when we reverse-continue, it will be null_frame_id. The attached patch(es) is on top of yours. I still believe inferior should resume + wait if it changes its PC. I am again not sure if the patch passes without FAILs due to my BTS hardware. Thanks, Jan --cNdxnHkX5QqsyA0e Content-Type: text/plain; charset=us-ascii Content-Disposition: inline; filename=1 Content-length: 2747 diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index c50e11b..4a57b51 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -1281,9 +1281,8 @@ record_btrace_start_replaying (struct thread_info *tp) { struct btrace_thread_info *btinfo; struct btrace_insn_iterator *replay; - const struct btrace_insn *insn; - struct symtab_and_line sal; - struct frame_info *frame; + volatile struct gdb_exception except; + int executing; btinfo = &tp->btrace; @@ -1291,24 +1290,50 @@ record_btrace_start_replaying (struct thread_info *tp) if (btinfo->begin == NULL) return NULL; - /* We start replaying at the end of the branch trace. This corresponds to the - current instruction. */ - replay = xmalloc (sizeof (*replay)); - btrace_insn_end (replay, btinfo); + executing = is_executing (tp->ptid); + TRY_CATCH (except, RETURN_MASK_ALL) + { + int update_step_frame_id, update_step_stack_frame_id; + struct frame_id frame_id; - /* We're not replaying, yet. */ - gdb_assert (btinfo->replay == NULL); - btinfo->replay = replay; + /* We start replaying at the end of the branch trace. This corresponds to the + current instruction. */ + replay = xmalloc (sizeof (*replay)); + btrace_insn_end (replay, btinfo); - /* Make sure we're not using any stale registers. */ - registers_changed_ptid (tp->ptid); + if (executing) + { + /* get_current_frame would error out otherwise. */ + set_executing (tp->ptid, 0); + } + + frame_id = get_frame_id (get_current_frame ()); - /* We just started replaying. The frame id cached for stepping is based - on unwinding, not on branch tracing. Recompute it. */ - frame = get_current_frame_nocheck (); - insn = btrace_insn_get (replay); - sal = find_pc_line (insn->pc, 0); - set_step_info (frame, sal); + update_step_frame_id = frame_id_eq (frame_id, tp->control.step_frame_id); + update_step_stack_frame_id = frame_id_eq (frame_id, + tp->control.step_stack_frame_id); + + /* We're not replaying, yet. */ + gdb_assert (btinfo->replay == NULL); + btinfo->replay = replay; + + /* Make sure we're not using any stale registers. */ + registers_changed_ptid (tp->ptid); + + /* We just started replaying. The frame id cached for stepping is based + on unwinding, not on branch tracing. Recompute it. */ + + frame_id = get_frame_id (get_current_frame ()); + + if (update_step_frame_id) + tp->control.step_frame_id = frame_id; + if (update_step_stack_frame_id) + tp->control.step_stack_frame_id = frame_id; + } + if (executing) + set_executing (tp->ptid, 1); + if (except.reason < 0) + throw_exception (except); return replay; } --cNdxnHkX5QqsyA0e Content-Type: text/plain; charset=us-ascii Content-Disposition: inline; filename=2 Content-length: 2047 diff --git a/gdb/frame.c b/gdb/frame.c index 5a6f107..0fd98ff 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1367,29 +1367,6 @@ unwind_to_current_frame (struct ui_out *ui_out, void *args) return 0; } -/* See frame.h. */ - -struct frame_info *get_current_frame_nocheck (void) -{ - if (current_frame == NULL) - { - struct frame_info *sentinel_frame = - create_sentinel_frame (current_program_space, get_current_regcache ()); - - if (catch_exceptions (current_uiout, unwind_to_current_frame, - sentinel_frame, RETURN_MASK_ERROR) != 0) - { - /* Oops! Fake a current frame? Is this useful? It has a PC - of zero, for instance. */ - current_frame = sentinel_frame; - } - } - - return current_frame; -} - -/* See frame.h. */ - struct frame_info * get_current_frame (void) { @@ -1415,7 +1392,19 @@ get_current_frame (void) error (_("Target is executing.")); } - return get_current_frame_nocheck (); + if (current_frame == NULL) + { + struct frame_info *sentinel_frame = + create_sentinel_frame (current_program_space, get_current_regcache ()); + if (catch_exceptions (current_uiout, unwind_to_current_frame, + sentinel_frame, RETURN_MASK_ERROR) != 0) + { + /* Oops! Fake a current frame? Is this useful? It has a PC + of zero, for instance. */ + current_frame = sentinel_frame; + } + } + return current_frame; } /* The "selected" stack frame is used by default for local and arg diff --git a/gdb/frame.h b/gdb/frame.h index cd2033d..f0da19e 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -242,10 +242,6 @@ enum frame_type error. */ extern struct frame_info *get_current_frame (void); -/* Similar to get_current_frame except that we omit all checks. May - return NULL if unwinding fails. */ -extern struct frame_info *get_current_frame_nocheck (void); - /* Does the current target interface have enough state to be able to query the current inferior for frame info, and is the inferior in a state where that is possible? */ --cNdxnHkX5QqsyA0e Content-Type: text/plain; charset=us-ascii Content-Disposition: inline; filename=3 Content-length: 1619 diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index c50e11b..4a57b51 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -1662,6 +1687,8 @@ record_btrace_goto_target (struct thread_info *tp, struct btrace_insn_iterator *goto_target; struct btrace_thread_info *btinfo; struct target_waitstatus ws; + struct btrace_insn_iterator target_it; + volatile struct gdb_exception exception; btinfo = &tp->btrace; @@ -1686,11 +1713,17 @@ record_btrace_goto_target (struct thread_info *tp, btinfo->flags |= BTHR_GOTO; btinfo->goto_target = goto_target; -#if 0 + TRY_CATCH (exception, RETURN_MASK_ALL) + { + +#if 1 if (goto_target != NULL) + target_it = *goto_target; + else + btrace_insn_end (&target_it, btinfo); tp->control.exception_resume_breakpoint = set_momentary_breakpoint_at_pc (target_gdbarch (), - btrace_insn_get (goto_target)->pc, + btrace_insn_get (&target_it)->pc, bp_until); #endif #if 0 @@ -1701,8 +1734,18 @@ record_btrace_goto_target (struct thread_info *tp, proceed ((CORE_ADDR) -1, GDB_SIGNAL_0, 0); #endif - if (btinfo->goto_target != NULL || (btinfo->flags & BTHR_GOTO) != 0) + // It will need a fix if reverse mode supports target-async mode. + if ((btinfo->flags & BTHR_GOTO) != 0) error (_("Record goto failed.")); + gdb_assert (btinfo->goto_target == NULL); + + } + if (exception.reason < 0) + { + xfree (btinfo->goto_target); + btinfo->goto_target = NULL; + btinfo->flags &= ~BTHR_GOTO; + } } /* The to_goto_record_begin method of target record-btrace. */ --cNdxnHkX5QqsyA0e--