From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28818 invoked by alias); 28 Jan 2015 16:52:31 -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 28734 invoked by uid 89); 28 Jan 2015 16:52:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,SPF_PASS autolearn=no version=3.3.2 X-HELO: mail-pa0-f53.google.com Received: from mail-pa0-f53.google.com (HELO mail-pa0-f53.google.com) (209.85.220.53) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 28 Jan 2015 16:38:42 +0000 Received: by mail-pa0-f53.google.com with SMTP id kx10so27160936pab.12 for ; Wed, 28 Jan 2015 08:38:31 -0800 (PST) X-Received: by 10.68.221.165 with SMTP id qf5mr6938326pbc.101.1422463111122; Wed, 28 Jan 2015 08:38:31 -0800 (PST) Received: from seba.sebabeach.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by mx.google.com with ESMTPSA id oq2sm5188301pbb.60.2015.01.28.08.38.29 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 28 Jan 2015 08:38:30 -0800 (PST) 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> <54076C7F.9050303@redhat.com> <54C6A395.9080502@redhat.com> Date: Wed, 28 Jan 2015 22:12:00 -0000 In-Reply-To: <54C6A395.9080502@redhat.com> (Keith Seitz's message of "Mon, 26 Jan 2015 12:29:09 -0800") 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: 2015-01/txt/msg00737.txt.bz2 Message-ID: <20150128221200.xHxA7lFB5jsguBrC-niZ5kIs0f-SpDUoPVnKKIKa6i0@z> Keith Seitz writes: > Hi, Doug, > > I apologize for the long hiatus on this, but it seems I needed a > (long) break to "rebase" myself! > > On 10/12/2014 02:39 PM, Doug Evans wrote: >> This is one part I still don't fully grok. >> How come there's both location and copy_location here, >> given that we don't do anything with location except make a copy of it? > > Okay, I have spent quite a bit of time trying to fix this, and while I > was very frustrated with this, I am now *very* grateful that you've > pushed me to readdress/revisit this. Thank you. > > In the end, I've changed the way all of this works. This temporary > copy hack, advance pointer functions, and more is all gone and no > longer necessary. > > I believe this revision addresses just about everything that you've raised. > > You might ask, "How?" [*Might*? You will! :-)] > > Really it all boils down to how linespecs are handled. The "usual" > mechanism that I am/have been proposing is: > > char *arg; /* e.g., break_command_1 et al */ > struct event_location *location; > struct cleanup *cleanup; > > location = string_to_event_location (&arg, current_language); > cleanup = make_cleanup_delete_event_location (location); > sals = decode_line_full (location, ...); > > /* ... */ > > do_cleanups (cleanup); > > The thing that has been holding all of this back is that > string_to_event_location has never been able to deal with > linespecs. Given any other location type, string_to_event_location can > advance the argument over any processed input. > > So the obvious solution to this is to teach it to do the same for > linespecs. I don't know why I didn't do this earlier. I even wrote a > proper lexer for linespecs a while ago. Using this, > string_to_event_location can now handle all location types the > same. [And it was *much* easier than I originally feared.] > > As a result, there is no more need for all this "advance input > pointer" fooey that I was doing before. In fact, I've changed the > linespec parser and decode_line_* so that they don't even take a > char** anymore. They don't "advance" anything anymore at all. That is > all done a priori by string_to_event_location (or rather, the function > that this function calls to do it for linespecs). > >> Also, event_locations are destructed differently, depending on how they're >> created, and IWBN to always just use the one destructor. > > All gone. > >> Also, I'd like to handle what event_location_advance_input_ptr >> and advance_event_location_ptr do differently, if only a little bit. > > All gone, too. > >> Which leads me to my last high level issue: maintaining intermediate >> parse state in event_location. I gather linespecs force some pragmatism >> on us (at least if we want to avoid yet more changes, which I'm totally >> happy to avoid for now). For example, this code is a bit confusing: >> >> breakpoint.c:location_to_sals: >> >> if (b->condition_not_parsed) >> { >> if (event_location_type (location) == LINESPEC_LOCATION) >> s = get_linespec_location (location); >> else >> s = b->extra_string; >> > > This code is modified, but essentially still exists. The one big > difference now is that this is done for all location types. In other > words, regardless of the location type, b->extra_string is /always/ > parsed for conditon/thread/task. [more on dprintf later] > > This happens because string_to_event_location can advance the input > string past the actual location specification, leaving, eg., "arg" > above pointing at any conditional/thread/task. The code passes this as > extra_string to create_breakpoint (which is so vaguely documented so > as to allow me, without much consternation, to use it this way). > > [This does not interfere with dprintfs, since conditions et al are not > supported on dprintf "inline" in this manner, e.g., 'dprintf > main,"hello" if argc == 1' is NOT supported. Although this patch > series makes it much easier to fix.] > >> If we add two pointers, one to the beginning of the string and one >> to the current parse location, then get_linespec_location could >> always return a pointer to the beginning of the string, and then >> a new function could return a pointer to the current parse state >> (get_current_linespec_text or some such). >> Does that make sense? >> [It seems so, and it would involve minimal changes.] > > Yup, and I actually did do this (at your suggestion). It's not so > quite so trivial, but it doesn't really matter anymore. It's all > gone. :-) > >>> As I mentioned the last time around, I've changed the event location >>> API to compute this string when it can (it caches a copy of it inside >>> the struct event_location -- I'm seeing this string computed many, >>> many times). >> >> Yeah, I like this. >> >>> There are two instances in which we save the string instead of computing it: >> >> This part not so much. :-) > > It's not so bad. Either we put members into struct event_location for > all these corner conditions, or we just save the string, which is what > we really need/use. I'm on the fence on this I guess. It's not critical, so I don't want this to be a blocker. >>> 1) pending breakpoints: we save the entire input string into the >>> location (needed to preserve, e.g., conditions) > > This is still the same. I did find a bug when a pending breakpoint is > resolved, i.e., > > (gdb) break -function main if argc == 1 > Set pending? y > (gdb) file myfile > ... > (gdb) save breakpoints bps > (gdb) shell cat bps > break -function main if argc == 1 > cond $bpnum argc == 1 > > [FWIW, this occurs today with linespecs/address strings.] > I've fixed this (for linespecs, too!) in the code by clearing the > string representation when the breakpoint is resolved. The next time > it is needed, it will be (properly) computed. > >>> 2) When converting/canonicalizing a linespec to explicit form, we must >>> save the linespec string representation (so that we can reproduce the >>> linespec the way the user originally specified it). The other option >>> here is to add a flag to event_location to note this. Then we can >>> compute the location. I chose to simply save the linespec string >>> representation. >> >> I like the consistency of event_location.as_string always being >> a cached copy of the string form of the location. >> Otherwise it's confusing. >> Does it make sense to add another string to event_location to >> handle (1) and (2) above? > > We could, sure. Is it necessary, though? I am not so sure. Like > linespecs/address strings, these event locations represent the user's > view of a location in the inferior. We already manipulate this string > representation in the same places today. [The one exception is the fix > for the above-mentioned bug. That's the one new place we manipulate > the string representation.] > >> I'm hoping these suggestions make sense and they won't involve >> too many changes. > > Just a bit of bookkeeping to get back into it. It's a large patch > series; it just takes a while to do anything. [Perhaps my process > needs improving, too.] > > Shall I submit this new series, or would you like to offer any further > comments on this or subsequent patches in the series? > > Keith > > PS. I will be leaving for FOSDEM tomorrow (Tues, 27 Jan) and returning > on Mon, 9 Feb). I will not have much access to anything other than > email at this time. I'd say submit it. I'll give the patch a timely review!