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

[Doug Evans <xdje42 at gmail dot com>]

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