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

Pedro Alves
2012-11-27T18:32:26Z

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