Re: [PATCH v2 4/7] Support breakpoint kinds for software breakpoints in GDBServer.

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

On 10/15/2015 11:51 AM, Pedro Alves wrote:
> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>> This patch teaches GDBServer to:
>>
>>   - choose the right breakpoint instruction for its own breakpoints, through API
>>     set_breakpoint_at.
>>
>>   - choose the right breakpoint instruction for breakpoints requested by GDB,
>>    according to the information in Z packets, through API set_gdb_breakpoint.
>>
>> New fields are introduced in struct raw_breakpoint:
>>
>> pcfull: The PC including possible arch specific flags encoded in it.
>
> "full" as opposed to "empty"?  Can we find a clearer term?
>

full as opposed to incomplete, meaning it includes all it could include. 
Other then that I would see :

pcencoded ?

pcflaged ?

pcwithflags ?

Not an easy one..

>> @@ -100,6 +98,16 @@ struct raw_breakpoint
>>        breakpoint for a given PC.  */
>>     CORE_ADDR pc;
>>
>> +  /* The breakpoint's insertion address, possibly with flags encoded in the pc
>> +     (e.g. the instruction mode on ARM).  */
>> +  CORE_ADDR pcfull;
>> +
>> +  /* The breakpoint's data */
>> +  const unsigned char *data;
>> +
>> +  /* The breakpoint's kind.  */
>> +  int kind;
>> +
>>     /* The breakpoint's size.  */
>>     int size;
>
> Can't we always find the size from pcfull and kind ?
>

We could but then we would have to call breakpoint_from_kind in a lot of 
places basically everywhere bp->size is referenced like :

check_mem_read
check_mem_write
insert_memory_breakpoint
remove_memory_breakpoint
set_raw_breakpoint_at
validate_inserted_breakpoint
delete_raw_breakpoint
uninsert_raw_breakpoint
reinsert_raw_breakpoint
find_raw_breakpoint_at

Also since these functions can be called in a stack one would have to be 
careful to call breakpoint_from_kind at the right level and pass it 
down.. and then size/kind becomes confusing.

Also, this is a bit what I did in v1 but changed based on discussions 
with Yao see :

https://sourceware.org/ml/gdb-patches/2015-09/msg00597.html

I think it's more clear to call the function once and set the variable.

>>
>> @@ -293,6 +301,30 @@ find_raw_breakpoint_at (CORE_ADDR addr, enum raw_bkpt_type type, int size)
>>     return NULL;
>>   }
>>
>> +/* Try to resolve the real breakpoint size from the breakpoint kind  */
>> +
>> +static int
>> +breakpoint_from_kind (int kind,
>> +		      const unsigned char **breakpoint_data,
>> +		      int *breakpoint_len)
>> +{
>> +  /* Get the arch dependent breakpoint.  */
>> +  if (*the_target->breakpoint_from_kind != NULL)
>> +    {
>> +      /* Update magic coded size to the right size if needed.  */
>> +      *breakpoint_data =
>> +       (*the_target->breakpoint_from_kind) (&kind);
>> +      *breakpoint_len = kind;
>> +    }
>> +  else {
>
> Formatting.
>

Done.

>> @@ -375,15 +399,16 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
>>      returns NULL and writes the error code to *ERR.  */
>>
>>   static struct raw_breakpoint *
>> -set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int size,
>> -		       int *err)
>> +set_raw_breakpoint_at (enum raw_bkpt_type type, const CORE_ADDR where,
>> +		       const CORE_ADDR pc, const unsigned char* data, int kind,
>> +		       int size, int *err)
>
> Which is which: "where" vs "pc" | "pc" vs "pcfull" ?  I think the terminology
> should be consistent throughout.  Also remember to update intro comments.
>

Yes indeed this is confusing but I hesitated to change it since across 
gdb "where" is used for a location, even before this change where was 
translated to pc in the breakpoint struct.

It felt a bit weird to call set_breakpoint_at(pcfull) compared to like 
find_breakpoint_at (where).

But in this case we have where and pc I think it's necessary indeed.

Done.

>> @@ -405,12 +430,15 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int size,
>>       }
>>
>>     bp = XCNEW (struct raw_breakpoint);
>> -  bp->pc = where;
>> +  bp->pcfull = where;
>> +  bp->pc = pc;
>> +  bp->data = data;
>
> Why do we need to store "data" per breakpoint?  Can't we just call
> the_target->breakpoint_from_pc when necessary?

For the same reasons as expressed before for ->size I think it's better 
not to call breakpoint_from_pc at the lowest level.

>> @@ -918,17 +952,24 @@ z_type_supported (char z_type)
>>   	  && the_target->supports_z_point_type (z_type));
>>   }
>>
>> -/* Create a new GDB breakpoint of type Z_TYPE at ADDR with size SIZE.
>> +/* Create a new GDB breakpoint of type Z_TYPE at ADDR with kind KIND.
>>      Returns a pointer to the newly created breakpoint on success.  On
>>      failure returns NULL and sets *ERR to either -1 for error, or 1 if
>>      Z_TYPE breakpoints are not supported on this target.  */
>>
>>   static struct breakpoint *
>> -set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
>> +set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind, int *err)
>>   {
>>     struct breakpoint *bp;
>>     enum bkpt_type type;
>>     enum raw_bkpt_type raw_type;
>> +  const unsigned char *breakpoint_data = NULL;
>> +  int breakpoint_len = kind;
>> +
>> +  if (z_type == Z_PACKET_SW_BP)
>> +    {
>> +      breakpoint_from_kind (kind, &breakpoint_data, &breakpoint_len);
>> +    }
>
> Unnecessary braces.
>

Done.