diff mbox

[V2,5/5] Support tracepoints for ARM linux in GDBServer

Message ID 20161103143300.24934-6-antoine.tremblay@ericsson.com
State New
Headers show

Commit Message

Antoine Tremblay Nov. 3, 2016, 2:33 p.m. UTC
This patch adds support for tracepoints for ARM linux in GDBServer.

To enable this, this patch introduces a new :K (kind) field in the
QTDP packet to encode the breakpoint kind, this is the same kind as a z0
packet.

This is the new qSupported feature: TracepointKinds

This field is decoded by sw_breakpoint_from_kind target ops in linux-low.

Tested on Ubuntu 14.04 ARMv7 and x86 with no regression.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:

	* NEWS: Add news for tracepoins on ARM.

gdb/doc/ChangeLog:

	* gdb.texinfo (General Query Packets): Add TracepointKinds packet.
	(ARM Breakpoint Kinds): Add QTDP reference.
	(Tracepoint Packets): Add kind parameter to QTDP packet.

gdb/gdbserver/ChangeLog:

	* linux-arm-low.c (arm_supports_tracepoints): New function.
	(struct linux_target_ops) <supports_tracepoints>: Initialize.
	* mem-break.c (set_breakpoint_at_with_kind): New function.
	* mem-break.h (set_breakpoint_at_with_kind): New function declaration.
	* server.c (handle_query): Add TracepointsKinds feature.
	* tracepoint.c (struct tracepoint) <kind>: New field.
	(add_tracepoint): Initialize kind field.
	(cmd_qtdp): Handle kind field 'K'.
	(install_tracepoint): Use set_breakpoint_at_with_kind when kind is
	present.
	(cmd_qtstart): Likewise.

gdb/ChangeLog:

	* remote.c (remote_supports_tracepoint_kinds): New function declaration.
	(PACKET_TracepointKinds): New enum field.
	(remote_protocol_features[]): New TracepointKinds element.
	(remote_supports_tracepoint_kinds): New function.
	(remote_download_tracepoint): Fetch the breakpoint kind and send
	it as K parameter to QTDP packet.
	(_initialize_remote): Add TracepointKinds packet_config_cmd.

gdb/testsuite/ChangeLog:

	* gdb.trace/collection.exp (gdb_collect_return_test): Set test
	unsupported for arm/aarch32 targets as it's not supported by the
	arch.
	* gdb.trace/trace-common.h: Add ARM fast tracepoint label to allow
	tracepoints tests.
	* lib/trace-support.exp: Add arm/aarch32 target support.
---
 gdb/NEWS                               |  2 ++
 gdb/doc/gdb.texinfo                    | 23 ++++++++++++++----
 gdb/gdbserver/linux-arm-low.c          | 10 +++++++-
 gdb/gdbserver/mem-break.c              | 13 ++++++++++
 gdb/gdbserver/mem-break.h              |  7 ++++++
 gdb/gdbserver/server.c                 |  1 +
 gdb/gdbserver/tracepoint.c             | 43 ++++++++++++++++++++++++++++------
 gdb/remote.c                           | 27 +++++++++++++++++++++
 gdb/testsuite/gdb.trace/collection.exp |  7 +++++-
 gdb/testsuite/gdb.trace/trace-common.h | 12 ++++++++--
 gdb/testsuite/lib/trace-support.exp    |  3 +++
 11 files changed, 132 insertions(+), 16 deletions(-)

Comments

Eli Zaretskii Nov. 3, 2016, 5:51 p.m. UTC | #1
> From: Antoine Tremblay <antoine.tremblay@ericsson.com>
> CC: Antoine Tremblay <antoine.tremblay@ericsson.com>
> Date: Thu, 3 Nov 2016 10:33:00 -0400
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a6b1282..233f11e 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,8 @@
>  
>  *** Changes since GDB 7.12
>  
> +* Support for tracepoints on arm-linux was added in GDBServer.
> +
>  * Building GDB and GDBserver now requires a C++11 compiler.
>  
>    For example, GCC 4.8 or later.

This part is OK.

> +encodings as described below. If a @samp{K} is present, it
> +indicates a target specific breakpoint kind. The kind can be the

Please use @var{kind} here, in reference to the packet parameter.

> +length of the breakpoint. E.g., the arm and mips can insert either a
> +2 or 4 byte breakpoint or have additional meaning see
> +@ref{Architecture-Specific Protocol Details}. If the trailing @samp{-}
> +is present, further @samp{QTDP} packets will follow to specify this
> +tracepoint's actions.

This paragraph needs to use 2 spaces between sentences, not one.

The patch for the manual is OK with these gotchas fixed.

Thanks.
Antoine Tremblay Nov. 3, 2016, 6:12 p.m. UTC | #2
Eli Zaretskii writes:

>> From: Antoine Tremblay <antoine.tremblay@ericsson.com>
>> CC: Antoine Tremblay <antoine.tremblay@ericsson.com>
>> Date: Thu, 3 Nov 2016 10:33:00 -0400
>> 
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index a6b1282..233f11e 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -3,6 +3,8 @@
>>  
>>  *** Changes since GDB 7.12
>>  
>> +* Support for tracepoints on arm-linux was added in GDBServer.
>> +
>>  * Building GDB and GDBserver now requires a C++11 compiler.
>>  
>>    For example, GCC 4.8 or later.
>
> This part is OK.
>
>> +encodings as described below. If a @samp{K} is present, it
>> +indicates a target specific breakpoint kind. The kind can be the
>
> Please use @var{kind} here, in reference to the packet parameter.
>

Ooops fixed.

>> +length of the breakpoint. E.g., the arm and mips can insert either a
>> +2 or 4 byte breakpoint or have additional meaning see
>> +@ref{Architecture-Specific Protocol Details}. If the trailing @samp{-}
>> +is present, further @samp{QTDP} packets will follow to specify this
>> +tracepoint's actions.
>
> This paragraph needs to use 2 spaces between sentences, not one.
>

Right, fixed.

> The patch for the manual is OK with these gotchas fixed.
>
> Thanks.
Yao Qi Nov. 10, 2016, 2:01 p.m. UTC | #3
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.
Antoine Tremblay Nov. 15, 2016, 2:36 p.m. UTC | #4
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.
Yao Qi Nov. 16, 2016, 8:49 p.m. UTC | #5
On Tue, Nov 15, 2016 at 2:36 PM, Antoine Tremblay
<antoine.tremblay@ericsson.com> wrote:
>>
>> 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.

Yes, that is what we should do.  Before we add something new, we need to
clean up the existing code if necessary.

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

It is not orthogonal to the tracepoint support.  In contrary, we must
"sub-struct" or "sub-class" tracepoint first, and them add "kind"
field for trap-based tracepoint.   Note that "struct tracepoint" is
used in IPA as well.
diff mbox

Patch

diff --git a/gdb/NEWS b/gdb/NEWS
index a6b1282..233f11e 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,8 @@ 
 
 *** Changes since GDB 7.12
 
+* Support for tracepoints on arm-linux was added in GDBServer.
+
 * Building GDB and GDBserver now requires a C++11 compiler.
 
   For example, GCC 4.8 or later.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index df548dc..7df63ee 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -37088,6 +37088,11 @@  These are the currently defined stub features and their properties:
 @tab @samp{-}
 @tab No
 
+@item @samp{TracepointKinds}
+@tab No
+@tab @samp{-}
+@tab No
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -37310,6 +37315,9 @@  The remote stub understands the @samp{QThreadEvents} packet.
 @item no-resumed
 The remote stub reports the @samp{N} stop reply.
 
+@item TracepointKinds
+The remote stub reports the @samp{:K} kind parameter for @samp{QTDP} packets.
+
 @end table
 
 @item qSymbol::
@@ -37820,7 +37828,8 @@  details of XML target descriptions for each architecture.
 @subsubsection @acronym{ARM} Breakpoint Kinds
 @cindex breakpoint kinds, @acronym{ARM}
 
-These breakpoint kinds are defined for the @samp{Z0} and @samp{Z1} packets.
+These breakpoint kinds are defined for the @samp{Z0}, @samp{Z1}
+and @samp{QTDP} packets.
 
 @table @r
 
@@ -37900,7 +37909,7 @@  tracepoints (@pxref{Tracepoints}).
 
 @table @samp
 
-@item QTDP:@var{n}:@var{addr}:@var{ena}:@var{step}:@var{pass}[:F@var{flen}][:X@var{len},@var{bytes}]@r{[}-@r{]}
+@item QTDP:@var{n}:@var{addr}:@var{ena}:@var{step}:@var{pass}[:F@var{flen}][:X@var{len},@var{bytes}][:K@var{kind}]@r{[}-@r{]}
 @cindex @samp{QTDP} packet
 Create a new tracepoint, number @var{n}, at @var{addr}.  If @var{ena}
 is @samp{E}, then the tracepoint is enabled; if it is @samp{D}, then
@@ -37911,9 +37920,13 @@  the number of bytes that the target should copy elsewhere to make room
 for the tracepoint.  If an @samp{X} is present, it introduces a
 tracepoint condition, which consists of a hexadecimal length, followed
 by a comma and hex-encoded bytes, in a manner similar to action
-encodings as described below.  If the trailing @samp{-} is present,
-further @samp{QTDP} packets will follow to specify this tracepoint's
-actions.
+encodings as described below. If a @samp{K} is present, it
+indicates a target specific breakpoint kind. The kind can be the
+length of the breakpoint. E.g., the arm and mips can insert either a
+2 or 4 byte breakpoint or have additional meaning see
+@ref{Architecture-Specific Protocol Details}. If the trailing @samp{-}
+is present, further @samp{QTDP} packets will follow to specify this
+tracepoint's actions.
 
 Replies:
 @table @samp
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index ed9b356..a1ca9b9 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -1032,6 +1032,14 @@  arm_regs_info (void)
     return &regs_info_arm;
 }
 
+/* Implementation of the linux_target_ops method "support_tracepoints".  */
+
+static int
+arm_supports_tracepoints (void)
+{
+  return 1;
+}
+
 struct linux_target_ops the_low_target = {
   arm_arch_setup,
   arm_regs_info,
@@ -1058,7 +1066,7 @@  struct linux_target_ops the_low_target = {
   arm_new_fork,
   arm_prepare_to_resume,
   NULL, /* process_qsupported */
-  NULL, /* supports_tracepoints */
+  arm_supports_tracepoints,
   NULL, /* get_thread_area */
   NULL, /* install_fast_tracepoint_jump_pad */
   NULL, /* emit_ops */
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index bee9c30..c8fb7c9 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -870,6 +870,19 @@  set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
   return set_breakpoint_type_at (other_breakpoint, where, handler);
 }
 
+/* See mem-break.h  */
+
+struct breakpoint *
+set_breakpoint_at_with_kind (CORE_ADDR where,
+			     int (*handler) (CORE_ADDR),
+			     int kind)
+{
+  int err_ignored;
+
+  return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
+			 where, kind, handler,
+			 &err_ignored);
+}
 
 static int
 delete_raw_breakpoint (struct process_info *proc, struct raw_breakpoint *todel)
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index 9e7ee93..07c6894 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -148,6 +148,13 @@  int gdb_breakpoint_here (CORE_ADDR where);
 struct breakpoint *set_breakpoint_at (CORE_ADDR where,
 				      int (*handler) (CORE_ADDR));
 
+/* Same as set_breakpoint_at but allow the kind to be specified */
+
+struct breakpoint *set_breakpoint_at_with_kind (CORE_ADDR where,
+						int (*handler)(CORE_ADDR),
+						int kind);
+
+
 /* Delete a breakpoint.  */
 
 int delete_breakpoint (struct breakpoint *bkpt);
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+");
 	}
 
       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;
+
 #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')
@@ -2564,11 +2577,13 @@  cmd_qtdp (char *own_buf)
 	}
 
       trace_debug ("Defined %stracepoint %d at 0x%s, "
-		   "enabled %d step %" PRIu64 " pass %" PRIu64,
+		   "enabled %d step %" PRIu64 " pass %" PRIu64
+		   " kind %" PRId32,
 		   tpoint->type == fast_tracepoint ? "fast "
 		   : tpoint->type == static_tracepoint ? "static " : "",
 		   tpoint->number, paddress (tpoint->address), tpoint->enabled,
-		   tpoint->step_count, tpoint->pass_count);
+		   tpoint->step_count, tpoint->pass_count,
+		   tpoint->kind);
     }
   else if (tpoint)
     add_tracepoint_action (tpoint, packet);
@@ -3150,9 +3165,17 @@  install_tracepoint (struct tracepoint *tpoint, char *own_buf)
       /* Tracepoints are installed as memory breakpoints.  Just go
 	 ahead and install the trap.  The breakpoints module
 	 handles duplicated breakpoints, and the memory read
-	 routine handles un-patching traps from memory reads.  */
-      tpoint->handle = set_breakpoint_at (tpoint->address,
-					  tracepoint_handler);
+	 routine handles un-patching traps from memory reads.
+	 If tracepoint kind is not set, use the default values
+	 otherwise what was set from the gdb client will be used.  */
+      if (tpoint->kind == -1)
+	  tpoint->handle = set_breakpoint_at (tpoint->address,
+					      tracepoint_handler);
+      else
+	  tpoint->handle =
+	    set_breakpoint_at_with_kind (tpoint->address,
+					 tracepoint_handler,
+					 tpoint->kind);
     }
   else if (tpoint->type == fast_tracepoint || tpoint->type == static_tracepoint)
     {
@@ -3253,8 +3276,14 @@  cmd_qtstart (char *packet)
 	     ahead and install the trap.  The breakpoints module
 	     handles duplicated breakpoints, and the memory read
 	     routine handles un-patching traps from memory reads.  */
-	  tpoint->handle = set_breakpoint_at (tpoint->address,
-					      tracepoint_handler);
+	  if (tpoint->kind == -1)
+	    tpoint->handle = set_breakpoint_at (tpoint->address,
+						tracepoint_handler);
+	  else
+	    tpoint->handle =
+	      set_breakpoint_at_with_kind (tpoint->address,
+					   tracepoint_handler,
+					   tpoint->kind);
 	}
       else if (tpoint->type == fast_tracepoint
 	       || tpoint->type == static_tracepoint)
diff --git a/gdb/remote.c b/gdb/remote.c
index 517e36d..377a6da 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -241,6 +241,8 @@  static void readahead_cache_invalidate (void);
 
 static void remote_unpush_and_throw (void);
 
+static int remote_supports_tracepoint_kinds (void);
+
 /* For "remote".  */
 
 static struct cmd_list_element *remote_cmdlist;
@@ -1521,6 +1523,9 @@  enum {
   /* Support TARGET_WAITKIND_NO_RESUMED.  */
   PACKET_no_resumed,
 
+  /* Support target dependant tracepoint kinds.  */
+  PACKET_TracepointKinds,
+
   PACKET_MAX
 };
 
@@ -4693,6 +4698,8 @@  static const struct protocol_feature remote_protocol_features[] = {
   { "vContSupported", PACKET_DISABLE, remote_supported_packet, PACKET_vContSupported },
   { "QThreadEvents", PACKET_DISABLE, remote_supported_packet, PACKET_QThreadEvents },
   { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
+  { "TracepointKinds", PACKET_DISABLE, remote_supported_packet,
+    PACKET_TracepointKinds }
 };
 
 static char *remote_support_xml;
@@ -12197,6 +12204,12 @@  remote_can_run_breakpoint_commands (struct target_ops *self)
   return packet_support (PACKET_BreakpointCommands) == PACKET_ENABLE;
 }
 
+static int
+remote_supports_tracepoint_kinds (void)
+{
+  return packet_support (PACKET_TracepointKinds) == PACKET_ENABLE;
+}
+
 static void
 remote_trace_init (struct target_ops *self)
 {
@@ -12285,6 +12298,7 @@  remote_download_tracepoint (struct target_ops *self, struct bp_location *loc)
   char *pkt;
   struct breakpoint *b = loc->owner;
   struct tracepoint *t = (struct tracepoint *) b;
+  int kind;
 
   encode_actions_rsp (loc, &tdp_actions, &stepping_actions);
   old_chain = make_cleanup (free_actions_list_cleanup_wrapper,
@@ -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);
+
   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.
+     Send the tracepoint kind if we support it.  */
+  if (remote_supports_tracepoint_kinds ())
+    xsnprintf (buf + strlen (buf), BUF_SIZE - strlen (buf), ":K%x", kind);
+
   if (b->commands || *default_collect)
     strcat (buf, "-");
   putpkt (buf);
@@ -14333,6 +14356,10 @@  Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (&remote_protocol_packets[PACKET_no_resumed],
 			 "N stop reply", "no-resumed-stop-reply", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_TracepointKinds],
+			 "TracepointKinds",
+			 "tracepoint-kinds", 0);
+
   /* Assert that we've registered "set remote foo-packet" commands
      for all packet configs.  */
   {
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
+    }
 
     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" \
        )
 
diff --git a/gdb/testsuite/lib/trace-support.exp b/gdb/testsuite/lib/trace-support.exp
index b307f3f..df0fda0 100644
--- a/gdb/testsuite/lib/trace-support.exp
+++ b/gdb/testsuite/lib/trace-support.exp
@@ -43,6 +43,9 @@  if [is_amd64_regs_target] {
 } elseif { [istarget "s390*-*-*"] } {
     set fpreg "r11"
     set spreg "r15"
+} elseif [is_aarch32_target] {
+    set fpreg "sp"
+    set spreg "sp"
     set pcreg "pc"
 } else {
     set fpreg "fp"