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

  Hi, Doug,

Thank you for taking a look!

On 05/12/2014 03:59 PM, Doug Evans wrote:
> Keith Seitz writes:
>   > +/* Free LOCATION and any associated data.  */
>   > +
>   > +void
>   > +delete_event_location (void *data)
>
> I'm sure there's a reason why data is void * and not
> properly typed.  It would be good to document that
> reason here. [Or can we make it struct event_location *?]

Yes -- it's used as a cleanup. I've changed this to typed and added a 
utility cleanup, make_cleanup_delete_event_location instead.

>   > +/* Parse the user input in *STRINGP and turn it into a struct
>   > +   event_location, advancing STRINGP past any parsed input.
>   > +   Return value is malloc'd.  */
>   > +
>   > +struct event_location *
>   > +string_to_event_location (char **stringp,
>   > +			  const struct language_defn *language)
>   > +{
>   > +  struct event_location *location;
>   > +
>   > +  location = new_event_location (EVENT_LOCATION_LINESPEC);
>   > +  if (*stringp != NULL)
>   > +    {
>   > +      EVENT_LOCATION_LINESPEC (location) = xstrdup (*stringp);
>   > +      *stringp += strlen (*stringp);
>   > +    }
>   > +
>   > +  return location;
>   > +}
>
> This consumes the entire string, so we can remove the side-effect
> of modifying the input argument.
> It might make the caller (slightly) more complicated, but I like the
> simplification here.
> [Or at any rate IWBN to have a version that took a const char *.]

It is true that this current version of this bit of code does some 
non-sensical stuff such as this, but in future versions, they actually 
do something useful.

Because of the requirement to pass create_breakpoint a structure 
representing the location (instead of simply a string), we must now 
split the "advancing past the input" problem in two. This function 
(string_to_event_location) will (eventually) handle explicit locations, 
address locations, and probe locations. However, for linespec locations, 
we must defer this until the linespec is parsed since the linespec 
grammar itself is ambiguous.

I could not come up with a scheme that could unify these operations 
(without violating the struct requirement). If you or anyone else has an 
idea, I'm all eyes.

See, for example, the complete version of this function in patch #7 
(where the UI elements for explicit locations is added).

> Plus, for robustness sake, should we treat "" as unspecified?
> [and thus leave the string as NULL so event_location_empty_p
> will return true]

""/NULL is only valid for a linespec (as in the user simply typing 
"break" with no argument). Not permitted for any other location type.

>   > +/* An event location used to set a stop event in the inferior.
>   > +   This structure is an amalgam of the various ways
>   > +   to specify a where a stop event should be set.  */
>
> specify where

Doh! Fixed.

>   > +
>   > +struct event_location
>   > +{
>   > +  /* The type of this breakpoint specification.  */
>   > +  enum event_location_type type;
>   > +#define EVENT_LOCATION_TYPE(S) ((S)->type)
>   > +
>   > +  union
>   > +  {
>   > +    /* A generic "this is a string specification" for a location.
>   > +       This representation is used by both "normal" linespecs and
>   > +       probes.  */
>   > +    char *addr_string;
>   > +#define EVENT_LOCATION_LINESPEC(S) ((S)->u.addr_string)
>   > +  } u;
>   > +
>   > +  /* A string representation of how this location may be
>   > +     saved.  This is used to save stop event locations to file.
>   > +     Malloc'd.  */
>   > +  char *save_spec;
>   > +#define EVENT_LOCATION_SAVE_SPEC(S) ((S)->save_spec)
>   > +};
>
> 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.

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

Ah, so here's why it's taken me so long to respond...

After reading this, I played around with changing 
event_location_to_string as you pondered above. At the time, with the 
version I had, it was easier to "pre-compute" (= "save") the original 
string the user typed (post-canonicalization).

I quickly ran into an edge case that was not satisfactorily handled by 
the code as submitted. If an MI client sets a breakpoint with an 
explicit location, how is this breakpoint serialized (for saving to a 
file or pending)? In the previous version, it would be converted into a 
linespec. While not horrible, it opens the door for ambiguity. I feel it 
is better to serialize to an explicit form instead, which is what this 
next revision does.

So, after all that, event_location_to_string will compute the string 
value if it doesn't have a cached copy available. [Caching is a win, 
IMO. I see this string representation hit multiple times during 
breakpoint_re_set.]

>   > +extern int event_location_empty_p (const struct event_location *location);
>
> blank line
>

Fixed.

Phew! I appreciate very much that you've taken the time to take a look 
at this. I know this is going to be painful for us all.

Keith

PS. I am not reposting #1 -- it hasn't changed.

Changes from previous version:
    - EVENT_LOCATION_SAVE_SPEC removed
      (sub as_string where necessary)
    - delete_event_location now takes struct event_location*
    - add make_cleanup_delete_event_location
    - add event_location_to_string_const (necessary evil IMO)
    - fix typo in locations.h (struct event_location decl)
    - struct event_location.save_spec -> as_string
    - Update event_location_to_string{,_const} decls
    - Add missing blank line at end of location.h
    - new_event_location -> new_linespec_location
    - EVENT_LOCATION_LINESPEC -> LINESPEC_LOCATION for type
    - new_linespec_location instead of new_event_location

ChangeLog
2014-05-29  Keith Seitz  <keiths@redhat.com>

	* Makefile.in (SFILES): Add location.c.
	(HFILES_NO_SRCDIR): Add location.h.
	(COMMON_OBS): Add location.o.
	* location.c: New file.
	* location.h: New file.

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

Commit Message

Comments

Patch