From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29032 invoked by alias); 4 Sep 2014 16:45:46 -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 29021 invoked by uid 89); 4 Sep 2014 16:45:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 04 Sep 2014 16:45:43 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s84GjcXM030609 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 4 Sep 2014 12:45:38 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s84GjbcP002388; Thu, 4 Sep 2014 12:45:37 -0400 Message-ID: <54089730.8040605@redhat.com> Date: Thu, 04 Sep 2014 16:45:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0 MIME-Version: 1.0 To: Simon Marchi , gdb-patches@sourceware.org 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> In-Reply-To: <1408734315-21207-1-git-send-email-simon.marchi@ericsson.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-09/txt/msg00123.txt.bz2 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