[v2,2/7] Add breakpoint_from_kind target_ops for software breakpoints in GDBServer.
Commit Message
This patch is in preparation for software breakpoints on ARM in
GDBServer.
This patch introduces a new target_ops and linux_target_ops called
breakpoint_from_kind that will be used to ask the target for the right
breakpoint for a kind as set in a Z0 packet.
No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }
gdb/gdbserver/ChangeLog:
* linux-low.c (linux_breakpoint_from_kind): New function.
(struct target_ops) <breakpoint_from_kind>: Initialize field.
* linux-low.h (struct linux_target_ops)
<breakpoint_from_kind>: New field.
* target.h (struct target_ops) <breakpoint_from_kind>: New field.
---
gdb/gdbserver/linux-low.c | 10 ++++++++++
gdb/gdbserver/linux-low.h | 4 ++++
gdb/gdbserver/target.h | 4 ++++
3 files changed, 18 insertions(+)
Comments
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
> index 603819e..74fb36d 100644
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -446,6 +446,10 @@ struct target_ops
> can be NULL, the default breakpoint for the target should be returned in
> this case. */
> const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
> +
> + /* Returns a breakpoint from a kind, a kind is a length can have target
> + specific meaning like the z0 kind parameter. */
> + const unsigned char *(*breakpoint_from_kind) (int *kind);
> };
Since this function is used for software breakpoint, how about renaming
it to software_breakpoint_from_kind? Sorry that I didn't raise this up
in the review lats time.
Otherwise it is OK.
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
> +static const unsigned char*
> +linux_breakpoint_from_kind (int *kind)
> +{
> + if (*the_low_target.breakpoint_from_kind != NULL)
> + return (*the_low_target.breakpoint_from_kind) (kind);
> + else
> + return NULL;
> +}
What does returned NULL mean? We need to assert
the_low_target.breakpoint_from_kind isn't NULL, as a sanity check?
On 10/15/2015 05:09 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> +static const unsigned char*
>> +linux_breakpoint_from_kind (int *kind)
>> +{
>> + if (*the_low_target.breakpoint_from_kind != NULL)
>> + return (*the_low_target.breakpoint_from_kind) (kind);
>> + else
>> + return NULL;
>> +}
>
> What does returned NULL mean? We need to assert
> the_low_target.breakpoint_from_kind isn't NULL, as a sanity check?
>
Yes that's a left over from previous implementations, it should assert
now as breakpoint_from_kind is mandatory.
I'll fix it.
On 10/15/2015 05:04 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
>> index 603819e..74fb36d 100644
>> --- a/gdb/gdbserver/target.h
>> +++ b/gdb/gdbserver/target.h
>> @@ -446,6 +446,10 @@ struct target_ops
>> can be NULL, the default breakpoint for the target should be returned in
>> this case. */
>> const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
>> +
>> + /* Returns a breakpoint from a kind, a kind is a length can have target
>> + specific meaning like the z0 kind parameter. */
>> + const unsigned char *(*breakpoint_from_kind) (int *kind);
>> };
>
> Since this function is used for software breakpoint, how about renaming
> it to software_breakpoint_from_kind? Sorry that I didn't raise this up
> in the review lats time.
>
Humm I would be for it for it if it did not translate in long names like
linux_software_breakpoint_from_kind (36 chars almost half a line) and I
can't see any confusion with hardware breakpoints since we return a byte
array.
I would also need to change breakpoint_from_pc from the previous patch
to software_breakpoint_from_pc ?
Also, the old names were too "breakpoint and breakpoint_len rather then
sofwware_breakpoint software_breakpoint_len"...
Overall I feel while it adds some meaning it removes no confusion in the
context it is used.
Would it be ok to leave it as is ?
> Otherwise it is OK.
>
On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
> +static const unsigned char*
> +linux_breakpoint_from_kind (int *kind)
> +{
gdb_byte?
Thanks,
Pedro Alves
On 10/15/2015 11:33 AM, Pedro Alves wrote:
> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>> +static const unsigned char*
>> +linux_breakpoint_from_kind (int *kind)
>> +{
>
> gdb_byte?
>
Done.
@@ -5965,6 +5965,15 @@ linux_supports_range_stepping (void)
return (*the_low_target.supports_range_stepping) ();
}
+static const unsigned char*
+linux_breakpoint_from_kind (int *kind)
+{
+ if (*the_low_target.breakpoint_from_kind != NULL)
+ return (*the_low_target.breakpoint_from_kind) (kind);
+ else
+ return NULL;
+}
+
/* Enumerate spufs IDs for process PID. */
static int
spu_enumerate_spu_ids (long pid, unsigned char *buf, CORE_ADDR offset, int len)
@@ -7040,6 +7049,7 @@ static struct target_ops linux_target_ops = {
linux_mntns_unlink,
linux_mntns_readlink,
linux_breakpoint_from_pc,
+ linux_breakpoint_from_kind
};
static void
@@ -227,6 +227,10 @@ struct linux_target_ops
/* Returns true if the low target supports range stepping. */
int (*supports_range_stepping) (void);
+
+ /* Returns the proper breakpoint from size, the kind can have target
+ specific meaning like the z0 kind parameter */
+ const unsigned char *(*breakpoint_from_kind) (int *kind);
};
extern struct linux_target_ops the_low_target;
@@ -446,6 +446,10 @@ struct target_ops
can be NULL, the default breakpoint for the target should be returned in
this case. */
const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
+
+ /* Returns a breakpoint from a kind, a kind is a length can have target
+ specific meaning like the z0 kind parameter. */
+ const unsigned char *(*breakpoint_from_kind) (int *kind);
};
extern struct target_ops *the_target;