Re: [python] Allow explicit locations in breakpoints.

[Simon Marchi <simon.marchi@ericsson.com>]

On 2017-11-17 06:02 AM, Phil Muldoon wrote:
> All,
> 
> Here's an updated patch. ChangeLog remains the same (except for
> additional NEWS entry)
> 
> I just realised this needs a doc review also.
> 
> Cheers
> 
> Phil

Hi Phil,

That

> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -32,6 +32,7 @@
>  #include "language.h"
>  #include "location.h"
>  #include "py-event.h"
> +#include "linespec.h"
>  
>  /* Number of live breakpoints.  */
>  static int bppy_live;
> @@ -638,20 +639,73 @@ static int
>  bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>  {
>    static const char *keywords[] = { "spec", "type", "wp_class", "internal",
> -				    "temporary", NULL };
> -  const char *spec;
> +				    "temporary","source", "function",
> +				    "label", "line", NULL };
> +  const char *spec = NULL;
>    int type = bp_breakpoint;
>    int access_type = hw_write;
>    PyObject *internal = NULL;
>    PyObject *temporary = NULL;
>    int internal_bp = 0;
>    int temporary_bp = 0;
> +  char *line = NULL;
> +  char *label = NULL;
> +  char *source = NULL;
> +  char *function = NULL;
>  
> -  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|iiOO", keywords,
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOssss", keywords,
>  					&spec, &type, &access_type,
> -					&internal, &temporary))
> +					&internal,
> +					&temporary, &source,
> +					&function, &label, &line))
>      return -1;
>  
> +  /* If spec is defined, ensure that none of the explicit location
> +     keywords are also defined.  */
> +  if (spec != NULL)
> +    {
> +      if (source != NULL || function != NULL || label != NULL || line != NULL)
> +	{
> +	  PyErr_SetString (PyExc_RuntimeError,
> +			   _("Breakpoints specified with spec cannot "
> +			     "have source, function, label or line defined."));
> +	  return -1;
> +	}
> +    }
> +  else
> +    {
> +      /* If spec isn't defined, ensure that the user is not trying to
> +	 define a watchpoint with an explicit location.  */
> +      if (type == bp_watchpoint)
> +	{
> +	  PyErr_SetString (PyExc_RuntimeError,
> +			   _("Watchpoints cannot be set by explicit "
> +			     "location parameters."));
> +	  return -1;
> +	}
> +      else
> +	{
> +	  /* Otherwise, ensure some explicit locations are defined.  */
> +	  if (source == NULL && function == NULL && label == NULL
> +	      && line == NULL)
> +	    {
> +	      PyErr_SetString (PyExc_RuntimeError,
> +			       _("Neither spec nor explicit location set."));
> +	      return -1;
> +	    }
> +	  /* Finally, if source is specified, ensure that line, label
> +	     or function are specified too.  */
> +	  if (source != NULL && function == NULL && label == NULL
> +	      && line == NULL)
> +	    {
> +	      PyErr_SetString (PyExc_RuntimeError,
> +			       _("Specifying a source must also include a "
> +				 "line, label or function."));
> +	      return -1;
> +	    }
> +	}
> +    }
> +

I would suggest putting the argument validation code in its own function (e.g.
bppy_init_validate_args), to keep bppy_init of a reasonnable size.

>    if (internal)
>      {
>        internal_bp = PyObject_IsTrue (internal);
> @@ -672,16 +726,37 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>  
>    TRY
>      {
> -      gdb::unique_xmalloc_ptr<char>
> -	copy_holder (xstrdup (skip_spaces (spec)));
> -      char *copy = copy_holder.get ();
> -
>        switch (type)
>  	{
>  	case bp_breakpoint:
>  	  {
> -	    event_location_up location
> -	      = string_to_event_location_basic (&copy, current_language);
> +	    event_location_up location;
> +
> +	    if (spec != NULL)
> +	      {
> +		gdb::unique_xmalloc_ptr<char>
> +		  copy_holder (xstrdup (skip_spaces (spec)));
> +		char *copy = copy_holder.get ();
> +
> +		location  = string_to_event_location (&copy,
> +						      current_language);
> +	      }
> +	    else
> +	      {
> +		struct explicit_location explicit_loc;
> +
> +		initialize_explicit_location (&explicit_loc);
> +		explicit_loc.source_filename = source;
> +		explicit_loc.function_name = function;
> +		explicit_loc.label_name = label;
> +
> +		if (line != NULL)
> +		  explicit_loc.line_offset =
> +		    linespec_parse_line_offset (line);

For convenience, I think it would be nice to accept integers for line.  So perhaps
something like this:

From d0e3122bca8b4fcb2d6f303bdc66f68929bdc14f Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 23 Nov 2017 16:58:35 -0500
Subject: [PATCH] suggestion

---
 gdb/python/py-breakpoint.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 7fb35ce..5ee0fed 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -648,23 +648,23 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
   PyObject *temporary = NULL;
   int internal_bp = 0;
   int temporary_bp = 0;
-  char *line = NULL;
+  PyObject *lineobj = NULL;
   char *label = NULL;
   char *source = NULL;
   char *function = NULL;

-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOssss", keywords,
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOsssO", keywords,
 					&spec, &type, &access_type,
 					&internal,
 					&temporary, &source,
-					&function, &label, &line))
+					&function, &label, &lineobj))
     return -1;

   /* If spec is defined, ensure that none of the explicit location
      keywords are also defined.  */
   if (spec != NULL)
     {
-      if (source != NULL || function != NULL || label != NULL || line != NULL)
+      if (source != NULL || function != NULL || label != NULL || lineobj != NULL)
 	{
 	  PyErr_SetString (PyExc_RuntimeError,
 			   _("Breakpoints specified with spec cannot "
@@ -687,7 +687,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	{
 	  /* Otherwise, ensure some explicit locations are defined.  */
 	  if (source == NULL && function == NULL && label == NULL
-	      && line == NULL)
+	      && lineobj == NULL)
 	    {
 	      PyErr_SetString (PyExc_RuntimeError,
 			       _("Neither spec nor explicit location set."));
@@ -696,7 +696,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	  /* Finally, if source is specified, ensure that line, label
 	     or function are specified too.  */
 	  if (source != NULL && function == NULL && label == NULL
-	      && line == NULL)
+	      && lineobj == NULL)
 	    {
 	      PyErr_SetString (PyExc_RuntimeError,
 			       _("Specifying a source must also include a "
@@ -706,6 +706,13 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	}
     }

+  if (lineobj != NULL && !PyInt_Check (lineobj) && !PyString_Check (lineobj))
+    {
+      PyErr_SetString (PyExc_TypeError,
+		       _ ("line must be an integer or a string."));
+      return -1;
+    }
+
   if (internal)
     {
       internal_bp = PyObject_IsTrue (internal);
@@ -750,9 +757,23 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 		explicit_loc.function_name = function;
 		explicit_loc.label_name = label;

-		if (line != NULL)
-		  explicit_loc.line_offset =
-		    linespec_parse_line_offset (line);
+		if (lineobj != NULL)
+		  {
+		    gdb::unique_xmalloc_ptr<char> line;
+
+		    if (PyInt_Check (lineobj))
+		      line.reset (xstrprintf ("%ld", PyInt_AsLong (lineobj)));
+		    else if (PyString_Check (lineobj))
+		      line = python_string_to_host_string (lineobj);
+		    else
+		      {
+			/* We should have validated the type up there... */
+			gdb_assert_not_reached ("Unexpected type of lineobj.");
+		      }
+
+		    explicit_loc.line_offset
+		      = linespec_parse_line_offset (line.get ());
+		  }

 		location = new_explicit_location (&explicit_loc);
 	      }
-- 
2.7.4


> +
> +		location = new_explicit_location (&explicit_loc);
> +	      }
> +
>  	    create_breakpoint (python_gdbarch,
>  			       location.get (), NULL, -1, NULL,
>  			       0,
> @@ -692,8 +767,12 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>  			       0, 1, internal_bp, 0);
>  	    break;
>  	  }
> -        case bp_watchpoint:
> +	case bp_watchpoint:
>  	  {
> +	    gdb::unique_xmalloc_ptr<char>
> +	      copy_holder (xstrdup (skip_spaces (spec)));
> +	    char *copy = copy_holder.get ();
> +
>  	    if (access_type == hw_write)
>  	      watch_command_wrapper (copy, 0, internal_bp);
>  	    else if (access_type == hw_access)
> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.c b/gdb/testsuite/gdb.python/py-breakpoint.c
> index df6163e53a..562cab35f7 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.c
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.c
> @@ -24,7 +24,7 @@ int multiply (int i)
>  
>  int add (int i)
>  {
> -  return i + i; 
> +  return i + i;  /* Break at function add.  */
>  }
>  
>  
> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
> index bd138ac3d2..d91e5deb50 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -547,6 +547,74 @@ proc test_bkpt_events {} {
>      check_last_event breakpoint_deleted
>  }
>  
> +proc test_bkpt_explicit_loc {} {
> +    global srcfile testfile
> +
> +    with_test_prefix test_bkpt_invisible {
> +	# Start with a fresh gdb.
> +	clean_restart ${testfile}
> +
> +	if ![runto_main] then {
> +	    fail "cannot run to main."
> +	    return 0
> +	}
> +
> +	delete_breakpoints
> +
> +	set bp_location1 [gdb_get_line_number "Break at multiply."]
> +	set bp_location2 [gdb_get_line_number "Break at add."]
> +
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"$bp_location1\")" \
> +	    "set explicit breakpoint by line" 0
> +	gdb_continue_to_breakpoint "Break at multiply" \
> +	    ".*Break at multiply.*"
> +
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"+1\")" \
> +	    "set explicit breakpoint by relative line" 0
> +	gdb_continue_to_breakpoint "Break at add" \
> +	    ".*Break at add.*"
> +
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"-1\")" \
> +	    "set explicit breakpoint by relative negative line" 0
> +	gdb_continue_to_breakpoint "Break at multiply" \
> +	    ".*Break at multiply.*"
> +
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (function=\"add\")" \
> +	    "set explicit breakpoint by function" 0
> +	gdb_continue_to_breakpoint "Break at function add" \
> +	    ".*Break at function add.*"
> +
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", function=\"add\")" \
> +	    "set explicit breakpoint by source file and function" 0
> +	gdb_continue_to_breakpoint "Break at function add" \
> +	    ".*Break at function add.*"
> +
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"$bp_location2\")" \
> +	    "set explicit breakpoint by source file and line number" 0
> +	gdb_continue_to_breakpoint "Break at add" \
> +	    ".*Break at add.*"
> +
> +	delete_breakpoints
> +	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\")" \
> +	    "RuntimeError: Specifying a source must also include a line, label or function.*" \
> +	    "set invalid explicit breakpoint by source only"
> +
> +	gdb_test "python bp1 = gdb.Breakpoint (source=\"foo.c\", line=\"5\")" \
> +	    "No source file named foo.*" \
> +	    "set invalid explicit breakpoint by missing source and line"
> +	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"900\")" \
> +	    "No line 900 in file \"$srcfile\".*" \
> +	    "set invalid explicit breakpoint by source and invalid line"
> +	gdb_test "python bp1 = gdb.Breakpoint (function=\"blah\")" \
> +	    "Function \"blah\" not defined.*" \
> +	    "set invalid explicit breakpoint by missing function"
> +    }

The test names should start with a lower case letter:

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Follow_the_test_name_convention

Also make sure that test names are not duplicated.  You can use this command to quickly
find duplicated names in gdb.sum:

$ cat testsuite/gdb.sum | grep -e PASS -e FAIL | sort | uniq -c | sort -n

Would it be good to test that explicit breakpoint locations through the spec
argument work as well?  Things like

  gdb.Breakpoint('-line 5')

?

Thanks,

Simon