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

[Keith Seitz <keiths at redhat dot com>]

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.

> > 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.