Re: [RFA/7.8] user breakpoint not inserted if software-single-step at same location

Joel Brobecker
2014-06-03T13:35:44Z

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.

--
Joel