[RFA,2/9] Explicit locations v2 - Event locations API

  Hi,

This patch is also a nop. In fact, the code added by this patch isn't 
even used. I present here the new locations API which will be 
used/elaborated upon by subsequent patches.

The basic premise here is that the breakpoint methods will no longer 
accept char*/char** argument "address strings." They will take a 
"location", which is a structure:

struct event_location
{
   /* The type of this breakpoint specification.  */
   enum event_location_type type;
#define EVENT_LOCATION_TYPE(S) ((S)->type)

   union
   {
      /* data needed for the various location types  */
   } u;
};

The UIs will use the new API function string_to_event_location to turn 
arbitrary text input into one of these structures.

This patch and the next only implement linespec locations. In other 
words, it only massages the current API without introducing any new 
features.

Keith

ChangeLog
2014-05-08  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.
  

Keith Seitz writes:
 > Hi,
 > 
 > This patch is also a nop. In fact, the code added by this patch isn't 
 > even used. I present here the new locations API which will be 
 > used/elaborated upon by subsequent patches.
 > 
 > The basic premise here is that the breakpoint methods will no longer 
 > accept char*/char** argument "address strings." They will take a 
 > "location", which is a structure:
 > 
 > struct event_location
 > {
 >    /* The type of this breakpoint specification.  */
 >    enum event_location_type type;
 > #define EVENT_LOCATION_TYPE(S) ((S)->type)
 > 
 >    union
 >    {
 >       /* data needed for the various location types  */
 >    } u;
 > };
 > 
 > The UIs will use the new API function string_to_event_location to turn 
 > arbitrary text input into one of these structures.
 > 
 > This patch and the next only implement linespec locations. In other 
 > words, it only massages the current API without introducing any new 
 > features.
 > 
 > Keith
 > 
 > ChangeLog
 > 2014-05-08  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.
 > 
 > 
 > diff --git a/gdb/Makefile.in b/gdb/Makefile.in
 > index 3efedc8..266a6ec 100644
 > --- a/gdb/Makefile.in
 > +++ b/gdb/Makefile.in
 > @@ -805,7 +805,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 \
 > @@ -887,7 +887,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 solib-pa64.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 solib-irix.h amd64-tdep.h \
 > @@ -964,7 +964,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/location.c b/gdb/location.c
 > new file mode 100644
 > index 0000000..3f50bed
 > --- /dev/null
 > +++ b/gdb/location.c
 > @@ -0,0 +1,159 @@
 > +/* Data structures and API for event locations in GDB.
 > +   Copyright (C) 2013-2014 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>
 > +
 > +/* Initialize the given LOCATION.  */
 > +
 > +void
 > +initialize_event_location (struct event_location *location,
 > +			  enum event_location_type type)
 > +{
 > +  memset (location, 0, sizeof (struct event_location));
 > +  EVENT_LOCATION_TYPE (location) = type;
 > +}
 > +
 > +/* Create a new location with the given TYPE.  */
 > +
 > +struct event_location *
 > +new_event_location (enum event_location_type type)
 > +{
 > +  struct event_location *location;
 > +
 > +  location = XNEW (struct event_location);
 > +  initialize_event_location (location, type);
 > +  return location;
 > +}
 > +
 > +/* Return a copy of the given SRC location.  */
 > +
 > +struct event_location *
 > +copy_event_location (const struct event_location *src)
 > +{
 > +  struct event_location *dst;
 > +
 > +  dst = XCNEW (struct event_location);
 > +  EVENT_LOCATION_TYPE (dst) = EVENT_LOCATION_TYPE (src);
 > +  if (EVENT_LOCATION_SAVE_SPEC (src) != NULL)
 > +    EVENT_LOCATION_SAVE_SPEC (dst) = xstrdup (EVENT_LOCATION_SAVE_SPEC (src));
 > +
 > +  switch (EVENT_LOCATION_TYPE (src))
 > +    {
 > +    case EVENT_LOCATION_LINESPEC:
 > +      EVENT_LOCATION_LINESPEC (dst) = xstrdup (EVENT_LOCATION_LINESPEC (src));
 > +      break;
 > +
 > +    default:
 > +      gdb_assert_not_reached ("unknown event location type");
 > +    }
 > +
 > +  return dst;
 > +}
 > +
 > +/* Free LOCATION and any associated data.  */
 > +
 > +void
 > +delete_event_location (void *data)

I'm sure there's a reason why data is void * and not
properly typed.  It would be good to document that
reason here. [Or can we make it struct event_location *?]

 > +{
 > +  struct event_location *location
 > +    = (struct event_location *) data;
 > +
 > +  if (location != NULL)
 > +    {
 > +      xfree (EVENT_LOCATION_SAVE_SPEC (location));
 > +
 > +      switch (EVENT_LOCATION_TYPE (location))
 > +	{
 > +	case EVENT_LOCATION_LINESPEC:
 > +	  xfree (EVENT_LOCATION_LINESPEC (location));
 > +	  break;
 > +
 > +	default:
 > +	  gdb_assert_not_reached ("unknown event location type");
 > +	}
 > +
 > +      xfree (location);
 > +    }
 > +}
 > +
 > +/* Return a string representation of the LOCATION.
 > +   This function may return NULL for unspecified linespecs,
 > +   e.g, EVENT_LOCATION_LINESPEC and addr_string is NULL.  */
 > +
 > +const char *
 > +event_location_to_string (const struct event_location *location)
 > +{
 > +  const char *result = NULL;
 > +
 > +  switch (EVENT_LOCATION_TYPE (location))
 > +    {
 > +    case EVENT_LOCATION_LINESPEC:
 > +      result = EVENT_LOCATION_LINESPEC (location);
 > +      break;
 > +
 > +    default:
 > +      gdb_assert_not_reached ("unknown event location type");
 > +    }
 > +
 > +  return result;
 > +}
 > +
 > +/* Parse the user input in *STRINGP and turn it into a struct
 > +   event_location, advancing STRINGP past any parsed input.
 > +   Return value is malloc'd.  */
 > +
 > +struct event_location *
 > +string_to_event_location (char **stringp,
 > +			  const struct language_defn *language)
 > +{
 > +  struct event_location *location;
 > +
 > +  location = new_event_location (EVENT_LOCATION_LINESPEC);
 > +  if (*stringp != NULL)
 > +    {
 > +      EVENT_LOCATION_LINESPEC (location) = xstrdup (*stringp);
 > +      *stringp += strlen (*stringp);
 > +    }
 > +
 > +  return location;
 > +}

This consumes the entire string, so we can remove the side-effect
of modifying the input argument.
It might make the caller (slightly) more complicated, but I like the
simplification here.
[Or at any rate IWBN to have a version that took a const char *.]

Plus, for robustness sake, should we treat "" as unspecified?
[and thus leave the string as NULL so event_location_empty_p
will return true]

 > +
 > +/* A convenience function for testing for unset locations.  */
 > +
 > +int
 > +event_location_empty_p (const struct event_location *location)
 > +{
 > +  switch (EVENT_LOCATION_TYPE (location))
 > +    {
 > +    case EVENT_LOCATION_LINESPEC:
 > +      return EVENT_LOCATION_LINESPEC (location) == NULL;
 > +
 > +    default:
 > +      gdb_assert_not_reached ("unknown event location type");
 > +    }
 > +}
 > diff --git a/gdb/location.h b/gdb/location.h
 > new file mode 100644
 > index 0000000..83fc3a4
 > --- /dev/null
 > +++ b/gdb/location.h
 > @@ -0,0 +1,100 @@
 > +/* Data structures and API for event locations in GDB.
 > +   Copyright (C) 2013, 2014 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;
 > +
 > +/* An enumeration of the various ways to specify a stop event
 > +   location (used with create_breakpoint).  */
 > +
 > +enum event_location_type
 > +{
 > +  /* A traditional linespec.  */
 > +  EVENT_LOCATION_LINESPEC
 > +};
 > +
 > +/* An event location used to set a stop event in the inferior.
 > +   This structure is an amalgam of the various ways
 > +   to specify a where a stop event should be set.  */

specify where

 > +
 > +struct event_location
 > +{
 > +  /* The type of this breakpoint specification.  */
 > +  enum event_location_type type;
 > +#define EVENT_LOCATION_TYPE(S) ((S)->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 EVENT_LOCATION_LINESPEC(S) ((S)->u.addr_string)
 > +  } u;
 > +
 > +  /* A string representation of how this location may be
 > +     saved.  This is used to save stop event locations to file.
 > +     Malloc'd.  */
 > +  char *save_spec;
 > +#define EVENT_LOCATION_SAVE_SPEC(S) ((S)->save_spec)
 > +};

Making this struct opaque to its users has benefits.
Thoughts?

 > +
 > +/* Return a string representation of the LOCATION.
 > +   This function may return NULL for unspecified linespecs,
 > +   e.g, EVENT_LOCATION_LINESPEC and addr_string is NULL.  */
 > +
 > +extern const char *
 > +  event_location_to_string (const struct event_location *location);

I wonder if the string form might be computed
for some kind of location.  In which case either we always
return a malloc'd copy here (remove const from result)
or lazily compute it and cache the computed string in the
struct (remove the const from the location parameter).
[Or switch to c++ and use mutable I guess. :)]

 > +
 > +/* Free an event location and any associated data.  */
 > +
 > +extern void delete_event_location (void *data);
 > +
 > +/* Create a new event location with the given TYPE.  */
 > +
 > +extern struct event_location *
 > +  new_event_location (enum event_location_type type);
 > +
 > +/* Return a copy of the given SRC location.  */
 > +extern struct event_location *
 > +  copy_event_location (const struct event_location *src);
 > +
 > +/* Initialize the given LOCATION.  */
 > +
 > +extern void initialize_event_location (struct event_location *location,
 > +				       enum event_location_type type);
 > +
 > +/* Attempt to convert the input string in *ARGP into an event location.
 > +   ARGP is advanced past any processed input.  Returns a 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.  */
 > +
 > +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);

blank line

 > +#endif /* LOCATIONS_H */