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

  On 06/05/2014 10:44 PM, Doug Evans wrote:

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

I guess I'll wait for someone to complain, then! :-P

>>>    > +
>>>    > +struct event_location
[snip]
>>>    > +};
>>>
>>> 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.]

Ok, in this next revision, I've changed this structure to be completely 
opaque...

So instead of doing:

struct location *location, copy_location;

location = string_to_event_location (&arg, current_language);
copy_location = *location;
decode_line_full (&copy_location, ...);
if (event_location_type (location) == LINESPEC_LOCATION)
      arg = get_linespec_location (&copy_location); /* to advance ARG 
over processed input */

the code now does:

struct event_location *location, *copy_location;

location = string_to_event_location (&arg, current_language);
copy_location = copy_event_location_tmp (location);
make_cleanup (xfree, copy_location);
decode_line_full (copy_location, ...);
event_location_advance_input_ptr (&arg, copy_location);
do_cleanups (...);

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

Well, I kept my accessor macros in location.c (and nowhere else)... I 
like them, but I am not married to them. If you/others really want me to 
remove them, just say the word, and I'll zap 'em.

>>>    > +/* 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. :)]

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

There are two instances in which we save the string instead of computing it:

1) pending breakpoints: we save the entire input string into the 
location (needed to preserve, e.g., conditions)

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.

Updated patch attached.

Keith

Changes since last revision:
     - Move struct event_location to location.c
     - Make struct event_location completely opaque
     - Turn all EVENT_LOCATION_* macros to functions.
     - Add get_linespec_location, copy_event_location_tmp,
       event_location_advance_input_ptr,
       advance_event_location_ptr, set_event_location_string
     - Remove duplicate function comments/"See description in foo.h."
  

Keith Seitz <keiths@redhat.com> writes:
>>>> [...]
>>>> 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.]
>
> Ok, in this next revision, I've changed this structure to be
> completely opaque...
>
> So instead of doing:
>
> struct location *location, copy_location;
>
> location = string_to_event_location (&arg, current_language);
> copy_location = *location;
> decode_line_full (&copy_location, ...);
> if (event_location_type (location) == LINESPEC_LOCATION)
>      arg = get_linespec_location (&copy_location); /* to advance ARG
> over processed input */
>
> the code now does:
>
> struct event_location *location, *copy_location;
>
> location = string_to_event_location (&arg, current_language);
> copy_location = copy_event_location_tmp (location);
> make_cleanup (xfree, copy_location);
> decode_line_full (copy_location, ...);
> event_location_advance_input_ptr (&arg, copy_location);
> do_cleanups (...);

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?
If there is a technical reason, best add a comment to the code
explaining why.  OTOH, IWBN to remove the copy.

until_break_command is an example:

  location = string_to_event_location (&arg, current_language);
  cleanup = make_cleanup_delete_event_location (location);
  copy_location = copy_event_location_tmp (location);
  make_cleanup (xfree, copy_location);

Also, event_locations are destructed differently, depending on how they're
created, and IWBN to always just use the one destructor.
Plus the _tmp in copy_event_location_tmp doesn't help me the reader much.
How about rename copy_event_location_tmp to shallow_copy_event_location?
It could set a flag in event_location and the destructor could
check this flag.
[There is no data to say we need to do the shallow copy for
performance reasons (and it seems unlikely), so on the one hand I'd say
let's just delete support for shallow copying.  However, OTOH, there
could be a technical reason for doing a shallow copy,
see "How come there's both ...?" above.  So depending on the answer
to that question it may be preferable to just delete support for
oing shallow copies.  I'm happy to keep it though if there's a good
technical reason for it.]

Also, I'd like to handle what event_location_advance_input_ptr
and advance_event_location_ptr do differently, if only a little bit.
I gather it's done this way to cope with linespec parsing.
Still, it's sufficiently odd that I'd like to see if we can improve it.

advance_event_location_ptr has the problem that it breaks the
destructor, the pointer no longer points to the beginning of the
malloc'd string.  It may be that advance_event_location_ptr is only
ever called on shallow copies, but the API needs to present something
more robust to its users.
How about keeping two pointers, one to the beginning of the string that
the destructor can safely use, and one to the current parsing location?

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;

The code is fetching the linespec location with get_linespec_location
and then passing that to find_condition_and_thread here:

	      find_condition_and_thread (s, sals.sals[0].pc,
					 &cond_string, &thread, &task,
					 &extra_string);

Eh?

This works, I gather, because this code earlier in the function:

  TRY_CATCH (e, RETURN_MASK_ERROR)
    {
      b->ops->decode_location (b, location, &sals);
    }

will leave EL_LINESPEC (location) pointing passed the linespec,
leaving it pointing at any potential condition or thread spec.
This happens because bkpt_decode_location calls
decode_location_default which calls decode_line_full
which calls event_location_to_sals which does this:

	advance_event_location_ptr (location, copy - orig);

Have I got that right?

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

>> 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.]
>
> Well, I kept my accessor macros in location.c (and nowhere else)... I
> like them, but I am not married to them. If you/others really want me
> to remove them, just say the word, and I'll zap 'em.

Since they're local to location.c I'd say just leave them.

>>>>    > +/* 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. :)]
>
> 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. :-)

> 1) pending breakpoints: we save the entire input string into the
> location (needed to preserve, e.g., conditions)
>
> 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?

---

I'm hoping these suggestions make sense and they won't involve
too many changes.
If we can get agreement on something (I'm happy to hear counter proposals),
then I'll submit the rest of my review, and then you can submit v3
which barring nits should be the final version.
  

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.
  

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!