Re: [PATCH v4 2/9] Explicit locations: introduce new struct event_location-based API

[Keith Seitz <keiths at redhat dot com>]

On 05/17/2015 01:53 PM, Doug Evans wrote:
>> 	* linespec.c (linespec_lex_to_end): Declare.
> 
> typo: linespec.h

Fixed.

>> diff --git a/gdb/linespec.c b/gdb/linespec.c
>> index d2089b5..a480be1 100644
>> --- a/gdb/linespec.c
>> +++ b/gdb/linespec.c
>> @@ -2435,6 +2435,61 @@ linespec_parser_delete (void *arg)
>>    linespec_state_destructor (PARSER_STATE (parser));
>>  }
>>  
>> +/* See description in linespec.h.  */
>> +
>> +void
>> +linespec_lex_to_end (char **stringp)
>> +{
>> +  linespec_parser parser;
>> +  struct cleanup *cleanup;
>> +  linespec_token token;
>> +  volatile struct gdb_exception e;
>> +  const char *orig;
>> +
>> +  if (stringp == NULL || *stringp == NULL)
>> +    return;
>> +
>> +  linespec_parser_new (&parser, 0, current_language, NULL, 0, NULL);
>> +  cleanup = make_cleanup (linespec_parser_delete, &parser);
>> +  parser.lexer.saved_arg = *stringp;
>> +  PARSER_STREAM (&parser) = orig = *stringp;
>> +
>> +  TRY
>> +    {
>> +      do
>> +	{
>> +	  /* Stop before any comma tokens;  we need it to keep it
>> +	     as the next token in the string.  */
>> +	  token = linespec_lexer_peek_token (&parser);
>> +	  if (token.type == LSTOKEN_COMMA)
>> +	    break;
>> +
>> +	  /* For addresses advance the parser stream past
>> +	     any parsed input and stop lexing.  */
>> +	  if (token.type == LSTOKEN_STRING
>> +	      && *LS_TOKEN_STOKEN (token).ptr == '*')
>> +	    {
>> +	      const char *arg;
>> +
>> +	      arg = *stringp;
>> +	      (void) linespec_expression_to_pc (&arg);
>> +	      PARSER_STREAM (&parser) = arg;
>> +	      break;
>> +	    }
>> +
>> +	  token = linespec_lexer_consume_token (&parser);
>> +	}
>> +      while (token.type != LSTOKEN_EOI && token.type != LSTOKEN_KEYWORD);
>> +    }
>> +  CATCH (e, RETURN_MASK_ERROR)
>> +    {
> 
> I'm guessing I'm missing something here.
> Is the intent to ignore errors here?
>

Yeah, that was the intent. However, I played with this a bit, and while
the end result is identical whether or not an exception is swallowed
here, I am now convinced that doing so is not good.

Fortunately, ls-errs.exp covers this block of code.

As it is right now, new_linespec_location will return the following new
location {type = LINESPEC_LOCATION, u = {addr_string = "\"", ...}, ...}.

This is then passed to the linespec parser which (incidentally IMO)
outputs the correct error.

I've removed the TRY/CATCH/END_CATCH here. It really doesn't make sense
(which I suspect you already knew ;-).

>> +    }
>> +  END_CATCH
>> +
>> +  *stringp += PARSER_STREAM (&parser) - orig;
>> +  do_cleanups (cleanup);
>> +}
>> +
>>  /* See linespec.h.  */
>>  
>>  void
[snip]
>> diff --git a/gdb/location.h b/gdb/location.h
>> new file mode 100644
>> index 0000000..992f21e
>> --- /dev/null
>> +++ b/gdb/location.h
>> @@ -0,0 +1,113 @@
>> +/* Data structures and API for event locations in GDB.
>> +   Copyright (C) 2013-2015 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef LOCATIONS_H
>> +#define LOCATIONS_H 1
>> +
>> +struct language_defn;
>> +struct event_location;
>> +
>> +/* An enumeration of the various ways to specify a stop event
>> +   location (used with create_breakpoint).  */
>> +
>> +enum event_location_type
>> +{
>> +  /* A traditional linespec.  */
>> +  LINESPEC_LOCATION
>> +};
>> +
>> +/* Return the type of the given event location.  */
>> +
>> +extern enum event_location_type
>> +  event_location_type (const struct event_location *);
>> +
>> +/* Return a string representation of the LOCATION.
>> +   This function may return NULL for unspecified linespecs,
>> +   e.g, LOCATION_LINESPEC and addr_string is NULL.
>> +
>> +   The result is cached in LOCATION.  */
>> +
>> +extern const char *
>> +  event_location_to_string (struct event_location *location);
>> +
>> +/* A const version of event_location_to_string that will not cache
>> +   the resulting string representation.  The result is malloc'd
>> +   and must be freed by the caller.  */
>> +
>> +extern char *
>> +  event_location_to_string_const (const struct event_location *location);
> 
> Note to self: Do we need both non-const and const versions?
> [e.g., treat cached value as mutable in c++ sense?]

Yeah, if we could do something like that in C, that would negate the
need for both versions. As it is, this seemed the easiest (and not
an uncommon) way to deal with this. If you have another option, I'm all
eyes.

>> +
>> +/* Create a new linespec location.  The return result is malloc'd
>> +   and should be freed with delete_event_location.  */
>> +
>> +extern struct event_location *
>> +  new_linespec_location (char **linespec);
>> +
>> +/* Return the linespec location (a string) of the given event_location
>> +   (which must be of type LINESPEC_LOCATION).  */
>> +
>> +extern const char *
>> +  get_linespec_location (const struct event_location *location);
>> +
>> +/* Free an event location and any associated data.  */
>> +
>> +extern void delete_event_location (struct event_location *location);
>> +
>> +/* Make a cleanup to free LOCATION.  */
>> +
>> +extern struct cleanup *
>> +  make_cleanup_delete_event_location (struct event_location *location);
>> +
>> +/* Return a copy of the given SRC location.  */
>> +
>> +extern struct event_location *
>> +  copy_event_location (const struct event_location *src);
>> +
>> +/* Allocate and "copy" the opaque struct event_location.  This is used
>> +   when decoding locations which must parse their inputs.  The return result
>> +   should be freed.  */
>> +
>> +extern struct event_location *
>> +  copy_event_location_tmp (const struct event_location *src);
> 
> This function doesn't exist in this patch.
> 

Doh! It should not exist *at all* anymore! Fixed.

>> +
>> +/* Attempt to convert the input string in *ARGP into an event location.
>> +   ARGP is advanced past any processed input.  Returns a event_location
> 
> nit: an event_location
> 

Fixed.

>> +   (malloc'd) if an event location was successfully found in *ARGP,
>> +   NULL otherwise.
>> +
>> +   This function may call error() if *ARGP looks like properly formed,
>> +   but invalid, input, e.g., if it is called with missing argument parameters
>> +   or invalid options.
>> +
>> +   The return result must be freed with delete_event_location.  */
>> +
>> +extern struct event_location *
>> +  string_to_event_location (char **argp,
>> +			    const struct language_defn *langauge);
>> +
>> +/* A convenience function for testing for unset locations.  */
>> +
>> +extern int event_location_empty_p (const struct event_location *location);
>> +
>> +/* Set the location's string representation.  If STRING is NULL, clear
>> +   the string representation.  */
>> +
>> +extern void
>> +  set_event_location_string (struct event_location *location,
>> +			     const char *string);
>> +#endif /* LOCATIONS_H */

Thanks for taking another look!

I'll send out new versions of the patches requiring revision shortly.

Keith