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

[Antoine Tremblay <antoine.tremblay@ericsson.com>]

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.