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

Yao Qi
2016-11-10T14:01:19Z

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.

> }
>
> 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.

> +
> #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.

> 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?

> 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.

>
> 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.

--
Yao (é½å°§)