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

Simon Marchi
2014-09-04T18:33:07Z

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