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

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

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

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.



[-- Attachment #2: explicit-event_location.patch --]
[-- Type: text/x-patch, Size: 9535 bytes --]

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)
+{
+  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;
+}
+
+/* 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.  */
+
+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)
+};
+
+/* 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);
+
+/* 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);
+#endif /* LOCATIONS_H */

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

[Doug Evans]

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

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

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 6606 bytes --]

Hi, Doug,

Thank you for taking a look!

On 05/12/2014 03:59 PM, Doug Evans wrote:
> Keith Seitz writes:
>   > +/* 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 *?]

Yes -- it's used as a cleanup. I've changed this to typed and added a 
utility cleanup, make_cleanup_delete_event_location instead.

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

It is true that this current version of this bit of code does some 
non-sensical stuff such as this, but in future versions, they actually 
do something useful.

Because of the requirement to pass create_breakpoint a structure 
representing the location (instead of simply a string), we must now 
split the "advancing past the input" problem in two. This function 
(string_to_event_location) will (eventually) handle explicit locations, 
address locations, and probe locations. However, for linespec locations, 
we must defer this until the linespec is parsed since the linespec 
grammar itself is ambiguous.

I could not come up with a scheme that could unify these operations 
(without violating the struct requirement). If you or anyone else has an 
idea, I'm all eyes.

See, for example, the complete version of this function in patch #7 
(where the UI elements for explicit locations is added).

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

""/NULL is only valid for a linespec (as in the user simply typing 
"break" with no argument). Not permitted for any other location type.

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

Doh! Fixed.

>   > +
>   > +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?

I briefly considered this but didn't see an real benefits. Can you 
elaborate? If you feel that strongly about it, I can change it.

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

Ah, so here's why it's taken me so long to respond...

After reading this, I played around with changing 
event_location_to_string as you pondered above. At the time, with the 
version I had, it was easier to "pre-compute" (= "save") the original 
string the user typed (post-canonicalization).

I quickly ran into an edge case that was not satisfactorily handled by 
the code as submitted. If an MI client sets a breakpoint with an 
explicit location, how is this breakpoint serialized (for saving to a 
file or pending)? In the previous version, it would be converted into a 
linespec. While not horrible, it opens the door for ambiguity. I feel it 
is better to serialize to an explicit form instead, which is what this 
next revision does.

So, after all that, event_location_to_string will compute the string 
value if it doesn't have a cached copy available. [Caching is a win, 
IMO. I see this string representation hit multiple times during 
breakpoint_re_set.]

>   > +extern int event_location_empty_p (const struct event_location *location);
>
> blank line
>

Fixed.

Phew! I appreciate very much that you've taken the time to take a look 
at this. I know this is going to be painful for us all.

Keith

PS. I am not reposting #1 -- it hasn't changed.

Changes from previous version:
    - EVENT_LOCATION_SAVE_SPEC removed
      (sub as_string where necessary)
    - delete_event_location now takes struct event_location*
    - add make_cleanup_delete_event_location
    - add event_location_to_string_const (necessary evil IMO)
    - fix typo in locations.h (struct event_location decl)
    - struct event_location.save_spec -> as_string
    - Update event_location_to_string{,_const} decls
    - Add missing blank line at end of location.h
    - new_event_location -> new_linespec_location
    - EVENT_LOCATION_LINESPEC -> LINESPEC_LOCATION for type
    - new_linespec_location instead of new_event_location

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

[-- Attachment #2: explicit-event_location-2.patch --]
[-- Type: text/x-patch, Size: 10970 bytes --]

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..57900bf
--- /dev/null
+++ b/gdb/location.c
@@ -0,0 +1,194 @@
+/* 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 linespec location.  The return result is malloc'd
+   and should be freed with delete_event_location.  */
+
+struct event_location *
+new_linespec_location (const char *linespec)
+{
+  struct event_location *location;
+
+  location = XNEW (struct event_location);
+  initialize_event_location (location, LINESPEC_LOCATION);
+  if (linespec != NULL)
+    EVENT_LOCATION_LINESPEC (location) = xstrdup (linespec);
+  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 (src->as_string != NULL)
+    dst->as_string = xstrdup (src->as_string);
+
+  switch (EVENT_LOCATION_TYPE (src))
+    {
+    case LINESPEC_LOCATION:
+      EVENT_LOCATION_LINESPEC (dst) = xstrdup (EVENT_LOCATION_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);
+}
+
+/* Make a cleanup to free LOCATION.  */
+
+struct cleanup *
+make_cleanup_delete_event_location (struct event_location *location)
+{
+  return make_cleanup (delete_event_location_cleanup, location);
+}
+
+/* Free LOCATION and any associated data.  */
+
+void
+delete_event_location (struct event_location *location)
+{
+  if (location != NULL)
+    {
+      xfree (location->as_string);
+
+      switch (EVENT_LOCATION_TYPE (location))
+	{
+	case LINESPEC_LOCATION:
+	  xfree (EVENT_LOCATION_LINESPEC (location));
+	  break;
+
+	default:
+	  gdb_assert_not_reached ("unknown event location type");
+	}
+
+      xfree (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.  */
+
+char *
+event_location_to_string_const (const struct event_location *location)
+{
+  char *result = NULL;
+
+  if (location->as_string != NULL)
+    return xstrdup (location->as_string);
+
+  switch (EVENT_LOCATION_TYPE (location))
+    {
+    case LINESPEC_LOCATION:
+      if (EVENT_LOCATION_LINESPEC (location) != NULL)
+	result = xstrdup (EVENT_LOCATION_LINESPEC (location));
+      break;
+
+    default:
+      gdb_assert_not_reached ("unknown event location type");
+    }
+
+  return result;
+}
+
+/* Return a string representation of the LOCATION.
+   This function may return NULL for unspecified linespecs,
+   e.g, LINESPEC_LOCATION and addr_string is NULL.
+
+   The result is cached in LOCATION.  */
+
+const char *
+event_location_to_string (struct event_location *location)
+{
+  /* Cache a copy of the string representation.  */
+  if (location->as_string == NULL)
+    location->as_string = event_location_to_string_const (location);
+
+  return location->as_string;
+}
+
+/* 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_linespec_location (*stringp);
+  if (*stringp != NULL)
+    *stringp += strlen (*stringp);
+
+  return location;
+}
+
+/* A convenience function for testing for unset locations.  */
+
+int
+event_location_empty_p (const struct event_location *location)
+{
+  switch (EVENT_LOCATION_TYPE (location))
+    {
+    case LINESPEC_LOCATION:
+      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..418e271
--- /dev/null
+++ b/gdb/location.h
@@ -0,0 +1,115 @@
+/* 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.  */
+  LINESPEC_LOCATION
+};
+
+/* 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 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;
+
+  /* Cached string representation of this location.  This is used to
+     save stop event locations to file.  Malloc'd.  */
+  char *as_string;
+};
+
+/* 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.
+
+   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);
+
+/* 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);
+
+/* 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 (const char *linespec);
+
+/* 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);
+
+#endif /* LOCATIONS_H */

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

[Doug Evans]

Keith Seitz <keiths@redhat.com> writes:

> Hi, Doug,
>
> Thank you for taking a look!
>
> On 05/12/2014 03:59 PM, Doug Evans wrote:
>> Keith Seitz writes:
>>   > +/* 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 *?]
>
> Yes -- it's used as a cleanup. I've changed this to typed and added a
> utility cleanup, make_cleanup_delete_event_location instead.

Alas I've been told we're no longer allowed to add new make_cleanup_foo
functions.
[Can't remember when, but I'm trusting my memory on this one.  :-)]
OTOH there's nothing written down in the coding standards section of the wiki.
I think the loss of type safety isn't warranted,
so I'm going to approve this part, and see if anyone complains
(or wants you to do things differently).

>>   > +/* 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 *.]
>
> It is true that this current version of this bit of code does some
> non-sensical stuff such as this, but in future versions, they actually
> do something useful.
>
> Because of the requirement to pass create_breakpoint a structure
> representing the location (instead of simply a string), we must now
> split the "advancing past the input" problem in two. This function
> (string_to_event_location) will (eventually) handle explicit
> locations, address locations, and probe locations. However, for
> linespec locations, we must defer this until the linespec is parsed
> since the linespec grammar itself is ambiguous.
>
> I could not come up with a scheme that could unify these operations
> (without violating the struct requirement). If you or anyone else has
> an idea, I'm all eyes.
>
> See, for example, the complete version of this function in patch #7
> (where the UI elements for explicit locations is added).

Ah. Ok.

>> Plus, for robustness sake, should we treat "" as unspecified?
>> [and thus leave the string as NULL so event_location_empty_p
>> will return true]
>
> ""/NULL is only valid for a linespec (as in the user simply typing
> "break" with no argument). Not permitted for any other location type.

Ah, righto.

>>   > +/* 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
>
> Doh! Fixed.
>
>>   > +
>>   > +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?
>
> I briefly considered this but didn't see an real benefits. Can you
> elaborate? If you feel that strongly about it, I can change it.

Making structs opaque (where there's a choice) helps keep things
maintainable. e.g., only the implementation details one wants to expose
actually do get exposed.  If it's a toss up, or close to one, I'd
go the opaque struct route.  The patch is already providing accessors,
so why not make those accessors functions instead of macros?
A teensy bit more to write, but I think it's worth it.
[OTOH I haven't seen the rest of the patch set yet,
if it proves excessively onerous I'm happy to revisit.]

Plus in this case the use of EVENT_LOCATION_LINESPEC as an enum
value and as a macro name would go away, and we can keep
EVENT_LOCATION_LINESPEC as the enum value. 1/2 :-)
Within location.c I would just access the struct members directly,
instead of with accessor macros.
[I know accessor macros is a GNU-ism, but we don't always use them
in dwarf2read.c (e.g.,: v.quick), and do just fine IMO.]

>>   > +/* 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. :)]
>
> Ah, so here's why it's taken me so long to respond...
>
> After reading this, I played around with changing
> event_location_to_string as you pondered above. At the time, with the
> version I had, it was easier to "pre-compute" (= "save") the original
> string the user typed (post-canonicalization).
>
> I quickly ran into an edge case that was not satisfactorily handled by
> the code as submitted. If an MI client sets a breakpoint with an
> explicit location, how is this breakpoint serialized (for saving to a
> file or pending)? In the previous version, it would be converted into
> a linespec. While not horrible, it opens the door for ambiguity. I
> feel it is better to serialize to an explicit form instead, which is
> what this next revision does.
>
> So, after all that, event_location_to_string will compute the string
> value if it doesn't have a cached copy available. [Caching is a win,
> IMO. I see this string representation hit multiple times during
> breakpoint_re_set.]

Ok.

>>   > +extern int event_location_empty_p (const struct event_location *location);
>>
>> blank line
>>
>
> Fixed.
>
> Phew! I appreciate very much that you've taken the time to take a look
> at this. I know this is going to be painful for us all.
>
> Keith
>
> PS. I am not reposting #1 -- it hasn't changed.

Sure.

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

[Doug Evans]

On Thu, Jun 5, 2014 at 10:44 PM, Doug Evans <xdje42@gmail.com> wrote:
> Plus in this case the use of EVENT_LOCATION_LINESPEC as an enum
> value and as a macro name would go away, and we can keep
> EVENT_LOCATION_LINESPEC as the enum value. 1/2 :-)

btw, keeping the enum value spelling as EVENT_LOCATION_LINESPEC
instead of LOCATION_LINESPEC is just a suggestion at this point.
Don't rush out and rename things back.

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

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 4381 bytes --]

On 06/05/2014 10:44 PM, Doug Evans wrote:

> Alas I've been told we're no longer allowed to add new make_cleanup_foo
> functions.
> [Can't remember when, but I'm trusting my memory on this one.  :-)]
> OTOH there's nothing written down in the coding standards section of the wiki.
> I think the loss of type safety isn't warranted,
> so I'm going to approve this part, and see if anyone complains
> (or wants you to do things differently).

I guess I'll wait for someone to complain, then! :-P

>>>    > +
>>>    > +struct event_location
[snip]
>>>    > +};
>>>
>>> Making this struct opaque to its users has benefits.
>>> Thoughts?
>>
>> I briefly considered this but didn't see an real benefits. Can you
>> elaborate? If you feel that strongly about it, I can change it.
>
> Making structs opaque (where there's a choice) helps keep things
> maintainable. e.g., only the implementation details one wants to expose
> actually do get exposed.  If it's a toss up, or close to one, I'd
> go the opaque struct route.  The patch is already providing accessors,
> so why not make those accessors functions instead of macros?
> A teensy bit more to write, but I think it's worth it.
> [OTOH I haven't seen the rest of the patch set yet,
> if it proves excessively onerous I'm happy to revisit.]

Ok, in this next revision, I've changed this structure to be completely 
opaque...

So instead of doing:

struct location *location, copy_location;

location = string_to_event_location (&arg, current_language);
copy_location = *location;
decode_line_full (&copy_location, ...);
if (event_location_type (location) == LINESPEC_LOCATION)
      arg = get_linespec_location (&copy_location); /* to advance ARG 
over processed input */

the code now does:

struct event_location *location, *copy_location;

location = string_to_event_location (&arg, current_language);
copy_location = copy_event_location_tmp (location);
make_cleanup (xfree, copy_location);
decode_line_full (copy_location, ...);
event_location_advance_input_ptr (&arg, copy_location);
do_cleanups (...);

> Within location.c I would just access the struct members directly,
> instead of with accessor macros.
> [I know accessor macros is a GNU-ism, but we don't always use them
> in dwarf2read.c (e.g.,: v.quick), and do just fine IMO.]

Well, I kept my accessor macros in location.c (and nowhere else)... I 
like them, but I am not married to them. If you/others really want me to 
remove them, just say the word, and I'll zap 'em.

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

As I mentioned the last time around, I've changed the event location API 
to compute this string when it can (it caches a copy of it inside the 
struct event_location -- I'm seeing this string computed many, many times).

There are two instances in which we save the string instead of computing it:

1) pending breakpoints: we save the entire input string into the 
location (needed to preserve, e.g., conditions)

2) When converting/canonicalizing a linespec to explicit form, we must 
save the linespec string representation (so that we can reproduce the 
linespec the way the user originally specified it). The other option 
here is to add a flag to event_location to note this. Then we can 
compute the location. I chose to simply save the linespec string 
representation.

Updated patch attached.

Keith

Changes since last revision:
     - Move struct event_location to location.c
     - Make struct event_location completely opaque
     - Turn all EVENT_LOCATION_* macros to functions.
     - Add get_linespec_location, copy_event_location_tmp,
       event_location_advance_input_ptr,
       advance_event_location_ptr, set_event_location_string
     - Remove duplicate function comments/"See description in foo.h."


[-- Attachment #2: explicit-event_location.patch --]
[-- Type: text/x-patch, Size: 13023 bytes --]

gdb/ChangeLog:

	* 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.
---
 gdb/Makefile.in |    6 +
 gdb/location.c  |  257 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/location.h  |  125 +++++++++++++++++++++++++++
 3 files changed, 385 insertions(+), 3 deletions(-)
 create mode 100644 gdb/location.c
 create mode 100644 gdb/location.h

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..8111d7c
--- /dev/null
+++ b/gdb/location.c
@@ -0,0 +1,257 @@
+/* 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>
+
+/* 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 (const char *linespec)
+{
+  struct event_location *location;
+
+  location = XCNEW (struct event_location);
+  EL_TYPE (location) = LINESPEC_LOCATION;
+  if (linespec != NULL)
+    EL_LINESPEC (location) = xstrdup (linespec);
+  return location;
+}
+
+/* See description in location.h.  */
+
+char *
+get_linespec_location (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.  */
+
+struct event_location *
+copy_event_location_tmp (const struct event_location *src)
+{
+  struct event_location *tmp = XCNEW (struct event_location);
+
+  memcpy (tmp, src, sizeof (struct event_location));
+  return tmp;
+}
+
+/* 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);
+  if (*stringp != NULL)
+    *stringp += strlen (*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 event_location_advance_input_ptr (char **inp,
+				       const struct event_location *location)
+{
+  if (EL_TYPE (location) == LINESPEC_LOCATION)
+    *inp = EL_LINESPEC (location);
+}
+
+/* See description in location.h.  */
+
+void
+advance_event_location_ptr (struct event_location *location, size_t num)
+{
+  if (EL_TYPE (location) == LINESPEC_LOCATION)
+    EL_LINESPEC (location) += num;
+}
+
+/* See description in location.h.  */
+
+void
+set_event_location_string (struct event_location *location,
+			   const char *string)
+{
+  EL_STRING (location) = xstrdup (string);
+}
diff --git a/gdb/location.h b/gdb/location.h
new file mode 100644
index 0000000..705c83b
--- /dev/null
+++ b/gdb/location.h
@@ -0,0 +1,125 @@
+/* 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;
+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);
+
+/* 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 (const char *linespec);
+
+/* Return the linespec location (a string) of the given event_location
+   (which must be of type LINESPEC_LOCATION).  */
+
+extern char *
+  get_linespec_location (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);
+
+/* 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.
+
+   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);
+
+/* Advance *INP past any processed input in the LOCATION.  This function
+   is used in conjunction with copy_event_location_tmp to allow
+   linespec and probe locations to advance the input pointer.  */
+
+extern void
+  event_location_advance_input_ptr (char **inp,
+				    const struct event_location *location);
+
+/* Advance the location's input pointer NUM bytes.  */
+
+extern void
+  advance_event_location_ptr (struct event_location *tmp_location, size_t num);
+
+/* Set the location's string representation.  */
+
+extern void
+  set_event_location_string (struct event_location *location,
+			     const char *string);
+#endif /* LOCATIONS_H */

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

[Doug Evans]

Keith Seitz <keiths@redhat.com> writes:
>>>> [...]
>>>> Making this struct opaque to its users has benefits.
>>>> Thoughts?
>>>
>>> I briefly considered this but didn't see an real benefits. Can you
>>> elaborate? If you feel that strongly about it, I can change it.
>>
>> Making structs opaque (where there's a choice) helps keep things
>> maintainable. e.g., only the implementation details one wants to expose
>> actually do get exposed.  If it's a toss up, or close to one, I'd
>> go the opaque struct route.  The patch is already providing accessors,
>> so why not make those accessors functions instead of macros?
>> A teensy bit more to write, but I think it's worth it.
>> [OTOH I haven't seen the rest of the patch set yet,
>> if it proves excessively onerous I'm happy to revisit.]
>
> Ok, in this next revision, I've changed this structure to be
> completely opaque...
>
> So instead of doing:
>
> struct location *location, copy_location;
>
> location = string_to_event_location (&arg, current_language);
> copy_location = *location;
> decode_line_full (&copy_location, ...);
> if (event_location_type (location) == LINESPEC_LOCATION)
>      arg = get_linespec_location (&copy_location); /* to advance ARG
> over processed input */
>
> the code now does:
>
> struct event_location *location, *copy_location;
>
> location = string_to_event_location (&arg, current_language);
> copy_location = copy_event_location_tmp (location);
> make_cleanup (xfree, copy_location);
> decode_line_full (copy_location, ...);
> event_location_advance_input_ptr (&arg, copy_location);
> do_cleanups (...);

This is one part I still don't fully grok.
How come there's both location and copy_location here,
given that we don't do anything with location except make a copy of it?
If there is a technical reason, best add a comment to the code
explaining why.  OTOH, IWBN to remove the copy.

until_break_command is an example:

  location = string_to_event_location (&arg, current_language);
  cleanup = make_cleanup_delete_event_location (location);
  copy_location = copy_event_location_tmp (location);
  make_cleanup (xfree, copy_location);

Also, event_locations are destructed differently, depending on how they're
created, and IWBN to always just use the one destructor.
Plus the _tmp in copy_event_location_tmp doesn't help me the reader much.
How about rename copy_event_location_tmp to shallow_copy_event_location?
It could set a flag in event_location and the destructor could
check this flag.
[There is no data to say we need to do the shallow copy for
performance reasons (and it seems unlikely), so on the one hand I'd say
let's just delete support for shallow copying.  However, OTOH, there
could be a technical reason for doing a shallow copy,
see "How come there's both ...?" above.  So depending on the answer
to that question it may be preferable to just delete support for
oing shallow copies.  I'm happy to keep it though if there's a good
technical reason for it.]

Also, I'd like to handle what event_location_advance_input_ptr
and advance_event_location_ptr do differently, if only a little bit.
I gather it's done this way to cope with linespec parsing.
Still, it's sufficiently odd that I'd like to see if we can improve it.

advance_event_location_ptr has the problem that it breaks the
destructor, the pointer no longer points to the beginning of the
malloc'd string.  It may be that advance_event_location_ptr is only
ever called on shallow copies, but the API needs to present something
more robust to its users.
How about keeping two pointers, one to the beginning of the string that
the destructor can safely use, and one to the current parsing location?

Which leads me to my last high level issue: maintaining intermediate
parse state in event_location.  I gather linespecs force some pragmatism
on us (at least if we want to avoid yet more changes, which I'm totally
happy to avoid for now).  For example, this code is a bit confusing:

breakpoint.c:location_to_sals:

      if (b->condition_not_parsed)
	{
	  if (event_location_type (location) == LINESPEC_LOCATION)
	    s = get_linespec_location (location);
	  else
	    s = b->extra_string;

The code is fetching the linespec location with get_linespec_location
and then passing that to find_condition_and_thread here:

	      find_condition_and_thread (s, sals.sals[0].pc,
					 &cond_string, &thread, &task,
					 &extra_string);

Eh?

This works, I gather, because this code earlier in the function:

  TRY_CATCH (e, RETURN_MASK_ERROR)
    {
      b->ops->decode_location (b, location, &sals);
    }

will leave EL_LINESPEC (location) pointing passed the linespec,
leaving it pointing at any potential condition or thread spec.
This happens because bkpt_decode_location calls
decode_location_default which calls decode_line_full
which calls event_location_to_sals which does this:

	advance_event_location_ptr (location, copy - orig);

Have I got that right?

If we add two pointers, one to the beginning of the string and one
to the current parse location, then get_linespec_location could
always return a pointer to the beginning of the string, and then
a new function could return a pointer to the current parse state
(get_current_linespec_text or some such).
Does that make sense?
[It seems so, and it would involve minimal changes.]

>> Within location.c I would just access the struct members directly,
>> instead of with accessor macros.
>> [I know accessor macros is a GNU-ism, but we don't always use them
>> in dwarf2read.c (e.g.,: v.quick), and do just fine IMO.]
>
> Well, I kept my accessor macros in location.c (and nowhere else)... I
> like them, but I am not married to them. If you/others really want me
> to remove them, just say the word, and I'll zap 'em.

Since they're local to location.c I'd say just leave them.

>>>>    > +/* 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. :)]
>
> As I mentioned the last time around, I've changed the event location
> API to compute this string when it can (it caches a copy of it inside
> the struct event_location -- I'm seeing this string computed many,
> many times).

Yeah, I like this.

> There are two instances in which we save the string instead of computing it:

This part not so much. :-)

> 1) pending breakpoints: we save the entire input string into the
> location (needed to preserve, e.g., conditions)
>
> 2) When converting/canonicalizing a linespec to explicit form, we must
> save the linespec string representation (so that we can reproduce the
> linespec the way the user originally specified it). The other option
> here is to add a flag to event_location to note this. Then we can
> compute the location. I chose to simply save the linespec string
> representation.

I like the consistency of event_location.as_string always being
a cached copy of the string form of the location.
Otherwise it's confusing.
Does it make sense to add another string to event_location to
handle (1) and (2) above?

---

I'm hoping these suggestions make sense and they won't involve
too many changes.
If we can get agreement on something (I'm happy to hear counter proposals),
then I'll submit the rest of my review, and then you can submit v3
which barring nits should be the final version.

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

[Keith Seitz]

Hi, Doug,

I apologize for the long hiatus on this, but it seems I needed a (long) 
break to "rebase" myself!

On 10/12/2014 02:39 PM, Doug Evans wrote:
> This is one part I still don't fully grok.
> How come there's both location and copy_location here,
> given that we don't do anything with location except make a copy of it?

Okay, I have spent quite a bit of time trying to fix this, and while I 
was very frustrated with this, I am now *very* grateful that you've 
pushed me to readdress/revisit this. Thank you.

In the end, I've changed the way all of this works. This temporary copy 
hack, advance pointer functions, and more is all gone and no longer 
necessary.

I believe this revision addresses just about everything that you've raised.

You might ask, "How?" [*Might*? You will! :-)]

Really it all boils down to how linespecs are handled. The "usual" 
mechanism that I am/have been proposing is:

char *arg; /* e.g., break_command_1 et al */
struct event_location *location;
struct cleanup *cleanup;

location = string_to_event_location (&arg, current_language);
cleanup = make_cleanup_delete_event_location (location);
sals = decode_line_full (location, ...);

/* ... */

do_cleanups (cleanup);

The thing that has been holding all of this back is that 
string_to_event_location has never been able to deal with linespecs. 
Given any other location type, string_to_event_location can advance the 
argument over any processed input.

So the obvious solution to this is to teach it to do the same for 
linespecs. I don't know why I didn't do this earlier. I even wrote a 
proper lexer for linespecs a while ago. Using this, 
string_to_event_location can now handle all location types the same. 
[And it was *much* easier than I originally feared.]

As a result, there is no more need for all this "advance input pointer" 
fooey that I was doing before. In fact, I've changed the linespec parser 
and decode_line_* so that they don't even take a char** anymore. They 
don't "advance" anything anymore at all. That is all done a priori by 
string_to_event_location (or rather, the function that this function 
calls to do it for linespecs).

> Also, event_locations are destructed differently, depending on how they're
> created, and IWBN to always just use the one destructor.

All gone.

> Also, I'd like to handle what event_location_advance_input_ptr
> and advance_event_location_ptr do differently, if only a little bit.

All gone, too.

> Which leads me to my last high level issue: maintaining intermediate
> parse state in event_location.  I gather linespecs force some pragmatism
> on us (at least if we want to avoid yet more changes, which I'm totally
> happy to avoid for now).  For example, this code is a bit confusing:
>
> breakpoint.c:location_to_sals:
>
>        if (b->condition_not_parsed)
> 	{
> 	  if (event_location_type (location) == LINESPEC_LOCATION)
> 	    s = get_linespec_location (location);
> 	  else
> 	    s = b->extra_string;
>

This code is modified, but essentially still exists. The one big 
difference now is that this is done for all location types. In other 
words, regardless of the location type, b->extra_string is /always/ 
parsed for conditon/thread/task. [more on dprintf later]

This happens because string_to_event_location can advance the input 
string past the actual location specification, leaving, eg., "arg" above 
pointing at any conditional/thread/task. The code passes this as 
extra_string to create_breakpoint (which is so vaguely documented so as 
to allow me, without much consternation, to use it this way).

[This does not interfere with dprintfs, since conditions et al are not 
supported on dprintf "inline" in this manner, e.g., 'dprintf 
main,"hello" if argc == 1' is NOT supported. Although this patch series 
makes it much easier to fix.]

> If we add two pointers, one to the beginning of the string and one
> to the current parse location, then get_linespec_location could
> always return a pointer to the beginning of the string, and then
> a new function could return a pointer to the current parse state
> (get_current_linespec_text or some such).
> Does that make sense?
> [It seems so, and it would involve minimal changes.]

Yup, and I actually did do this (at your suggestion). It's not so quite 
so trivial, but it doesn't really matter anymore. It's all gone. :-)

>> As I mentioned the last time around, I've changed the event location
>> API to compute this string when it can (it caches a copy of it inside
>> the struct event_location -- I'm seeing this string computed many,
>> many times).
>
> Yeah, I like this.
>
>> There are two instances in which we save the string instead of computing it:
>
> This part not so much. :-)

It's not so bad. Either we put members into struct event_location for 
all these corner conditions, or we just save the string, which is what 
we really need/use.

>> 1) pending breakpoints: we save the entire input string into the
>> location (needed to preserve, e.g., conditions)

This is still the same. I did find a bug when a pending breakpoint is 
resolved, i.e.,

(gdb) break -function main if argc == 1
Set pending? y
(gdb) file myfile
...
(gdb) save breakpoints bps
(gdb) shell cat bps
break -function main if argc == 1
   cond $bpnum argc == 1

[FWIW, this occurs today with linespecs/address strings.]
I've fixed this (for linespecs, too!) in the code by clearing the string 
representation when the breakpoint is resolved. The next time it is 
needed, it will be (properly) computed.

>> 2) When converting/canonicalizing a linespec to explicit form, we must
>> save the linespec string representation (so that we can reproduce the
>> linespec the way the user originally specified it). The other option
>> here is to add a flag to event_location to note this. Then we can
>> compute the location. I chose to simply save the linespec string
>> representation.
>
> I like the consistency of event_location.as_string always being
> a cached copy of the string form of the location.
> Otherwise it's confusing.
> Does it make sense to add another string to event_location to
> handle (1) and (2) above?

We could, sure. Is it necessary, though? I am not so sure. Like 
linespecs/address strings, these event locations represent the user's 
view of a location in the inferior. We already manipulate this string 
representation in the same places today. [The one exception is the fix 
for the above-mentioned bug. That's the one new place we manipulate the 
string representation.]

> I'm hoping these suggestions make sense and they won't involve
> too many changes.

Just a bit of bookkeeping to get back into it. It's a large patch 
series; it just takes a while to do anything. [Perhaps my process needs 
improving, too.]

Shall I submit this new series, or would you like to offer any further 
comments on this or subsequent patches in the series?

Keith

PS. I will be leaving for FOSDEM tomorrow (Tues, 27 Jan) and returning 
on Mon, 9 Feb). I will not have much access to anything other than email 
at this time.

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

[Doug Evans]

Keith Seitz <keiths@redhat.com> writes:
> Hi, Doug,
>
> I apologize for the long hiatus on this, but it seems I needed a
> (long) break to "rebase" myself!
>
> On 10/12/2014 02:39 PM, Doug Evans wrote:
>> This is one part I still don't fully grok.
>> How come there's both location and copy_location here,
>> given that we don't do anything with location except make a copy of it?
>
> Okay, I have spent quite a bit of time trying to fix this, and while I
> was very frustrated with this, I am now *very* grateful that you've
> pushed me to readdress/revisit this. Thank you.
>
> In the end, I've changed the way all of this works. This temporary
> copy hack, advance pointer functions, and more is all gone and no
> longer necessary.
>
> I believe this revision addresses just about everything that you've raised.
>
> You might ask, "How?" [*Might*? You will! :-)]
>
> Really it all boils down to how linespecs are handled. The "usual"
> mechanism that I am/have been proposing is:
>
> char *arg; /* e.g., break_command_1 et al */
> struct event_location *location;
> struct cleanup *cleanup;
>
> location = string_to_event_location (&arg, current_language);
> cleanup = make_cleanup_delete_event_location (location);
> sals = decode_line_full (location, ...);
>
> /* ... */
>
> do_cleanups (cleanup);
>
> The thing that has been holding all of this back is that
> string_to_event_location has never been able to deal with
> linespecs. Given any other location type, string_to_event_location can
> advance the argument over any processed input.
>
> So the obvious solution to this is to teach it to do the same for
> linespecs. I don't know why I didn't do this earlier. I even wrote a
> proper lexer for linespecs a while ago. Using this,
> string_to_event_location can now handle all location types the
> same. [And it was *much* easier than I originally feared.]
>
> As a result, there is no more need for all this "advance input
> pointer" fooey that I was doing before. In fact, I've changed the
> linespec parser and decode_line_* so that they don't even take a
> char** anymore. They don't "advance" anything anymore at all. That is
> all done a priori by string_to_event_location (or rather, the function
> that this function calls to do it for linespecs).
>
>> Also, event_locations are destructed differently, depending on how they're
>> created, and IWBN to always just use the one destructor.
>
> All gone.
>
>> Also, I'd like to handle what event_location_advance_input_ptr
>> and advance_event_location_ptr do differently, if only a little bit.
>
> All gone, too.
>
>> Which leads me to my last high level issue: maintaining intermediate
>> parse state in event_location.  I gather linespecs force some pragmatism
>> on us (at least if we want to avoid yet more changes, which I'm totally
>> happy to avoid for now).  For example, this code is a bit confusing:
>>
>> breakpoint.c:location_to_sals:
>>
>>        if (b->condition_not_parsed)
>> 	{
>> 	  if (event_location_type (location) == LINESPEC_LOCATION)
>> 	    s = get_linespec_location (location);
>> 	  else
>> 	    s = b->extra_string;
>>
>
> This code is modified, but essentially still exists. The one big
> difference now is that this is done for all location types. In other
> words, regardless of the location type, b->extra_string is /always/
> parsed for conditon/thread/task. [more on dprintf later]
>
> This happens because string_to_event_location can advance the input
> string past the actual location specification, leaving, eg., "arg"
> above pointing at any conditional/thread/task. The code passes this as
> extra_string to create_breakpoint (which is so vaguely documented so
> as to allow me, without much consternation, to use it this way).
>
> [This does not interfere with dprintfs, since conditions et al are not
> supported on dprintf "inline" in this manner, e.g., 'dprintf
> main,"hello" if argc == 1' is NOT supported. Although this patch
> series makes it much easier to fix.]
>
>> If we add two pointers, one to the beginning of the string and one
>> to the current parse location, then get_linespec_location could
>> always return a pointer to the beginning of the string, and then
>> a new function could return a pointer to the current parse state
>> (get_current_linespec_text or some such).
>> Does that make sense?
>> [It seems so, and it would involve minimal changes.]
>
> Yup, and I actually did do this (at your suggestion). It's not so
> quite so trivial, but it doesn't really matter anymore. It's all
> gone. :-)
>
>>> As I mentioned the last time around, I've changed the event location
>>> API to compute this string when it can (it caches a copy of it inside
>>> the struct event_location -- I'm seeing this string computed many,
>>> many times).
>>
>> Yeah, I like this.
>>
>>> There are two instances in which we save the string instead of computing it:
>>
>> This part not so much. :-)
>
> It's not so bad. Either we put members into struct event_location for
> all these corner conditions, or we just save the string, which is what
> we really need/use.

I'm on the fence on this I guess.
It's not critical, so I don't want this to be a blocker.

>>> 1) pending breakpoints: we save the entire input string into the
>>> location (needed to preserve, e.g., conditions)
>
> This is still the same. I did find a bug when a pending breakpoint is
> resolved, i.e.,
>
> (gdb) break -function main if argc == 1
> Set pending? y
> (gdb) file myfile
> ...
> (gdb) save breakpoints bps
> (gdb) shell cat bps
> break -function main if argc == 1
>   cond $bpnum argc == 1
>
> [FWIW, this occurs today with linespecs/address strings.]
> I've fixed this (for linespecs, too!) in the code by clearing the
> string representation when the breakpoint is resolved. The next time
> it is needed, it will be (properly) computed.
>
>>> 2) When converting/canonicalizing a linespec to explicit form, we must
>>> save the linespec string representation (so that we can reproduce the
>>> linespec the way the user originally specified it). The other option
>>> here is to add a flag to event_location to note this. Then we can
>>> compute the location. I chose to simply save the linespec string
>>> representation.
>>
>> I like the consistency of event_location.as_string always being
>> a cached copy of the string form of the location.
>> Otherwise it's confusing.
>> Does it make sense to add another string to event_location to
>> handle (1) and (2) above?
>
> We could, sure. Is it necessary, though? I am not so sure. Like
> linespecs/address strings, these event locations represent the user's
> view of a location in the inferior. We already manipulate this string
> representation in the same places today. [The one exception is the fix
> for the above-mentioned bug. That's the one new place we manipulate
> the string representation.]
>
>> I'm hoping these suggestions make sense and they won't involve
>> too many changes.
>
> Just a bit of bookkeeping to get back into it. It's a large patch
> series; it just takes a while to do anything. [Perhaps my process
> needs improving, too.]
>
> Shall I submit this new series, or would you like to offer any further
> comments on this or subsequent patches in the series?
>
> Keith
>
> PS. I will be leaving for FOSDEM tomorrow (Tues, 27 Jan) and returning
> on Mon, 9 Feb). I will not have much access to anything other than
> email at this time.

I'd say submit it.
I'll give the patch a timely review!

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

[Doug Evans]

Keith Seitz <keiths@redhat.com> writes:
> Hi, Doug,
>
> I apologize for the long hiatus on this, but it seems I needed a
> (long) break to "rebase" myself!
>
> On 10/12/2014 02:39 PM, Doug Evans wrote:
>> This is one part I still don't fully grok.
>> How come there's both location and copy_location here,
>> given that we don't do anything with location except make a copy of it?
>
> Okay, I have spent quite a bit of time trying to fix this, and while I
> was very frustrated with this, I am now *very* grateful that you've
> pushed me to readdress/revisit this. Thank you.
>
> In the end, I've changed the way all of this works. This temporary
> copy hack, advance pointer functions, and more is all gone and no
> longer necessary.
>
> I believe this revision addresses just about everything that you've raised.
>
> You might ask, "How?" [*Might*? You will! :-)]
>
> Really it all boils down to how linespecs are handled. The "usual"
> mechanism that I am/have been proposing is:
>
> char *arg; /* e.g., break_command_1 et al */
> struct event_location *location;
> struct cleanup *cleanup;
>
> location = string_to_event_location (&arg, current_language);
> cleanup = make_cleanup_delete_event_location (location);
> sals = decode_line_full (location, ...);
>
> /* ... */
>
> do_cleanups (cleanup);
>
> The thing that has been holding all of this back is that
> string_to_event_location has never been able to deal with
> linespecs. Given any other location type, string_to_event_location can
> advance the argument over any processed input.
>
> So the obvious solution to this is to teach it to do the same for
> linespecs. I don't know why I didn't do this earlier. I even wrote a
> proper lexer for linespecs a while ago. Using this,
> string_to_event_location can now handle all location types the
> same. [And it was *much* easier than I originally feared.]
>
> As a result, there is no more need for all this "advance input
> pointer" fooey that I was doing before. In fact, I've changed the
> linespec parser and decode_line_* so that they don't even take a
> char** anymore. They don't "advance" anything anymore at all. That is
> all done a priori by string_to_event_location (or rather, the function
> that this function calls to do it for linespecs).
>
>> Also, event_locations are destructed differently, depending on how they're
>> created, and IWBN to always just use the one destructor.
>
> All gone.
>
>> Also, I'd like to handle what event_location_advance_input_ptr
>> and advance_event_location_ptr do differently, if only a little bit.
>
> All gone, too.
>
>> Which leads me to my last high level issue: maintaining intermediate
>> parse state in event_location.  I gather linespecs force some pragmatism
>> on us (at least if we want to avoid yet more changes, which I'm totally
>> happy to avoid for now).  For example, this code is a bit confusing:
>>
>> breakpoint.c:location_to_sals:
>>
>>        if (b->condition_not_parsed)
>> 	{
>> 	  if (event_location_type (location) == LINESPEC_LOCATION)
>> 	    s = get_linespec_location (location);
>> 	  else
>> 	    s = b->extra_string;
>>
>
> This code is modified, but essentially still exists. The one big
> difference now is that this is done for all location types. In other
> words, regardless of the location type, b->extra_string is /always/
> parsed for conditon/thread/task. [more on dprintf later]
>
> This happens because string_to_event_location can advance the input
> string past the actual location specification, leaving, eg., "arg"
> above pointing at any conditional/thread/task. The code passes this as
> extra_string to create_breakpoint (which is so vaguely documented so
> as to allow me, without much consternation, to use it this way).
>
> [This does not interfere with dprintfs, since conditions et al are not
> supported on dprintf "inline" in this manner, e.g., 'dprintf
> main,"hello" if argc == 1' is NOT supported. Although this patch
> series makes it much easier to fix.]
>
>> If we add two pointers, one to the beginning of the string and one
>> to the current parse location, then get_linespec_location could
>> always return a pointer to the beginning of the string, and then
>> a new function could return a pointer to the current parse state
>> (get_current_linespec_text or some such).
>> Does that make sense?
>> [It seems so, and it would involve minimal changes.]
>
> Yup, and I actually did do this (at your suggestion). It's not so
> quite so trivial, but it doesn't really matter anymore. It's all
> gone. :-)
>
>>> As I mentioned the last time around, I've changed the event location
>>> API to compute this string when it can (it caches a copy of it inside
>>> the struct event_location -- I'm seeing this string computed many,
>>> many times).
>>
>> Yeah, I like this.
>>
>>> There are two instances in which we save the string instead of computing it:
>>
>> This part not so much. :-)
>
> It's not so bad. Either we put members into struct event_location for
> all these corner conditions, or we just save the string, which is what
> we really need/use.

I'm on the fence on this I guess.
It's not critical, so I don't want this to be a blocker.

>>> 1) pending breakpoints: we save the entire input string into the
>>> location (needed to preserve, e.g., conditions)
>
> This is still the same. I did find a bug when a pending breakpoint is
> resolved, i.e.,
>
> (gdb) break -function main if argc == 1
> Set pending? y
> (gdb) file myfile
> ...
> (gdb) save breakpoints bps
> (gdb) shell cat bps
> break -function main if argc == 1
>   cond $bpnum argc == 1
>
> [FWIW, this occurs today with linespecs/address strings.]
> I've fixed this (for linespecs, too!) in the code by clearing the
> string representation when the breakpoint is resolved. The next time
> it is needed, it will be (properly) computed.
>
>>> 2) When converting/canonicalizing a linespec to explicit form, we must
>>> save the linespec string representation (so that we can reproduce the
>>> linespec the way the user originally specified it). The other option
>>> here is to add a flag to event_location to note this. Then we can
>>> compute the location. I chose to simply save the linespec string
>>> representation.
>>
>> I like the consistency of event_location.as_string always being
>> a cached copy of the string form of the location.
>> Otherwise it's confusing.
>> Does it make sense to add another string to event_location to
>> handle (1) and (2) above?
>
> We could, sure. Is it necessary, though? I am not so sure. Like
> linespecs/address strings, these event locations represent the user's
> view of a location in the inferior. We already manipulate this string
> representation in the same places today. [The one exception is the fix
> for the above-mentioned bug. That's the one new place we manipulate
> the string representation.]
>
>> I'm hoping these suggestions make sense and they won't involve
>> too many changes.
>
> Just a bit of bookkeeping to get back into it. It's a large patch
> series; it just takes a while to do anything. [Perhaps my process
> needs improving, too.]
>
> Shall I submit this new series, or would you like to offer any further
> comments on this or subsequent patches in the series?
>
> Keith
>
> PS. I will be leaving for FOSDEM tomorrow (Tues, 27 Jan) and returning
> on Mon, 9 Feb). I will not have much access to anything other than
> email at this time.

I'd say submit it.
I'll give the patch a timely review!

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

[Doug Evans]

Keith Seitz <keiths@redhat.com> writes:
> Hi, Doug,
>
> I apologize for the long hiatus on this, but it seems I needed a
> (long) break to "rebase" myself!
>
> On 10/12/2014 02:39 PM, Doug Evans wrote:
>> This is one part I still don't fully grok.
>> How come there's both location and copy_location here,
>> given that we don't do anything with location except make a copy of it?
>
> Okay, I have spent quite a bit of time trying to fix this, and while I
> was very frustrated with this, I am now *very* grateful that you've
> pushed me to readdress/revisit this. Thank you.
>
> In the end, I've changed the way all of this works. This temporary
> copy hack, advance pointer functions, and more is all gone and no
> longer necessary.
>
> I believe this revision addresses just about everything that you've raised.
>
> You might ask, "How?" [*Might*? You will! :-)]
>
> Really it all boils down to how linespecs are handled. The "usual"
> mechanism that I am/have been proposing is:
>
> char *arg; /* e.g., break_command_1 et al */
> struct event_location *location;
> struct cleanup *cleanup;
>
> location = string_to_event_location (&arg, current_language);
> cleanup = make_cleanup_delete_event_location (location);
> sals = decode_line_full (location, ...);
>
> /* ... */
>
> do_cleanups (cleanup);
>
> The thing that has been holding all of this back is that
> string_to_event_location has never been able to deal with
> linespecs. Given any other location type, string_to_event_location can
> advance the argument over any processed input.
>
> So the obvious solution to this is to teach it to do the same for
> linespecs. I don't know why I didn't do this earlier. I even wrote a
> proper lexer for linespecs a while ago. Using this,
> string_to_event_location can now handle all location types the
> same. [And it was *much* easier than I originally feared.]
>
> As a result, there is no more need for all this "advance input
> pointer" fooey that I was doing before. In fact, I've changed the
> linespec parser and decode_line_* so that they don't even take a
> char** anymore. They don't "advance" anything anymore at all. That is
> all done a priori by string_to_event_location (or rather, the function
> that this function calls to do it for linespecs).
>
>> Also, event_locations are destructed differently, depending on how they're
>> created, and IWBN to always just use the one destructor.
>
> All gone.
>
>> Also, I'd like to handle what event_location_advance_input_ptr
>> and advance_event_location_ptr do differently, if only a little bit.
>
> All gone, too.
>
>> Which leads me to my last high level issue: maintaining intermediate
>> parse state in event_location.  I gather linespecs force some pragmatism
>> on us (at least if we want to avoid yet more changes, which I'm totally
>> happy to avoid for now).  For example, this code is a bit confusing:
>>
>> breakpoint.c:location_to_sals:
>>
>>        if (b->condition_not_parsed)
>> 	{
>> 	  if (event_location_type (location) == LINESPEC_LOCATION)
>> 	    s = get_linespec_location (location);
>> 	  else
>> 	    s = b->extra_string;
>>
>
> This code is modified, but essentially still exists. The one big
> difference now is that this is done for all location types. In other
> words, regardless of the location type, b->extra_string is /always/
> parsed for conditon/thread/task. [more on dprintf later]
>
> This happens because string_to_event_location can advance the input
> string past the actual location specification, leaving, eg., "arg"
> above pointing at any conditional/thread/task. The code passes this as
> extra_string to create_breakpoint (which is so vaguely documented so
> as to allow me, without much consternation, to use it this way).
>
> [This does not interfere with dprintfs, since conditions et al are not
> supported on dprintf "inline" in this manner, e.g., 'dprintf
> main,"hello" if argc == 1' is NOT supported. Although this patch
> series makes it much easier to fix.]
>
>> If we add two pointers, one to the beginning of the string and one
>> to the current parse location, then get_linespec_location could
>> always return a pointer to the beginning of the string, and then
>> a new function could return a pointer to the current parse state
>> (get_current_linespec_text or some such).
>> Does that make sense?
>> [It seems so, and it would involve minimal changes.]
>
> Yup, and I actually did do this (at your suggestion). It's not so
> quite so trivial, but it doesn't really matter anymore. It's all
> gone. :-)
>
>>> As I mentioned the last time around, I've changed the event location
>>> API to compute this string when it can (it caches a copy of it inside
>>> the struct event_location -- I'm seeing this string computed many,
>>> many times).
>>
>> Yeah, I like this.
>>
>>> There are two instances in which we save the string instead of computing it:
>>
>> This part not so much. :-)
>
> It's not so bad. Either we put members into struct event_location for
> all these corner conditions, or we just save the string, which is what
> we really need/use.

I'm on the fence on this I guess.
It's not critical, so I don't want this to be a blocker.

>>> 1) pending breakpoints: we save the entire input string into the
>>> location (needed to preserve, e.g., conditions)
>
> This is still the same. I did find a bug when a pending breakpoint is
> resolved, i.e.,
>
> (gdb) break -function main if argc == 1
> Set pending? y
> (gdb) file myfile
> ...
> (gdb) save breakpoints bps
> (gdb) shell cat bps
> break -function main if argc == 1
>   cond $bpnum argc == 1
>
> [FWIW, this occurs today with linespecs/address strings.]
> I've fixed this (for linespecs, too!) in the code by clearing the
> string representation when the breakpoint is resolved. The next time
> it is needed, it will be (properly) computed.
>
>>> 2) When converting/canonicalizing a linespec to explicit form, we must
>>> save the linespec string representation (so that we can reproduce the
>>> linespec the way the user originally specified it). The other option
>>> here is to add a flag to event_location to note this. Then we can
>>> compute the location. I chose to simply save the linespec string
>>> representation.
>>
>> I like the consistency of event_location.as_string always being
>> a cached copy of the string form of the location.
>> Otherwise it's confusing.
>> Does it make sense to add another string to event_location to
>> handle (1) and (2) above?
>
> We could, sure. Is it necessary, though? I am not so sure. Like
> linespecs/address strings, these event locations represent the user's
> view of a location in the inferior. We already manipulate this string
> representation in the same places today. [The one exception is the fix
> for the above-mentioned bug. That's the one new place we manipulate
> the string representation.]
>
>> I'm hoping these suggestions make sense and they won't involve
>> too many changes.
>
> Just a bit of bookkeeping to get back into it. It's a large patch
> series; it just takes a while to do anything. [Perhaps my process
> needs improving, too.]
>
> Shall I submit this new series, or would you like to offer any further
> comments on this or subsequent patches in the series?
>
> Keith
>
> PS. I will be leaving for FOSDEM tomorrow (Tues, 27 Jan) and returning
> on Mon, 9 Feb). I will not have much access to anything other than
> email at this time.

I'd say submit it.
I'll give the patch a timely review!