From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10753 invoked by alias); 18 Aug 2013 19:09:46 -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 10739 invoked by uid 89); 18 Aug 2013 19:09:46 -0000 X-Spam-SWARE-Status: No, score=-7.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Sun, 18 Aug 2013 19:09:45 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7IJ9h65024093 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 18 Aug 2013 15:09:44 -0400 Received: from host2.jankratochvil.net (ovpn-116-37.ams2.redhat.com [10.36.116.37]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r7IJ9ZO3010252 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Sun, 18 Aug 2013 15:09:38 -0400 Date: Sun, 18 Aug 2013 19:09:00 -0000 From: Jan Kratochvil To: Markus Metzger Cc: gdb-patches@sourceware.org Subject: Re: [patch v4 23/24] record-btrace: add (reverse-)stepping support Message-ID: <20130818190935.GR24153@host2.jankratochvil.net> References: <1372842874-28951-1-git-send-email-markus.t.metzger@intel.com> <1372842874-28951-24-git-send-email-markus.t.metzger@intel.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="UHN/qo2QbUvPLonB" Content-Disposition: inline In-Reply-To: <1372842874-28951-24-git-send-email-markus.t.metzger@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Virus-Found: No X-IsSubscribed: yes X-SW-Source: 2013-08/txt/msg00474.txt.bz2 --UHN/qo2QbUvPLonB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 6490 On Wed, 03 Jul 2013 11:14:33 +0200, Markus Metzger wrote: > There's an open regarding frame unwinding. When I start stepping, the frame > cache will still be based on normal unwinding as will the frame cached in the > thread's stepping context. This will prevent me from detecting that i stepped > into a subroutine. Where do you detect you have stepped into a subroutine? That is up to GDB after your to_wait returns, in handle_inferior_event. > To overcome that, I'm resetting the frame cache and setting the thread's > stepping cache based on the current frame - which is now computed using branch > tracing unwind. I had to split get_current_frame to avoid checks that would > prevent me from doing this. This is not correct, till to_wait finishes the inferior is still executing and you cannot query its current state (such as its frame/pc/register). I probably still miss why you do so. Proposing some hacked draft patch but for some testcases it FAILs for me; but they FAIL even without this patch as I run it on Nehalem. I understand I may miss some problem there, though. > It looks like I don't need any special support for breakpoints. Is there a > scenario where normal breakpoints won't work? You already handle it specially in BTHR_CONT and in BTHR_RCONT by breakpoint_here_p. As btrace does not record any data changes that may be enough. "record full" has different situation as it records data changes. I think it is fine as you wrote it. You could handle BTHR_CONT and BTHR_RCONT equally to BTHR_STEP and BTHR_RSTEP, just returning TARGET_WAITKIND_SPURIOUS instead of TARGET_WAITKIND_STOPPED. This way you would not need to handle specially breakpoint_here_p. But it would be sure slower. > Non-stop mode is not working. Do not allow record-btrace in non-stop mode. While that seems OK for the initial check-in I do not think it is convenient. Some users use for example Eclipse in non-stop mode, they would not be able to use btrace then as one cannot change non-stop state when the inferior is running. You can just disable the ALL_THREADS cases in record-btrace.c, can't you? This mail is not really reviewed yet as the design should be settled down first. > --- a/gdb/btrace.h > +++ b/gdb/btrace.h > @@ -149,6 +149,25 @@ struct btrace_call_history > struct btrace_call_iterator end; > }; > > +/* Branch trace thread flags. */ > +enum btrace_thread_flag > + { enum btrace_thread_flag { > + /* The thread is to be stepped forwards. */ > + BTHR_STEP = (1 << 0), > + > + /* The thread is to be stepped backwards. */ > + BTHR_RSTEP = (1 << 1), > + > + /* The thread is to be continued forwards. */ > + BTHR_CONT = (1 << 2), > + > + /* The thread is to be continued backwards. */ > + BTHR_RCONT = (1 << 3), > + > + /* The thread is to be moved. */ > + BTHR_MOVE = (BTHR_STEP | BTHR_RSTEP | BTHR_CONT | BTHR_RCONT) > + }; > + > /* Branch trace information per thread. > > This represents the branch trace configuration as well as the entry point > @@ -176,6 +195,9 @@ struct btrace_thread_info > becomes zero. */ > int level; > > + /* A bit-vector of btrace_thread_flag. */ > + unsigned int flags; enum btrace_thread_flag The values are then also properly displayed by GDB. > + > /* The instruction history iterator. */ > struct btrace_insn_history *insn_history; > [...] > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -1367,6 +1367,29 @@ 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) > { > @@ -1381,6 +1404,7 @@ get_current_frame (void) > error (_("No stack.")); > if (!target_has_memory) > error (_("No memory.")); > + > /* Traceframes are effectively a substitute for the live inferior. */ > if (get_traceframe_number () < 0) > { Unrelated patch chunk. But the get_current_frame() part of the patch should be dropped anyway. > @@ -1392,19 +1416,7 @@ get_current_frame (void) > error (_("Target is executing.")); > } > > - 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; > + return get_current_frame_nocheck (); > } > > /* The "selected" stack frame is used by default for local and arg [...] > + case BTHR_CONT: > + /* We're done if we're not replaying. */ > + if (replay == NULL) > + return btrace_step_no_history (); > + > + /* I'd much rather go from TP to its inferior, but how? */ find_inferior_pid (ptid_get_pid (tp->ptid)) Although I do not see why you prefer the inferior here. > + aspace = current_inferior ()->aspace; > + > + /* Determine the end of the instruction trace. */ > + btrace_insn_end (&end, btinfo); > + > + for (;;) > + { > + const struct btrace_insn *insn; > + > + /* We are always able to step at least once. */ > + steps = btrace_insn_next (replay, 1); > + gdb_assert (steps == 1); > + > + /* We stop replaying if we reached the end of the trace. */ > + if (btrace_insn_cmp (replay, &end) == 0) > + { > + record_btrace_stop_replaying (btinfo); > + return btrace_step_no_history (); > + } > + > + insn = btrace_insn_get (replay); > + gdb_assert (insn); > + > + DEBUG ("stepping %d (%s) ... %s", tp->num, > + target_pid_to_str (tp->ptid), > + core_addr_to_string_nz (insn->pc)); > + > + if (breakpoint_here_p (aspace, insn->pc)) > + return btrace_step_stopped (); > + } > + --UHN/qo2QbUvPLonB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline; filename="btrace-towait.patch" Content-length: 4297 diff --git a/gdb/btrace.h b/gdb/btrace.h index 22fabb5..8eceec4 100644 --- a/gdb/btrace.h +++ b/gdb/btrace.h @@ -27,6 +27,7 @@ list of sequential control-flow blocks, one such list per thread. */ #include "btrace-common.h" +#include "target.h" struct thread_info; struct btrace_function; @@ -198,6 +199,8 @@ struct btrace_thread_info /* A bit-vector of btrace_thread_flag. */ unsigned int flags; +struct target_waitstatus status; + /* The instruction history iterator. */ struct btrace_insn_history *insn_history; diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index 9feda30..633990a 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -1190,6 +1190,8 @@ static const struct frame_unwind record_btrace_frame_unwind = record_btrace_frame_dealloc_cache }; +static struct target_waitstatus record_btrace_step_thread (struct thread_info *tp); + /* Indicate that TP should be resumed according to FLAG. */ static void @@ -1209,6 +1211,10 @@ record_btrace_resume_thread (struct thread_info *tp, btrace_fetch (tp); btinfo->flags |= flag; + + +/* We only move a single thread. We're not able to correlate threads. */ +btinfo->status = record_btrace_step_thread (tp); } /* Find the thread to resume given a PTID. */ @@ -1248,6 +1254,7 @@ record_btrace_start_replaying (struct btrace_thread_info *btinfo) gdb_assert (btinfo->replay == NULL); btinfo->replay = replay; +#if 0 /* Make sure we're not using any stale registers or frames. */ registers_changed (); reinit_frame_cache (); @@ -1258,6 +1265,7 @@ record_btrace_start_replaying (struct btrace_thread_info *btinfo) insn = btrace_insn_get (replay); sal = find_pc_line (insn->pc, 0); set_step_info (frame, sal); +#endif return replay; } @@ -1271,6 +1279,8 @@ record_btrace_stop_replaying (struct btrace_thread_info *btinfo) btinfo->replay = NULL; } +static int forward_to_beneath; + /* The to_resume method of target record-btrace. */ static void @@ -1290,7 +1300,9 @@ record_btrace_resume (struct target_ops *ops, ptid_t ptid, int step, record_btrace_stop_replaying (&other->btrace); /* As long as we're not replaying, just forward the request. */ - if (!record_btrace_is_replaying () && execution_direction != EXEC_REVERSE) + forward_to_beneath = (!record_btrace_is_replaying () + && execution_direction != EXEC_REVERSE); + if (forward_to_beneath) { for (ops = ops->beneath; ops != NULL; ops = ops->beneath) if (ops->to_resume != NULL) @@ -1400,7 +1412,7 @@ record_btrace_step_thread (struct thread_info *tp) replay = btinfo->replay; flag = btinfo->flags & BTHR_MOVE; - btinfo->flags &= ~BTHR_MOVE; +// btinfo->flags &= ~BTHR_MOVE; DEBUG ("stepping %d (%s): %u", tp->num, target_pid_to_str (tp->ptid), flag); @@ -1517,7 +1529,7 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid, DEBUG ("wait %s (0x%x)", target_pid_to_str (ptid), options); /* As long as we're not replaying, just forward the request. */ - if (!record_btrace_is_replaying () && execution_direction != EXEC_REVERSE) + if (forward_to_beneath) { for (ops = ops->beneath; ops != NULL; ops = ops->beneath) if (ops->to_wait != NULL) @@ -1536,8 +1548,11 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid, return minus_one_ptid; } +#if 0 /* We only move a single thread. We're not able to correlate threads. */ *status = record_btrace_step_thread (tp); +#endif +*status=tp->btrace.status; /* Stop all other threads. */ if (!non_stop) @@ -1547,9 +1562,11 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid, /* Start record histories anew from the current position. */ record_btrace_clear_histories (&tp->btrace); +#if 0 /* GDB seems to need this. Without, a stale PC seems to be used resulting in the current location to be displayed incorrectly. */ registers_changed (); +#endif return tp->ptid; } diff --git a/gdb/target.h b/gdb/target.h index 4a20533..e85b063 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -62,7 +62,7 @@ struct expression; #include "memattr.h" #include "vec.h" #include "gdb_signals.h" -#include "btrace.h" +#include "btrace-common.h" #include "command.h" enum strata --UHN/qo2QbUvPLonB--