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

[Pedro Alves <palves@redhat.com>]

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