[v2,4/7] Support breakpoint kinds for software breakpoints in GDBServer.
Commit Message
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.
data: This is the breakpoint's raw data.
kind: As meant in a z0 packet this is the kind of breakpoint to be set.
Note this also clarifies existing fields usage :
pc: This is PC without any flags as a valid memory address.
size: This is the breakpoint's size in memory.
Function signatures, and variables names have also been updated to make the
distinction between size and kind clear.
Note also that an unimplemented breakpoint_from_kind operation is not an error
as this would break targets like QNX Neutrino (nto).
To check for this kind of error a check for the breakpoint_data is done in
insert_memory_breakpoint.
No regressions on Ubuntu 14.04 on ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }
gdb/gdbserver/ChangeLog:
* linux-low.c (initialize_low): Remove breakpoint_data initialization.
* mem-break.c (struct raw_breakpoint) <pcfull>: New field.
(struct raw_breakpoint) <data>: New field.
(struct raw_breakpoint) <kind>: New field.
(breakpoint_from_kind): New function.
(insert_memory_breakpoint): Adjust to use bp->size and bp->data.
(remove_memory_breakpoint): Adjust to use bp->size.
(set_raw_breakpoint_at): Add pc, data, kind fields arguments and
(set_breakpoint): Likewise.
(set_breakpoint_at): Call breakpoint_from_pc.
(delete_breakpoint): Rename size for kind.
(find_gdb_breakpoint): Use kind rather than size.
(set_gdb_breakpoint_1): Rename size for kind, call breakpoint_from_kind.
(set_gdb_breakpoint): Rename size for kind.
(delete_gdb_breakpoint_1): Rename size for kind.
(delete_gdb_breakpoint_1): Likewise.
(set_breakpoint_data): Remove.
(validate_inserted_breakpoint): Adjust to use bp->size and bp->data.
(check_mem_read): Adjust to use bp->size.
(check_mem_write): Adjust to use bp->size and bp->data.
(clone_one_breakpoint): Clone new fields, pcfull, data, kind.
* server.c (process_serial_event): Rename len for kind.
---
gdb/gdbserver/linux-low.c | 6 --
gdb/gdbserver/mem-break.c | 154 +++++++++++++++++++++++++++++-----------------
gdb/gdbserver/server.c | 8 +--
3 files changed, 100 insertions(+), 68 deletions(-)
Comments
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?
> data: This is the breakpoint's raw data.
> kind: As meant in a z0 packet this is the kind of breakpoint to be set.
>
> Note this also clarifies existing fields usage :
>
> pc: This is PC without any flags as a valid memory address.
> size: This is the breakpoint's size in memory.
>
> Function signatures, and variables names have also been updated to make the
> distinction between size and kind clear.
>
> Note also that an unimplemented breakpoint_from_kind operation is not an error
> as this would break targets like QNX Neutrino (nto).
>
> To check for this kind of error a check for the breakpoint_data is done in
> insert_memory_breakpoint.
>
> No regressions on Ubuntu 14.04 on ARMv7 and x86.
> With gdbserver-{native,extended} / { -marm -mthumb }
>
> gdb/gdbserver/ChangeLog:
> * linux-low.c (initialize_low): Remove breakpoint_data initialization.
> * mem-break.c (struct raw_breakpoint) <pcfull>: New field.
> (struct raw_breakpoint) <data>: New field.
> (struct raw_breakpoint) <kind>: New field.
> (breakpoint_from_kind): New function.
> (insert_memory_breakpoint): Adjust to use bp->size and bp->data.
> (remove_memory_breakpoint): Adjust to use bp->size.
> (set_raw_breakpoint_at): Add pc, data, kind fields arguments and
> (set_breakpoint): Likewise.
> (set_breakpoint_at): Call breakpoint_from_pc.
> (delete_breakpoint): Rename size for kind.
> (find_gdb_breakpoint): Use kind rather than size.
> (set_gdb_breakpoint_1): Rename size for kind, call breakpoint_from_kind.
> (set_gdb_breakpoint): Rename size for kind.
> (delete_gdb_breakpoint_1): Rename size for kind.
> (delete_gdb_breakpoint_1): Likewise.
> (set_breakpoint_data): Remove.
> (validate_inserted_breakpoint): Adjust to use bp->size and bp->data.
> (check_mem_read): Adjust to use bp->size.
> (check_mem_write): Adjust to use bp->size and bp->data.
> (clone_one_breakpoint): Clone new fields, pcfull, data, kind.
> * server.c (process_serial_event): Rename len for kind.
> ---
> gdb/gdbserver/linux-low.c | 6 --
> gdb/gdbserver/mem-break.c | 154 +++++++++++++++++++++++++++++-----------------
> gdb/gdbserver/server.c | 8 +--
> 3 files changed, 100 insertions(+), 68 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 5aa2b1d..c410deb 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -7077,16 +7077,10 @@ void
> initialize_low (void)
> {
> struct sigaction sigchld_action;
> - int breakpoint_len = 0;
> - const unsigned char *breakpoint = NULL;
>
> memset (&sigchld_action, 0, sizeof (sigchld_action));
> set_target_ops (&linux_target_ops);
>
> - breakpoint = the_target->breakpoint_from_pc (NULL, &breakpoint_len);
> -
> - set_breakpoint_data (breakpoint,
> - breakpoint_len);
> linux_init_signals ();
> linux_ptrace_init_warnings ();
>
> diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
> index f077497..4299cfa 100644
> --- a/gdb/gdbserver/mem-break.c
> +++ b/gdb/gdbserver/mem-break.c
> @@ -21,8 +21,6 @@
> #include "server.h"
> #include "regcache.h"
> #include "ax.h"
> -const unsigned char *breakpoint_data;
> -int breakpoint_len;
>
> #define MAX_BREAKPOINT_LEN 8
>
> @@ -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 ?
>
> @@ -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.
> + if (debug_threads)
> + debug_printf ("Don't know breakpoints of size %d.\n",
> + kind);
> + return -1;
> + }
> + return 0;
> +}
> +
> /* See mem-break.h. */
>
> int
> @@ -301,24 +333,17 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
> unsigned char buf[MAX_BREAKPOINT_LEN];
> int err;
>
> - if (breakpoint_data == NULL)
> - return 1;
> -
> - /* If the architecture treats the size field of Z packets as a
> - 'kind' field, then we'll need to be able to know which is the
> - breakpoint instruction too. */
> - if (bp->size != breakpoint_len)
> + if (bp->data == NULL)
> {
> if (debug_threads)
> - debug_printf ("Don't know how to insert breakpoints of size %d.\n",
> - bp->size);
> + debug_printf ("No breakpoint data present");
> return -1;
> }
>
> /* Note that there can be fast tracepoint jumps installed in the
> same memory range, so to get at the original memory, we need to
> use read_inferior_memory, which masks those out. */
> - err = read_inferior_memory (bp->pc, buf, breakpoint_len);
> + err = read_inferior_memory (bp->pc, buf, bp->size);
> if (err != 0)
> {
> if (debug_threads)
> @@ -328,10 +353,9 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
> }
> else
> {
> - memcpy (bp->old_data, buf, breakpoint_len);
> + memcpy (bp->old_data, buf, bp->size);
>
> - err = (*the_target->write_memory) (bp->pc, breakpoint_data,
> - breakpoint_len);
> + err = (*the_target->write_memory) (bp->pc, bp->data, bp->size);
> if (err != 0)
> {
> if (debug_threads)
> @@ -358,8 +382,8 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
> note that we need to pass the current shadow contents, because
> write_inferior_memory updates any shadow memory with what we pass
> here, and we want that to be a nop. */
> - memcpy (buf, bp->old_data, breakpoint_len);
> - err = write_inferior_memory (bp->pc, buf, breakpoint_len);
> + memcpy (buf, bp->old_data, bp->size);
> + err = write_inferior_memory (bp->pc, buf, bp->size);
> if (err != 0)
> {
> if (debug_threads)
> @@ -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.
> {
> struct process_info *proc = current_process ();
> struct raw_breakpoint *bp;
>
> if (type == raw_bkpt_type_sw || type == raw_bkpt_type_hw)
> {
> - bp = find_enabled_raw_code_breakpoint_at (where, type);
> + bp = find_enabled_raw_code_breakpoint_at (pc, type);
> if (bp != NULL && bp->size != size)
> {
> /* A different size than previously seen. The previous
> @@ -396,7 +421,7 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int size,
> }
> }
> else
> - bp = find_raw_breakpoint_at (where, type, size);
> + bp = find_raw_breakpoint_at (pc, type, size);
>
> if (bp != NULL)
> {
> @@ -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?
> + bp->kind = kind;
> bp->size = size;
> bp->refcount = 1;
> bp->raw_type = type;
>
> - *err = the_target->insert_point (bp->raw_type, bp->pc, bp->size, bp);
> + *err = the_target->insert_point (bp->raw_type, bp->pcfull, kind, bp);
> if (*err != 0)
> {
> if (debug_threads)
> @@ -740,14 +768,14 @@ reinsert_fast_tracepoint_jumps_at (CORE_ADDR where)
>
> static struct breakpoint *
> set_breakpoint (enum bkpt_type type, enum raw_bkpt_type raw_type,
> - CORE_ADDR where, int size,
> - int (*handler) (CORE_ADDR), int *err)
> + CORE_ADDR where, CORE_ADDR pc, const unsigned char *data,
> + int kind, int size, int (*handler) (CORE_ADDR), int *err)
> {
> struct process_info *proc = current_process ();
> struct breakpoint *bp;
> struct raw_breakpoint *raw;
>
> - raw = set_raw_breakpoint_at (raw_type, where, size, err);
> + raw = set_raw_breakpoint_at (raw_type, where, pc, data, kind, size, err);
>
> if (raw == NULL)
> {
> @@ -774,9 +802,15 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
> {
> int err_ignored;
>
> + const unsigned char *breakpoint_data;
> + int breakpoint_len;
> + CORE_ADDR pc = where;
> +
> + breakpoint_data = the_target->breakpoint_from_pc (&pc, &breakpoint_len);
> +
> return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
> - where, breakpoint_len, handler,
> - &err_ignored);
> + where, pc, breakpoint_data, breakpoint_len,
> + breakpoint_len, handler, &err_ignored);
> }
>
>
> @@ -891,12 +925,12 @@ delete_breakpoint (struct breakpoint *todel)
> return delete_breakpoint_1 (proc, todel);
> }
>
> -/* Locate a GDB breakpoint of type Z_TYPE and size SIZE placed at
> - address ADDR and return a pointer to its structure. If SIZE is -1,
> +/* Locate a GDB breakpoint of type Z_TYPE and kind KIND placed at
> + address ADDR and return a pointer to its structure. If KIND is -1,
> the breakpoints' sizes are ignored. */
>
> static struct breakpoint *
> -find_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
> +find_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
> {
> struct process_info *proc = current_process ();
> struct breakpoint *bp;
> @@ -904,7 +938,7 @@ find_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
>
> for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
> if (bp->type == type && bp->raw->pc == addr
> - && (size == -1 || bp->raw->size == size))
> + && (kind == -1 || bp->raw->kind == kind))
> return bp;
>
> return NULL;
> @@ -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.
>
> /* If we see GDB inserting a second code breakpoint at the same
> address, then either: GDB is updating the breakpoint's conditions
> @@ -952,9 +993,9 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
>
> if (bp != NULL)
> {
> - if (bp->raw->size != size)
> + if (bp->raw->kind != kind)
> {
> - /* A different size than previously seen. The previous
> + /* A different kind than previously seen. The previous
> breakpoint must be gone then. */
> bp->raw->inserted = -1;
> delete_breakpoint (bp);
> @@ -978,7 +1019,7 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
> /* Data breakpoints for the same address but different size are
> expected. GDB doesn't merge these. The backend gets to do
> that if it wants/can. */
> - bp = find_gdb_breakpoint (z_type, addr, size);
> + bp = find_gdb_breakpoint (z_type, addr, kind);
> }
>
> if (bp != NULL)
> @@ -993,7 +1034,8 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
>
> raw_type = Z_packet_to_raw_bkpt_type (z_type);
> type = Z_packet_to_bkpt_type (z_type);
> - return set_breakpoint (type, raw_type, addr, size, NULL, err);
> + return set_breakpoint (type, raw_type, addr, addr, breakpoint_data, kind,
> + breakpoint_len, NULL, err);
> }
>
> static int
> @@ -1024,7 +1066,7 @@ check_gdb_bp_preconditions (char z_type, int *err)
> knows to prepare to access memory for Z0 breakpoints. */
>
> struct breakpoint *
> -set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err)
> +set_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind, int *err)
> {
> struct breakpoint *bp;
>
> @@ -1040,7 +1082,7 @@ set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err)
> return NULL;
> }
>
> - bp = set_gdb_breakpoint_1 (z_type, addr, size, err);
> + bp = set_gdb_breakpoint_1 (z_type, addr, kind, err);
>
> if (z_type == Z_PACKET_SW_BP)
> done_accessing_memory ();
> @@ -1048,18 +1090,18 @@ set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err)
> return bp;
> }
>
> -/* Delete a GDB breakpoint of type Z_TYPE and size SIZE previously
> +/* Delete a GDB breakpoint of type Z_TYPE and kind KIND previously
> inserted at ADDR with set_gdb_breakpoint_at. Returns 0 on success,
> -1 on error, and 1 if Z_TYPE breakpoints are not supported on this
> target. */
>
> static int
> -delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size)
> +delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind)
> {
> struct breakpoint *bp;
> int err;
>
> - bp = find_gdb_breakpoint (z_type, addr, size);
> + bp = find_gdb_breakpoint (z_type, addr, kind);
> if (bp == NULL)
> return -1;
>
> @@ -1077,7 +1119,7 @@ delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size)
> knows to prepare to access memory for Z0 breakpoints. */
>
> int
> -delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
> +delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
> {
> int ret;
>
> @@ -1095,7 +1137,7 @@ delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
> return -1;
> }
>
> - ret = delete_gdb_breakpoint_1 (z_type, addr, size);
> + ret = delete_gdb_breakpoint_1 (z_type, addr, kind);
>
> if (z_type == Z_PACKET_SW_BP)
> done_accessing_memory ();
> @@ -1588,13 +1630,6 @@ check_breakpoints (CORE_ADDR stop_pc)
> }
> }
>
> -void
> -set_breakpoint_data (const unsigned char *bp_data, int bp_len)
> -{
> - breakpoint_data = bp_data;
> - breakpoint_len = bp_len;
> -}
> -
> int
> breakpoint_here (CORE_ADDR addr)
> {
> @@ -1669,9 +1704,9 @@ validate_inserted_breakpoint (struct raw_breakpoint *bp)
> gdb_assert (bp->inserted);
> gdb_assert (bp->raw_type == raw_bkpt_type_sw);
>
> - buf = (unsigned char *) alloca (breakpoint_len);
> - err = (*the_target->read_memory) (bp->pc, buf, breakpoint_len);
> - if (err || memcmp (buf, breakpoint_data, breakpoint_len) != 0)
> + buf = (unsigned char *) alloca (bp->size);
> + err = (*the_target->read_memory) (bp->pc, buf, bp->size);
> + if (err || memcmp (buf, bp->data, bp->size) != 0)
> {
> /* Tag it as gone. */
> bp->inserted = -1;
> @@ -1762,7 +1797,7 @@ check_mem_read (CORE_ADDR mem_addr, unsigned char *buf, int mem_len)
>
> for (; bp != NULL; bp = bp->next)
> {
> - CORE_ADDR bp_end = bp->pc + breakpoint_len;
> + CORE_ADDR bp_end = bp->pc + bp->size;
> CORE_ADDR start, end;
> int copy_offset, copy_len, buf_offset;
>
> @@ -1851,7 +1886,7 @@ check_mem_write (CORE_ADDR mem_addr, unsigned char *buf,
>
> for (; bp != NULL; bp = bp->next)
> {
> - CORE_ADDR bp_end = bp->pc + breakpoint_len;
> + CORE_ADDR bp_end = bp->pc + bp->size;
> CORE_ADDR start, end;
> int copy_offset, copy_len, buf_offset;
>
> @@ -1882,7 +1917,7 @@ check_mem_write (CORE_ADDR mem_addr, unsigned char *buf,
> if (bp->inserted > 0)
> {
> if (validate_inserted_breakpoint (bp))
> - memcpy (buf + buf_offset, breakpoint_data + copy_offset, copy_len);
> + memcpy (buf + buf_offset, bp->data + copy_offset, copy_len);
> else
> disabled_one = 1;
> }
> @@ -1963,6 +1998,9 @@ clone_one_breakpoint (const struct breakpoint *src)
> dest_raw->raw_type = src->raw->raw_type;
> dest_raw->refcount = src->raw->refcount;
> dest_raw->pc = src->raw->pc;
> + dest_raw->pcfull = src->raw->pcfull;
> + dest_raw->data = src->raw->data;
> + dest_raw->kind = src->raw->kind;
> dest_raw->size = src->raw->size;
> memcpy (dest_raw->old_data, src->raw->old_data, MAX_BREAKPOINT_LEN);
> dest_raw->inserted = src->raw->inserted;
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index e25b7c7..ad6626e 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -4069,20 +4069,20 @@ process_serial_event (void)
> {
> char *dataptr;
> ULONGEST addr;
> - int len;
> + int kind;
> char type = own_buf[1];
> int res;
> const int insert = ch == 'Z';
> char *p = &own_buf[3];
>
> p = unpack_varlen_hex (p, &addr);
> - len = strtol (p + 1, &dataptr, 16);
> + kind = strtol (p + 1, &dataptr, 16);
>
> if (insert)
> {
> struct breakpoint *bp;
>
> - bp = set_gdb_breakpoint (type, addr, len, &res);
> + bp = set_gdb_breakpoint (type, addr, kind, &res);
> if (bp != NULL)
> {
> res = 0;
> @@ -4097,7 +4097,7 @@ process_serial_event (void)
> }
> }
> else
> - res = delete_gdb_breakpoint (type, addr, len);
> + res = delete_gdb_breakpoint (type, addr, kind);
>
> if (res == 0)
> write_ok (own_buf);
>
Thanks,
Pedro Alves
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.
On 10/15/2015 07:02 PM, Antoine Tremblay wrote:
>
>
> 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..
GDB calls them "placed address" and "requested address":
struct bp_target_info
{
...
/* Address at which the breakpoint was placed. This is normally
the same as REQUESTED_ADDRESS, except when adjustment happens in
gdbarch_breakpoint_from_pc. The most common form of adjustment
is stripping an alternate ISA marker from the PC which is used
to determine the type of breakpoint to insert. */
CORE_ADDR placed_address;
/* Address at which the breakpoint was requested. */
CORE_ADDR reqstd_address;
>
>>> @@ -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
See below.
>
> 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.
I don't see why my comment conflicts with Yao's. But I think we
could simplify the interfaces and entry points, and get rid of the
duplication, like this:
Replace the breakpoint_from_pc method with a breakpoint_kind_from_pc
method. This adjusts the PC (if necessary) and returns the
breakpoint _kind_ instead of the breakpoint opcode / data.
enum arm_breakpoint_kinds
{
ARM_BP_KIND_THUMB = 2,
ARM_BP_KIND_THUMB2 = 3,
ARM_BP_KIND_ARM = 4,
};
static int
arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr, int len)
{
if (IS_THUMB_ADDR (*pcptr))
{
gdb_byte buf[2];
*pcptr = UNMAKE_THUMB_ADDR (*pcptr);
/* Check whether we are replacing a thumb2 32-bit instruction. */
if ((*the_target->read_memory) (*pcptr, buf, 2) == 0)
{
unsigned short inst1 = 0;
(*the_target->read_memory) (*pcptr, (gdb_byte *) &inst1, 2);
if (thumb_insn_size (inst1) == 4)
return ARM_BP_KIND_THUMB2;
}
return ARM_BP_KIND_THUMB;
}
else
return ARM_BP_KIND_ARM;
}
Then the breakpoints functions and structures always work
with the already-adjusted PC, and with a breakpoint-kind.
for internal breakpoints, we have:
set_breakpoint_at (breakpoint_kind_from_pc, to find bp kind,
rest the same as today)
set_gdb_breakpoint_1 (same as today)
|
`--> set_breakpoint (address, kind)
|
`-->set_raw_breakpoint_at (address, kind)
|
`--> the_target->insert_point (address, kind)
Everything thinks in terms of breakpoint kind. Then the only
places that need to know the real breakpoint instruction opcode
and opcode size can query the breakpoint_from_kind target method
you already added.
About:
> 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
Minimizing the patch size is less important than making sure the
resulting code is clear
Sounds like that's manageable with a trivial replace of bp->size
with a call to something like:
static int
bp_size (struct raw_breakpoint *bp)
{
int size = bp->kind;
breakpoint_from_kind (&size);
return size;
}
Likewise for the opcode data:
static const gdb_byte *
bp_opcode (struct raw_breakpoint *bp)
{
int size = bp->kind;
return breakpoint_from_kind (&size);
}
Doesn't seem to me like the end result would be any less clear.
Thanks,
Pedro Alves
On 10/16/2015 12:06 PM, Pedro Alves wrote:
> GDB calls them "placed address" and "requested address":
>
> struct bp_target_info
> {
> ...
> /* Address at which the breakpoint was placed. This is normally
> the same as REQUESTED_ADDRESS, except when adjustment happens in
> gdbarch_breakpoint_from_pc. The most common form of adjustment
> is stripping an alternate ISA marker from the PC which is used
> to determine the type of breakpoint to insert. */
> CORE_ADDR placed_address;
>
> /* Address at which the breakpoint was requested. */
> CORE_ADDR reqstd_address;
>
>
Nice I had not seen that !
I'll change pcfull for reqstd_address;
But leave pc as pc for now, we can change it in a later patch.
> I don't see why my comment conflicts with Yao's. But I think we
> could simplify the interfaces and entry points, and get rid of the
> duplication, like this:
>
> Replace the breakpoint_from_pc method with a breakpoint_kind_from_pc
> method. This adjusts the PC (if necessary) and returns the
> breakpoint _kind_ instead of the breakpoint opcode / data.
>
> enum arm_breakpoint_kinds
> {
> ARM_BP_KIND_THUMB = 2,
> ARM_BP_KIND_THUMB2 = 3,
> ARM_BP_KIND_ARM = 4,
> };
>
> static int
> arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr, int len)
> {
> if (IS_THUMB_ADDR (*pcptr))
> {
> gdb_byte buf[2];
>
> *pcptr = UNMAKE_THUMB_ADDR (*pcptr);
>
> /* Check whether we are replacing a thumb2 32-bit instruction. */
> if ((*the_target->read_memory) (*pcptr, buf, 2) == 0)
> {
> unsigned short inst1 = 0;
>
> (*the_target->read_memory) (*pcptr, (gdb_byte *) &inst1, 2);
> if (thumb_insn_size (inst1) == 4)
> return ARM_BP_KIND_THUMB2;
> }
>
> return ARM_BP_KIND_THUMB;
> }
> else
> return ARM_BP_KIND_ARM;
> }
>
> Then the breakpoints functions and structures always work
> with the already-adjusted PC, and with a breakpoint-kind.
>
> for internal breakpoints, we have:
>
> set_breakpoint_at (breakpoint_kind_from_pc, to find bp kind,
> rest the same as today)
> set_gdb_breakpoint_1 (same as today)
> |
> `--> set_breakpoint (address, kind)
> |
> `-->set_raw_breakpoint_at (address, kind)
> |
> `--> the_target->insert_point (address, kind)
>
> Everything thinks in terms of breakpoint kind. Then the only
> places that need to know the real breakpoint instruction opcode
> and opcode size can query the breakpoint_from_kind target method
> you already added.
>
> About:
>
>> 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
>
> Minimizing the patch size is less important than making sure the
> resulting code is clear
>
I was thinking about making the code clear.
> Sounds like that's manageable with a trivial replace of bp->size
> with a call to something like:
>
> static int
> bp_size (struct raw_breakpoint *bp)
> {
> int size = bp->kind;
>
> breakpoint_from_kind (&size);
> return size;
> }
>
> Likewise for the opcode data:
>
> static const gdb_byte *
> bp_opcode (struct raw_breakpoint *bp)
> {
> int size = bp->kind;
>
> return breakpoint_from_kind (&size);
> }
>
> Doesn't seem to me like the end result would be any less clear.
>
I see what you mean more clearly now.
I like the idea to treat only in kinds but I'm not sure about the
advantage in terms of clarity.
I would say it's debatable like you said in the end result is not any
less clear but the current implementation doesn't seem less clear to me
either.
I do not like the detour we need to make when we do not have a real
reason to have a kind, adding a "fake" unique kind and having to add
breakpoint_from_kind implementations on all architectures, not just the
ones that support software breakpoints. (Regardless of the patch size).
Also even if we can call functions to get the size and opcode when
needed, it still seems like since those values do not change for the
life of the breakpoint and that they can be set only once from a
meaningful context it's perfectly acceptable and more clear to set them
once and avoid this level of abstraction to access these values.
At this point I could do either but to avoid redoing the patch set
multiple times, I would like to ask for Yao's opinion and I will work on
the consensual option.
Regards,
Antoine
On 10/16/2015 07:08 PM, Antoine Tremblay wrote:
> I see what you mean more clearly now.
>
> I like the idea to treat only in kinds but I'm not sure about the
> advantage in terms of clarity.
The main advantage is that it eliminates the redundant info
in the breakpoint structures fields. I really see no reason for every
breakpoint to have to carry around a pointer to the opcode to use,
since that's a property that can be inferred from other fields.
Less redundancy means fewer chances of things getting out of sync.
>
> I would say it's debatable like you said in the end result is not any
> less clear but the current implementation doesn't seem less clear to me
> either.
>
> I do not like the detour we need to make when we do not have a real
> reason to have a kind, adding a "fake" unique kind and having to add
> breakpoint_from_kind implementations on all architectures, not just the
> ones that support software breakpoints. (Regardless of the patch size).
>
The default implementation of breakpoint_from_pc can simply be:
int
default_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
{
/* Most architectures have only one kind of software breakpoint. */
return 0;
}
and then most architectures would just implement the
breakpoint_from_kind method simply as:
static gdb_byte *
arch_breakpoint_from_kind (int kind /* ignored, there's only one */)
{
return arch_breakpoint;
}
which is quite like what you already wrote.
That is, most architectures only have one 'kind' of breakpoint,
so they can ignore the parameter. Just like in your breakpoint_from_pc
hook, most archs would ignore the length.
Note that there's always a need to implement _one_ hook on all
architecture. In your version, it's the breakpoint_from_pc hook.
In my suggestion, it's breakpoint_from_kind. But it's the same
number of "hooks x architectures implementations".
> Also even if we can call functions to get the size and opcode when
> needed, it still seems like since those values do not change for the
> life of the breakpoint and that they can be set only once from a
> meaningful context it's perfectly acceptable and more clear to set them
> once and avoid this level of abstraction to access these values.
I don't buy that. The opcode to use for a particular mode can be
determined from that info alone ("which mode"), and as such doesn't
need to be stored on every breakpoint. If you look at it from the
other angle, you can see it as factorization.
> At this point I could do either but to avoid redoing the patch set
> multiple times, I would like to ask for Yao's opinion and I will work on
> the consensual option.
Fair enough.
Thanks,
Pedro Alves
On 10/16/2015 03:04 PM, Pedro Alves wrote:
>
> Note that there's always a need to implement _one_ hook on all
> architecture. In your version, it's the breakpoint_from_pc hook.
> In my suggestion, it's breakpoint_from_kind. But it's the same
> number of "hooks x architectures implementations".
Good point it would only transfer the operation to archs that can
software single step basically, I withdraw this concern.
On 10/16/2015 03:23 PM, Antoine Tremblay wrote:
>
>
> On 10/16/2015 03:04 PM, Pedro Alves wrote:
>>
>> Note that there's always a need to implement _one_ hook on all
>> architecture. In your version, it's the breakpoint_from_pc hook.
>> In my suggestion, it's breakpoint_from_kind. But it's the same
>> number of "hooks x architectures implementations".
>
> Good point it would only transfer the operation to archs that can
> software single step basically, I withdraw this concern.
Humm thinking more about it however if we were to apply the same logic
to pc and pcfull.
Removing the pc from the struct would cause a call to
breakpoint_kind_from_pc to be mandatory.
Would you see too pc to be removed ?
And replaced by a call to :
static CORE_ADDR
bp_pc (CORE_ADDR *pcptr)
{
return breakpoint_kind_from_pc (pcptr, 0)
}
On 10/16/2015 03:44 PM, Antoine Tremblay wrote:
>
>
> On 10/16/2015 03:23 PM, Antoine Tremblay wrote:
>>
>>
>> On 10/16/2015 03:04 PM, Pedro Alves wrote:
>>>
>>> Note that there's always a need to implement _one_ hook on all
>>> architecture. In your version, it's the breakpoint_from_pc hook.
>>> In my suggestion, it's breakpoint_from_kind. But it's the same
>>> number of "hooks x architectures implementations".
>>
>> Good point it would only transfer the operation to archs that can
>> software single step basically, I withdraw this concern.
>
> Humm thinking more about it however if we were to apply the same logic
> to pc and pcfull.
>
> Removing the pc from the struct would cause a call to
> breakpoint_kind_from_pc to be mandatory.
>
> Would you see too pc to be removed ?
>
> And replaced by a call to :
>
> static CORE_ADDR
> bp_pc (CORE_ADDR *pcptr)
> {
> return breakpoint_kind_from_pc (pcptr, 0)
> }
>
Oops I mean
static CORE_ADDR
bp_pc (struct raw_breakpoint *bp)
{
CORE_ADDR pc = bp->pcfull;
breakpoint_kind_from_pc (&pc, 0)
return pc;
}
On 10/16/2015 08:44 PM, Antoine Tremblay wrote:
> Humm thinking more about it however if we were to apply the same logic
> to pc and pcfull.
>
> Removing the pc from the struct would cause a call to
> breakpoint_kind_from_pc to be mandatory.
>
> Would you see too pc to be removed ?
>
I was seeing pcfull being removed, actually.
Z0 breakpoints always have their address already adjusted by GDB, right?
You had:
@@ -774,9 +802,15 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
{
int err_ignored;
+ const unsigned char *breakpoint_data;
+ int breakpoint_len;
+ CORE_ADDR pc = where;
+
+ breakpoint_data = the_target->breakpoint_from_pc (&pc, &breakpoint_len);
+
return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
- where, breakpoint_len, handler,
- &err_ignored);
+ where, pc, breakpoint_data, breakpoint_len,
+ breakpoint_len, handler, &err_ignored);
}
But I think you should be able to instead do:
@@ -774,9 +802,15 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
{
int err_ignored;
+ CORE_ADDR adjusted_pc = where;
+ int bp_kind;
+
+ bp_kind = the_target->breakpoint_kind_from_pc (&adjusted_pc);
+
return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
- where, breakpoint_len, handler,
+ adjusted_pc, bp_kind, handler,
&err_ignored);
}
Thanks,
Pedro Alves
On 10/19/2015 05:35 AM, Pedro Alves wrote:
> I was seeing pcfull being removed, actually.
Yes good point, in fact, I don't even need it in the current implementation.
Your solution has grown on me over the weekend, so I will post a v3 with
this implemented unless Yao has an objection.
@@ -7077,16 +7077,10 @@ void
initialize_low (void)
{
struct sigaction sigchld_action;
- int breakpoint_len = 0;
- const unsigned char *breakpoint = NULL;
memset (&sigchld_action, 0, sizeof (sigchld_action));
set_target_ops (&linux_target_ops);
- breakpoint = the_target->breakpoint_from_pc (NULL, &breakpoint_len);
-
- set_breakpoint_data (breakpoint,
- breakpoint_len);
linux_init_signals ();
linux_ptrace_init_warnings ();
@@ -21,8 +21,6 @@
#include "server.h"
#include "regcache.h"
#include "ax.h"
-const unsigned char *breakpoint_data;
-int breakpoint_len;
#define MAX_BREAKPOINT_LEN 8
@@ -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;
@@ -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 {
+ if (debug_threads)
+ debug_printf ("Don't know breakpoints of size %d.\n",
+ kind);
+ return -1;
+ }
+ return 0;
+}
+
/* See mem-break.h. */
int
@@ -301,24 +333,17 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
unsigned char buf[MAX_BREAKPOINT_LEN];
int err;
- if (breakpoint_data == NULL)
- return 1;
-
- /* If the architecture treats the size field of Z packets as a
- 'kind' field, then we'll need to be able to know which is the
- breakpoint instruction too. */
- if (bp->size != breakpoint_len)
+ if (bp->data == NULL)
{
if (debug_threads)
- debug_printf ("Don't know how to insert breakpoints of size %d.\n",
- bp->size);
+ debug_printf ("No breakpoint data present");
return -1;
}
/* Note that there can be fast tracepoint jumps installed in the
same memory range, so to get at the original memory, we need to
use read_inferior_memory, which masks those out. */
- err = read_inferior_memory (bp->pc, buf, breakpoint_len);
+ err = read_inferior_memory (bp->pc, buf, bp->size);
if (err != 0)
{
if (debug_threads)
@@ -328,10 +353,9 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
}
else
{
- memcpy (bp->old_data, buf, breakpoint_len);
+ memcpy (bp->old_data, buf, bp->size);
- err = (*the_target->write_memory) (bp->pc, breakpoint_data,
- breakpoint_len);
+ err = (*the_target->write_memory) (bp->pc, bp->data, bp->size);
if (err != 0)
{
if (debug_threads)
@@ -358,8 +382,8 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
note that we need to pass the current shadow contents, because
write_inferior_memory updates any shadow memory with what we pass
here, and we want that to be a nop. */
- memcpy (buf, bp->old_data, breakpoint_len);
- err = write_inferior_memory (bp->pc, buf, breakpoint_len);
+ memcpy (buf, bp->old_data, bp->size);
+ err = write_inferior_memory (bp->pc, buf, bp->size);
if (err != 0)
{
if (debug_threads)
@@ -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)
{
struct process_info *proc = current_process ();
struct raw_breakpoint *bp;
if (type == raw_bkpt_type_sw || type == raw_bkpt_type_hw)
{
- bp = find_enabled_raw_code_breakpoint_at (where, type);
+ bp = find_enabled_raw_code_breakpoint_at (pc, type);
if (bp != NULL && bp->size != size)
{
/* A different size than previously seen. The previous
@@ -396,7 +421,7 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int size,
}
}
else
- bp = find_raw_breakpoint_at (where, type, size);
+ bp = find_raw_breakpoint_at (pc, type, size);
if (bp != NULL)
{
@@ -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;
+ bp->kind = kind;
bp->size = size;
bp->refcount = 1;
bp->raw_type = type;
- *err = the_target->insert_point (bp->raw_type, bp->pc, bp->size, bp);
+ *err = the_target->insert_point (bp->raw_type, bp->pcfull, kind, bp);
if (*err != 0)
{
if (debug_threads)
@@ -740,14 +768,14 @@ reinsert_fast_tracepoint_jumps_at (CORE_ADDR where)
static struct breakpoint *
set_breakpoint (enum bkpt_type type, enum raw_bkpt_type raw_type,
- CORE_ADDR where, int size,
- int (*handler) (CORE_ADDR), int *err)
+ CORE_ADDR where, CORE_ADDR pc, const unsigned char *data,
+ int kind, int size, int (*handler) (CORE_ADDR), int *err)
{
struct process_info *proc = current_process ();
struct breakpoint *bp;
struct raw_breakpoint *raw;
- raw = set_raw_breakpoint_at (raw_type, where, size, err);
+ raw = set_raw_breakpoint_at (raw_type, where, pc, data, kind, size, err);
if (raw == NULL)
{
@@ -774,9 +802,15 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
{
int err_ignored;
+ const unsigned char *breakpoint_data;
+ int breakpoint_len;
+ CORE_ADDR pc = where;
+
+ breakpoint_data = the_target->breakpoint_from_pc (&pc, &breakpoint_len);
+
return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
- where, breakpoint_len, handler,
- &err_ignored);
+ where, pc, breakpoint_data, breakpoint_len,
+ breakpoint_len, handler, &err_ignored);
}
@@ -891,12 +925,12 @@ delete_breakpoint (struct breakpoint *todel)
return delete_breakpoint_1 (proc, todel);
}
-/* Locate a GDB breakpoint of type Z_TYPE and size SIZE placed at
- address ADDR and return a pointer to its structure. If SIZE is -1,
+/* Locate a GDB breakpoint of type Z_TYPE and kind KIND placed at
+ address ADDR and return a pointer to its structure. If KIND is -1,
the breakpoints' sizes are ignored. */
static struct breakpoint *
-find_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
+find_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
{
struct process_info *proc = current_process ();
struct breakpoint *bp;
@@ -904,7 +938,7 @@ find_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
if (bp->type == type && bp->raw->pc == addr
- && (size == -1 || bp->raw->size == size))
+ && (kind == -1 || bp->raw->kind == kind))
return bp;
return NULL;
@@ -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);
+ }
/* If we see GDB inserting a second code breakpoint at the same
address, then either: GDB is updating the breakpoint's conditions
@@ -952,9 +993,9 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
if (bp != NULL)
{
- if (bp->raw->size != size)
+ if (bp->raw->kind != kind)
{
- /* A different size than previously seen. The previous
+ /* A different kind than previously seen. The previous
breakpoint must be gone then. */
bp->raw->inserted = -1;
delete_breakpoint (bp);
@@ -978,7 +1019,7 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
/* Data breakpoints for the same address but different size are
expected. GDB doesn't merge these. The backend gets to do
that if it wants/can. */
- bp = find_gdb_breakpoint (z_type, addr, size);
+ bp = find_gdb_breakpoint (z_type, addr, kind);
}
if (bp != NULL)
@@ -993,7 +1034,8 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
raw_type = Z_packet_to_raw_bkpt_type (z_type);
type = Z_packet_to_bkpt_type (z_type);
- return set_breakpoint (type, raw_type, addr, size, NULL, err);
+ return set_breakpoint (type, raw_type, addr, addr, breakpoint_data, kind,
+ breakpoint_len, NULL, err);
}
static int
@@ -1024,7 +1066,7 @@ check_gdb_bp_preconditions (char z_type, int *err)
knows to prepare to access memory for Z0 breakpoints. */
struct breakpoint *
-set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err)
+set_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind, int *err)
{
struct breakpoint *bp;
@@ -1040,7 +1082,7 @@ set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err)
return NULL;
}
- bp = set_gdb_breakpoint_1 (z_type, addr, size, err);
+ bp = set_gdb_breakpoint_1 (z_type, addr, kind, err);
if (z_type == Z_PACKET_SW_BP)
done_accessing_memory ();
@@ -1048,18 +1090,18 @@ set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err)
return bp;
}
-/* Delete a GDB breakpoint of type Z_TYPE and size SIZE previously
+/* Delete a GDB breakpoint of type Z_TYPE and kind KIND previously
inserted at ADDR with set_gdb_breakpoint_at. Returns 0 on success,
-1 on error, and 1 if Z_TYPE breakpoints are not supported on this
target. */
static int
-delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size)
+delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind)
{
struct breakpoint *bp;
int err;
- bp = find_gdb_breakpoint (z_type, addr, size);
+ bp = find_gdb_breakpoint (z_type, addr, kind);
if (bp == NULL)
return -1;
@@ -1077,7 +1119,7 @@ delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size)
knows to prepare to access memory for Z0 breakpoints. */
int
-delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
+delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
{
int ret;
@@ -1095,7 +1137,7 @@ delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
return -1;
}
- ret = delete_gdb_breakpoint_1 (z_type, addr, size);
+ ret = delete_gdb_breakpoint_1 (z_type, addr, kind);
if (z_type == Z_PACKET_SW_BP)
done_accessing_memory ();
@@ -1588,13 +1630,6 @@ check_breakpoints (CORE_ADDR stop_pc)
}
}
-void
-set_breakpoint_data (const unsigned char *bp_data, int bp_len)
-{
- breakpoint_data = bp_data;
- breakpoint_len = bp_len;
-}
-
int
breakpoint_here (CORE_ADDR addr)
{
@@ -1669,9 +1704,9 @@ validate_inserted_breakpoint (struct raw_breakpoint *bp)
gdb_assert (bp->inserted);
gdb_assert (bp->raw_type == raw_bkpt_type_sw);
- buf = (unsigned char *) alloca (breakpoint_len);
- err = (*the_target->read_memory) (bp->pc, buf, breakpoint_len);
- if (err || memcmp (buf, breakpoint_data, breakpoint_len) != 0)
+ buf = (unsigned char *) alloca (bp->size);
+ err = (*the_target->read_memory) (bp->pc, buf, bp->size);
+ if (err || memcmp (buf, bp->data, bp->size) != 0)
{
/* Tag it as gone. */
bp->inserted = -1;
@@ -1762,7 +1797,7 @@ check_mem_read (CORE_ADDR mem_addr, unsigned char *buf, int mem_len)
for (; bp != NULL; bp = bp->next)
{
- CORE_ADDR bp_end = bp->pc + breakpoint_len;
+ CORE_ADDR bp_end = bp->pc + bp->size;
CORE_ADDR start, end;
int copy_offset, copy_len, buf_offset;
@@ -1851,7 +1886,7 @@ check_mem_write (CORE_ADDR mem_addr, unsigned char *buf,
for (; bp != NULL; bp = bp->next)
{
- CORE_ADDR bp_end = bp->pc + breakpoint_len;
+ CORE_ADDR bp_end = bp->pc + bp->size;
CORE_ADDR start, end;
int copy_offset, copy_len, buf_offset;
@@ -1882,7 +1917,7 @@ check_mem_write (CORE_ADDR mem_addr, unsigned char *buf,
if (bp->inserted > 0)
{
if (validate_inserted_breakpoint (bp))
- memcpy (buf + buf_offset, breakpoint_data + copy_offset, copy_len);
+ memcpy (buf + buf_offset, bp->data + copy_offset, copy_len);
else
disabled_one = 1;
}
@@ -1963,6 +1998,9 @@ clone_one_breakpoint (const struct breakpoint *src)
dest_raw->raw_type = src->raw->raw_type;
dest_raw->refcount = src->raw->refcount;
dest_raw->pc = src->raw->pc;
+ dest_raw->pcfull = src->raw->pcfull;
+ dest_raw->data = src->raw->data;
+ dest_raw->kind = src->raw->kind;
dest_raw->size = src->raw->size;
memcpy (dest_raw->old_data, src->raw->old_data, MAX_BREAKPOINT_LEN);
dest_raw->inserted = src->raw->inserted;
@@ -4069,20 +4069,20 @@ process_serial_event (void)
{
char *dataptr;
ULONGEST addr;
- int len;
+ int kind;
char type = own_buf[1];
int res;
const int insert = ch == 'Z';
char *p = &own_buf[3];
p = unpack_varlen_hex (p, &addr);
- len = strtol (p + 1, &dataptr, 16);
+ kind = strtol (p + 1, &dataptr, 16);
if (insert)
{
struct breakpoint *bp;
- bp = set_gdb_breakpoint (type, addr, len, &res);
+ bp = set_gdb_breakpoint (type, addr, kind, &res);
if (bp != NULL)
{
res = 0;
@@ -4097,7 +4097,7 @@ process_serial_event (void)
}
}
else
- res = delete_gdb_breakpoint (type, addr, len);
+ res = delete_gdb_breakpoint (type, addr, kind);
if (res == 0)
write_ok (own_buf);