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

[Yao Qi <qiyaoltc at gmail dot com>]

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 (齐尧)