[RFA,6/9] Explicit locations v2 - Add explicit locations

  On 08/02/2014 06:49 PM, Doug Evans wrote:
> Keith Seitz writes:
>
>   > @@ -10010,6 +10022,20 @@ create_breakpoint (struct gdbarch *gdbarch,
>   >        init_raw_breakpoint_without_location (b, gdbarch, type_wanted, ops);
>   >        b->location = copy_event_location (location);
>   >
>   > +      /* If the location has a string representation,
>   > +	 save it to the breakpoint's location string cache, since this
>   > +	 may be used to save the breakpoint to a file.  */
>   > +      if (EVENT_LOCATION_TYPE (b->location) == EXPLICIT_LOCATION)
>   > +	{
>   > +	  char *old = event_location_to_string_const (location);
>   > +
>   > +	  b->location->as_string
>   > +	    = xstrprintf ("%s%s%s", old,
>   > +			  (extra_string == NULL ? "" : " "),
>   > +			  (extra_string == NULL ? "" : extra_string));
>
> Here's another case where a file outside of the event-location-specific code
> is reaching into the internal implementation details of an event location.
>

I've added a function for this.

> Plus it's a bit confusing to do this:
>
>        b->location = copy_event_location (location);
>
> and then change the as_string representation of the location we just copied,
> but only for EXPLICIT_LOCATION.  E.g., As the reader I'm left wondering
> what happens to extra_string if the event location was a different kind.
>
> Can you elaborate on what's going on here?

I've changed this a bit... If extra_string is not NULL, the code now 
ALWAYS appends this to the string representation it saves. This block of 
code was moved earlier where the pending breakpoint was first detected 
(in the big switch block after TRY_CATCH).

However, this string representation must still be saved to the (pending) 
breakpoint's location. Since we don't have a resolved 
breakpoint/location, we need a way to serialize the given location. That 
was computed earlier (again the big switch statement). So now, the code 
uses that.

>   > +
>   > +      /* For explicit locations, EXTRA_STRING may contain breakpoint
>   > +	 condition information.  Make a private copy of this for the
>   > +	 breakpoint.  */
>
> The comment predicates itself on being specific to explicit locations,
> but the code doesn't check for EXPLICIT_LOCATION.
>
> Can you elaborate on what's going on here?

This was removed. [Or more properly, it was moved, as I discuss right 
above.]

>   > @@ -14253,11 +14290,12 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
>   >  	  b->loc->symtab = sym != NULL ? sal2.symtab : NULL;
>   >
>   >  	  delete_event_location (b->location);
>   > -	  b->location = new_linespec_location (NULL);
>   > -	  EVENT_LOCATION_LINESPEC (b->location)
>   > -	    = xstrprintf ("%s:%d",
>   > -			  symtab_to_filename_for_display (sal2.symtab),
>   > -			  b->loc->line_number);
>   > +	  initialize_explicit_location (&explicit);
>   > +	  explicit.source_filename
>   > +	    = ASTRDUP (symtab_to_filename_for_display (sal2.symtab));
>   > +	  explicit.line_offset.offset = b->loc->line_number;
>   > +	  explicit.line_offset.sign = LINE_OFFSET_NONE;
>   > +	  b->location = new_explicit_location (&explicit);
>
> Hmmm, the name new_explicit_location is ambiguous.
> Does it return an explicit_location* or an event_location* ?
> I like the consistency with new_linespec_location, new_address_location, etc.
> so I understand why it's the latter, though my first guess would be the former
> (though that would be confusing given that we're passing it an
> explicit_location*).
> No need to change anything, just thinking out loud.

Yeah, there's a lot of unfortunate naming fallout. As you correctly 
deduce, I kept the ctors names consistent. Likewise with the 
get_*_locations:

function                 takes                returns
--------                 -----                -------
new_linespec_location    char*                event_location*
get_linespec_location    event_location*      char*
new_address_location     CORE_ADDR            event_location*
get_address_location     event_location*      CORE_ADDR
new_probe_location       char*                event_location*
get_probe_location       event_location*      char*
new_explicit_location    explicit_location*   event_location*
get_explicit_location    event_location*      explicit_location*

>   > -  /* The user-supplied source filename or NULL if none was specified.  */
>   > -  const char *source_filename;
>   > +  /* An explicit location describing the SaLs.  */
>   > +  struct explicit_location explicit;
>
> Does this read better to you? [here, and at all uses]
>
>    struct explicit_location location;
>
> Dunno.

My convention has been to reserve the generic "location" as meaning an 
(struct) event_location.

> When I read "explicit" I'm left wondering "Does that mean there's an implicit
> or some other possibility for the location that I have to think about?"

In this context, of course, the answer is "no." :-) An explicit location 
is an explicit location. It can be nothing else. If it said (instead) 
"struct event_location explicit;" I would agree -- that *does/would* 
leave me wondering about other possibilities. But then I've been staring 
at this for so long, I probably take (too much) for granted!

> The struct definition is local to linespec.c so the scope of any confusion
> is nicely confined, so I wouldn't make any change unless you think
> it's worth it.

I could change it to "explicit_location"?

>   > @@ -1761,42 +1739,19 @@ linespec_parse_basic (linespec_parser *parser)
>   >  }
>   >
>   >  /* Canonicalize the linespec contained in LS.  The result is saved into
>   > -   STATE->canonical.  */
>   > +   STATE->canonical.  This function handles both linespec and explicit
>   > +   locations.  */
>   >
>   >  static void
>   >  canonicalize_linespec (struct linespec_state *state, linespec_p ls)
>   >  {
>   > -  struct ui_file *buf;
>   > -  int need_colon = 0;
>   > -
>   >    /* If canonicalization was not requested, no need to do anything.  */
>   >    if (!state->canonical)
>
> No requested change here.
> I just wanted to note that here's a good example where
> "if (state->canonical == NULL)" would be clearer.
> At this point in my reading for half a second I wondered if canonical was
> a boolean, and was confused because the function comment said the result
> was stored there.

FYI, I didn't change that line. I've been called-out many times on 
making unrelated/superfluous changes, so I've left it (and other similar 
places) alone.

>   > @@ -1805,29 +1760,22 @@ canonicalize_linespec (struct linespec_state *state, linespec_p ls)
>   >  		      && (VEC_length (symbolp, ls->labels.function_symbols)
>   >  			  == 1));
>   >  	  s = VEC_index (symbolp, ls->labels.function_symbols, 0);
>   > -	  fputs_unfiltered (SYMBOL_NATURAL_NAME (s), buf);
>   > -	  fputc_unfiltered (':', buf);
>   > +	  ls->explicit.function_name = xstrdup (SYMBOL_NATURAL_NAME (s));
>
> This is a side-effect not adequately explained in the function comment.
> It's not clear from the function comment that LS is modified.
> Is the side-effect necessary (in which case let's make the function
> comment clearer), or can we avoid it?

I agree. I don't know why I did that. To be more strict, I've changed 
this function to take a const linespec_p. Obviously, what I meant to do 
(or should have done) is canonicalize the canonical location in STATE.

>   > -  EVENT_LOCATION_LINESPEC (state->canonical->location)
>   > -    = ui_file_xstrdup (buf, NULL);
>   > -  ui_file_delete (buf);
>   > +  /* Save a string representation of this linespec.  */
>   > +  if (state->is_linespec)
>   > +    state->canonical->location->as_string
>   > +      = explicit_location_to_linespec (&ls->explicit);
>   > +  else
>   > +    state->canonical->location->as_string
>   > +      = explicit_location_to_string (&ls->explicit);
>
> Why do we need to set as_string here again?
> Can users of it call event_location_to_string themselves?
> There's still the state->is_linespec distinction of course,
> feels odd to have it though: something outside of an event_location
> is deciding what to store there, feels like an abstraction violation.

Yes, this is one of the scenarios I discussed in a previous revision 
(#2/9). Since all linespecs are converted to explicit form /and/ we 
compute string serializations of event_locations on demand, we need a 
way to discern between explicit and linespec locations.

There are two ways to do this:
1) Add a flag to event_location to indicate that this location really 
represents a linespec (and use that flag later when computing the string)
2) Save the linespec representation when the linespec is converted to 
explicit.

Not seeing any immediate advantage, I've chosen #2.

>   > +/* Convert the explicit location EXPLICIT into SaLs.  */
>   > +
>   > +static struct symtabs_and_lines
>   > +convert_explicit_location_to_sals (struct linespec_state *self,
>   > +				   linespec_p result,
>   > +				   struct explicit_location *explicit)
>
> How many of these parameters can be const?

Only the last. [I've changed that in this revision.] Obviously we need 
to be able to manipulate RESULT, but other functions need to use SELF, 
too, for storing symbol search iterator information.

>   > @@ -82,6 +93,49 @@ new_probe_location (const char *probe)
>   >    return location;
>   >  }
>   >
>   > +/* Create a new explicit location.  If not NULL, EXPLICIT is checked for
>   > +   validity.  If invalid, an exception is thrown.
>   > +
>   > +   The return result is malloc'd and should be freed with
>   > +   delete_event_location.  */
>   > +
>   > +struct event_location *
>   > +new_explicit_location (const struct explicit_location *explicit)
>   > +{
>   > +  struct event_location tmp;
>   > +
>   > +  initialize_event_location (&tmp, EXPLICIT_LOCATION);
>   > +
>   > +  if (explicit != NULL)
>   > +    {
>   > +      if (explicit->source_filename != NULL)
>   > +	{
>   > +	  /* Error check -- we must have one of the other
>   > +	     parameters specified.  */
>   > +	  if (explicit->function_name == NULL
>   > +	      && explicit->label_name == NULL
>   > +	      && explicit->line_offset.sign == LINE_OFFSET_UNKNOWN)
>   > +	    error (_("Source filename requires function, label, or "
>   > +		     "line offset."));
>
> How about moving this error check into the caller(s)
> and have an assert here instead?  One could write a utility to
> error check an explicit_location, and the callers could call that.

Ha. Great idea. I can only guess this was necessary at some time. I've 
double-checked it all, and it is /not/ necessary for the CLI. I have 
added something to MI to check this.

>   > @@ -108,6 +175,15 @@ extern struct event_location *
>   >  extern struct event_location *
>   >    new_probe_location (const char *probe);
>   >
>   > +/* Create a new explicit location.  If not NULL, EXPLICIT is checked for
>   > +   validity.  If invalid, an exception is thrown.
>   > +
>   > +   The return result is malloc'd and should be freed with
>   > +   delete_event_location.  */
>
> Copy of function comment in .c file.  Delete one.

I've removed all the duplicate function comments from location.c and 
replaced them with "See description in location.h."

Updated patch attached.

Keith

Changes since last revision:
   - Save extra_string for all locations (pending bps)
   - canonicalize_linespec: have explicit label but no function,
     set function; We need it for:
    (gdb) start
    (gdb) b -label done
    (gdb) save breakpoints bps
    (gdb) shell cat bps
    break -label done <-- not correct!
    We need "--function main" added, just like in the linespec case.
    I've made it so that the behavior is EXACTLY the same
    as linespec case, made LS const, and saved everything in the
    canonical location.
    - make EXPLICIT const in convert_explicit_location_to_sals
    - canonicalize_linespec: only save string representation for
      linespec.  Needed so that we display linespecs as linespecs.
      Other option is to not do this and save is_linespec somewhere
    - new_explicit_location: remove error check
    - removed duplicate function comments

[RFA,6/9] Explicit locations v2 - Add explicit locations

Commit Message

Patch