[RFA/7.8] user breakpoint not inserted if software-single-step at same location
Commit Message
On 06/03/2014 09:22 AM, Pedro Alves wrote:
> On 06/03/2014 12:16 AM, Pedro Alves wrote:
>
>> Let me know what you think, and feel free to push if it
>> looks OK. Tested on all combinations of
>> native|gdbserver X hardware-|software- stepping. At least,
>> I think I did. If not this exact version, some other minor
>> variation. :-)
>
> Bah, I woke up realizing that the version I posted forgets to
> clone the shadow buffer! Let me fix that and repost...
>
Here is the fixed version. Retested on all combinations of
native|gdbserver X hardware-|software- stepping on x86_64 Fedora 20.
Fixes both new tests.
8<------------
Subject: [PATCH] User breakpoint ignored if software-single-step at same
location
with the following code...
12 Nested; -- break #1
13 return I; -- break #2
14 end;
(line 12 is a call to function Nested)
... we have noticed the following errorneous behavior on ppc-aix,
where, after having inserted a breakpoint at line 12 and line 13,
and continuing from the breakpoint at line 12, the program never
stops at line 13, running away until the program terminates:
% gdb -q func
(gdb) b func.adb:12
Breakpoint 1 at 0x10000a24: file func.adb, line 12.
(gdb) b func.adb:13
Breakpoint 2 at 0x10000a28: file func.adb, line 13.
(gdb) run
Starting program: /[...]/func
Breakpoint 1, func () at func.adb:12
12 Nested; -- break #1
(gdb) c
Continuing.
[Inferior 1 (process 4128872) exited with code 02]
When resuming from the first breakpoint, GDB first tries to step out
of that first breakpoint. We rely on software single-stepping on this
platform, and it just so happens that the address of the first
software single-step breakpoint is the same as the user's breakpoint
#2 (0x10000a28). So, with infrun and target traces turned on (but
uninteresting traces snip'ed off), the "continue" operation looks like
this:
(gdb) c
### First, we insert the user breakpoints (the second one is an internal
### breakpoint on __pthread_init). The first user breakpoint is not
### inserted as we need to step out of it first.
target_insert_breakpoint (0x0000000010000a28, xxx) = 0
target_insert_breakpoint (0x00000000d03f3800, xxx) = 0
### Then we proceed with the step-out-of-breakpoint...
infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread [process 15335610] at 0x10000a24
### That's when we insert the SSS breakpoints...
target_insert_breakpoint (0x0000000010000a28, xxx) = 0
target_insert_breakpoint (0x00000000100009ac, xxx) = 0
### ... then let the inferior resume...
target_resume (15335610, continue, 0)
infrun: wait_for_inferior ()
target_wait (-1, status, options={}) = 15335610, status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: target_wait (-1, status) =
infrun: 15335610 [process 15335610],
infrun: status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: infwait_normal_state
infrun: TARGET_WAITKIND_STOPPED
infrun: stop_pc = 0x100009ac
### At this point, we stopped at the second SSS breakpoint...
target_stopped_by_watchpoint () = 0
### We remove the SSS breakpoints...
target_remove_breakpoint (0x0000000010000a28, xxx) = 0
target_remove_breakpoint (0x00000000100009ac, xxx) = 0
target_stopped_by_watchpoint () = 0
### We find that we're not done, so we resume....
infrun: no stepping, continue
### And thus insert the user breakpoints again, except we're not
### inserting the second breakpoint?!?
target_insert_breakpoint (0x0000000010000a24, xxx) = 0
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [process 15335610] at 0x100009ac
target_resume (-1, continue, 0)
infrun: prepare_to_wait
target_wait (-1, status, options={}) = 15335610, status->kind = exited, status = 2
What happens is that the removal of the software single-step
breakpoints effectively removed the breakpoint instruction from
inferior memory. But because such breakpoints are inserted directly
as raw breakpoints rather than through the normal chain of
breakpoints, we fail to notice that one of the user breakpoints points
to the same address and that this user breakpoint is therefore
effectively un-inserted. When resuming after the single-step, GDB
thinks that the user breakpoint is still inserted and therefore does
not need to insert it again.
This patch teaches the insert and remove routines of both regular and
raw breakpoints to be aware of each other. Special care needs to be
applied in case the target supports evaluation of breakpoint
conditions or commands.
gdb/ChangeLog:
PR breakpoints/17000
* breakpoint.c (find_non_raw_software_breakpoint_inserted_here):
New function, extracted from software_breakpoint_inserted_here_p.
(software_breakpoint_inserted_here_p): Replace factored out code
by call to find_non_raw_software_breakpoint_inserted_here.
(bp_target_info_copy_insertion_state): New function.
(bkpt_insert_location): Handle the case of a single-step
breakpoint already inserted at the same address.
(bkpt_remove_location): Handle the case of a single-step
breakpoint still inserted at the same address.
(deprecated_insert_raw_breakpoint): Handle the case of non-raw
breakpoint already inserted at the same address.
(deprecated_remove_raw_breakpoint): Handle the case of a
non-raw breakpoint still inserted at the same address.
(find_single_step_breakpoint): New function, extracted from
single_step_breakpoint_inserted_here_p.
(find_single_step_breakpoint): New function,
factored out from single_step_breakpoint_inserted_here_p.
(single_step_breakpoint_inserted_here_p): Reimplement.
gdb/testsuite/ChangeLog:
PR breakpoints/17000
* gdb.base/sss-bp-on-user-bp.exp: Remove kfail.
* gdb.base/sss-bp-on-user-bp-2.exp: Remove kfail.
Tested on ppc-aix with AdaCore's testsuite. Tested on x86_64-linux,
(native and gdbserver) with the official testsuite. Also tested on
x86_64-linux through Pedro's branch enabling software single-stepping
on that platform (native and gdbserver).
---
gdb/breakpoint.c | 141 ++++++++++++++++++++++---
gdb/testsuite/gdb.base/sss-bp-on-user-bp-2.exp | 1 -
gdb/testsuite/gdb.base/sss-bp-on-user-bp.exp | 3 +-
3 files changed, 125 insertions(+), 20 deletions(-)
Comments
Hi Pedro,
> > Bah, I woke up realizing that the version I posted forgets to
> > clone the shadow buffer! Let me fix that and repost...
You are producing patches so fast, I am wondering if I will be able
to keep up! :-)
> gdb/ChangeLog:
>
> PR breakpoints/17000
> * breakpoint.c (find_non_raw_software_breakpoint_inserted_here):
> New function, extracted from software_breakpoint_inserted_here_p.
> (software_breakpoint_inserted_here_p): Replace factored out code
> by call to find_non_raw_software_breakpoint_inserted_here.
> (bp_target_info_copy_insertion_state): New function.
> (bkpt_insert_location): Handle the case of a single-step
> breakpoint already inserted at the same address.
> (bkpt_remove_location): Handle the case of a single-step
> breakpoint still inserted at the same address.
> (deprecated_insert_raw_breakpoint): Handle the case of non-raw
> breakpoint already inserted at the same address.
> (deprecated_remove_raw_breakpoint): Handle the case of a
> non-raw breakpoint still inserted at the same address.
> (find_single_step_breakpoint): New function, extracted from
> single_step_breakpoint_inserted_here_p.
> (find_single_step_breakpoint): New function,
> factored out from single_step_breakpoint_inserted_here_p.
> (single_step_breakpoint_inserted_here_p): Reimplement.
>
> gdb/testsuite/ChangeLog:
>
> PR breakpoints/17000
> * gdb.base/sss-bp-on-user-bp.exp: Remove kfail.
> * gdb.base/sss-bp-on-user-bp-2.exp: Remove kfail.
You are making it us realize that the problem is more and more
complex than we thought! :-(. And I think we'll need a small
adjustment to your patch in order to account for something that
may have been missed. See below:
> @@ -15138,12 +15196,30 @@ deprecated_insert_raw_breakpoint (struct gdbarch *gdbarch,
> struct address_space *aspace, CORE_ADDR pc)
> {
> struct bp_target_info *bp_tgt;
> + struct bp_location *bl;
>
> bp_tgt = XCNEW (struct bp_target_info);
>
> bp_tgt->placed_address_space = aspace;
> bp_tgt->placed_address = pc;
>
> + /* If an unconditional non-raw breakpoint is already inserted at
> + that location, there's no need to insert another. However, with
> + target-side evaluation of breakpoint conditions, if the
> + breakpoint that is currently inserted on the target is
> + conditional, we need to make it unconditional. Note that a
> + breakpoint with target-side commands is not reported even if
> + unconditional, so we need to remove the commands from the target
> + as well. */
> + bl = find_non_raw_software_breakpoint_inserted_here (aspace, pc);
> + if (bl != NULL
> + && VEC_empty (agent_expr_p, bl->target_info.conditions)
> + && VEC_empty (agent_expr_p, bl->target_info.tcommands))
> + {
> + bp_target_info_copy_insertion_state (bp_tgt, &bl->target_info);
> + return bp_tgt;
> + }
> +
ISTM that you are assuming that there would only be one other breakpoint
inserted at this location. What if there were more?
If I am right, I suggest the addition of an extra parameter to
find_non_raw_software_breakpoint_inserted_here which would be
a pointer to a filtering function. If NULL, no filtering is done,
but if not NULL, the filter function must accept the bp_location
for find_non_raw_software_breakpoint_inserted_here to return it.
> deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
> {
> struct bp_target_info *bp_tgt = bp;
> + struct address_space *aspace = bp_tgt->placed_address_space;
> + CORE_ADDR address = bp_tgt->placed_address;
> + struct bp_location *bl;
> int ret;
>
> - ret = target_remove_breakpoint (gdbarch, bp_tgt);
> + bl = find_non_raw_software_breakpoint_inserted_here (aspace, address);
> +
> + /* Only remove the raw breakpoint if there are no other non-raw
> + breakpoints still inserted at this location. Otherwise, we would
> + be effectively disabling those breakpoints. */
> + if (bl == NULL)
> + ret = target_remove_breakpoint (gdbarch, bp_tgt);
> + else if (!VEC_empty (agent_expr_p, bl->target_info.conditions)
> + || !VEC_empty (agent_expr_p, bl->target_info.tcommands))
> + {
> + /* The target is evaluating conditions, and when we inserted the
> + software single-step breakpoint, we had made the breakpoint
> + unconditional and command-less on the target side. Reinsert
> + to restore the conditions/commands. */
> + ret = target_insert_breakpoint (bl->gdbarch, &bl->target_info);
> + }
> + else
> + ret = 0;
Same here, I think.
On 06/03/2014 02:35 PM, Joel Brobecker wrote:
> Hi Pedro,
>
>>> Bah, I woke up realizing that the version I posted forgets to
>>> clone the shadow buffer! Let me fix that and repost...
>
> You are producing patches so fast, I am wondering if I will be able
> to keep up! :-)
:-)
>> @@ -15138,12 +15196,30 @@ deprecated_insert_raw_breakpoint (struct gdbarch *gdbarch,
>> struct address_space *aspace, CORE_ADDR pc)
>> {
>> struct bp_target_info *bp_tgt;
>> + struct bp_location *bl;
>>
>> bp_tgt = XCNEW (struct bp_target_info);
>>
>> bp_tgt->placed_address_space = aspace;
>> bp_tgt->placed_address = pc;
>>
>> + /* If an unconditional non-raw breakpoint is already inserted at
>> + that location, there's no need to insert another. However, with
>> + target-side evaluation of breakpoint conditions, if the
>> + breakpoint that is currently inserted on the target is
>> + conditional, we need to make it unconditional. Note that a
>> + breakpoint with target-side commands is not reported even if
>> + unconditional, so we need to remove the commands from the target
>> + as well. */
>> + bl = find_non_raw_software_breakpoint_inserted_here (aspace, pc);
>> + if (bl != NULL
>> + && VEC_empty (agent_expr_p, bl->target_info.conditions)
>> + && VEC_empty (agent_expr_p, bl->target_info.tcommands))
>> + {
>> + bp_target_info_copy_insertion_state (bp_tgt, &bl->target_info);
>> + return bp_tgt;
>> + }
>> +
>
> ISTM that you are assuming that there would only be one other breakpoint
> inserted at this location. What if there were more?
Yep, it's a valid assumption. Only one of those can be the one that
is actually inserted in the target. All others breakpoints are considered
duplicates, with bl->duplicate == 1 and bl->inserted == 0, and never reach
the target. The duplicate location logic in the tail of update_global_location_list
takes care of it:
/* This and the above ensure the invariant that the first location
is not duplicated, and is the inserted one.
All following are marked as duplicated, and are not inserted. */
if (loc->inserted)
swap_insertion (loc, *loc_first_p);
loc->duplicate = 1;
The one that is inserted will hold a merge of all the agent
expressions (in target_info.conditions and target_info.tcommands) of
the target-side conditions and commands of all breakpoints at that
address. Those are computed just before that single breakpoint
is inserted (build_target_condition_list, build_target_command_list).
> Yep, it's a valid assumption. Only one of those can be the one that
> is actually inserted in the target. All others breakpoints are considered
> duplicates, with bl->duplicate == 1 and bl->inserted == 0, and never reach
> the target. The duplicate location logic in the tail of update_global_location_list
> takes care of it:
>
> /* This and the above ensure the invariant that the first location
> is not duplicated, and is the inserted one.
> All following are marked as duplicated, and are not inserted. */
> if (loc->inserted)
> swap_insertion (loc, *loc_first_p);
> loc->duplicate = 1;
>
> The one that is inserted will hold a merge of all the agent
> expressions (in target_info.conditions and target_info.tcommands) of
> the target-side conditions and commands of all breakpoints at that
> address. Those are computed just before that single breakpoint
> is inserted (build_target_condition_list, build_target_command_list).
Indeed! Thanks for explaining it.
I don't have any other comment on the patch. I ran it through our
testsuite on ppc-aix, JIC, and as expected, found that it fixed
the regression without introducing new ones. I think you can push
that version!
Thanks for your help, Pedro.
On 06/03/2014 05:23 PM, Joel Brobecker wrote:
> I don't have any other comment on the patch. I ran it through our
> testsuite on ppc-aix, JIC, and as expected, found that it fixed
> the regression without introducing new ones. I think you can push
> that version!
Awesome, thank you! Now pushed.
> Awesome, thank you! Now pushed.
Yeepee! I see that you marked the PR as fixed. I moved the AI
in the wiki to Done.
Thanks!
@@ -229,6 +229,9 @@ static void tcatch_command (char *arg, int from_tty);
static void detach_single_step_breakpoints (void);
+static int find_single_step_breakpoint (struct address_space *aspace,
+ CORE_ADDR pc);
+
static void free_bp_location (struct bp_location *loc);
static void incref_bp_location (struct bp_location *loc);
static void decref_bp_location (struct bp_location **loc);
@@ -4165,12 +4168,12 @@ breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
return 0;
}
-/* This function returns non-zero iff there is a software breakpoint
- inserted at PC. */
+/* Ignoring deprecated raw breakpoints, return non-zero iff there is a
+ software breakpoint inserted at PC. */
-int
-software_breakpoint_inserted_here_p (struct address_space *aspace,
- CORE_ADDR pc)
+static struct bp_location *
+find_non_raw_software_breakpoint_inserted_here (struct address_space *aspace,
+ CORE_ADDR pc)
{
struct bp_location *bl, **blp_tmp;
@@ -4188,10 +4191,23 @@ software_breakpoint_inserted_here_p (struct address_space *aspace,
&& !section_is_mapped (bl->section))
continue; /* unmapped overlay -- can't be a match */
else
- return 1;
+ return bl;
}
}
+ return NULL;
+}
+
+/* This function returns non-zero iff there is a software breakpoint
+ inserted at PC. */
+
+int
+software_breakpoint_inserted_here_p (struct address_space *aspace,
+ CORE_ADDR pc)
+{
+ if (find_non_raw_software_breakpoint_inserted_here (aspace, pc) != NULL)
+ return 1;
+
/* Also check for software single-step breakpoints. */
if (single_step_breakpoint_inserted_here_p (aspace, pc))
return 1;
@@ -13112,6 +13128,19 @@ bkpt_re_set (struct breakpoint *b)
breakpoint_re_set_default (b);
}
+/* Copy SRC's shadow buffer and whatever else we'd set if we actually
+ inserted DEST, so we can remove it later, in case SRC is removed
+ first. */
+
+static void
+bp_target_info_copy_insertion_state (struct bp_target_info *dest,
+ const struct bp_target_info *src)
+{
+ dest->shadow_len = src->shadow_len;
+ memcpy (dest->shadow_contents, src->shadow_contents, src->shadow_len);
+ dest->placed_size = src->placed_size;
+}
+
static int
bkpt_insert_location (struct bp_location *bl)
{
@@ -13119,8 +13148,25 @@ bkpt_insert_location (struct bp_location *bl)
return target_insert_hw_breakpoint (bl->gdbarch,
&bl->target_info);
else
- return target_insert_breakpoint (bl->gdbarch,
- &bl->target_info);
+ {
+ struct bp_target_info *bp_tgt = &bl->target_info;
+ int ret;
+ int sss_slot;
+
+ /* There is no need to insert a breakpoint if an unconditional
+ raw/sss breakpoint is already inserted at that location. */
+ sss_slot = find_single_step_breakpoint (bp_tgt->placed_address_space,
+ bp_tgt->placed_address);
+ if (sss_slot >= 0)
+ {
+ struct bp_target_info *sss_bp_tgt = single_step_breakpoints[sss_slot];
+
+ bp_target_info_copy_insertion_state (bp_tgt, sss_bp_tgt);
+ return 0;
+ }
+
+ return target_insert_breakpoint (bl->gdbarch, bp_tgt);
+ }
}
static int
@@ -13129,7 +13175,19 @@ bkpt_remove_location (struct bp_location *bl)
if (bl->loc_type == bp_loc_hardware_breakpoint)
return target_remove_hw_breakpoint (bl->gdbarch, &bl->target_info);
else
- return target_remove_breakpoint (bl->gdbarch, &bl->target_info);
+ {
+ struct bp_target_info *bp_tgt = &bl->target_info;
+ struct address_space *aspace = bp_tgt->placed_address_space;
+ CORE_ADDR address = bp_tgt->placed_address;
+
+ /* Only remove the breakpoint if there is no raw/sss breakpoint
+ still inserted at this location. Otherwise, we would be
+ effectively disabling the raw/sss breakpoint. */
+ if (single_step_breakpoint_inserted_here_p (aspace, address))
+ return 0;
+
+ return target_remove_breakpoint (bl->gdbarch, bp_tgt);
+ }
}
static int
@@ -15138,12 +15196,30 @@ deprecated_insert_raw_breakpoint (struct gdbarch *gdbarch,
struct address_space *aspace, CORE_ADDR pc)
{
struct bp_target_info *bp_tgt;
+ struct bp_location *bl;
bp_tgt = XCNEW (struct bp_target_info);
bp_tgt->placed_address_space = aspace;
bp_tgt->placed_address = pc;
+ /* If an unconditional non-raw breakpoint is already inserted at
+ that location, there's no need to insert another. However, with
+ target-side evaluation of breakpoint conditions, if the
+ breakpoint that is currently inserted on the target is
+ conditional, we need to make it unconditional. Note that a
+ breakpoint with target-side commands is not reported even if
+ unconditional, so we need to remove the commands from the target
+ as well. */
+ bl = find_non_raw_software_breakpoint_inserted_here (aspace, pc);
+ if (bl != NULL
+ && VEC_empty (agent_expr_p, bl->target_info.conditions)
+ && VEC_empty (agent_expr_p, bl->target_info.tcommands))
+ {
+ bp_target_info_copy_insertion_state (bp_tgt, &bl->target_info);
+ return bp_tgt;
+ }
+
if (target_insert_breakpoint (gdbarch, bp_tgt) != 0)
{
/* Could not insert the breakpoint. */
@@ -15161,9 +15237,30 @@ int
deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
{
struct bp_target_info *bp_tgt = bp;
+ struct address_space *aspace = bp_tgt->placed_address_space;
+ CORE_ADDR address = bp_tgt->placed_address;
+ struct bp_location *bl;
int ret;
- ret = target_remove_breakpoint (gdbarch, bp_tgt);
+ bl = find_non_raw_software_breakpoint_inserted_here (aspace, address);
+
+ /* Only remove the raw breakpoint if there are no other non-raw
+ breakpoints still inserted at this location. Otherwise, we would
+ be effectively disabling those breakpoints. */
+ if (bl == NULL)
+ ret = target_remove_breakpoint (gdbarch, bp_tgt);
+ else if (!VEC_empty (agent_expr_p, bl->target_info.conditions)
+ || !VEC_empty (agent_expr_p, bl->target_info.tcommands))
+ {
+ /* The target is evaluating conditions, and when we inserted the
+ software single-step breakpoint, we had made the breakpoint
+ unconditional and command-less on the target side. Reinsert
+ to restore the conditions/commands. */
+ ret = target_insert_breakpoint (bl->gdbarch, &bl->target_info);
+ }
+ else
+ ret = 0;
+
xfree (bp_tgt);
return ret;
@@ -15269,12 +15366,12 @@ detach_single_step_breakpoints (void)
single_step_breakpoints[i]);
}
-/* Check whether a software single-step breakpoint is inserted at
- PC. */
+/* Find the software single-step breakpoint that inserted at PC.
+ Returns its slot if found, and -1 if not found. */
-int
-single_step_breakpoint_inserted_here_p (struct address_space *aspace,
- CORE_ADDR pc)
+static int
+find_single_step_breakpoint (struct address_space *aspace,
+ CORE_ADDR pc)
{
int i;
@@ -15285,10 +15382,20 @@ single_step_breakpoint_inserted_here_p (struct address_space *aspace,
&& breakpoint_address_match (bp_tgt->placed_address_space,
bp_tgt->placed_address,
aspace, pc))
- return 1;
+ return i;
}
- return 0;
+ return -1;
+}
+
+/* Check whether a software single-step breakpoint is inserted at
+ PC. */
+
+int
+single_step_breakpoint_inserted_here_p (struct address_space *aspace,
+ CORE_ADDR pc)
+{
+ return find_single_step_breakpoint (aspace, pc) >= 0;
}
/* Returns 0 if 'bp' is NOT a syscall catchpoint,
@@ -121,7 +121,6 @@ gdb_test_multiple $test $test {
set command "stepi_del_break"
set test $command
-setup_kfail "breakpoints/17000" "*-*-*"
gdb_test_multiple $command $test {
-re "^$command\r\n$gdb_prompt " {
# Note no end anchor, because "si&" finishes and prints the
@@ -47,6 +47,5 @@ gdb_test "si" "Breakpoint .* bar break .*"
# If the breakpoint is still correctly inserted, then this jump should
# re-trigger it. Otherwise, GDB will lose control and the program
-# will exit.
-setup_kfail "breakpoints/17000" "*-*-*"
+# will exit. See PR breakpoints/17000.
gdb_test "jump *\$pc" "Breakpoint .* bar break .*"