[RFA,3/9] Explicit locations v2 - Locations refactor

  On 05/12/2014 05:37 PM, Doug Evans wrote:
> Thanks!

Thank you! Last time I tried submitting these, no one responded. [Not 
that I blame anyone, this patchset is *huge* (and very invasive).]

> Keith Seitz writes:
>   > 	* breakpoint.c: Include location.h.
>   > 	(create_overlay_event_breakpoint): Use linespec location
>   > 	location instead of address string.
>
> s/location//

Fixed.

>   > 	Gaurd against NULL address string when not using the default
>
> Guard

Fixed. And all ChangeLog entries consequently spellchecked!

>   > 	Copy the location inlto the breakpoint instead of an address string.
>
> into

Fixed.

>   > 	<addr_string_range_end>: Change to struct event_location and reanme
>
> rename

Fixed.

>   > 	* mi/mi-cmd-break.c: Include langauge.h, location.h, and linespec.h.
>
> language.h

Fixed.

>   > diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
>   > index b77716d..256187c 100644
>   > --- a/gdb/ax-gdb.c
>   > +++ b/gdb/ax-gdb.c
>   > @@ -41,6 +41,7 @@
>   >  #include "arch-utils.h"
>   >  #include "cli/cli-utils.h"
>   >  #include "linespec.h"
>   > +#include "location.h"
>   >  #include "objfiles.h"
>   >
>   >  #include "valprint.h"
>   > @@ -2643,14 +2644,19 @@ agent_command_1 (char *exp, int eval)
>   >        int ix;
>   >        struct linespec_sals *iter;
>   >        struct cleanup *old_chain;
>   > +      struct event_location *location, copy_location;
>   >
>   >        exp = skip_spaces (exp);
>   >        init_linespec_result (&canonical);
>   > -      decode_line_full (&exp, DECODE_LINE_FUNFIRSTLINE,
>   > +      location = new_event_location (EVENT_LOCATION_LINESPEC);
>   > +      EVENT_LOCATION_LINESPEC (location) = exp;
>   > +      copy_location = *location;
>
> As I'm reading this the ownership of the location string is murky.

I can sorta see what you're saying, if you mean it isn't immediately 
obvious. Otherwise, I'm not sure.

In any case, this is all academic now because:

> I'm not sure how many event locations there'll be but I wonder
> if it would be preferable to have a constructor for each kind,
> and then the constructor for linespec locations can xstrdup
> the string.

I've done this for all location types (new_linespec_location, 
new_address_location, new_probe_location, new_explicit_location).

> Plus you're making a copy with "copy_location = *location",
> but locations have a (pseudo-)copy-constructor
> (e.g., copy_event_location will xstrdup the linespec string).

Yes, that's because for linespecs, we need to advance past the input 
/without/ modifying the original structure (it was marked const). That 
allows create_breakpoint to do the dreaded "garbage after arguments" 
error checking. [I talked about this unfortunate circumstance in my 
response to patch #2.]

As you may read later, for all other location types, EXTRA_STRING has 
been drafted for this purpose. This is also needed for implementing 
conditions with those location types.

>   >  /* Enums for exception-handling support.  */
>   >  enum exception_event_kind
>   > @@ -211,9 +212,12 @@ re_set_exception_catchpoint (struct breakpoint *self)
>   >    /* We first try to use the probe interface.  */
>   >    TRY_CATCH (e, RETURN_MASK_ERROR)
>   >      {
>   > -      char *spec = ASTRDUP (exception_functions[kind].probe);
>   > +      struct event_location location;
>   >
>   > -      sals = parse_probes (&spec, NULL);
>   > +      initialize_event_location (&location, EVENT_LOCATION_LINESPEC);
>   > +      EVENT_LOCATION_LINESPEC (&location)
>   > +	= ASTRDUP (exception_functions[kind].probe);
>
> Another case of murky ownership of the location string.
> I see that it could work since one isn't going to call delete_event_location
> here (which would xfree an ASTRDUP's string).
> [And with c++ we could write code without having to worry about such things.]
> I guess I don't have *that* strong an opinion on needing to change things here.

Right. I've tried to be *very* careful that create_breakpoint take a 
*const* struct event_location* for this reason. Now, I could change 
everything to use new_*_location/delete_event_location, but there's an 
awful lot of copying going on already, I didn't want to introduce it 
anywhere it wasn't needed.

Truth be told, there are probably more optimizations to be done on this 
front.

>   > @@ -3421,7 +3426,8 @@ create_longjmp_master_breakpoint (void)
>   >  	  addr = BMSYMBOL_VALUE_ADDRESS (bp_objfile_data->longjmp_msym[i]);
>   >  	  b = create_internal_breakpoint (gdbarch, addr, bp_longjmp_master,
>   >  					  &internal_breakpoint_ops);
>   > -	  b->addr_string = xstrdup (func_name);
>   > +	  b->location = new_event_location (EVENT_LOCATION_LINESPEC);
>
> struct breakpoint now has "loc" and "location".
> Yeah, I think we need some naming changes.
> [not sure what to though]

Yeah, I struggled with this quite a bit. Just about every permutation of 
"location" or "breakpoint location" that I came up with was already 
used. So I had to invent something altogether "new," which only makes 
the water murkier. :-(

>   > @@ -3477,7 +3483,8 @@ create_std_terminate_master_breakpoint (void)
>   >        b = create_internal_breakpoint (get_objfile_arch (objfile), addr,
>   >                                        bp_std_terminate_master,
>   >  				      &internal_breakpoint_ops);
>   > -      b->addr_string = xstrdup (func_name);
>   > +      b->location = new_event_location (EVENT_LOCATION_LINESPEC);
>   > +      EVENT_LOCATION_LINESPEC (b->location) = xstrdup (func_name);
>
> I like the idea of a linespec-specific constructor the more I read this
> pattern of new_event_location followed by setting EVENT_LOCATION_LINESPEC.
> E.g.,
>    b->location = new_linespec_event_location (func_name);
> or some such.

Done.

>   > @@ -7528,8 +7543,9 @@ create_thread_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
>   >  				  &internal_breakpoint_ops);
>   >
>   >    b->enable_state = bp_enabled;
>   > -  /* addr_string has to be used or breakpoint_re_set will delete me.  */
>   > -  b->addr_string
>   > +  /* location has to be used or breakpoint_re_set will delete me.  */
>   > +  b->location = new_event_location (EVENT_LOCATION_LINESPEC);
>   > +  EVENT_LOCATION_LINESPEC (b->location)
>   >      = xstrprintf ("*%s", paddress (b->loc->gdbarch, b->loc->address));
>
> Using a linespec-specific constructor would be harder here though.
> [absent c++]
> Still, it feels warranted (i.e., delete new_event_location in favor
> of constructors for each kind).
> If using xstrprintf to get *address appears often enough one could
> write a simpler wrapper.
> Haven't seen the rest of the patchset though.

Yeah, this is a temporary hack that will disappear in patch #4, where 
address locations are introduced. This current patch only converts 
everything to linespec locations (which is basically what we have today).

Perhaps not the best split, but this is how I thought it might be 
easiest to comprehend by introducing new features in follow-on patches 
in the series.

>   > +   Returns true if any breakpoint was created; false otherwise.  */
>
> Not a problem introduced by your patch of course, so don't change anything,
> but boy this function's API feels overloaded (parse_arg == 0, != 0).

I am already working on a patch for this. :-) [To be fair, so was Hui, 
and he put that off until this was ready.]

>   >  	  /* Create a private copy of condition string.  */
>   > -	  if (cond_string)
>   > +	  if (cond_string != NULL)
>
> Some would flag the good though gratuitous change here as verboten.
> I don't mind them too much in such a large patch.

Eek. I've removed that. I must stop doing things like this. I always 
figure[*d*] that while I was touching it... Sorry about that. Old habits 
die hard.

>   > +      /* If the location has a string representation,
>   > +	 save it to the breakpoint's location save spec, since this
>   > +	 may be used to save the breakpoint to a file.  */
>   > +      if (EVENT_LOCATION_SAVE_SPEC (b->location) == NULL
>   > +	       && event_location_to_string (b->location) != NULL)
>   > +	{
>   > +	  EVENT_LOCATION_SAVE_SPEC (b->location)
>   > +	    = xstrdup (event_location_to_string (b->location));
>   > +	}
>
> This feels a bit clumsy.
> How about adding a location.c API function to set save_spec?
>
>    if (EVENT_LOCATION_SAVE_SPEC (b->location) == NULL)
>      set_event_location_save_spec (b->location);
>
> or some such?

This code snippet is actually no longer needed.

>   > @@ -9981,7 +10036,13 @@ create_breakpoint (struct gdbarch *gdbarch,
>   >  	    }
>   >  	  b->cond_string = cond_string;
>   >  	}
>   > -      b->extra_string = NULL;
>   > +      if (extra_string != NULL && *extra_string != '\0')
>   > +	{
>   > +	  b->extra_string = xstrdup (extra_string);
>   > +	  make_cleanup (xfree, b->extra_string);
>
> The cleanup here looks odd.

Previously, b->extra_string was always set to NULL for pending 
breakpoints. extra_string is now needed for explicit locations, but this 
patch isn't dealing with this. I've moved this to the right patch (#6) 
[and added a comment]. I apologize for that.

>   > @@ -10468,7 +10548,10 @@ break_range_command (char *arg, int from_tty)
>   >       symtab and line as the default symtab and line for the end of the
>   >       range.  This makes it possible to have ranges like "foo.c:27, +14",
>   >       where +14 means 14 lines from the start location.  */
>   > -  decode_line_full (&arg, DECODE_LINE_FUNFIRSTLINE,
>   > +  end_location = string_to_event_location (&arg, current_language);
>   > +  make_cleanup (delete_event_location, end_location);
>   > +  copy_location = *end_location;
>
> Why is copy_location is needed here?
> [and elsewhere too I guess - I see this pattern used a lot,
> and I'm just wondering if there's a simplification possible here, dunno]

It's not. It was simply a mindless application of the idiom you've noted:

    location = string_to_event_location (&arg);
    copy_location = *location;
    sals = decode_location* (&copy_location, ...);
    if (EVENT_LOCATION_TYPE (location) == LINESPEC_LOCATION)
      arg = EVENT_LOCATION_LINESPEC (&copy_location);

[again ref: the discussion in #2]

>   >
>   >    if (sals.nelts != 1)
>   >      error (_("Couldn't get information on specified line."));
>   >
>   > +  if (EVENT_LOCATION_TYPE (&copy_location) == EVENT_LOCATION_LINESPEC)
>   > +    arg = EVENT_LOCATION_LINESPEC (&copy_location);
>
> A short comment explaining why setting arg here is necessary would help
> me the reader.

It's the idiom that is forced upon us by the ambiguity of linespecs. 
Only AFTER creating SaLs do we know what the user specified.

You'll see this all over the place as a result.

> Looking at breakpoint.c I see it checking *arg for whether
> there is junk at the end of the arguments, so now I'm wondering if this
> part is correct.

It is correct. string_to_event_location will note that arg is a linespec 
and then skip the whole thing (because it can't know any differently). 
So when that function returns, arg = "\0". For explicit locations, e.g., 
string_to_event_location can forward past the whole thing. As a result, 
arg will directly point at any "garbage" or condition.

copy_location, however, will point at the "remainder" from 
decode_location's parsing of the linespec. Thus, for linespecs, we set 
arg to copy_location, which then later gets checked for "garbage."

>   > @@ -12917,9 +13011,9 @@ base_breakpoint_dtor (struct breakpoint *self)
>   >    decref_counted_command_line (&self->commands);
>   >    xfree (self->cond_string);
>   >    xfree (self->extra_string);
>   > -  xfree (self->addr_string);
>   >    xfree (self->filter);
>   > -  xfree (self->addr_string_range_end);
>   > +  delete_event_location (self->location);
>   > +  delete_event_location (self->location_range_end);
>
> Making delete_event_location type-safe and having another for
> cleanups that takes a void * feels better.

Done.

>   > @@ -13794,12 +13890,14 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
>   >  }
>   >
>   >  static void
>   > -strace_marker_decode_location (struct breakpoint *b, char **s,
>   > +strace_marker_decode_location (struct breakpoint *b,
>   > +			       struct event_location *location,
>   >  			       struct symtabs_and_lines *sals)
>   >  {
>   >    struct tracepoint *tp = (struct tracepoint *) b;
>   > +  char *s = EVENT_LOCATION_LINESPEC (location);
>
> IWBN if this could be const char *.
> If that requires too many further changes,
> disregard and leave for another day.

It would seem that decode_static_tracepoint_spec could be changed to 
take a const char ** (I haven't tried it), but I thought that would be 
even a bigger, unrelated change. I can work up a separate patch for that.

>   > @@ -14389,24 +14491,54 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
>   >    if (e.reason == 0 || e.error != NOT_FOUND_ERROR)
>   >      {
>   >        int i;
>   > +      const char *s;
>   >
>   >        for (i = 0; i < sals.nelts; ++i)
>   >  	resolve_sal_pc (&sals.sals[i]);
>   > -      if (b->condition_not_parsed && s && s[0])
>   > +      if (b->condition_not_parsed)
>   >  	{
>   > -	  char *cond_string, *extra_string;
>   > -	  int thread, task;
>   > +	  if (EVENT_LOCATION_TYPE (location) == EVENT_LOCATION_LINESPEC)
>   > +	    s = EVENT_LOCATION_LINESPEC (location);
>   > +	  else
>   > +	    s = b->extra_string;
>   >
>   > -	  find_condition_and_thread (s, sals.sals[0].pc,
>   > -				     &cond_string, &thread, &task,
>   > -				     &extra_string);
>   > -	  if (cond_string)
>   > -	    b->cond_string = cond_string;
>   > -	  b->thread = thread;
>   > -	  b->task = task;
>   > -	  if (extra_string)
>   > -	    b->extra_string = extra_string;
>   > -	  b->condition_not_parsed = 0;
>   > +	  if (s != NULL && *s != '\0')
>   > +	    {
>   > +	      char *cond_string, *extra_string;
>   > +	      int thread, task;
>   > +	      const char *orig = s;
>   > +
>   > +	      find_condition_and_thread (s, sals.sals[0].pc,
>   > +					 &cond_string, &thread, &task,
>   > +					 &extra_string);
>   > +	      if (cond_string != NULL)
>   > +		b->cond_string = cond_string;
>   > +	      b->thread = thread;
>   > +	      b->task = task;
>   > +	      if (extra_string != NULL)
>   > +		{
>   > +		  xfree (b->extra_string);
>   > +		  b->extra_string = extra_string;
>   > +		}
>   > +	      b->condition_not_parsed = 0;
>   > +
>
> The following is unexpected extra logic.
> Can you elaborate on why it's needed?
> [Not in comments in the code, unless you want to.
> Just in email is fine with me.]

I'm glad you asked. It's not needed anymore. :-)

>   > +	      /* For pending breakpoints which were just resolved, reset the
>   > +		 location's save spec.  */
>   > +	      if (b->loc == NULL)
>   > +		{
>   > +		  size_t len;
>   > +		  char *p, *str;
>   > +		  char *old = EVENT_LOCATION_SAVE_SPEC (location);
>   > +
>   > +		  len = orig - prev;
>   > +
>   > +		  str = savestring (prev, len);
>   > +		  p = remove_trailing_whitespace (str, str + len);
>   > +		  *p = '\0';
>   > +		  EVENT_LOCATION_SAVE_SPEC (b->location) = str;
>   > +		  xfree (old);
>   > +		}
>   > +	    }
>   >  	}

For the record, because of the switch to a computed string 
representation, the above is no longer needed, either. Another nice 
benefit of that change.

>   > @@ -15336,16 +15471,20 @@ static void
>   >  trace_command (char *arg, int from_tty)
>   >  {
>   >    struct breakpoint_ops *ops;
>   > +  struct event_location *location;
>   > +  struct cleanup *back_to;
>   >    const char *arg_cp = arg;
>   >
>   > -  if (arg && probe_linespec_to_ops (&arg_cp))
>   > +  location = string_to_event_location (&arg, current_language);
>   > +  back_to = make_cleanup (delete_event_location, location);
>   > +  if (arg_cp != NULL  && probe_linespec_to_ops (&arg_cp) != NULL)
>
> extra space after NULL

Fixed. [This disappears in patch #5 anyway.]

>   > @@ -15378,17 +15524,27 @@ static void
>   >  strace_command (char *arg, int from_tty)
>   >  {
>   >    struct breakpoint_ops *ops;
>   > +  struct event_location *location;
>   > +  struct cleanup *back_to;
>   >
>   >    /* Decide if we are dealing with a static tracepoint marker (`-m'),
>   >       or with a normal static tracepoint.  */
>   >    if (arg && strncmp (arg, "-m", 2) == 0 && isspace (arg[2]))
>   > -    ops = &strace_marker_breakpoint_ops;
>   > +    {
>   > +      ops = &strace_marker_breakpoint_ops;
>   > +      location = new_event_location (EVENT_LOCATION_LINESPEC);
>   > +      EVENT_LOCATION_LINESPEC (location) = xstrdup (arg);
>
> It's weird storing "-m " as part of the linespec.
> Is this addressed in a later patch maybe?

No, I did not change this.

This seems like we could certainly extend this the same way as for 
probes, but I'm not sure I would. It rather seems to set a dangerous 
precedent of turning everything into a new location type.

Then string_to_event_location would have to know how to deal with it, 
which means using "-m" on /any/ breakpoint/tracepoint/dprintf command 
will cause the string to be turned into a "marker" location.

Marker locations are only used by one command, so I don't consider that 
truly generic enough to warrant special treatment anywhere but at the 
command. [I would actually throw probe locations into that, too, had I 
not been asked to include them.]

All string-like locations (linespecs, probes) use the same member of the 
struct (addr_string). For simplicity, I've chosen generic string 
"locations" to represented as linespec locations. The "-m" part is 
stored because all of arg is currently passed to create_breakpoint, so 
we need to preserve that behavior. Perhaps I could/should introduce a 
generic "string" location (new_string_location)?

>   > @@ -784,21 +784,32 @@ edit_command (char *arg, int from_tty)
>   >      }
>   >    else
>   >      {
>   > -      /* Now should only be one argument -- decode it in SAL.  */
>   > +      struct cleanup *cleanup;
>   > +      struct event_location *location, copy_location;
>   > +      char *arg1;
>   >
>   > +      /* Now should only be one argument -- decode it in SAL.  */
>   >        arg1 = arg;
>   > -      sals = decode_line_1 (&arg1, DECODE_LINE_LIST_MODE, 0, 0);
>   > +      location = string_to_event_location (&arg1, current_language);
>   > +      cleanup = make_cleanup (delete_event_location, location);
>   > +      copy_location = *location;
>   > +      sals = decode_line_1 (&copy_location, DECODE_LINE_LIST_MODE, 0, 0);
>   > +
>   > +      if (EVENT_LOCATION_TYPE (&copy_location) == EVENT_LOCATION_LINESPEC)
>   > +	arg1 = EVENT_LOCATION_LINESPEC (&copy_location);
>
> Assigning to arg1 here is another case of setting something right before
> the "junk at end of line spec" check.  Is this right?

Yes. As discussed earlier, string_to_event_location advances arg1 to the 
end, because that's all it can do without actually doing the full-blown 
SaL collection. [I apologize if I am beginning to sound like a broken CD.]

> [It seems like arg1 would point to a beginning of the linespec and
> thus always trigger the error.]

Nope. Either event_location_to_string or decode_line_1 will advance the 
pointer. Again, the unfortunate consequence of linespec ambiguity.

>   > @@ -2416,10 +2432,60 @@ linespec_parser_delete (void *arg)
>   >    linespec_state_destructor (PARSER_STATE (parser));
>   >  }
>   >
>   > +/* A helper function for decode_line_full and decode_line_1 to
>   > +   turn LOCATION into symtabs_and_lines.  */
>   > +
>   > +static struct symtabs_and_lines
>   > +event_location_to_sals (linespec_parser *parser,
>   > +			struct event_location *location)
>   > +{
>   > +  struct symtabs_and_lines result = {NULL, 0};
>   > +
>   > +  switch (EVENT_LOCATION_TYPE (location))
>   > +    {
>   > +    case EVENT_LOCATION_LINESPEC:
>   > +      {
>   > +	const char *copy, *orig;
>   > +	volatile struct gdb_exception except;
>   > +
>   > +	TRY_CATCH (except, RETURN_MASK_ERROR)
>   > +	  {
>   > +	    orig = copy = EVENT_LOCATION_LINESPEC (location);
>   > +	    result = parse_linespec (parser, &copy);
>   > +	  }
>   > +	EVENT_LOCATION_LINESPEC (location) += copy - orig;
>
> Yikes, I just noticed EVENT_LOCATION_LINESPEC is both an enum and a macro. :)
> One of them needs a better name.

Done. I've renamed all the types LINESPEC_LOCATION, PROBE_LOCATION, etc. 
I don't know why I didn't do this to begin with. Silly me.

Keith

Changes since last version:
   - s/location// in ChangeLog
   - use make_cleanup_delete_event_location
   - init_breakpoints_sal: remove SAVE_SPEC no longer needed
   - refactor parse_breakpoint_sals to not use event_location_to_string.
   - create_breakpoint: use event_location_to_string_const
   - create_breakpoint: no need to do the SAVE_SPEC thing
   - create_breakpoint: move extra_string handling to #6
   - create_breakpoint: revert gratuitous coding style changes
   - use event_location_to_string instead of SAVE_SPEC
   - use new_linepsec_location instead of new_event_location
     (there are some hacks -- most/all go away with subsequent patches)
     (nope. sometimes we need to construct the string. So instead of
      saving it twice, we use the accessor and use new_linespec_location 
(NULL))
   - break_range_command: remove unnecessary use of copy_location
   - trace_command: remove extra space after "arg_cp != NULL"

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,3/9] Explicit locations v2 - Locations refactor

Commit Message

Comments

Patch