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

[Simon Marchi <simon.marchi@ericsson.com>]

On 14-09-04 12:45 PM, Pedro Alves wrote:
> 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?

Right now, persist can only be set for dprintfs (in build_target_command_list),
so it shouldn't change anything. But like you said, there is no reason why the
persist field should apply to dprintf only, so I agree we can remove the check.

> 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   :-)

It makes it cleaner anyway, good suggestion.

> OK with that change.
> 
> Thanks,
> Pedro Alves