From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26058 invoked by alias); 27 Nov 2012 18:38:38 -0000 Received: (qmail 25925 invoked by uid 22791); 27 Nov 2012 18:38:35 -0000 X-SWARE-Spam-Status: No, hits=-5.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_BT X-Spam-Check-By: sourceware.org Received: from mail-bk0-f41.google.com (HELO mail-bk0-f41.google.com) (209.85.214.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 27 Nov 2012 18:38:30 +0000 Received: by mail-bk0-f41.google.com with SMTP id jg9so5721777bkc.0 for ; Tue, 27 Nov 2012 10:38:28 -0800 (PST) Received: by 10.204.147.78 with SMTP id k14mr4818705bkv.7.1354041508433; Tue, 27 Nov 2012 10:38:28 -0800 (PST) Received: from [192.168.0.105] (bl16-4-221.dsl.telepac.pt. [188.81.4.221]) by mx.google.com with ESMTPS id t11sm11099001bkv.11.2012.11.27.10.38.26 (version=SSLv3 cipher=OTHER); Tue, 27 Nov 2012 10:38:27 -0800 (PST) Message-ID: <50B508A1.9090800@gmail.com> Date: Tue, 27 Nov 2012 18:38: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: Pedro Alves CC: markus.t.metzger@intel.com, 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> <50B5072E.4070709@redhat.com> In-Reply-To: <50B5072E.4070709@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 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: 2012-11/txt/msg00768.txt.bz2 On 11/27/2012 06:32 PM, Pedro Alves wrote: > 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); I meant to suggest bypassing frames, and using regcache_read_pc instead. >> } >> } >> } >> } > > >> +/* 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