RE: [patch v4 02/13] thread, btrace: add generic branch trace support

["Metzger, Markus T" <markus.t.metzger@intel.com>]

> -----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.kratochvil@redhat.com; tromey@redhat.com;
> kettenis@gnu.org
> Subject: Re: [patch v4 02/13] thread, btrace: add generic branch trace support

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 structures.

I added a general comment to each of them and I also describe structure declarations in more detail.


> > +
> > +/* 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.

Fixed. I avoid referring to parameter names when possible.


> > +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?

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 confusing errors, I check whether the target supports btrace before I try to disable it.


> > +  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?

This is to play safe and avoid confusing error messages at inappropriate moments - 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 around 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 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.

Fixed.


> > +/* 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".

Fixed.


> > /* See btrace.h.  */
> > void
> 
> 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 = &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"

I have two functions - one giving an error message and another wrapped around 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 around enable_btrace () and disable_btrace () that turn the error into a warning, since I use them in iterate_over_threads () context.

Would you rather want me to silently ignore this or to pass a "silent" parameter? 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 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.)

I find it less bulky and more natural to read (we're not doing != 0 checks for integers, either), but I'll stick to GDB's style, of course. Is this != NULL GDB style?


> >     {
> >       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.

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 thread I'm working on.


> >
> > 	  if (frame)
> > 	    {
> 
> 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 =
> > 		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).

Fixed. I got this consistently wrong.


> >
> > 	      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?

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);
> 
>  #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.

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 Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052