[PATCH V2 2/4] python/19506 -- gdb.Breakpoint address location regression

[PATCH V2 2/4] python/19506 -- gdb.Breakpoint address location regression

[Keith Seitz]

Now that "legacy" linespecs benefit from consolidated support in
string_to_event_location_basic, python's Breakpoint command should use this
function to turn strings into event locations.

As a result, this patch fixes python/19506. Before:

(gdb) python gdb.Breakpoint("*main")
Traceback (most recent call last):
  File "<string>", line 1, in <module>
RuntimeError: Function "*main" not defined.
Error while executing Python code.

After:

(gdb) python gdb.Breakpoint("*main")
Breakpoint 1 at 0x4005fb: file ../../../src/gdb/testsuite/gdb.python/py-breakpoint.c, line 32.

gdb/ChangeLog

	PR python/19506
	* python/py-breakpoint.c (bppy_init): Use
	string_to_event_location_basic instead of new_linespec_location.

gdb/testsuite/ChangeLog

	PR python/19506
	* gdb.python/py-breakpoint.exp (test_bkpt_address): New procedure.
	(toplevel): Call test_bkpt_address.
---
 gdb/python/py-breakpoint.c                 |    5 +++-
 gdb/testsuite/gdb.python/py-breakpoint.exp |   33 ++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 85b17d5..964ec62 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -663,7 +663,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 
   TRY
     {
-      char *copy = xstrdup (spec);
+      char *copy = xstrdup (skip_spaces_const (spec));
       struct cleanup *cleanup = make_cleanup (xfree, copy);
 
       switch (type)
@@ -672,7 +672,8 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	  {
 	    struct event_location *location;
 
-	    location = new_linespec_location (&copy);
+	    location
+	      = string_to_event_location_basic (&copy, current_language);
 	    make_cleanup_delete_event_location (location);
 	    create_breakpoint (python_gdbarch,
 			       location, NULL, -1, NULL,
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index af6c5fc..d1d1b22 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -462,6 +462,38 @@ proc test_bkpt_temporary { } {
     }
 }
 
+# Test address locations.
+
+proc test_bkpt_address {} {
+    global gdb_prompt decimal srcfile
+
+    # Delete all breakpoints
+    delete_breakpoints
+
+    gdb_test "python gdb.Breakpoint(\"*main\")" \
+	".*Breakpoint ($decimal)+ at .*$srcfile, line ($decimal)+\."
+
+    gdb_py_test_silent_cmd \
+	"python main_loc = gdb.parse_and_eval(\"main\").address" \
+	"eval address of main" 0
+
+    # Python 2 vs 3 ... Check `int' first. If that fails, try `long'.
+    gdb_test_multiple "python main_addr = int(main_loc)" "int value of main" {
+	-re "Traceback.*$gdb_prompt $" {
+	    gdb_test_no_output "python main_addr = long(main_loc)" \
+		"long value of main"
+	}
+	-re "$gdb_prompt $" {
+	    pass "int value of main"
+	}
+    }
+
+    # Include whitespace in the linespec to double-check proper
+    # grokking of argument to gdb.Breakpoint.
+    gdb_test "python gdb.Breakpoint(\"  *{}\".format(str(main_addr)))" \
+	".*Breakpoint ($decimal)+ at .*$srcfile, line ($decimal)+\."
+}
+
 test_bkpt_basic
 test_bkpt_deletion
 test_bkpt_cond_and_cmds
@@ -470,3 +502,4 @@ test_watchpoints
 test_bkpt_internal
 test_bkpt_eval_funcs
 test_bkpt_temporary
+test_bkpt_address

[PATCH V2 0/4] Add support for "legacy" linespecs

[Keith Seitz]

When I committed the locations API patchset, I ignored the need for
"legacy" support. Specifically, MI, python, and guile all require the
ability to convert an old-style linespec (which can be a linespec, address,
or probe location) into an event_location.

Python and guile code assumes everything is a linespec location, so neither
probe nor address locations are available to them.

MI uses string_to_event_location, but that allows CLI-style explicit
locations. MI already supports MI-style explicit locations.

This patchset is an attempt to address this by adding a new locations API
function, string_to_event_location_basic, which implements a generic
string-to-struct event_location routine which converts these (so-called)
"legacy" linespecs.

---

Keith Seitz (4):
      Refactor string_to_event_location for legacy linespec support.
      python/19506 -- gdb.Breakpoint address location regression
      Use string_to_event_location_basic in guile.
      Enable/update legacy linespecs in MI.


 gdb/guile/scm-breakpoint.c                 |    4 +
 gdb/location.c                             |   87 ++++++++++++++++------------
 gdb/location.h                             |   14 ++++-
 gdb/mi/mi-cmd-break.c                      |    2 -
 gdb/python/py-breakpoint.c                 |    6 +-
 gdb/testsuite/gdb.guile/scm-breakpoint.exp |   13 ++++
 gdb/testsuite/gdb.python/py-breakpoint.exp |   33 +++++++++++
 7 files changed, 117 insertions(+), 42 deletions(-)

[PATCH V2 4/4] Enable/update legacy linespecs in MI.

[Keith Seitz]

MI is currently using string_to_event_location to enable the use of legacy
linespecs, but using this function (until this patchset) had the (as yet
unnoticed) side effect of allowing both MI and CLI representation for
explicit locations.

This patch simply changes MI to use the same legacy linespec functions
that the python and guile interpreters use.  This eliminates the CLI syntax
for explicit locations (in MI).

gdb/ChangeLog

	* mi/mi-cmd-break.c (mi_cmd_break_insert_1): Use
	string_to_event_location_basic instead of string_to_event_location.
---
 gdb/mi/mi-cmd-break.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index ef3ce29..3d40629 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -341,7 +341,7 @@ mi_cmd_break_insert_1 (int dprintf, char *command, char **argv, int argc)
     }
   else
     {
-      location = string_to_event_location (&address, current_language);
+      location = string_to_event_location_basic (&address, current_language);
       if (*address)
 	{
 	  delete_event_location (location);

[PATCH V2 1/4] Refactor string_to_event_location for legacy linespec support.

[Keith Seitz]

This patch refactors string_to_event_location, breaking it into two
separate functions:

1) string_to_event_location_basic
A "basic" string parser that implements support for "legacy" linespecs
(linespec, address, and probe locations).  This function is intended to
be used by any UI wishing/needing to support this legacy behavior.

2) string_to_event_location
This is now intended as a CLI-only function which adds explicit location
parsing in a CLI-appropriate manner (in the form of traditional option/value
pairs).

Together these patches serve to simplify string-to-event location parsing
for all existing non-CLI interfaces (MI, guile, and python).

gdb/ChangeLog

	* location.c (string_to_explicit_location): Note that "-p" is
	reserved for probe locations and return NULL for any input
	that starts with that.
	(string_to_event_location): Move "legacy" linespec code to ...
	(string_to_event_location_basic): ... here.
	* location.h (string_to_event_location): Update comment.
	(string_to_event_location_basic): New function.
---
 gdb/location.c |   87 +++++++++++++++++++++++++++++++++-----------------------
 gdb/location.h |   14 ++++++++-
 2 files changed, 64 insertions(+), 37 deletions(-)

diff --git a/gdb/location.c b/gdb/location.c
index e43ebf1..bbd2696 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -522,11 +522,13 @@ string_to_explicit_location (const char **argp,
   struct event_location *location;
 
   /* It is assumed that input beginning with '-' and a non-digit
-     character is an explicit location.  */
+     character is an explicit location.  "-p" is reserved, though,
+     for probe locations.  */
   if (argp == NULL
       || *argp == '\0'
       || *argp[0] != '-'
-      || !isalpha ((*argp)[1]))
+      || !isalpha ((*argp)[1])
+      || ((*argp)[0] == '-' && (*argp)[1] == 'p'))
     return NULL;
 
   location = new_explicit_location (NULL);
@@ -634,51 +636,36 @@ string_to_explicit_location (const char **argp,
 /* See description in location.h.  */
 
 struct event_location *
-string_to_event_location (char **stringp,
-			  const struct language_defn *language)
+string_to_event_location_basic (char **stringp,
+				const struct language_defn *language)
 {
   struct event_location *location;
+  const char *arg, *orig, *cs;
 
-  /* First, check if the string is an address location.  */
-  if (*stringp != NULL && **stringp == '*')
+  /* Try the input as a probe spec.  */
+  cs = *stringp;
+  if (cs != NULL && probe_linespec_to_ops (&cs) != NULL)
     {
-      const char *arg, *orig;
-      CORE_ADDR addr;
-
-      orig = arg = *stringp;
-      addr = linespec_expression_to_pc (&arg);
-      location = new_address_location (addr, orig, arg - orig);
-      *stringp += arg - orig;
+      location = new_probe_location (*stringp);
+      *stringp += strlen (*stringp);
     }
   else
     {
-      const char *cs;
-
-      /* Next, try the input as a probe spec.  */
-      cs = *stringp;
-      if (cs != NULL && probe_linespec_to_ops (&cs) != NULL)
-	{
-	  location = new_probe_location (*stringp);
-	  *stringp += strlen (*stringp);
-	}
-      else
+      /* Try an address location.  */
+      if (*stringp != NULL && **stringp == '*')
 	{
 	  const char *arg, *orig;
+	  CORE_ADDR addr;
 
-	  /* Next, try an explicit location.  */
 	  orig = arg = *stringp;
-	  location = string_to_explicit_location (&arg, language, 0);
-	  if (location != NULL)
-	    {
-	      /* It was a valid explicit location.  Advance STRINGP to
-		 the end of input.  */
-	      *stringp += arg - orig;
-	    }
-	  else
-	    {
-	      /* Everything else is a linespec.  */
-	      location = new_linespec_location (stringp);
-	    }
+	  addr = linespec_expression_to_pc (&arg);
+	  location = new_address_location (addr, orig, arg - orig);
+	  *stringp += arg - orig;
+	}
+      else
+	{
+	  /* Everything else is a linespec.  */
+	  location = new_linespec_location (stringp);
 	}
     }
 
@@ -687,6 +674,34 @@ string_to_event_location (char **stringp,
 
 /* See description in location.h.  */
 
+struct event_location *
+string_to_event_location (char **stringp,
+			  const struct language_defn *language)
+{
+  struct event_location *location;
+  const char *arg, *orig;
+
+  /* Try an explicit location.  */
+  orig = arg = *stringp;
+  location = string_to_explicit_location (&arg, language, 0);
+  if (location != NULL)
+    {
+      /* It was a valid explicit location.  Advance STRINGP to
+	 the end of input.  */
+      *stringp += arg - orig;
+    }
+  else
+    {
+      /* Everything else is a "basic" linespec, address, or probe
+	 location.  */
+      location = string_to_event_location_basic (stringp, language);
+    }
+
+  return location;
+}
+
+/* See description in location.h.  */
+
 int
 event_location_empty_p (const struct event_location *location)
 {
diff --git a/gdb/location.h b/gdb/location.h
index b2cf45e..8b19f34 100644
--- a/gdb/location.h
+++ b/gdb/location.h
@@ -207,12 +207,24 @@ extern struct event_location *
    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.  */
+   The return result must be freed with delete_event_location.
+
+   This function is intended to be used by CLI commands and will parse
+   explicit locations in a CLI-centric way.  Other interfaces should use
+   string_to_event_location_basic if they want to maintain support for
+   legacy specifications of probe, address, and linespec locations.  */
 
 extern struct event_location *
   string_to_event_location (char **argp,
 			    const struct language_defn *langauge);
 
+/* Like string_to_event_location, but does not attempt to parse explicit
+   locations.  */
+
+extern struct event_location *
+  string_to_event_location_basic (char **argp,
+				  const struct language_defn *language);
+
 /* Attempt to convert the input string in *ARGP into an explicit location.
    ARGP is advanced past any processed input.  Returns an event_location
    (malloc'd) if an explicit location was successfully found in *ARGP,

[PATCH V2 3/4] Use string_to_event_location_basic in guile.

[Keith Seitz]

This patch, analogous to the previous python patch, implements proper
legacy linespec support in guile code using the newly introduced
string_to_event_location_basic.

gdb/ChangeLog

	* guile/scm-breakpoint.c (gdbscm_register_breakpoint_x): Skip
	leading whitespace and use string_to_event_location_basic instead
	of new_linespec_location.

gdb/testsuite/ChangeLog

	* gdb.guile/scm-breakpoint.exp (test_bkpt_address): New procedure.
	(toplevel): Call test_bkpt_address.
---
 gdb/guile/scm-breakpoint.c                 |    4 ++--
 gdb/testsuite/gdb.guile/scm-breakpoint.exp |   13 +++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c
index 716fe4c..baecf01 100644
--- a/gdb/guile/scm-breakpoint.c
+++ b/gdb/guile/scm-breakpoint.c
@@ -425,8 +425,8 @@ gdbscm_register_breakpoint_x (SCM self)
 
   pending_breakpoint_scm = self;
   location = bp_smob->spec.location;
-  copy = location;
-  eloc = new_linespec_location (&copy);
+  copy = skip_spaces (location);
+  eloc = string_to_event_location_basic (&copy, current_language);
   cleanup = make_cleanup_delete_event_location (eloc);
 
   TRY
diff --git a/gdb/testsuite/gdb.guile/scm-breakpoint.exp b/gdb/testsuite/gdb.guile/scm-breakpoint.exp
index 542a40f..33edf86 100644
--- a/gdb/testsuite/gdb.guile/scm-breakpoint.exp
+++ b/gdb/testsuite/gdb.guile/scm-breakpoint.exp
@@ -487,6 +487,18 @@ proc test_bkpt_registration {} {
     }
 }
 
+proc test_bkpt_address {} {
+    global decimal srcfile
+
+    # Leading whitespace is intentional!
+    gdb_scm_test_silent_cmd \
+	"guile (define bp1 (make-breakpoint \"  *multiply\"))" \
+	"create address breakpoint a '  *multiply'" 1
+
+    gdb_test "guile (register-breakpoint! bp1)" \
+	".*Breakpoint ($decimal)+ at .*$srcfile, line ($decimal)+\."
+}
+
 test_bkpt_basic
 test_bkpt_deletion
 test_bkpt_cond_and_cmds
@@ -495,3 +507,4 @@ test_watchpoints
 test_bkpt_internal
 test_bkpt_eval_funcs
 test_bkpt_registration
+test_bkpt_address

Re: [PATCH V2 0/4] Add support for "legacy" linespecs

[Joel Brobecker]

> When I committed the locations API patchset, I ignored the need for
> "legacy" support. Specifically, MI, python, and guile all require the
> ability to convert an old-style linespec (which can be a linespec, address,
> or probe location) into an event_location.
> 
> Python and guile code assumes everything is a linespec location, so neither
> probe nor address locations are available to them.
> 
> MI uses string_to_event_location, but that allows CLI-style explicit
> locations. MI already supports MI-style explicit locations.
> 
> This patchset is an attempt to address this by adding a new locations API
> function, string_to_event_location_basic, which implements a generic
> string-to-struct event_location routine which converts these (so-called)
> "legacy" linespecs.

FWIW, I like this patch series, and I saw no issues with it.
It is nice to see both Python and Guile + how easily GDB/MI
was adjusted.

Thanks, Keith!

-- 
Joel