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

  This patch introduces the new breakpoint/"linespec" API based on
a new struct event_location.  This API currently only supports
traditional linespecs, maintaining the status quo of the code base.
Future patches will add additional functionality for other location
types such as address locations.

gdb/ChangeLog:

	* Makefile.in (SFILES): Add location.c.
	(HFILES_NO_SRCDIR): Add location.h.
	(COMMON_OBS): Add location.o.
	* linespec.c (linespec_lex_to_end): New function.
	* linespec.c (linespec_lex_to_end): Declare.
	* location.c: New file.
	* location.h: New file.
---
 gdb/Makefile.in |    6 +
 gdb/linespec.c  |   55 +++++++++++++
 gdb/linespec.h  |    5 +
 gdb/location.c  |  234 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/location.h  |  113 +++++++++++++++++++++++++++
 5 files changed, 410 insertions(+), 3 deletions(-)
 create mode 100644 gdb/location.c
 create mode 100644 gdb/location.h
  

Keith Seitz <keiths@redhat.com> writes:
> This patch introduces the new breakpoint/"linespec" API based on
> a new struct event_location.  This API currently only supports
> traditional linespecs, maintaining the status quo of the code base.
> Future patches will add additional functionality for other location
> types such as address locations.

Hi. Just a couple of nits and a couple of questions.
I wrote down a question for myself below.
I haven't read the entire series yet.

>
> gdb/ChangeLog:
>
> 	* Makefile.in (SFILES): Add location.c.
> 	(HFILES_NO_SRCDIR): Add location.h.
> 	(COMMON_OBS): Add location.o.
> 	* linespec.c (linespec_lex_to_end): New function.
> 	* linespec.c (linespec_lex_to_end): Declare.

typo: linespec.h

> 	* location.c: New file.
> 	* location.h: New file.
>...
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 95104ef..0a51564 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -851,7 +851,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
>  	inline-frame.c \
>  	interps.c \
>  	jv-exp.y jv-lang.c jv-valprint.c jv-typeprint.c jv-varobj.c \
> -	language.c linespec.c minidebug.c \
> +	language.c linespec.c location.c minidebug.c \
>  	m2-exp.y m2-lang.c m2-typeprint.c m2-valprint.c \
>  	macrotab.c macroexp.c macrocmd.c macroscope.c main.c maint.c \
>  	mdebugread.c memattr.c mem-break.c minsyms.c mipsread.c memory-map.c \
> @@ -937,7 +937,7 @@ mi/mi-out.h mi/mi-main.h mi/mi-common.h mi/mi-cmds.h linux-nat.h \
>  complaints.h gdb_proc_service.h gdb_regex.h xtensa-tdep.h inf-loop.h \
>  common/gdb_wait.h common/gdb_assert.h solib.h ppc-tdep.h cp-support.h glibc-tdep.h \
>  interps.h auxv.h gdbcmd.h tramp-frame.h mipsnbsd-tdep.h	\
> -amd64-linux-tdep.h linespec.h i387-tdep.h mn10300-tdep.h \
> +amd64-linux-tdep.h linespec.h location.h i387-tdep.h mn10300-tdep.h \
>  sparc64-tdep.h monitor.h ppcobsd-tdep.h srec.h \
>  coff-pe-read.h parser-defs.h gdb_ptrace.h mips-linux-tdep.h \
>  m68k-tdep.h spu-tdep.h jv-lang.h environ.h amd64-tdep.h \
> @@ -1021,7 +1021,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>  	source.o value.o eval.o valops.o valarith.o valprint.o printcmd.o \
>  	block.o symtab.o psymtab.o symfile.o symfile-debug.o symmisc.o \
>  	linespec.o dictionary.o \
> -	infcall.o \
> +	location.o infcall.o \
>  	infcmd.o infrun.o \
>  	expprint.o environ.o stack.o thread.o \
>  	exceptions.o \
> 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?

> +    }
> +  END_CATCH
> +
> +  *stringp += PARSER_STREAM (&parser) - orig;
> +  do_cleanups (cleanup);
> +}
> +
>  /* See linespec.h.  */
>  
>  void
> diff --git a/gdb/linespec.h b/gdb/linespec.h
> index 7e66024..77ec46d 100644
> --- a/gdb/linespec.h
> +++ b/gdb/linespec.h
> @@ -156,4 +156,9 @@ extern struct symtabs_and_lines decode_line_with_last_displayed (char *, int);
>     the keyword.  If not, return NULL.  */
>  
>  extern const char *linespec_lexer_lex_keyword (const char *p);
> +
> +/* Find the end of the (first) linespec pointed to by *STRINGP.
> +   STRINGP will be advanced to this point.  */
> +
> +extern void linespec_lex_to_end (char **stringp);
>  #endif /* defined (LINESPEC_H) */
> diff --git a/gdb/location.c b/gdb/location.c
> new file mode 100644
> index 0000000..39e09c1
> --- /dev/null
> +++ b/gdb/location.c
> @@ -0,0 +1,234 @@
> +/* 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/>.  */
> +
> +#include "defs.h"
> +#include "gdb_assert.h"
> +#include "location.h"
> +#include "symtab.h"
> +#include "language.h"
> +#include "linespec.h"
> +#include "cli/cli-utils.h"
> +#include "probe.h"
> +
> +#include <ctype.h>
> +#include <string.h>
> +
> +/* An event location used to set a stop event in the inferior.
> +   This structure is an amalgam of the various ways
> +   to specify where a stop event should be set.  */
> +
> +struct event_location
> +{
> +  /* The type of this breakpoint specification.  */
> +  enum event_location_type type;
> +#define EL_TYPE(PTR) (PTR)->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 EL_LINESPEC(PTR) ((PTR)->u.addr_string)
> +  } u;
> +
> +  /* Cached string representation of this location.  This is used, e.g., to
> +     save stop event locations to file.  Malloc'd.  */
> +  char *as_string;
> +#define EL_STRING(PTR) ((PTR)->as_string)
> +};
> +
> +/* See description in location.h.  */
> +
> +enum event_location_type
> +event_location_type (const struct event_location *location)
> +{
> +  return EL_TYPE (location);
> +}
> +
> +/* See description in location.h.  */
> +
> +struct event_location *
> +new_linespec_location (char **linespec)
> +{
> +  struct event_location *location;
> +
> +  location = XCNEW (struct event_location);
> +  EL_TYPE (location) = LINESPEC_LOCATION;
> +  if (*linespec != NULL)
> +    {
> +      char *p;
> +      char *orig = *linespec;
> +
> +      linespec_lex_to_end (linespec);
> +      p = remove_trailing_whitespace (orig, *linespec);
> +      if ((p - orig) > 0)
> +	EL_LINESPEC (location) = savestring (orig, p - orig);
> +    }
> +  return location;
> +}
> +
> +/* See description in location.h.  */
> +
> +const char *
> +get_linespec_location (const struct event_location *location)
> +{
> +  gdb_assert (EL_TYPE (location) == LINESPEC_LOCATION);
> +  return EL_LINESPEC (location);
> +}
> +
> +/* See description in location.h.  */
> +
> +struct event_location *
> +copy_event_location (const struct event_location *src)
> +{
> +  struct event_location *dst;
> +
> +  dst = XCNEW (struct event_location);
> +  EL_TYPE (dst) = EL_TYPE (src);
> +  if (EL_STRING (src) != NULL)
> +    EL_STRING (dst) = xstrdup (EL_STRING (src));
> +
> +  switch (EL_TYPE (src))
> +    {
> +    case LINESPEC_LOCATION:
> +      if (EL_LINESPEC (src) != NULL)
> +	EL_LINESPEC (dst) = xstrdup (EL_LINESPEC (src));
> +      break;
> +
> +    default:
> +      gdb_assert_not_reached ("unknown event location type");
> +    }
> +
> +  return dst;
> +}
> +
> +/* A cleanup function for struct event_location.  */
> +
> +static void
> +delete_event_location_cleanup (void *data)
> +{
> +  struct event_location *location = (struct event_location *) data;
> +
> +  delete_event_location (location);
> +}
> +
> +/* See description in location.h.  */
> +
> +struct cleanup *
> +make_cleanup_delete_event_location (struct event_location *location)
> +{
> +  return make_cleanup (delete_event_location_cleanup, location);
> +}
> +
> +/* See description in location.h.  */
> +
> +void
> +delete_event_location (struct event_location *location)
> +{
> +  if (location != NULL)
> +    {
> +      xfree (EL_STRING (location));
> +
> +      switch (EL_TYPE (location))
> +	{
> +	case LINESPEC_LOCATION:
> +	  xfree (EL_LINESPEC (location));
> +	  break;
> +
> +	default:
> +	  gdb_assert_not_reached ("unknown event location type");
> +	}
> +
> +      xfree (location);
> +    }
> +}
> +
> +/* See description in location.h.  */
> +
> +char *
> +event_location_to_string_const (const struct event_location *location)
> +{
> +  char *result = NULL;
> +
> +  if (EL_STRING (location) != NULL)
> +    return xstrdup (EL_STRING (location));
> +
> +  switch (EL_TYPE (location))
> +    {
> +    case LINESPEC_LOCATION:
> +      if (EL_LINESPEC (location) != NULL)
> +	result = xstrdup (EL_LINESPEC (location));
> +      break;
> +
> +    default:
> +      gdb_assert_not_reached ("unknown event location type");
> +    }
> +
> +  return result;
> +}
> +
> +/* See description in location.h.  */
> +
> +const char *
> +event_location_to_string (struct event_location *location)
> +{
> +  /* Cache a copy of the string representation.  */
> +  if (EL_STRING (location) == NULL)
> +    EL_STRING (location) = event_location_to_string_const (location);
> +
> +  return EL_STRING (location);
> +}
> +
> +/* See description in location.h.  */
> +
> +struct event_location *
> +string_to_event_location (char **stringp,
> +			  const struct language_defn *language)
> +{
> +  struct event_location *location;
> +
> +  location = new_linespec_location (stringp);
> +  return location;
> +}
> +
> +/* See description in location.h.  */
> +
> +int
> +event_location_empty_p (const struct event_location *location)
> +{
> +  switch (EL_TYPE (location))
> +    {
> +    case LINESPEC_LOCATION:
> +      /* Linespecs are never "empty."  (NULL is a valid linespec)  */
> +      return 0;
> +
> +    default:
> +      gdb_assert_not_reached ("unknown event location type");
> +    }
> +}
> +
> +/* See description in location.h.  */
> +
> +void
> +set_event_location_string (struct event_location *location,
> +			   const char *string)
> +{
> +  xfree (EL_STRING (location));
> +  EL_STRING (location) = string == NULL ?  NULL : xstrdup (string);
> +}
> 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?]

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

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

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

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
  

On 05/19/2015 09:41 PM, Keith Seitz wrote:
>> 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.

You can cast away const for that.  See ada_decode_symbol for an example
of exactly that.  "const" in C does not mean that the object is set to
stone in read-only storage.

Thanks,
Pedro Alves