[2/5] Make breakpoint and breakpoint_len local variables in GDBServer.
Commit Message
This patch is in preparation for software breakpoints on ARM linux.
It refactors the breakpoint and breakpoint_len global variables to be local
so that multiple types of breakpoints can be used for an arch.
One important implementation detail is the introduction of the pcfull field
in struct raw_breakpoint.
In order to be able to reinsert a breakpoint we need to remember what were the
flags encoded in the PC, however since functions that compare the program pc
to the breakpoint pc expect an unencoded memory address, we can't put the
encoded value directly in the pc field.
So this patch introduces the pcfull field that contains the flags encoded
in the pc so that we can properly reinsert a breakpoint that has its type
information encoded in the pc. pcfull shall only be used when
inserting/removing/reinserting a breakpoint, all other breakpoint->pc
references can remain the same.
Note this is for software breakpoints only, when using hardware breakpoints
then pcfull is not set or used.
No regressions on Ubuntu 14.04 on ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }
gdbserver/ChangeLog:
* linux-low.c (initialize_low): Remove
breakpoint_data initialization.
* mem-break.c (struct raw_breakpoint): Add pcfull.
(insert_memory_breakpoint): Call breakpoint_from_pc.
(remove_memory_breakpoint): Likewise.
(set_raw_breakpoint_at): Likewise.
(set_breakpoint_at): Set default breakpoint size to 0.
(set_breakpoint_data): Remove.
(validate_inserted_breakpoint): Call breakpoint_from_pc.
(check_mem_read): Call breakpoint_from_pc.
(check_mem_write): Call breakpoint_from_pc.
(clone_one_breakpoint): Copy pcfull field.
---
gdb/gdbserver/linux-low.c | 6 ----
gdb/gdbserver/mem-break.c | 76 ++++++++++++++++++++++++++++++++++++-----------
2 files changed, 58 insertions(+), 24 deletions(-)
Comments
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
> @@ -774,8 +800,9 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
> {
> int err_ignored;
>
> + /* default breakpoint_len will be initialized downstream. */
> return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
> - where, breakpoint_len, handler,
> + where, 0, handler,
> &err_ignored);
> }
Why do you set breakpoint length to zero? I know you'll set the length
downstream properly, but we should compute the breakpoint length here.
After thinking about it a bit more, I think this reveals some design
issues in GDBserver brekapoint, nowadays, GDBserver inserts its own
breakpoints and breakpoints requested by GDB. After this patch series,
GDBserver should be able to:
1) choose the right breakpoint instruction for its own breakpoints,
according to the breakpoint address, status register flag, etc,
through API set_breakpoint_at,
2) choose the right breakpoint instruction for breakpoints requested
by GDB, according to the information in Z packets, through API
set_gdb_breakpoint
there should be two paths for them, and each path needs different target
hook to choose breakpoint instructions. breakpoint_from_pc is needed for
#1, and breakpoint_from_length is needed for #2. In your current patch
set (in patch 4/5), two things are mixed together, which doesn't look
good to me. The current functions calls in GDBserver to create
breakpoint is like
set_breakpoint_at
set_gdb_breakpoint_1
|
`--> set_breakpoint
|
`-->set_raw_breakpoint_at
|
`--> the_target->insert_point
we are handling the breakpoint length at the lowest level, in
insert_memory_breakpoint, and we use breakpoint_from_pc and
breakpoint_from_length together there, which looks unclean. Ideally, we
can move these code handling breakpoint length (breakpoint_from_pc and
breakpoint_from_length) to upper levels, like this,
set_breakpoint_at (call breakpoint_from_pc)
set_gdb_breakpoint_1 (call breakpoint_from_length)
|
`--> set_breakpoint
|
`-->set_raw_breakpoint_at
|
`--> the_target->insert_point
all needed information is saved in struct breakpoint or struct
raw_breakpoint, and function set_breakpoint and it callees can use
breakpoint or raw_breakpoint directly. It'll be cleaner in this way,
let me know what do you think?
> From: Yao Qi <qiyaoltc@gmail.com>
> Cc: <gdb-patches@sourceware.org>
> Date: Mon, 28 Sep 2015 10:54:48 +0100
>
> After thinking about it a bit more, I think this reveals some design
> issues in GDBserver brekapoint, nowadays, GDBserver inserts its own
> breakpoints and breakpoints requested by GDB. After this patch series,
> GDBserver should be able to:
>
> 1) choose the right breakpoint instruction for its own breakpoints,
> according to the breakpoint address, status register flag, etc,
> through API set_breakpoint_at,
> 2) choose the right breakpoint instruction for breakpoints requested
> by GDB, according to the information in Z packets, through API
> set_gdb_breakpoint
>
> there should be two paths for them, and each path needs different target
> hook to choose breakpoint instructions. breakpoint_from_pc is needed for
> #1, and breakpoint_from_length is needed for #2. In your current patch
> set (in patch 4/5), two things are mixed together, which doesn't look
> good to me. The current functions calls in GDBserver to create
> breakpoint is like
>
> set_breakpoint_at
> set_gdb_breakpoint_1
> |
> `--> set_breakpoint
> |
> `-->set_raw_breakpoint_at
> |
> `--> the_target->insert_point
>
> we are handling the breakpoint length at the lowest level, in
> insert_memory_breakpoint, and we use breakpoint_from_pc and
> breakpoint_from_length together there, which looks unclean. Ideally, we
> can move these code handling breakpoint length (breakpoint_from_pc and
> breakpoint_from_length) to upper levels, like this,
>
> set_breakpoint_at (call breakpoint_from_pc)
> set_gdb_breakpoint_1 (call breakpoint_from_length)
> |
> `--> set_breakpoint
> |
> `-->set_raw_breakpoint_at
> |
> `--> the_target->insert_point
>
> all needed information is saved in struct breakpoint or struct
> raw_breakpoint, and function set_breakpoint and it callees can use
> breakpoint or raw_breakpoint directly. It'll be cleaner in this way,
> let me know what do you think?
Sometimes only the target layer knows how to choose the length
correctly. Are we sure this isn't one of those cases, and can never
be?
On 09/28/2015 06:05 AM, Eli Zaretskii wrote:
>> From: Yao Qi <qiyaoltc@gmail.com>
>> Cc: <gdb-patches@sourceware.org>
>> Date: Mon, 28 Sep 2015 10:54:48 +0100
>>
>> After thinking about it a bit more, I think this reveals some design
>> issues in GDBserver brekapoint, nowadays, GDBserver inserts its own
>> breakpoints and breakpoints requested by GDB. After this patch series,
>> GDBserver should be able to:
>>
>> 1) choose the right breakpoint instruction for its own breakpoints,
>> according to the breakpoint address, status register flag, etc,
>> through API set_breakpoint_at,
>> 2) choose the right breakpoint instruction for breakpoints requested
>> by GDB, according to the information in Z packets, through API
>> set_gdb_breakpoint
>>
>> there should be two paths for them, and each path needs different target
>> hook to choose breakpoint instructions. breakpoint_from_pc is needed for
>> #1, and breakpoint_from_length is needed for #2. In your current patch
>> set (in patch 4/5), two things are mixed together, which doesn't look
>> good to me. The current functions calls in GDBserver to create
>> breakpoint is like
>>
>> set_breakpoint_at
>> set_gdb_breakpoint_1
>> |
>> `--> set_breakpoint
>> |
>> `-->set_raw_breakpoint_at
>> |
>> `--> the_target->insert_point
>>
>> we are handling the breakpoint length at the lowest level, in
>> insert_memory_breakpoint, and we use breakpoint_from_pc and
>> breakpoint_from_length together there, which looks unclean. Ideally, we
>> can move these code handling breakpoint length (breakpoint_from_pc and
>> breakpoint_from_length) to upper levels, like this,
>>
>> set_breakpoint_at (call breakpoint_from_pc)
>> set_gdb_breakpoint_1 (call breakpoint_from_length)
>> |
>> `--> set_breakpoint
>> |
>> `-->set_raw_breakpoint_at
>> |
>> `--> the_target->insert_point
>>
>> all needed information is saved in struct breakpoint or struct
>> raw_breakpoint, and function set_breakpoint and it callees can use
>> breakpoint or raw_breakpoint directly. It'll be cleaner in this way,
>> let me know what do you think?
>
> Sometimes only the target layer knows how to choose the length
> correctly. Are we sure this isn't one of those cases, and can never
> be?
>
breakpoint_from_pc and breakpoint_from_length are target ops so indeed
only the target knows but it's ok we can query the target properly.
@@ -7069,16 +7069,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,10 @@ 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 size. */
int size;
@@ -300,6 +302,12 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
{
unsigned char buf[MAX_BREAKPOINT_LEN];
int err;
+ const unsigned char *breakpoint_data;
+ int breakpoint_len;
+ CORE_ADDR pc;
+
+ pc = bp->pcfull;
+ breakpoint_data = the_target->breakpoint_from_pc (&pc, &breakpoint_len);
if (breakpoint_data == NULL)
return 1;
@@ -349,6 +357,11 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
{
unsigned char buf[MAX_BREAKPOINT_LEN];
int err;
+ int breakpoint_len;
+ CORE_ADDR pc;
+
+ pc = bp->pcfull;
+ the_target->breakpoint_from_pc (&pc, &breakpoint_len);
/* Since there can be trap breakpoints inserted in the same address
range, we use `write_inferior_memory', which takes care of
@@ -375,15 +388,27 @@ 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,
+set_raw_breakpoint_at (enum raw_bkpt_type type, const CORE_ADDR where, int size,
int *err)
{
struct process_info *proc = current_process ();
struct raw_breakpoint *bp;
+ CORE_ADDR pc;
+ int breakpoint_len;
+
+ /* pc could be modified by breakpoint_from_pc, use the modified
+ version to find breakpoints and use the full where pc for
+ insert_point so that arch specific data can be passed. */
+ pc = where;
+
+ the_target->breakpoint_from_pc (&pc, &breakpoint_len);
+
+ if (size == 0)
+ size = breakpoint_len;
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,7 +430,8 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int size,
}
bp = XCNEW (struct raw_breakpoint);
- bp->pc = where;
+ bp->pc = pc;
+ bp->pcfull = where;
bp->size = size;
bp->refcount = 1;
bp->raw_type = type;
@@ -774,8 +800,9 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
{
int err_ignored;
+ /* default breakpoint_len will be initialized downstream. */
return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
- where, breakpoint_len, handler,
+ where, 0, handler,
&err_ignored);
}
@@ -1588,13 +1615,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)
{
@@ -1665,6 +1685,13 @@ validate_inserted_breakpoint (struct raw_breakpoint *bp)
{
unsigned char *buf;
int err;
+ const unsigned char *breakpoint_data;
+ int breakpoint_len;
+ CORE_ADDR raw_pc;
+
+ raw_pc = bp->pcfull;
+
+ breakpoint_data = the_target->breakpoint_from_pc (&raw_pc, &breakpoint_len);
gdb_assert (bp->inserted);
gdb_assert (bp->raw_type == raw_bkpt_type_sw);
@@ -1762,10 +1789,15 @@ 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 start, end;
+ int breakpoint_len;
+ CORE_ADDR raw_pc;
+ CORE_ADDR bp_end, start, end;
int copy_offset, copy_len, buf_offset;
+ raw_pc = bp->pcfull;
+ the_target->breakpoint_from_pc (&raw_pc, &breakpoint_len);
+ bp_end = bp->pc + breakpoint_len;
+
if (bp->raw_type != raw_bkpt_type_sw)
continue;
@@ -1851,10 +1883,17 @@ 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 start, end;
+ int breakpoint_len;
+ const unsigned char *breakpoint_data;
+ CORE_ADDR raw_pc;
+ CORE_ADDR bp_end, start, end;
int copy_offset, copy_len, buf_offset;
+ raw_pc = bp->pcfull;
+ breakpoint_data =
+ the_target->breakpoint_from_pc (&raw_pc, &breakpoint_len);
+ bp_end = bp->pc + breakpoint_len;
+
if (bp->raw_type != raw_bkpt_type_sw)
continue;
@@ -1963,6 +2002,7 @@ 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->size = src->raw->size;
memcpy (dest_raw->old_data, src->raw->old_data, MAX_BREAKPOINT_LEN);
dest_raw->inserted = src->raw->inserted;