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

[Joel Brobecker <brobecker@adacore.com>]

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