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

[Pedro Alves <alves.ped@gmail.com>]

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