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

[Doug Evans <xdje42@gmail.com>]

Keith Seitz <keiths@redhat.com> writes:

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

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

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

Ah. Ok.

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

Ah, righto.

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

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

Plus in this case the use of EVENT_LOCATION_LINESPEC as an enum
value and as a macro name would go away, and we can keep
EVENT_LOCATION_LINESPEC as the enum value. 1/2 :-)
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.]

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

Ok.

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

Sure.