From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15811 invoked by alias); 4 Sep 2014 18:33:07 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 15794 invoked by uid 89); 4 Sep 2014 18:33:07 -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; Thu, 04 Sep 2014 18:33:05 +0000 Received: from EUSAAHC005.ericsson.se (Unknown_Domain [147.117.188.87]) by usevmg20.ericsson.net (Symantec Mail Security) with SMTP id E2.1A.05330.F2B58045; Thu, 4 Sep 2014 14:29:35 +0200 (CEST) Received: from [142.133.110.254] (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.87) with Microsoft SMTP Server (TLS) id 14.3.174.1; Thu, 4 Sep 2014 14:33:02 -0400 Message-ID: <5408B05D.8070800@ericsson.com> Date: Thu, 04 Sep 2014 18:33:00 -0000 From: Simon Marchi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 MIME-Version: 1.0 To: Pedro Alves , Subject: Re: [PATCH v3 1/2] Only leave dprintf inserted if it is marked as persistent (PR breakpoints/17012) References: <1408734315-21207-1-git-send-email-simon.marchi@ericsson.com> <54089730.8040605@redhat.com> In-Reply-To: <54089730.8040605@redhat.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00128.txt.bz2 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