Re: [RFA 2/9] Explicit locations v2 - Event locations API

[Doug Evans <xdje42 at gmail dot com>]

Keith Seitz <keiths@redhat.com> 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!