This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/3] Support up to 3 conditional branches in an atomic sequence
- From: Anton Blanchard <anton at samba dot org>
- To: Joel Brobecker <brobecker at adacore dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 16 Jul 2012 16:33:55 +1000
- Subject: Re: [PATCH 2/3] Support up to 3 conditional branches in an atomic sequence
- References: <20120606135557.7da37cbe@kryten> <20120606135655.57bd5b54@kryten> <20120613160208.GH18729@adacore.com>
Hi Joel,
Thanks for the review.
> Mostly OK. I would like you to change the name of the macro to
> MAX_SINGLE_STEP_BREAKPOINTS, though. Can you, please?
Makes sense, done.
> > + if (last_breakpoint >= (NR_SINGLE_STEP_BREAKPOINTS-1))
> > + return 0; /* too many conditional branches found,
> > fallback
>
> Can you remove the extra parens which are useless in this case?
> And binary operators should have a space before and after.
Done.
> > Index: gdb/gdb/breakpoint.h
> [...]
> > -/* Manage a software single step breakpoint (or two). Insert may
> > be
> > - called twice before remove is called. */
> > +/* Manage software single step breakpoints. */
> > +#define NR_SINGLE_STEP_BREAKPOINTS 4
> > +
>
> Just curious: Why did you remove the second sentence from the comment?
> Is it no longer true? Maybe it is the "twice" that should be changed
> into "multiple times"?
Agreed. Here is a new version.
Anton
--
gdb currently supports 1 conditional branch inside a ppc larx/stcx
critical region. Unfortunately there is existing code that contains more
than 1, for example in the ppc64 Linux kernel:
c00000000003ac18 <.__hash_page_4K>:
...
c00000000003ac4c: 7f e0 30 a8 ldarx r31,0,r6
c00000000003ac50: 7c 80 f8 79 andc. r0,r4,r31
c00000000003ac54: 40 82 02 94 bne- c00000000003aee8 <htab_wrong_access>
c00000000003ac58: 73 e0 08 00 andi. r0,r31,2048
c00000000003ac5c: 40 82 01 b0 bne- c00000000003ae0c <htab_bail_ok>
c00000000003ac60: 54 9e f6 30 rlwinm r30,r4,30,24,24
c00000000003ac64: 7f de fb 78 or r30,r30,r31
c00000000003ac68: 63 de 09 00 ori r30,r30,2304
c00000000003ac6c: 67 de 10 00 oris r30,r30,4096
c00000000003ac70: 7f c0 31 ad stdcx. r30,0,r6
If we try to single step through this we get stuck forever because
the reservation is never set when we step over the stdcx.
The following patch bumps the number to 3 conditional branches + 1
terminating branch. With this patch applied I can single step through
the offending function in the ppc64 Linux kernel.
2012-06-05 Anton Blanchard <anton@samba.org>
* gdb/breakpoint.h: Define MAX_SINGLE_STEP_BREAKPOINTS
* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
than two breakpoints.
* gdb/breakpoint.c (insert_single_step_breakpoint): Likewise
(insert_single_step_breakpoint): Likewise
(single_step_breakpoints_inserted): Likewise
(cancel_single_step_breakpoints): Likewise
(detach_single_step_breakpoints): Likewise
(single_step_breakpoint_inserted_here_p): Likewise
Index: b/gdb/rs6000-tdep.c
===================================================================
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1087,7 +1087,7 @@ ppc_deal_with_atomic_sequence (struct fr
struct address_space *aspace = get_frame_address_space (frame);
enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
CORE_ADDR pc = get_frame_pc (frame);
- CORE_ADDR breaks[2] = {-1, -1};
+ CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS];
CORE_ADDR loc = pc;
CORE_ADDR closing_insn; /* Instruction that closes the atomic sequence. */
int insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
@@ -1096,7 +1096,6 @@ ppc_deal_with_atomic_sequence (struct fr
int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed). */
const int atomic_sequence_length = 16; /* Instruction sequence length. */
int opcode; /* Branch instruction's OPcode. */
- int bc_insn_count = 0; /* Conditional branch instruction count. */
/* Assume all atomic sequences start with a lwarx/ldarx instruction. */
if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
@@ -1110,24 +1109,20 @@ ppc_deal_with_atomic_sequence (struct fr
loc += PPC_INSN_SIZE;
insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
- /* Assume that there is at most one conditional branch in the atomic
- sequence. If a conditional branch is found, put a breakpoint in
- its destination address. */
if ((insn & BRANCH_MASK) == BC_INSN)
{
int immediate = ((insn & 0xfffc) ^ 0x8000) - 0x8000;
int absolute = insn & 2;
- if (bc_insn_count >= 1)
- return 0; /* More than one conditional branch found, fallback
+ if (last_breakpoint >= MAX_SINGLE_STEP_BREAKPOINTS - 1)
+ return 0; /* too many conditional branches found, fallback
to the standard single-step code. */
if (absolute)
- breaks[1] = immediate;
+ breaks[last_breakpoint] = immediate;
else
- breaks[1] = loc + immediate;
+ breaks[last_breakpoint] = loc + immediate;
- bc_insn_count++;
last_breakpoint++;
}
@@ -1146,18 +1141,29 @@ ppc_deal_with_atomic_sequence (struct fr
insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
/* Insert a breakpoint right after the end of the atomic sequence. */
- breaks[0] = loc;
+ breaks[last_breakpoint] = loc;
- /* Check for duplicated breakpoints. Check also for a breakpoint
- placed (branch instruction's destination) anywhere in sequence. */
- if (last_breakpoint
- && (breaks[1] == breaks[0]
- || (breaks[1] >= pc && breaks[1] <= closing_insn)))
- last_breakpoint = 0;
-
- /* Effectively inserts the breakpoints. */
for (index = 0; index <= last_breakpoint; index++)
- insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
+ {
+ int index2;
+ int insert_bp = 1;
+
+ /* Check for a breakpoint placed (branch instruction's destination)
+ anywhere in sequence. */
+ if (breaks[index] >= pc && breaks[index] <= closing_insn)
+ continue;
+
+ /* Check for duplicated breakpoints. */
+ for (index2 = 0; index2 < index; index2++)
+ {
+ if (breaks[index] == breaks[index2])
+ insert_bp = 0;
+ }
+
+ /* insert the breakpoint. */
+ if (insert_bp)
+ insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
+ }
return 1;
}
Index: b/gdb/breakpoint.c
===================================================================
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14741,11 +14741,10 @@ deprecated_remove_raw_breakpoint (struct
return ret;
}
-/* One (or perhaps two) breakpoints used for software single
- stepping. */
+/* Breakpoints used for software single stepping. */
-static void *single_step_breakpoints[2];
-static struct gdbarch *single_step_gdbarch[2];
+static void *single_step_breakpoints[MAX_SINGLE_STEP_BREAKPOINTS];
+static struct gdbarch *single_step_gdbarch[MAX_SINGLE_STEP_BREAKPOINTS];
/* Create and insert a breakpoint for software single step. */
@@ -14754,19 +14753,17 @@ insert_single_step_breakpoint (struct gd
struct address_space *aspace,
CORE_ADDR next_pc)
{
+ int i;
void **bpt_p;
- if (single_step_breakpoints[0] == NULL)
- {
- bpt_p = &single_step_breakpoints[0];
- single_step_gdbarch[0] = gdbarch;
- }
- else
- {
- gdb_assert (single_step_breakpoints[1] == NULL);
- bpt_p = &single_step_breakpoints[1];
- single_step_gdbarch[1] = gdbarch;
- }
+ for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
+ if (single_step_breakpoints[i] == NULL)
+ break;
+
+ gdb_assert (i < MAX_SINGLE_STEP_BREAKPOINTS);
+
+ bpt_p = &single_step_breakpoints[i];
+ single_step_gdbarch[i] = gdbarch;
/* NOTE drow/2006-04-11: A future improvement to this function would
be to only create the breakpoints once, and actually put them on
@@ -14787,8 +14784,13 @@ insert_single_step_breakpoint (struct gd
int
single_step_breakpoints_inserted (void)
{
- return (single_step_breakpoints[0] != NULL
- || single_step_breakpoints[1] != NULL);
+ int i;
+
+ for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
+ if (single_step_breakpoints[i] != NULL)
+ return 1;
+
+ return 0;
}
/* Remove and delete any breakpoints used for software single step. */
@@ -14796,22 +14798,21 @@ single_step_breakpoints_inserted (void)
void
remove_single_step_breakpoints (void)
{
+ int i;
+
gdb_assert (single_step_breakpoints[0] != NULL);
/* See insert_single_step_breakpoint for more about this deprecated
call. */
- deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
- single_step_breakpoints[0]);
- single_step_gdbarch[0] = NULL;
- single_step_breakpoints[0] = NULL;
- if (single_step_breakpoints[1] != NULL)
- {
- deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
- single_step_breakpoints[1]);
- single_step_gdbarch[1] = NULL;
- single_step_breakpoints[1] = NULL;
- }
+ for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
+ if (single_step_breakpoints[i] != NULL)
+ {
+ deprecated_remove_raw_breakpoint (single_step_gdbarch[i],
+ single_step_breakpoints[i]);
+ single_step_gdbarch[i] = NULL;
+ single_step_breakpoints[i] = NULL;
+ }
}
/* Delete software single step breakpoints without removing them from
@@ -14824,7 +14825,7 @@ cancel_single_step_breakpoints (void)
{
int i;
- for (i = 0; i < 2; i++)
+ for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
if (single_step_breakpoints[i])
{
xfree (single_step_breakpoints[i]);
@@ -14841,7 +14842,7 @@ detach_single_step_breakpoints (void)
{
int i;
- for (i = 0; i < 2; i++)
+ for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
if (single_step_breakpoints[i])
target_remove_breakpoint (single_step_gdbarch[i],
single_step_breakpoints[i]);
@@ -14856,7 +14857,7 @@ single_step_breakpoint_inserted_here_p (
{
int i;
- for (i = 0; i < 2; i++)
+ for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
{
struct bp_target_info *bp_tgt = single_step_breakpoints[i];
if (bp_tgt
Index: b/gdb/breakpoint.h
===================================================================
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1408,8 +1408,10 @@ extern int is_catchpoint (struct breakpo
deletes all breakpoints. */
extern void delete_command (char *arg, int from_tty);
-/* Manage a software single step breakpoint (or two). Insert may be
- called twice before remove is called. */
+/* Manage software single step breakpoints. Insert may be
+ called multiple times before remove is called. */
+#define MAX_SINGLE_STEP_BREAKPOINTS 4
+
extern void insert_single_step_breakpoint (struct gdbarch *,
struct address_space *,
CORE_ADDR);