This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 4/7] Support breakpoint kinds for software breakpoints in GDBServer.
- From: Antoine Tremblay <antoine dot tremblay at ericsson dot com>
- To: Pedro Alves <palves at redhat dot com>, <gdb-patches at sourceware dot org>
- Date: Thu, 15 Oct 2015 14:02:34 -0400
- Subject: Re: [PATCH v2 4/7] Support breakpoint kinds for software breakpoints in GDBServer.
- Authentication-results: sourceware.org; auth=none
- References: <1444063455-31558-1-git-send-email-antoine dot tremblay at ericsson dot com> <1444063455-31558-5-git-send-email-antoine dot tremblay at ericsson dot com> <561FCB85 dot 4020500 at redhat dot 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.