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

[Doug Evans <xdje42 at gmail dot com>]

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.