Re: [PATCH V2 5/5] Support tracepoints for ARM linux in GDBServer

Antoine Tremblay
2016-11-15T14:42:12Z

Yao Qi writes:

> On Thu, Nov 03, 2016 at 10:33:00AM -0400, Antoine Tremblay wrote:
>> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
>> index 3f9ff2b..3b3c371 100644
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -2350,6 +2350,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>> strcat (own_buf, ";EnableDisableTracepoints+");
>> strcat (own_buf, ";QTBuffer:size+");
>> strcat (own_buf, ";tracenz+");
>> + strcat (own_buf, ";TracepointKinds+");
>
> Tracepoint "Kinds" is only useful to arm so far, and it is not needed
> to other archs support tracepoint, like x86. We should only reply
> ";TracepointKinds+" on archs where it is useful.
>

OK I'll add a target method _supports_tracepoint_kinds to avoid that.

I've also moved the kind resolution in remote.c under a check for
tracepoint support like so :

/* Send the tracepoint kind if GDBServer supports it. */
if (remote_supports_tracepoint_kinds ())
{
/* Fetch the proper tracepoint kind. */
int kind = gdbarch_breakpoint_kind_from_pc (target_gdbarch (), &tpaddr);

xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":K%x", kind);
}

>> }
>>
>> if (target_supports_hardware_single_step ()
>> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
>> index 7700ad1..cdb2c1d 100644
>> --- a/gdb/gdbserver/tracepoint.c
>> +++ b/gdb/gdbserver/tracepoint.c
>> @@ -747,6 +747,11 @@ struct tracepoint
>> /* Link to the next tracepoint in the list. */
>> struct tracepoint *next;
>>
>> + /* Optional kind of the breakpoint to be used. Note this can mean
>> + different things for different archs as z0 breakpoint command.
>> + Value is -1 if not persent. */
>> + int32_t kind;
>
> This field is only useful to trap-based tracepoint. It signals that we
> need to create a sub-class trap_based_tracepoint of struct tracepoint.
>

Currently struct tracepoint is a merged struct if you will of all the
tracepoint types, fast, static, trap.

Moving to a subclass for trap-based tracepoints, would require making a
subclass for all the others too, static, fast. It would be quite
inconsistent otherwise.

While I do not object to this change, I think it should be part of
another patch series and that this change is orthogonal to the
tracepoint support for arm.

WDYT ?

>> +
>> #ifndef IN_PROCESS_AGENT
>> /* The list of actions to take when the tracepoint triggers, in
>> string/packet form. */
>> @@ -1813,6 +1818,7 @@ add_tracepoint (int num, CORE_ADDR addr)
>> tpoint->compiled_cond = 0;
>> tpoint->handle = NULL;
>> tpoint->next = NULL;
>> + tpoint->kind = -1;
>>
>> /* Find a place to insert this tracepoint into list in order to keep
>> the tracepoint list still in the ascending order. There may be
>> @@ -2484,6 +2490,7 @@ cmd_qtdp (char *own_buf)
>> ULONGEST num;
>> ULONGEST addr;
>> ULONGEST count;
>> + ULONGEST kind;
>> struct tracepoint *tpoint;
>> char *actparm;
>> char *packet = own_buf;
>> @@ -2550,6 +2557,12 @@ cmd_qtdp (char *own_buf)
>> tpoint->cond = gdb_parse_agent_expr (&actparm);
>> packet = actparm;
>> }
>> + else if (*packet == 'K')
>> + {
>> + ++packet;
>> + packet = unpack_varlen_hex (packet, &kind);
>> + tpoint->kind = kind;
>> + }
>> else if (*packet == '-')
>> break;
>> else if (*packet == '\0')
>> @@ -12293,6 +12307,10 @@ remote_download_tracepoint (struct target_ops *self, struct bp_location *loc)
>> stepping_actions);
>>
>> tpaddr = loc->address;
>> +
>> + /* Fetch the proper tracepoint kind. */
>> + gdbarch_remote_breakpoint_from_pc (target_gdbarch (), &tpaddr, &kind);
>> +
>
> This function is already removed recently.

Fixed. Thanks.

>
>> sprintf_vma (addrbuf, tpaddr);
>> xsnprintf (buf, BUF_SIZE, "QTDP:%x:%s:%c:%lx:%x", b->number,
>> addrbuf, /* address */
>> @@ -12367,6 +12385,11 @@ remote_download_tracepoint (struct target_ops *self, struct bp_location *loc)
>> "ignoring tp %d cond"), b->number);
>> }
>>
>> + /* Tracepoint Kinds are modeled after the breakpoint Z0 kind packet.
>
> What do you mean?

I meant that the kind field in the tracepoints is the same as the kind
field for the breakpoints.

I think that comment was more confusing than anything, kinds are
described in the doc anyway so I'll forgo that comment and just write:

/* Send the tracepoint kind if GDBServer supports it. */
if (remote_supports_tracepoint_kinds ())

>
>> diff --git a/gdb/testsuite/gdb.trace/collection.exp b/gdb/testsuite/gdb.trace/collection.exp
>> index f225429..a30234f 100644
>> --- a/gdb/testsuite/gdb.trace/collection.exp
>> +++ b/gdb/testsuite/gdb.trace/collection.exp
>> @@ -764,7 +764,12 @@ proc gdb_trace_collection_test {} {
>> gdb_collect_expression_test globals_test_func \
>> "globalarr\[\(l6, l7\)\]" "7" "a\[\(b, c\)\]"
>>
>> - gdb_collect_return_test
>> + #This architecture has no method to collect a return address.
>> + if { [is_aarch32_target] } {
>> + unsupported "collect \$_ret: This architecture has no method to collect a return address"
>> + } else {
>> + gdb_collect_return_test
>> + }
>
> You need to implement arm_gen_return_address.
>

Done. Thanks.

>>
>> gdb_collect_strings_test strings_test_func "locstr" "abcdef" "" \
>> "local string"
>> diff --git a/gdb/testsuite/gdb.trace/trace-common.h b/gdb/testsuite/gdb.trace/trace-common.h
>> index 60cf9e8..9d607f7 100644
>> --- a/gdb/testsuite/gdb.trace/trace-common.h
>> +++ b/gdb/testsuite/gdb.trace/trace-common.h
>> @@ -40,7 +40,8 @@ x86_trace_dummy ()
>> " call " SYMBOL(x86_trace_dummy) "\n" \
>> )
>>
>> -#elif (defined __aarch64__) || (defined __powerpc__)
>> +#elif (defined __aarch64__) || (defined __powerpc__) \
>> + || (defined __arm__ && !defined __thumb__)
>>
>> #define FAST_TRACEPOINT_LABEL(name) \
>> asm (" .global " SYMBOL(name) "\n" \
>> @@ -48,11 +49,18 @@ x86_trace_dummy ()
>> " nop\n" \
>> )
>>
>> -#elif (defined __s390__)
>> +#elif (defined __arm__ && defined __thumb2__)
>>
>> #define FAST_TRACEPOINT_LABEL(name) \
>> asm (" .global " SYMBOL(name) "\n" \
>> SYMBOL(name) ":\n" \
>> + " nop.w\n" \
>> + )
>> +
>> +#elif (defined __s390__)
>> +#define FAST_TRACEPOINT_LABEL(name) \
>> + asm (" .global " SYMBOL(name) "\n" \
>> + SYMBOL(name) ":\n" \
>> " mvc 0(8, %r15), 0(%r15)\n" \
>> )
>>
>
> (defined __arm__ && defined __thumb__) (thumb-1) is still not handled.

thumb-1 is not supported in the future fast tracepoints thus I had not
included it here but indeed it should work with normal tracepoints.

Fast tracepoints with thumb-1 should just error out anyway.

I'll add thumb-1 in there, thanks.