From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19147 invoked by alias); 6 Jun 2014 05:45:12 -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 19136 invoked by uid 89); 6 Jun 2014 05:45:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pb0-f44.google.com Received: from mail-pb0-f44.google.com (HELO mail-pb0-f44.google.com) (209.85.160.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 06 Jun 2014 05:45:09 +0000 Received: by mail-pb0-f44.google.com with SMTP id rq2so2199458pbb.17 for ; Thu, 05 Jun 2014 22:45:08 -0700 (PDT) X-Received: by 10.68.245.162 with SMTP id xp2mr4846712pbc.69.1402033507821; Thu, 05 Jun 2014 22:45:07 -0700 (PDT) Received: from seba.sebabeach.org.gmail.com (173-13-178-50-sfba.hfc.comcastbusiness.net. [173.13.178.50]) by mx.google.com with ESMTPSA id it4sm30994315pbc.39.2014.06.05.22.45.06 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 05 Jun 2014 22:45:07 -0700 (PDT) From: Doug Evans To: Keith Seitz Cc: "gdb-patches\@sourceware.org ml" Subject: Re: [RFA 2/9] Explicit locations v2 - Event locations API References: <536BC5DE.5050707@redhat.com> <21361.21080.298225.332248@ruffy.mtv.corp.google.com> <5388CB00.6040205@redhat.com> Date: Fri, 06 Jun 2014 05:45:00 -0000 In-Reply-To: <5388CB00.6040205@redhat.com> (Keith Seitz's message of "Fri, 30 May 2014 11:16:32 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-06/txt/msg00299.txt.bz2 Keith Seitz writes: > Hi, Doug, > > Thank you for taking a look! > > On 05/12/2014 03:59 PM, Doug Evans wrote: >> Keith Seitz writes: >> > +/* Free LOCATION and any associated data. */ >> > + >> > +void >> > +delete_event_location (void *data) >> >> I'm sure there's a reason why data is void * and not >> properly typed. It would be good to document that >> reason here. [Or can we make it struct event_location *?] > > Yes -- it's used as a cleanup. I've changed this to typed and added a > utility cleanup, make_cleanup_delete_event_location instead. Alas I've been told we're no longer allowed to add new make_cleanup_foo functions. [Can't remember when, but I'm trusting my memory on this one. :-)] OTOH there's nothing written down in the coding standards section of the wiki. I think the loss of type safety isn't warranted, so I'm going to approve this part, and see if anyone complains (or wants you to do things differently). >> > +/* Parse the user input in *STRINGP and turn it into a struct >> > + event_location, advancing STRINGP past any parsed input. >> > + Return value is malloc'd. */ >> > + >> > +struct event_location * >> > +string_to_event_location (char **stringp, >> > + const struct language_defn *language) >> > +{ >> > + struct event_location *location; >> > + >> > + location = new_event_location (EVENT_LOCATION_LINESPEC); >> > + if (*stringp != NULL) >> > + { >> > + EVENT_LOCATION_LINESPEC (location) = xstrdup (*stringp); >> > + *stringp += strlen (*stringp); >> > + } >> > + >> > + return location; >> > +} >> >> This consumes the entire string, so we can remove the side-effect >> of modifying the input argument. >> It might make the caller (slightly) more complicated, but I like the >> simplification here. >> [Or at any rate IWBN to have a version that took a const char *.] > > It is true that this current version of this bit of code does some > non-sensical stuff such as this, but in future versions, they actually > do something useful. > > Because of the requirement to pass create_breakpoint a structure > representing the location (instead of simply a string), we must now > split the "advancing past the input" problem in two. This function > (string_to_event_location) will (eventually) handle explicit > locations, address locations, and probe locations. However, for > linespec locations, we must defer this until the linespec is parsed > since the linespec grammar itself is ambiguous. > > I could not come up with a scheme that could unify these operations > (without violating the struct requirement). If you or anyone else has > an idea, I'm all eyes. > > See, for example, the complete version of this function in patch #7 > (where the UI elements for explicit locations is added). Ah. Ok. >> Plus, for robustness sake, should we treat "" as unspecified? >> [and thus leave the string as NULL so event_location_empty_p >> will return true] > > ""/NULL is only valid for a linespec (as in the user simply typing > "break" with no argument). Not permitted for any other location type. Ah, righto. >> > +/* An event location used to set a stop event in the inferior. >> > + This structure is an amalgam of the various ways >> > + to specify a where a stop event should be set. */ >> >> specify where > > Doh! Fixed. > >> > + >> > +struct event_location >> > +{ >> > + /* The type of this breakpoint specification. */ >> > + enum event_location_type type; >> > +#define EVENT_LOCATION_TYPE(S) ((S)->type) >> > + >> > + union >> > + { >> > + /* A generic "this is a string specification" for a location. >> > + This representation is used by both "normal" linespecs and >> > + probes. */ >> > + char *addr_string; >> > +#define EVENT_LOCATION_LINESPEC(S) ((S)->u.addr_string) >> > + } u; >> > + >> > + /* A string representation of how this location may be >> > + saved. This is used to save stop event locations to file. >> > + Malloc'd. */ >> > + char *save_spec; >> > +#define EVENT_LOCATION_SAVE_SPEC(S) ((S)->save_spec) >> > +}; >> >> Making this struct opaque to its users has benefits. >> Thoughts? > > I briefly considered this but didn't see an real benefits. Can you > elaborate? If you feel that strongly about it, I can change it. Making structs opaque (where there's a choice) helps keep things maintainable. e.g., only the implementation details one wants to expose actually do get exposed. If it's a toss up, or close to one, I'd go the opaque struct route. The patch is already providing accessors, so why not make those accessors functions instead of macros? A teensy bit more to write, but I think it's worth it. [OTOH I haven't seen the rest of the patch set yet, if it proves excessively onerous I'm happy to revisit.] Plus in this case the use of EVENT_LOCATION_LINESPEC as an enum value and as a macro name would go away, and we can keep EVENT_LOCATION_LINESPEC as the enum value. 1/2 :-) Within location.c I would just access the struct members directly, instead of with accessor macros. [I know accessor macros is a GNU-ism, but we don't always use them in dwarf2read.c (e.g.,: v.quick), and do just fine IMO.] >> > +/* Return a string representation of the LOCATION. >> > + This function may return NULL for unspecified linespecs, >> > + e.g, EVENT_LOCATION_LINESPEC and addr_string is NULL. */ >> > + >> > +extern const char * >> > + event_location_to_string (const struct event_location *location); >> >> I wonder if the string form might be computed >> for some kind of location. In which case either we always >> return a malloc'd copy here (remove const from result) >> or lazily compute it and cache the computed string in the >> struct (remove the const from the location parameter). >> [Or switch to c++ and use mutable I guess. :)] > > Ah, so here's why it's taken me so long to respond... > > After reading this, I played around with changing > event_location_to_string as you pondered above. At the time, with the > version I had, it was easier to "pre-compute" (= "save") the original > string the user typed (post-canonicalization). > > I quickly ran into an edge case that was not satisfactorily handled by > the code as submitted. If an MI client sets a breakpoint with an > explicit location, how is this breakpoint serialized (for saving to a > file or pending)? In the previous version, it would be converted into > a linespec. While not horrible, it opens the door for ambiguity. I > feel it is better to serialize to an explicit form instead, which is > what this next revision does. > > So, after all that, event_location_to_string will compute the string > value if it doesn't have a cached copy available. [Caching is a win, > IMO. I see this string representation hit multiple times during > breakpoint_re_set.] Ok. >> > +extern int event_location_empty_p (const struct event_location *location); >> >> blank line >> > > Fixed. > > Phew! I appreciate very much that you've taken the time to take a look > at this. I know this is going to be painful for us all. > > Keith > > PS. I am not reposting #1 -- it hasn't changed. Sure.