This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]