[v2,2/2] Only leave dprintf inserted if it is marked as persistent (PR breakpoints/17012)

  On Linux native, if dprintf are inserted when detaching, they are left
in the inferior which causes it to crash from a SIGTRAP. It also happens
with dprintfs on remote targets, when set disconnected-dprintf is off.

I believe that the rationale of the line I modified was to leave dprinfs
inserted in order to support disconnected dprintfs. This adds a check to
see if the dprintf should actually stay inserted or not.

bl->target_info.persist will be 1 only if disconnected-dprintf is on and
we are debugging a remote target. On native, it will always be 0,
regardless of the value of disconnected-dprintf. This makes sense, since
disconnected dprintfs are not supported by the native target.

gdb/Changelog:

2014-07-07  Simon Marchi  <simon.marchi@ericsson.com>

	PR breakpoints/17012
	* breakpoint.c (remove_breakpoints_pid): Only skip removing
	dprintf if it is marked as persistent.

---
 gdb/breakpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

> 2014-07-07  Simon Marchi  <simon.marchi@ericsson.com>
> 
> 	PR breakpoints/17012
> 	* breakpoint.c (remove_breakpoints_pid): Only skip removing
> 	dprintf if it is marked as persistent.

Pedro is really the maintainer who is the most familiar with
breakpoint.c, I think, so it'd be better to wait for his feedback.
But, with my patch champion hat on, I would suggest merging both
patches into one, or else schedule the testsuite patch after the
actual fix. It's a bit of a detail in this case, but generally
speaking, we avoid having tests that fail until the fix is in.

> ---
>  gdb/breakpoint.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 908a1ea..fb833d0 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3112,7 +3112,7 @@ remove_breakpoints_pid (int pid)
>      if (bl->pspace != inf->pspace)
>        continue;
>  
> -    if (bl->owner->type == bp_dprintf)
> +    if (bl->owner->type == bp_dprintf && bl->target_info.persist == 1)
>        continue;
>  
>      if (bl->inserted)
> -- 
> 2.0.0
  

On 07/07/2014 08:17 PM, Simon Marchi wrote:
> On Linux native, if dprintf are inserted when detaching, they are left
> in the inferior which causes it to crash from a SIGTRAP. It also happens
> with dprintfs on remote targets, when set disconnected-dprintf is off.
> 
> I believe that the rationale of the line I modified was to leave dprinfs
> inserted in order to support disconnected dprintfs. This adds a check to
> see if the dprintf should actually stay inserted or not.
> 
> bl->target_info.persist will be 1 only if disconnected-dprintf is on and
> we are debugging a remote target. On native, it will always be 0,
> regardless of the value of disconnected-dprintf. This makes sense, since
> disconnected dprintfs are not supported by the native target.
> 
> gdb/Changelog:
> 
> 2014-07-07  Simon Marchi  <simon.marchi@ericsson.com>
> 
> 	PR breakpoints/17012
> 	* breakpoint.c (remove_breakpoints_pid): Only skip removing
> 	dprintf if it is marked as persistent.
> 
> ---
>  gdb/breakpoint.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 908a1ea..fb833d0 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3112,7 +3112,7 @@ remove_breakpoints_pid (int pid)
>      if (bl->pspace != inf->pspace)
>        continue;
>  
> -    if (bl->owner->type == bp_dprintf)
> +    if (bl->owner->type == bp_dprintf && bl->target_info.persist == 1)
>        continue;

I think that we don't really need the "bl->owner->type == bp_dprintf"
check anymore.  persist" carries all the semantics we need here.

Also, this is a boolean, so best write:

    if (bl->target_info.persist)
        continue;

without the "== 1" part.

But ... most importantly.  How do we end up with 'persist' set
on a call-style dprintf, given:

static void
build_target_command_list (struct bp_location *bl)
{
...
  /* For now, limit to agent-style dprintf breakpoints.  */
  if (dprintf_style != dprintf_style_agent)
    return;

I'm confused, as you say the test fails now, but nowhere in the
test are you setting the style to agent.  I think I'm missing
something.  :-)
  

On 07/15/2014 06:26 PM, Pedro Alves wrote:

> But ... most importantly.  How do we end up with 'persist' set
> on a call-style dprintf, given:
> 
> static void
> build_target_command_list (struct bp_location *bl)
> {
> ...
>   /* For now, limit to agent-style dprintf breakpoints.  */
>   if (dprintf_style != dprintf_style_agent)
>     return;
> 
> I'm confused, as you say the test fails now, but nowhere in the
> test are you setting the style to agent.  I think I'm missing
> something.  :-)
> 

Yeah, I was missing dinner...  :-)

Of course, the point is that indeed, for dprintf style != agent,
'persist' is not set, and those are the locations we want to remove.
Silly me.

BTW, I think it'd be good to make the test exercise all
dprintf styles, as all types are affected, and also
"set disconnected-dprintf off/on", to make "agent" style
affected.  With disconnected on, the agent style dprintf
will be persistent when testing against gdbserver
(extended-remote), but we should likewise be able to
detach and reattach back.

Something along the lines of:

foreach always_inserted { "off" "on" } {
    foreach style { "gdb" "call" "agent" } {
        foreach disconnected { "on" "off" } {
	   test $always_inserted $style $disconnected
	}
    }
}