Message ID | 1408734315-21207-1-git-send-email-simon.marchi@ericsson.com |
---|---|
State | Committed |
Headers |
Received: (qmail 13487 invoked by alias); 22 Aug 2014 19:05:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 13476 invoked by uid 89); 22 Aug 2014 19:05:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: usevmg20.ericsson.net Received: from usevmg20.ericsson.net (HELO usevmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 22 Aug 2014 19:05:22 +0000 Received: from EUSAAHC006.ericsson.se (Unknown_Domain [147.117.188.90]) by usevmg20.ericsson.net (Symantec Mail Security) with SMTP id 03.01.05330.AEF37F35; Fri, 22 Aug 2014 15:04:42 +0200 (CEST) Received: from simark-hp.mo.ca.am.ericsson.se (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.90) with Microsoft SMTP Server (TLS) id 14.3.174.1; Fri, 22 Aug 2014 15:05:18 -0400 From: Simon Marchi <simon.marchi@ericsson.com> To: <gdb-patches@sourceware.org> CC: Simon Marchi <simon.marchi@ericsson.com> Subject: [PATCH v3 1/2] Only leave dprintf inserted if it is marked as persistent (PR breakpoints/17012) Date: Fri, 22 Aug 2014 15:05:14 -0400 Message-ID: <1408734315-21207-1-git-send-email-simon.marchi@ericsson.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
Commit Message
Simon Marchi
Aug. 22, 2014, 7:05 p.m. UTC
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. 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. gdb/Changelog: PR breakpoints/17012 * breakpoint.c (remove_breakpoints_pid): Only skip removing dprintf if it is marked as persistent. --- gdb/breakpoint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 14-08-22 03: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. > > 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. > > gdb/Changelog: > > PR breakpoints/17012 > * breakpoint.c (remove_breakpoints_pid): Only skip removing > dprintf if it is marked as persistent. > --- > gdb/breakpoint.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 683ed2b..01e9b36 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -3110,7 +3110,7 @@ remove_breakpoints_pid (int pid) > if (bl->pspace != inf->pspace) > continue; > > - if (bl->owner->type == bp_dprintf) > + if (bl->owner->type == bp_dprintf && bl->target_info.persist) > continue; > > if (bl->inserted) Ping.
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
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
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 683ed2b..01e9b36 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -3110,7 +3110,7 @@ remove_breakpoints_pid (int pid) if (bl->pspace != inf->pspace) continue; - if (bl->owner->type == bp_dprintf) + if (bl->owner->type == bp_dprintf && bl->target_info.persist) continue; if (bl->inserted)