Re: [PATCH v3 1/2] Only leave dprintf inserted if it is marked as persistent (PR breakpoints/17012)

[Pedro Alves <palves@redhat.com>]

On 08/22/2014 08:05 PM, Simon Marchi wrote:
> On Linux native, if dprintf are inserted when detaching, they are left

"dprintfs"

> 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.

s/dprinfs/dprintfs/

A nit: personally I prefer if logs sounds a little more confident
once questions are resolved.  I'd suggest:

 s/I believe that the/The/
 s/line I modified/line modified by the patch/

resulting in:

 The rationale of the line modified by the patch was to leave dprintfs
 inserted in order to support disconnected dprintfs.  However, not all
 dprintfs are persistent.  Also, there's no reason other kinds of
 breakpoints can't be persistent either.  So this replaces the bp_dprintf
 check with a check on whether the location is persistent.

> 
> 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.
> 

> New in v3:
> * Follow-up Pedro's review
>   * Remove == 1 for check on boolean.

There was also a point about removing the "type == bp_dprintf"
check completely.  Did you find we actually need it for some reason?

I think it's better to treat bl->target_info's contents as
undefined if the breakpoint is not inserted.  So I think the
clearest and best would be to merge this check with the one below,
resulting in

-    if (bl->owner->type == bp_dprintf)
-      continue;
-
-    if (bl->inserted)
     if (bl->inserted && !bl->target_info.persist)

I realize this may sound like a nit, but just this past week I was
playing with replacing the bl->target_info field with a pointer to
a refcounted target_info object, and the pointer would only be
set when the breakpoint is inserted   :-)

OK with that change.

Thanks,
Pedro Alves