From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30793 invoked by alias); 27 Nov 2012 18:32:26 -0000 Received: (qmail 30782 invoked by uid 22791); 27 Nov 2012 18:32:25 -0000 X-SWARE-Spam-Status: No, hits=-7.8 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BT X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 27 Nov 2012 18:32:20 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qARIWHQB030872 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 27 Nov 2012 13:32:17 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qARIWFgL024351; Tue, 27 Nov 2012 13:32:15 -0500 Message-ID: <50B5072E.4070709@redhat.com> Date: Tue, 27 Nov 2012 18:32:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: markus.t.metzger@intel.com CC: gdb-patches@sourceware.org, markus.t.metzger@gmail.com, jan.kratochvil@redhat.com, tromey@redhat.com, kettenis@gnu.org Subject: Re: [patch v4 02/13] thread, btrace: add generic branch trace support References: <1354013351-14791-1-git-send-email-markus.t.metzger@intel.com> <1354013351-14791-3-git-send-email-markus.t.metzger@intel.com> In-Reply-To: <1354013351-14791-3-git-send-email-markus.t.metzger@intel.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2012-11/txt/msg00766.txt.bz2 My main comment for this patch is that btrace.h or btrace-common.h lack a general overview of what branch tracing is, and the role of the data structures. > + > +/* Disable branch tracing for @tp. Ignore errors. */ "@tp" is not the standard GNU way to refer to arguments. Write "TP". Always double-space after period that ends sentence. > +static int > +do_disconnect_btrace (struct thread_info *tp, void *ignored) > + /* When killing the inferior, we may have lost our target before we disable > + branch tracing. */ Hmm, how does that happen? Can you explain better? > + if (target_supports_btrace ()) > + target_disable_btrace (btp->target); > +/* Disable branch tracing for @tp. Ignore errors. */ > +static int > +do_disconnect_btrace (struct thread_info *tp, void *ignored) > +{ > + if (tp->btrace.target) > + { > + volatile struct gdb_exception error; > + > + TRY_CATCH (error, RETURN_MASK_ERROR) > + disable_btrace (tp); > + } > + > + return 0; > +} > + Likewise, what kind of errors are expected here? +/* Functions to iterate over a thread's branch trace. + There is one global iterator per thread. The iterator is reset implicitly + when branch trace for this thread changes. + On success, read_btrace sets the iterator to the returned trace entry. + Returns the selected block or NULL if there is no trace or the iteratoris + out of bounds. */ +extern struct btrace_block *read_btrace (struct thread_info *, int); +extern struct btrace_block *prev_btrace (struct thread_info *); +extern struct btrace_block *next_btrace (struct thread_info *); Typo "iteratoris". Why is there an iterator per thread? I realize later patches may make that clearer, but from reading this code, it's natural do draw a parallel to "selected frame", and in that case, you don't have one per-thread. > +/* Return the current branch trace vector for a thread, or NULL if ther is no > + trace. */ > +extern VEC (btrace_block_s) *get_btrace (struct thread_info *); Typo "there". > /* See btrace.h. */ > void Space between comment and function start. > disable_btrace (struct thread_info *tp) > { > struct btrace_thread_info *btp = &tp->btrace; > int errcode = 0; > > if (!btp->target) > error (_("Branch tracing not enabled for %s."), > target_pid_to_str (tp->ptid)); No sure these errors are a good idea. Might be better to make them idempotent. So that e.g., "thread apply all btrace" > > /* When killing the inferior, we may have lost our target before we disable > branch tracing. */ > if (target_supports_btrace ()) > target_disable_btrace (btp->target); > > btp->target = NULL; > VEC_free (btrace_block_s, btp->btrace); > } > /* Update @btp's trace data in case of new trace. */ > static void > update_btrace (struct btrace_thread_info *btp) > { > if (btp->target && target_btrace_has_changed (btp->target)) (Personally, I very much dislike pointer->boolean implicit conversions.) > { > btp->btrace = target_read_btrace (btp->target); > btp->iterator = -1; > > /* The first block ends at the current pc. */ > if (!VEC_empty (btrace_block_s, btp->btrace)) > { > struct frame_info *frame = get_current_frame (); This get_current_frame call here looks fishy. This function takes a btrace_thread_info, and its callers work with a thread_info directly, which indicates that they may work with some current thread other than the thread passed in as argument. > > if (frame) > { What's this check supposed to mean? get_current_frame never returns NULL. > struct btrace_block *head = > VEC_index (btrace_block_s, btp->btrace, 0); = goes at the start of the next line. Other instances of this in the patch (and probably the series). > > if (head && !head->end) > head->end = get_frame_pc (frame); > } > } > } > } > +/* See btrace.h. */ > +struct btrace_block * > +read_btrace (struct thread_info *tp, int index) > +{ > + struct btrace_thread_info *btp = &tp->btrace; > + > + if (index < 0) > + error (_("Invalid index: %d."), index); Can this happen normally, or should this be an assertion/internal error? --- a/gdb/target.c +++ b/gdb/target.c @@ -701,6 +701,11 @@ update_current_target (void) INHERIT (to_traceframe_info, t); INHERIT (to_use_agent, t); INHERIT (to_can_use_agent, t); + INHERIT (to_supports_btrace, t); + INHERIT (to_enable_btrace, t); + INHERIT (to_disable_btrace, t); + INHERIT (to_btrace_has_changed, t); + INHERIT (to_read_btrace, t); INHERIT (to_magic, t); INHERIT (to_supports_evaluation_of_breakpoint_conditions, t); INHERIT (to_can_run_breakpoint_commands, t); @@ -943,6 +948,21 @@ update_current_target (void) (int (*) (void)) return_zero); de_fault (to_execution_direction, default_execution_direction); + de_fault (to_supports_btrace, + (int (*) (void)) + return_zero); + de_fault (to_enable_btrace, + (struct btrace_target_info * (*) (ptid_t)) + tcomplain); + de_fault (to_disable_btrace, + (void (*) (struct btrace_target_info *)) + tcomplain); + de_fault (to_btrace_has_changed, + (int (*) (struct btrace_target_info *)) + tcomplain); + de_fault (to_read_btrace, + (VEC (btrace_block_s) * (*) (struct btrace_target_info *)) + tcomplain); #undef de_fault @@ -4149,6 +4169,75 @@ target_ranged_break_num_registers (void) return -1; } +/* See target.h. */ +int +target_supports_btrace (void) +{ + struct target_ops *t; + + for (t = current_target.beneath; t != NULL; t = t->beneath) + if (t->to_supports_btrace != NULL) + return t->to_supports_btrace (); + + return 0; +} You either implement target_supports_btrace like this, doing the explicit walk, or use the INHERIT/de_fault mechanism, and define target_supports_btrace as macro that calls current_target.to_supports_btrace. Never both ways at the same time. -- Pedro Alves