This is the mail archive of the
mailing list for the GDB project.
Re: [PATCH v3 1/2] Only leave dprintf inserted if it is marked as persistent (PR breakpoints/17012)
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: Pedro Alves <palves at redhat dot com>, <gdb-patches at sourceware dot org>
- Date: Thu, 4 Sep 2014 14:33:01 -0400
- Subject: Re: [PATCH v3 1/2] Only leave dprintf inserted if it is marked as persistent (PR breakpoints/17012)
- Authentication-results: sourceware.org; auth=none
- References: <1408734315-21207-1-git-send-email-simon dot marchi at ericsson dot com> <54089730 dot 8040605 at redhat dot 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
>> 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.
> 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.
> Pedro Alves