From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45228 invoked by alias); 14 Dec 2015 20:56:21 -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 45219 invoked by uid 89); 14 Dec 2015 20:56:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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; Mon, 14 Dec 2015 20:56:20 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id F23D66565D; Mon, 14 Dec 2015 20:56:18 +0000 (UTC) Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tBEKuHHp021874 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 14 Dec 2015 15:56:18 -0500 Subject: Re: [PATCH v6 4/9] Explicit locations: introduce address locations To: Joel Brobecker References: <20150805232802.21646.88440.stgit@valrhona.uglyboxes.com> <20150805232951.21646.67733.stgit@valrhona.uglyboxes.com> <20151214071113.GA6230@adacore.com> Cc: gdb-patches@sourceware.org From: Keith Seitz X-Enigmail-Draft-Status: N1110 Message-ID: <566F2CF1.1030003@redhat.com> Date: Mon, 14 Dec 2015 20:56:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20151214071113.GA6230@adacore.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2015-12/txt/msg00271.txt.bz2 Hi, Joel, Thank you for bringing this to my attention. I was wondering when a bug about this mega-patch would surface! [Aside: Bah humbug. There is no test covering this simple/common use case!] On 12/13/2015 11:11 PM, Joel Brobecker wrote: > We just found an issue with this patch. For instance, if you try > to debug a program (any program) which has PIE enabled, you'll see: > > (gdb) b *main > Breakpoint 1 at 0x51a: file hello.c, line 3. > (gdb) run > Starting program: /[...]/hello > Error in re-setting breakpoint 1: Warning: > Cannot insert breakpoint 1. > Cannot access memory at address 0x51a > > Warning: > Cannot insert breakpoint 1. > Cannot access memory at address 0x51a > > What happens is that the patch makes the implicit assumption that > the address computed the first time is static, as if it was designed > to only support litteral expressions (Eg. "*0x1234"). This allows > the shortcut of not re-computing the breakpoint location's address > when re-setting breakpoints. Ha. Yeah. That would be a bug. :-) > For my initial attempt at fixing this, I tried to extend what you did > to handle *EXPR as an ADDRESS_LOCATION event_location, and the attached > patch is an attempt to do just that. One of the holes I had to plug was > the fact that the original expression was not stored anywhere (which > makes sense, given that we explicitly skipping the re-evaluation). > I re-used the EL_STRING part of the location rather than making the > address field in the event_location union a struct of its own holding > both address and expression. That looks like the correct approach at fixing the bug. Or at least, I'm fairly confident your patch is (darn close if not identical) to what I would have done. > But while working on this approach, I had a growing feeling that we > needed to step back and re-evaluate whether using an ADDRESS_LOCATION > for handling those was the right approach at all. Obviously, this is a decision which needs to be made before I can commit to any course of action. > For instance, in light of the above, would it make better sense to > just handle "*EXPR" the same way we handle non-explicit locations? I've given this some thought in an attempt to convincingly steer the direction in favor of removing address locations or keeping them. More on this after I offer this opinion (all $0.000000000002 of it): > We could keep ADDRESS_LOCATION event locations for cases where we > know the breakpoint's address is indeed static. Regardless of how address locations are specified via the UI, I have a pretty strong preference that they are all treated alike, and I think you'd probably wouldn't (completely) disagree. So this boils down to whether address locations are linespecs or are their own class of location. This question is a little difficult for me to definitely answer. Certainly address locations are treated differently than true linespecs. No colons are permitted; after the expression is parsed, any further linespec-y options are ignored (actually error). Likewise on the explicit side, -address precludes the use of any other (location-specific) option. If we later added some sort of address-y specific feature, it could be introduced in a linespec-y way using a ':' (break *EXPR:option / break option:*EXPR). On the explicit side, we'd probably have a new option name (or share an existing one). Regardless of how we chose to implement this expansion, though, it is (quite likely) incompatible with generic source:func:label:line linespecs. So once again, we have an all-or-nothing representation. Use this set of rules for "linespec locations" and this set of rules for "address locations." [I have tried to linguistically distinguish between our historical linespecs (which include *EXPR) and "linespec locations" (which currently exclude *EXPR).] One could argue that this either-or behavior supports maintaining separate location types. One could also argue that this behavior is degenerate. It's a special case. [Wow, am I running for political office or what?!? Nothing like a simple, straightforward answer to a question!] I guess when it boils down to it, I don't really have a super strong preference for either case, but I do have a preference for maintaining addresses as a separate location type. What can I say? I hate special cases! :-P > WDYT? I admit I'm a little lost still between the various layers > of locations, event_locations, etc. Do you want to take it from there? I am more than happy to fix anything related to this code in any appropriate manner dictated by maintainers. [Of course, if this involves a massive rewrite, I will have to clear with my management!] Let me know how you and other maintainers would like me to proceed. Keith