From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32765 invoked by alias); 29 Nov 2012 16:39:32 -0000 Received: (qmail 32534 invoked by uid 22791); 29 Nov 2012 16:39:28 -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,TW_BT,TW_EG X-Spam-Check-By: sourceware.org Received: from mga09.intel.com (HELO mga09.intel.com) (134.134.136.24) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 29 Nov 2012 16:39:20 +0000 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 29 Nov 2012 08:38:35 -0800 X-ExtLoop1: 1 Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by orsmga001.jf.intel.com with ESMTP; 29 Nov 2012 08:38:57 -0800 Received: from irsmsx151.ger.corp.intel.com (163.33.192.59) by IRSMSX103.ger.corp.intel.com (163.33.3.157) with Microsoft SMTP Server (TLS) id 14.1.355.2; Thu, 29 Nov 2012 16:37:32 +0000 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.95]) by IRSMSX151.ger.corp.intel.com ([169.254.4.21]) with mapi id 14.01.0355.002; Thu, 29 Nov 2012 16:37:32 +0000 From: "Metzger, Markus T" To: Pedro Alves 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 Date: Thu, 29 Nov 2012 16:39:00 -0000 Message-ID: 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="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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/msg00881.txt.bz2 > -----Original Message----- > From: Pedro Alves [mailto:palves@redhat.com] > Sent: Tuesday, November 27, 2012 7:32 PM > To: Metzger, Markus T > Cc: gdb-patches@sourceware.org; markus.t.metzger@gmail.com; jan.kratochvi= l@redhat.com; tromey@redhat.com; > kettenis@gnu.org > Subject: Re: [patch v4 02/13] thread, btrace: add generic branch trace su= pport Thanks for your review! > 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 stru= ctures. I added a general comment to each of them and I also describe structure dec= larations in more detail. > > + > > +/* Disable branch tracing for @tp. Ignore errors. */ >=20 > "@tp" is not the standard GNU way to refer to arguments. > Write "TP". Always double-space after period that ends sentence. Fixed. I avoid referring to parameter names when possible. > > +static int > > +do_disconnect_btrace (struct thread_info *tp, void *ignored) >=20 >=20 > > + /* When killing the inferior, we may have lost our target before we = disable > > + branch tracing. */ >=20 > Hmm, how does that happen? Can you explain better? When I kill an inferior, e.g. using the kill or run command, I used to get = errors that the target does not support this operation. To avoid such confu= sing errors, I check whether the target supports btrace before I try to dis= able it. > > + if (target_supports_btrace ()) > > + target_disable_btrace (btp->target); >=20 >=20 > > +/* 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; > > +} > > + >=20 > Likewise, what kind of errors are expected here? This is to play safe and avoid confusing error messages at inappropriate mo= ments - and also to avoid that an iterate_over_threads () is aborted. I can= 't remember if I ever actually got an error, here. I put those try-catch ar= ound calls in notification handlers where I can't afford an exception. > +/* Functions to iterate over a thread's branch trace. > + There is one global iterator per thread. The iterator is reset impli= citly > + 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 iterat= oris > + 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 *); >=20 > 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. Fixed. > > +/* Return the current branch trace vector for a thread, or NULL if the= r is no > > + trace. */ > > +extern VEC (btrace_block_s) *get_btrace (struct thread_info *); >=20 > Typo "there". Fixed. > > /* See btrace.h. */ > > void >=20 > Space between comment and function start. Fixed. There are a lot of those - I got this wrong consistently;-) > > disable_btrace (struct thread_info *tp) > > { > > struct btrace_thread_info *btp =3D &tp->btrace; > > int errcode =3D 0; > > > > if (!btp->target) > > error (_("Branch tracing not enabled for %s."), > > target_pid_to_str (tp->ptid)); >=20 > No sure these errors are a good idea. Might be better to make > them idempotent. So that e.g., "thread apply all btrace" I have two functions - one giving an error message and another wrapped arou= nd it that silently ignores the error. The former is used for user commands= , the latter is used for handling notifications. See for example disconnect= _btrace () and disable_btrace (). In a later patch I also have wrappers aro= und enable_btrace () and disable_btrace () that turn the error into a warni= ng, since I use them in iterate_over_threads () context. Would you rather want me to silently ignore this or to pass a "silent" para= meter? But then, how can I ever be sure that no other function I'm calling = throws an exception? Would it make sense to put this "turn exception into warning" or "silently = ignore exception" behavior into iterate_over_threads ()? > > > > /* When killing the inferior, we may have lost our target before we d= isable > > branch tracing. */ > > if (target_supports_btrace ()) > > target_disable_btrace (btp->target); > > > > btp->target =3D NULL; > > VEC_free (btrace_block_s, btp->btrace); > > } >=20 >=20 >=20 >=20 > > /* 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)) >=20 > (Personally, I very much dislike pointer->boolean implicit conversions.) I find it less bulky and more natural to read (we're not doing !=3D 0 check= s for integers, either), but I'll stick to GDB's style, of course. Is this = !=3D NULL GDB style? > > { > > btp->btrace =3D target_read_btrace (btp->target); > > btp->iterator =3D -1; > > > > /* The first block ends at the current pc. */ > > if (!VEC_empty (btrace_block_s, btp->btrace)) > > { > > struct frame_info *frame =3D get_current_frame (); >=20 > 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. In another email, you suggest to use regcache_read_pc (), instead. I'll do = that. And I'll use get_thread_regcache () to obtain the regcache for the th= read I'm working on. > > > > if (frame) > > { >=20 > What's this check supposed to mean? get_current_frame never > returns NULL. I'm overly conservative, it seems. I'll remove those checks. > > struct btrace_block *head =3D > > VEC_index (btrace_block_s, btp->btrace, 0); >=20 > =3D goes at the start of the next line. Other instances of this in the > patch (and probably the series). Fixed. I got this consistently wrong. > > > > if (head && !head->end) > > head->end =3D get_frame_pc (frame); > > } > > } > > } > > } >=20 >=20 > > +/* See btrace.h. */ > > +struct btrace_block * > > +read_btrace (struct thread_info *tp, int index) > > +{ > > + struct btrace_thread_info *btp =3D &tp->btrace; > > + > > + if (index < 0) > > + error (_("Invalid index: %d."), index); >=20 > Can this happen normally, or should this be an assertion/internal > error? This may happen. I do not check the index the user types in the cli. Should= I rather handle this in the cli, instead? > --- 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); >=20 > #undef de_fault >=20 > @@ -4149,6 +4169,75 @@ target_ranged_break_num_registers (void) > return -1; > } >=20 > +/* See target.h. */ > +int > +target_supports_btrace (void) > +{ > + struct target_ops *t; > + > + for (t =3D current_target.beneath; t !=3D NULL; t =3D t->beneath) > + if (t->to_supports_btrace !=3D NULL) > + return t->to_supports_btrace (); > + > + return 0; > +} >=20 > 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. I removed the INHERIT/de_fault. Regards, Markus. Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen, Deutschland Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Peter Gleissner, Christian Lamprechter, Hannes Schwadere= r, Douglas Lusk Registergericht: Muenchen HRB 47456 Ust.-IdNr./VAT Registration No.: DE129385895 Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052