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

[Doug Evans <dje at google dot com>]

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

-- 
/dje