Re: [python] Allow explicit locations in breakpoints.

Simon Marchi
2017-11-23T22:17:28Z

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