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

Pedro Alves
2012-11-27T18:38:38Z

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