| Message ID | f39f6365-fc45-ea7d-10dc-5e0053db5cbb@redhat.com |
|---|---|
| State | New |
| Headers | show |
> From: Phil Muldoon <pmuldoon@redhat.com> > Date: Wed, 11 Oct 2017 12:30:17 +0100 > > 2017-10-11 Phil Muldoon <pmuldoon@redhat.com> > > * python.texi (Basic Python): Add rbreak documentation. This should go into a separate gdb/doc/ChangeLog file. > +@findex gdb.rbreak > +@defun gdb.rbreak (regex @r{[}, minisyms @r{[}, throttle, @r{[}, symtabs @r{]]]}) You don't need that @findex, since @defun automatically inserts the function into the function index. The documentation part is okay with this fixed, but I think we also want a NEWS entry. Thanks.
On 11/10/17 13:11, Eli Zaretskii wrote: >> From: Phil Muldoon <pmuldoon@redhat.com> >> Date: Wed, 11 Oct 2017 12:30:17 +0100 >> >> 2017-10-11 Phil Muldoon <pmuldoon@redhat.com> >> >> * python.texi (Basic Python): Add rbreak documentation. > > This should go into a separate gdb/doc/ChangeLog file. It is locally. Not sure why the relative paths were not included. Anyway, noted and it will be in the doc/ ChangeLog. > >> +@findex gdb.rbreak >> +@defun gdb.rbreak (regex @r{[}, minisyms @r{[}, throttle, @r{[}, symtabs @r{]]]}) > > You don't need that @findex, since @defun automatically inserts the > function into the function index. Thanks. > The documentation part is okay with this fixed, but I think we also > want a NEWS entry. I'll add a NEWS entry and repost the doc part of the patch. Cheers Phil
On Wed, 11 Oct 2017 12:30:17 +0100 Phil Muldoon <pmuldoon@redhat.com> wrote: > So, instead, I've made the Python rbreak functionality a little more > tuneable than the console command equivalent. The first tuneable is to > allow the user to exclude mini symbols from the pattern matching. A [...] > gdb.rbreak ("", minisyms=False) While reading through your preamble, I noticed some terminology with which I was unfamiliar: "mini symbols" and "minisyms". In a private discussion, you informed me that you actually meant "minimal symbols" and "minsyms", which cleared things up for me. But, you also asked me to remind you about this via a public reply, so here it is... :) Kevin
On 11/10/17 17:19, Kevin Buettner wrote: > On Wed, 11 Oct 2017 12:30:17 +0100 > Phil Muldoon <pmuldoon@redhat.com> wrote: > >> So, instead, I've made the Python rbreak functionality a little more >> tuneable than the console command equivalent. The first tuneable is to >> allow the user to exclude mini symbols from the pattern matching. A > > [...] > >> gdb.rbreak ("", minisyms=False) > > While reading through your preamble, I noticed some terminology with > which I was unfamiliar: "mini symbols" and "minisyms". > > In a private discussion, you informed me that you actually meant > "minimal symbols" and "minsyms", which cleared things up for me. > > But, you also asked me to remind you about this via a public reply, so > here it is... :) > > Kevin Kevin, Thanks for catching the inconsistency in terminology! I'll clear it up and post it in the version 2 of the patch. I'll won't repost for now to see if other reviewers have other changes to request. Cheers Phil
On 11/10/17 12:30, Phil Muldoon wrote: > This introduces a Python rbreak function to the Python API. Its > functionality was designed to closely match that of the console rbreak > but with a number of caveats. > + > + gdbpy_ref<> argList (Py_BuildValue("(s)", symbol_name.c_str ())); > + gdbpy_ref<> obj (PyObject_CallObject ((PyObject *) > + &breakpoint_object_type, > + argList.get ())); > + > + /* Tolerate individual breakpoint failures. */ > + if (obj == NULL) > + gdbpy_print_stack (); > + else > + { > + PyTuple_SET_ITEM (return_tuple.get (), count, obj.get ()); > + count++; Not sure how I missed this on the first patch run, but that obj.get () should be obj.release (). Apologies for noise and fixed locally. Cheers Phil
On 2017-10-11 07:30 AM, Phil Muldoon wrote: > This introduces a Python rbreak function to the Python API. Its > functionality was designed to closely match that of the console rbreak > but with a number of caveats. > > That being said, there's a bit of preamble first. > > My original design was to have the gdb.Breakpoint constructor take a > regex pattern. While breakpoints can, and do, have multiple locations > they are always tied in with one linespec. I looked at the > linespec/location code and the relationship is pretty firmly embedded > as a one to many relationship. One linespec can have multiple > locations but only one linespec can be represented in the location > object. (Or address or probe or whatever the location is > representing). A linespec has to be definitive; it has to point to one > symbol. I thought about modifying this to have multiple symbols and > multiple locations but quickly decided this was really going to > violate the design contract of linespecs/location. It would also be > considerable and disruptive work. I looked then, again, at a > breakpoint having multiple linespecs but again decided the amount of > disruption this would generate was not worth a Python rbreak returning > one breakpoint representing multiple symbols and multiple locations. > > So, instead, I've made the Python rbreak functionality a little more > tuneable than the console command equivalent. The first tuneable is to > allow the user to exclude mini symbols from the pattern matching. A > loosely written pattern passed to the console rbreak command can > generate 5000+ breakpoints from a simple C "hello world" > program. rbreak "" will place a breakpoint on every single mini symbol > representing a function. I can't possibly see why this would be > desirable. In order to facilitate broadly-written patterns that seek to > place a breakpoint on every user-defined function, the equivalent > would be: > > gdb.rbreak ("", minisyms=False) Hi Phil, As Kevin noted (IIUC), this should be "minsyms" if we want to follow standard GDB terminology. > > That would place a breakpoint on all functions that are actually > defined in the inferior (and not those that are inserted by the > compiler, linker, etc). The default for this keyword is False. > > The second tuneable is a throttle. Beyond the name (which I am unsure > about but could not think of a better one), this allows the user to > enter a fail-safe limit for breakpoint creation. So, for the following > example, an inferior with ten user provided functions: > > gdb.rbreak ("", minisyms=False, throttle=5) max_results? max_breakpoints? > Would set no breakpoints and raise a runtime error. This functionality > is more aimed at those who want to utilise Python rbreak from a > toolset perspective (say for a GDB UI). The default for this keyword > is unlimited. > > The last tuneable is the ability to limit the pattern matching to a > set of gdb.Symtab objects. So, given the example below: > > gdb.rbreak ("", minisyms=False, throttle=5, symtabs=symTuple) > > where "symTuple" is a tuple of gdb.Symtab objects, the search/pattern > matching would be restricted to those gdb.Symtab objects only. The > default for this keyword (i.e, no keyword provided) is to search all > symbol tables. > > All of the examples above have "" as the search pattern but you can, > of course, specify any valid GDB regex there. > > On a review note I seek a little advice on the interface to > search_symbols. If the user does provide a symtabs keyword, the code > collects the name of the file attached to each symtab specified. It > does that through the usual Python method > (python_string_to_target_string). That returns a > gdb::unique_xmalloc_ptr<char>. I planned to store these in a > std::vector, however, I was unable to coerce a > std::vector<gdb::unique_xmalloc_ptr<char>> into a char **p[] > representation the search_symbols function requires. I ended up > releasing the object (that results in a char *) and wrapping the > std::vector in a simple type that provides a destructor to properly > free the released strings when the object falls out of scope. I > thought about just changing the search_symbols interface to accept a > vector of (gdb) unique pointers but noted a C++ run had already been > performed over the function. Any thoughts here would be appreciated. > > Long, windy, preamble over the patch follows at the end of the email. > > Cheers > > Phil > > -- > > 2017-10-11 Phil Muldoon <pmuldoon@redhat.com> > > * python/python.c (gdbpy_rbreak): New function. > > 2017-10-11 Phil Muldoon <pmuldoon@redhat.com> > > * gdb.python/py-rbreak.exp: New file. > * gdb.python/py-rbreak.c: New file. > * gdb.python/py-rbreak-func2.c: New file. > > 2017-10-11 Phil Muldoon <pmuldoon@redhat.com> > > * python.texi (Basic Python): Add rbreak documentation. > > -- > > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi > index f661e489bb..b2d4516886 100644 > --- a/gdb/doc/python.texi > +++ b/gdb/doc/python.texi > @@ -243,6 +243,24 @@ were no breakpoints. This peculiarity was subsequently fixed, and now > @code{gdb.breakpoints} returns an empty sequence in this case. > @end defun > > +@findex gdb.rbreak > +@defun gdb.rbreak (regex @r{[}, minisyms @r{[}, throttle, @r{[}, symtabs @r{]]]}) > +Return a Python tuple holding a collection of newly set > +@code{gdb.Breakpoint} objects matching function names defined by the > +@var{regex} pattern. If the @var{minisyms} keyword is @code{True}, > +all system functions (those not explicitly defined in the inferior) > +will also be included in the match. The @var{throttle} keyword takes > +an integer that defines the maximum number of pattern matches for > +functions matched by the @var{regex} pattern. If the number of > +matches exceeds the integer value of @var{throttle}, a > +@code{RuntimeError} will be raised and no breakpoints will be created. > +If @var{throttle} is not defined then there is no imposed limit on the > +maximum number of matches and breakpoints to be created. The > +@var{symtabs} keyword takes a Python iterable that yields a collection > +of @code{gdb.Symtab} objects and will restrict the search to those > +functions only contained within the @code{gdb.Symtab} objects. > +@end defun > + > @findex gdb.parameter > @defun gdb.parameter (parameter) > Return the value of a @value{GDBN} @var{parameter} given by its name, > diff --git a/gdb/python/python.c b/gdb/python/python.c > index b04057ec4a..4d591df30c 100644 > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -642,6 +642,191 @@ gdbpy_solib_name (PyObject *self, PyObject *args) > return str_obj; > } > > +/* Implementation of Python rbreak command. Take a REGEX and > + optionally a MINISYMS, THROTTLE and SYMTABS keyword and return a > + Python tuple that contains newly set breakpoints that match that > + criteria. REGEX refers to a GDB format standard regex pattern of > + symbols names to search; MINISYMS is an optional boolean (default > + False) that indicates if the function should search GDB's minimal > + symbols; THROTTLE is an optional integer (default unlimited) that > + indicates the maximum amount of breakpoints allowable before the > + function exits (note, if the throttle bound is passed, no > + breakpoints will be set and a runtime error returned); SYMTABS is > + an optional iterator that contains a set of gdb.Symtabs to iterator or iterable? It would make sense to be able to pass a list here, for example. > + constrain the search within. */ > + > +static PyObject * > +gdbpy_rbreak (PyObject *self, PyObject *args, PyObject *kw) > +{ > + /* A simple type to ensure clean up of a vector of allocated strings > + when a C interface demands a const char *array[] type > + interface. */ > + struct symtab_list_type > + { > + ~symtab_list_type () > + { > + for (const char *elem: vec) > + xfree ((void *) elem); > + } > + std::vector<const char *> vec; > + }; > + > + char *regex = NULL; > + std::vector<symbol_search> symbols; > + unsigned long count = 0; > + PyObject *symtab_list = NULL; > + PyObject *minisyms_p_obj = NULL; > + int minisyms_p = 0; > + unsigned int throttle = 0; > + static const char *keywords[] = {"regex","minisyms", "throttle", "symtabs", NULL}; Nit: line too long and missing space. > + symtab_list_type symtab_paths; > + > + if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|O!IO", keywords, > + ®ex, &PyBool_Type, > + &minisyms_p_obj, &throttle, > + &symtab_list)) > + return NULL; > + > + /* Parse minisyms keyword. */ > + if (minisyms_p_obj != NULL) > + { > + int cmp = PyObject_IsTrue (minisyms_p_obj); > + if (cmp < 0) > + return NULL; > + minisyms_p = cmp; > + } > + > + /* The "symtabs" keyword is any Python iterable object that returns > + a gdb.Symtab on each iteration. If specified, iterate through > + the provided gdb.Symtabs and extract their full path. As > + python_string_to_target_string returns a > + gdb::unique_xmalloc_ptr<char> and a vector containing these types > + cannot be coerced to a const char **p[] via the vector.data call, > + release the value from the unique_xmalloc_ptr and place it in a > + simple type symtab_list_type (which holds the vector and a > + destructor that frees the contents of the allocated strings. */ > + if (symtab_list != NULL) > + { > + gdbpy_ref<> iter (PyObject_GetIter (symtab_list)); > + > + if (iter == NULL) > + return NULL; > + > + while (true) > + { > + gdbpy_ref<> next (PyIter_Next (iter.get ())); > + > + if (next == NULL) > + { > + if (PyErr_Occurred ()) > + return NULL; > + break; > + } > + > + gdbpy_ref<> obj_name (PyObject_GetAttrString (next.get (), > + "filename")); > + > + if (obj_name == NULL) > + return NULL; > + > + /* Is the object file still valid? */ > + if (obj_name == Py_None) > + continue; > + > + gdb::unique_xmalloc_ptr<char> filename = > + python_string_to_target_string (obj_name.get ()); > + > + if (filename == NULL) > + return NULL; > + > + /* Make sure there is a definite place to store the value of > + s before it is released. */ > + symtab_paths.vec.push_back (nullptr); > + symtab_paths.vec.back () = filename.release (); > + } > + } > + > + if (symtab_list) > + { > + const char **files = symtab_paths.vec.data (); > + > + symbols = search_symbols (regex, FUNCTIONS_DOMAIN, > + symtab_paths.vec.size (), files); > + } > + else > + symbols = search_symbols (regex, FUNCTIONS_DOMAIN, 0, NULL); > + > + /* Count the number of symbols (both symbols and optionally mini > + symbols) so we can correctly size the tuple. */ > + for (const symbol_search &p : symbols) > + { > + /* Mini symbols included? */ > + if (minisyms_p) > + { > + if (p.msymbol.minsym != NULL) > + count++; > + } Would it be easy to pass the minisyms_p flag to search_symbols, so that we don't need to search in the minsym tables if we don't even care about them? > + > + if (p.symbol != NULL) > + count++; > + } > + > + /* Check throttle bounds and exit if in excess. */ > + if (throttle != 0 && count > throttle) > + { > + PyErr_SetString (PyExc_RuntimeError, > + _("Number of breakpoints exceeds throttled maximum.")); > + return NULL; > + } > + > + gdbpy_ref<> return_tuple (PyTuple_New (count)); > + > + if (return_tuple == NULL) > + return NULL; How do you decide if this function should return a tuple or a list? Instinctively I would have returned a list, but I can't really explain why. > + > + count = 0; > + > + /* Construct full path names for symbols and call the Python > + breakpoint constructor on the resulting names. Be tolerant of > + individual breakpoint failures. */ > + for (const symbol_search &p : symbols) > + { > + std::string symbol_name; > + > + /* Skipping mini-symbols? */ > + if (minisyms_p == 0) > + if (p.msymbol.minsym != NULL) > + continue; > + > + if (p.msymbol.minsym == NULL) > + { > + struct symtab *symtab = symbol_symtab (p.symbol); > + const char *fullname = symtab_to_fullname (symtab); > + > + symbol_name = fullname; > + symbol_name += ":"; > + symbol_name += SYMBOL_LINKAGE_NAME (p.symbol); > + } > + else > + symbol_name = MSYMBOL_LINKAGE_NAME (p.msymbol.minsym); > + > + gdbpy_ref<> argList (Py_BuildValue("(s)", symbol_name.c_str ())); > + gdbpy_ref<> obj (PyObject_CallObject ((PyObject *) > + &breakpoint_object_type, > + argList.get ())); > + > + /* Tolerate individual breakpoint failures. */ > + if (obj == NULL) > + gdbpy_print_stack (); > + else > + { > + PyTuple_SET_ITEM (return_tuple.get (), count, obj.get ()); The Python doc says that SET_ITEM steals a reference to obj. Isn't it a problem, because gdbpy_ref also keeps the reference? > + count++; > + } > + } Hmm maybe this is a reason to use a list? If a breakpoint fails to be created, the tuple will not be filled completely. What happens to tuple elements that were not set? With the list, you can simply PyList_Append. > + return return_tuple.release (); > +} > + > /* A Python function which is a wrapper for decode_line_1. */ > > static PyObject * > @@ -1912,7 +2097,9 @@ Return the name of the current target charset." }, > { "target_wide_charset", gdbpy_target_wide_charset, METH_NOARGS, > "target_wide_charset () -> string.\n\ > Return the name of the current target wide charset." }, > - > + { "rbreak", (PyCFunction) gdbpy_rbreak, METH_VARARGS | METH_KEYWORDS, > + "rbreak (Regex) -> Tuple.\n\ > +Return a Tuple containing gdb.Breakpoint objects that match the given Regex." }, > { "string_to_argv", gdbpy_string_to_argv, METH_VARARGS, > "string_to_argv (String) -> Array.\n\ > Parse String and return an argv-like array.\n\ > diff --git a/gdb/testsuite/gdb.python/py-rbreak-func2.c b/gdb/testsuite/gdb.python/py-rbreak-func2.c > new file mode 100644 > index 0000000000..1de8bfd5b2 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-rbreak-func2.c > @@ -0,0 +1,31 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2017 Free Software Foundation, Inc. > + > + 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/>. */ > + > +int efunc1 () > +{ > + return 1; > +} > + > +int efunc2 () > +{ > + return 2; > +} > + > +int efunc3 () > +{ > + return 3; > +} > diff --git a/gdb/testsuite/gdb.python/py-rbreak.c b/gdb/testsuite/gdb.python/py-rbreak.c > new file mode 100644 > index 0000000000..287aba6c03 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-rbreak.c > @@ -0,0 +1,62 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2013-2017 Free Software Foundation, Inc. > + > + 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/>. */ > + > +int func1 () As for GDB code, put the return type on its own line. > +{ > + return 1; > +} > + > +int func2 () > +{ > + return 2; > +} > + > +int func3 () > +{ > + return 3; > +} > + > +int func4 () > +{ > + return 4; > +} > + > +int func5 () > +{ > + return 5; > +} > + > +void func6 () > +{ > + return; > +} > + > +void outside_scope () > +{ > + return; > +} > + > +int main() > +{ > + func1 (); /* Break func1. */ > + func2 (); > + func3 (); > + func4 (); > + func5 (); > + func6 (); > + outside_scope (); > +} > diff --git a/gdb/testsuite/gdb.python/py-rbreak.exp b/gdb/testsuite/gdb.python/py-rbreak.exp > new file mode 100644 > index 0000000000..9385aeae08 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-rbreak.exp > @@ -0,0 +1,61 @@ > +# Copyright (C) 2017 Free Software Foundation, Inc. > +# > +# 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/>. > + > +# This file is part of the GDB testsuite. It tests the mechanism > +# exposing values to Python. > + > +load_lib gdb-python.exp > + > +standard_testfile py-rbreak.c py-rbreak-func2.c > + > +if {[prepare_for_testing "failed to prepare" ${testfile} [list $srcfile $srcfile2]] } { > + return 1 > +} > + > +# Skip all tests if Python scripting is not enabled. > +if { [skip_python_tests] } { continue } > + > +if ![runto_main] then { > + fail "can't run to main" > + return 0 > +} > + > +gdb_py_test_silent_cmd "py sl = gdb.rbreak(\"\",minisyms=False)" \ > + "Get all function breakpoints" 0 > +gdb_test "py print(len(sl))" "11" \ > + "Check number of returned breakpoints is 11" > +gdb_py_test_silent_cmd "py sl = gdb.rbreak(\"main\.\*\",minisyms=False)" \ > + "Get main function breakpoint" 0 > +gdb_test "py print(len(sl))" "1" \ > + "Check number of returned breakpoints is 1" > +gdb_py_test_silent_cmd "py sl = gdb.rbreak(\"func\.\*\",minisyms=False,throttle=10)" \ > + "Get functions matching func.*" 0 > +gdb_test "py print(len(sl))" "9" \ > + "Check number of returned breakpoints is 9" > +gdb_test "py gdb.rbreak(\"func\.\*\",minisyms=False,throttle=1)" \ > + "Number of breakpoints exceeds throttled maximum.*" \ > + "Check throttle errors on too many breakpoints." > +gdb_py_test_silent_cmd "py sl = gdb.rbreak(\"func1\",minisyms=True)" \ > + "Including mini symbols, get functions matching func.*" 0 > +gdb_test "py print(len(sl))" "2" \ > + "Check number of returned breakpoints is 2" > +gdb_py_test_silent_cmd "python sym = gdb.lookup_symbol(\"efunc1\")" \ > + "Find a symbol in objfile" 1 > +gdb_py_test_silent_cmd "python symtab = sym\[0\].symtab" \ > + "Get backing symbol table" 1 > +gdb_py_test_silent_cmd "py sl = gdb.rbreak(\"func\.\*\",minisyms=False,throttle=10,symtabs=\[symtab\])" \ > + "Get functions matching func.* in one symtab only." 0 > +gdb_test "py print(len(sl))" "3" \ > + "Check number of returned breakpoints is 3" > I can't find a reference, but I think we want test names to start with a lower case letter and not end with a dot. I'll see if we can add this to the testcase cookbook wiki page. Simon
On 16/10/17 23:22, Simon Marchi wrote: > On 2017-10-11 07:30 AM, Phil Muldoon wrote: > Hi Phil, > > As Kevin noted (IIUC), this should be "minsyms" if we want to follow standard > GDB terminology. I've already fixed this up locally after Kevin noted it. So just noting that here. >> That would place a breakpoint on all functions that are actually >> defined in the inferior (and not those that are inserted by the >> compiler, linker, etc). The default for this keyword is False. >> >> The second tuneable is a throttle. Beyond the name (which I am unsure >> about but could not think of a better one), this allows the user to >> enter a fail-safe limit for breakpoint creation. So, for the following >> example, an inferior with ten user provided functions: >> >> gdb.rbreak ("", minisyms=False, throttle=5) > > max_results? max_breakpoints? I've no preference. I tried to imply in the keyword that if the maximum was reached no breakpoints would be set. max_breakpoints, I thought, implies that "if the maximum is reached breakpoints would be set up to that limit." I've no strong opinion on this name, so if you do, let me know. > +/* Implementation of Python rbreak command. Take a REGEX and >> + optionally a MINISYMS, THROTTLE and SYMTABS keyword and return a >> + Python tuple that contains newly set breakpoints that match that >> + criteria. REGEX refers to a GDB format standard regex pattern of >> + symbols names to search; MINISYMS is an optional boolean (default >> + False) that indicates if the function should search GDB's minimal >> + symbols; THROTTLE is an optional integer (default unlimited) that >> + indicates the maximum amount of breakpoints allowable before the >> + function exits (note, if the throttle bound is passed, no >> + breakpoints will be set and a runtime error returned); SYMTABS is >> + an optional iterator that contains a set of gdb.Symtabs to > > iterator or iterable? It would make sense to be able to pass a list here, > for example. The Python function does not care what you pass it: tuple, list, whatever, as long as it is iterable. So iterable is probably right here. >> + static const char *keywords[] = {"regex","minisyms", "throttle", "symtabs", NULL}; > > Nit: line too long and missing space. Thanks for catching both nits. >> + for (const symbol_search &p : symbols) >> + { >> + /* Mini symbols included? */ >> + if (minisyms_p) >> + { >> + if (p.msymbol.minsym != NULL) >> + count++; >> + } > > Would it be easy to pass the minisyms_p flag to search_symbols, so that > we don't need to search in the minsym tables if we don't even care about > them? I thought about it. But instead of refactoring search_symbols to be more selective, I wanted this patch to focus on Pythonic rbreak and the added functionality it provides. I can change search_symbols, I've no problem with that, but in a separate, more focused patch? >> + gdbpy_ref<> return_tuple (PyTuple_New (count)); >> + >> + if (return_tuple == NULL) >> + return NULL; > > How do you decide if this function should return a tuple or a list? > Instinctively I would have returned a list, but I can't really explain > why. I tend to think any collection a Python function returns normally should be a tuple. Tuple's are immutable. That's the only reason why. We have to count the symbols anyway to check the "throttle" feature and, as we know the size of the array, I thought we might as well make it a tuple. >> + /* Tolerate individual breakpoint failures. */ >> + if (obj == NULL) >> + gdbpy_print_stack (); >> + else >> + { >> + PyTuple_SET_ITEM (return_tuple.get (), count, obj.get ()); > > The Python doc says that SET_ITEM steals a reference to obj. Isn't it > a problem, because gdbpy_ref also keeps the reference? Sorry for the noise. I already self-caught this and I'm puzzled how it got through (really, the tests should have failed as the objects would have been garbage collected). But, already fixed. See: https://sourceware.org/ml/gdb-patches/2017-10/msg00341.html > Hmm maybe this is a reason to use a list? If a breakpoint fails to > be created, the tuple will not be filled completely. What happens > to tuple elements that were not set? > > With the list, you can simply PyList_Append. That's a good reason. I remember in a lot of other functions I've written in the past I used PyList_AsTuple. I'm a bit worried about that, though, as we could be dealing with thousands of breakpoints. >> +int func1 () > > As for GDB code, put the return type on its own line. I'll change this, it's not a problem, but I thought there was a large degree of largess granted to testcase files with the idea that "GDB has to work on real life (often messy) code." > I can't find a reference, but I think we want test names to start > with a lower case letter and not end with a dot. I'll see if we > can add this to the testcase cookbook wiki page. As I mentioned on IRC, I've not heard of it but will happily change the names to comply. Cheers, Phil
On 2017-10-16 19:01, Phil Muldoon wrote: >>> That would place a breakpoint on all functions that are actually >>> defined in the inferior (and not those that are inserted by the >>> compiler, linker, etc). The default for this keyword is False. >>> >>> The second tuneable is a throttle. Beyond the name (which I am unsure >>> about but could not think of a better one), this allows the user to >>> enter a fail-safe limit for breakpoint creation. So, for the >>> following >>> example, an inferior with ten user provided functions: >>> >>> gdb.rbreak ("", minisyms=False, throttle=5) >> >> max_results? max_breakpoints? > > I've no preference. I tried to imply in the keyword that if the > maximum was reached no breakpoints would be set. max_breakpoints, I > thought, implies that "if the maximum is reached breakpoints would be > set up to that limit." I've no strong opinion on this name, so if you > do, let me know. Doesn't throttle imply the same thing? I understand it as something that caps at a certain level. I don't have a strong opinion, it just struck me as a not very common name to use for these kinds of things. The important thing is that it's documented properly. >>> + for (const symbol_search &p : symbols) >>> + { >>> + /* Mini symbols included? */ >>> + if (minisyms_p) >>> + { >>> + if (p.msymbol.minsym != NULL) >>> + count++; >>> + } >> >> Would it be easy to pass the minisyms_p flag to search_symbols, so >> that >> we don't need to search in the minsym tables if we don't even care >> about >> them? > > I thought about it. But instead of refactoring search_symbols to be > more selective, I wanted this patch to focus on Pythonic rbreak and > the added functionality it provides. I can change search_symbols, I've > no problem with that, but in a separate, more focused patch? That's fine with me. >>> + gdbpy_ref<> return_tuple (PyTuple_New (count)); >>> + >>> + if (return_tuple == NULL) >>> + return NULL; >> >> How do you decide if this function should return a tuple or a list? >> Instinctively I would have returned a list, but I can't really explain >> why. > > I tend to think any collection a Python function returns normally > should be a tuple. Tuple's are immutable. That's the only reason > why. We have to count the symbols anyway to check the "throttle" > feature and, as we know the size of the array, I thought we might as > well make it a tuple. Ok. >>> + /* Tolerate individual breakpoint failures. */ >>> + if (obj == NULL) >>> + gdbpy_print_stack (); >>> + else >>> + { >>> + PyTuple_SET_ITEM (return_tuple.get (), count, obj.get ()); >> >> The Python doc says that SET_ITEM steals a reference to obj. Isn't it >> a problem, because gdbpy_ref also keeps the reference? > > Sorry for the noise. I already self-caught this and I'm puzzled how it > got through (really, the tests should have failed as the objects would > have been garbage collected). But, already fixed. See: > > https://sourceware.org/ml/gdb-patches/2017-10/msg00341.html Ah sorry. I read that message before reviewing the patch, but because I didn't have the context then, it just flew over my head. >> Hmm maybe this is a reason to use a list? If a breakpoint fails to >> be created, the tuple will not be filled completely. What happens >> to tuple elements that were not set? >> >> With the list, you can simply PyList_Append. > > That's a good reason. I remember in a lot of other functions I've > written in the past I used PyList_AsTuple. I'm a bit worried about > that, though, as we could be dealing with thousands of breakpoints. As a Python user, I would have no problem with the API returning a list. 'hello you'.split() returns a list, for example. >>> +int func1 () >> >> As for GDB code, put the return type on its own line. > > I'll change this, it's not a problem, but I thought there was a large > degree of largess granted to testcase files with the idea that "GDB > has to work on real life (often messy) code." As with the other "rule" below, I don't think it's written anywhere, but that's what I have seen in reviews since I've started contributing to GDB. I'll add this to the wiki as well. >> I can't find a reference, but I think we want test names to start >> with a lower case letter and not end with a dot. I'll see if we >> can add this to the testcase cookbook wiki page. > > As I mentioned on IRC, I've not heard of it but will happily change > the names to comply. > > Cheers, > > Phil Thanks, Simon
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi index f661e489bb..b2d4516886 100644 --- a/gdb/doc/python.texi +++ b/gdb/doc/python.texi @@ -243,6 +243,24 @@ were no breakpoints. This peculiarity was subsequently fixed, and now @code{gdb.breakpoints} returns an empty sequence in this case. @end defun +@findex gdb.rbreak +@defun gdb.rbreak (regex @r{[}, minisyms @r{[}, throttle, @r{[}, symtabs @r{]]]}) +Return a Python tuple holding a collection of newly set +@code{gdb.Breakpoint} objects matching function names defined by the +@var{regex} pattern. If the @var{minisyms} keyword is @code{True}, +all system functions (those not explicitly defined in the inferior) +will also be included in the match. The @var{throttle} keyword takes +an integer that defines the maximum number of pattern matches for +functions matched by the @var{regex} pattern. If the number of +matches exceeds the integer value of @var{throttle}, a +@code{RuntimeError} will be raised and no breakpoints will be created. +If @var{throttle} is not defined then there is no imposed limit on the +maximum number of matches and breakpoints to be created. The +@var{symtabs} keyword takes a Python iterable that yields a collection +of @code{gdb.Symtab} objects and will restrict the search to those +functions only contained within the @code{gdb.Symtab} objects. +@end defun + @findex gdb.parameter @defun gdb.parameter (parameter) Return the value of a @value{GDBN} @var{parameter} given by its name, diff --git a/gdb/python/python.c b/gdb/python/python.c index b04057ec4a..4d591df30c 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -642,6 +642,191 @@ gdbpy_solib_name (PyObject *self, PyObject *args) return str_obj; } +/* Implementation of Python rbreak command. Take a REGEX and + optionally a MINISYMS, THROTTLE and SYMTABS keyword and return a + Python tuple that contains newly set breakpoints that match that + criteria. REGEX refers to a GDB format standard regex pattern of + symbols names to search; MINISYMS is an optional boolean (default + False) that indicates if the function should search GDB's minimal + symbols; THROTTLE is an optional integer (default unlimited) that + indicates the maximum amount of breakpoints allowable before the + function exits (note, if the throttle bound is passed, no + breakpoints will be set and a runtime error returned); SYMTABS is + an optional iterator that contains a set of gdb.Symtabs to + constrain the search within. */ + +static PyObject * +gdbpy_rbreak (PyObject *self, PyObject *args, PyObject *kw) +{ + /* A simple type to ensure clean up of a vector of allocated strings + when a C interface demands a const char *array[] type + interface. */ + struct symtab_list_type + { + ~symtab_list_type () + { + for (const char *elem: vec) + xfree ((void *) elem); + } + std::vector<const char *> vec; + }; + + char *regex = NULL; + std::vector<symbol_search> symbols; + unsigned long count = 0; + PyObject *symtab_list = NULL; + PyObject *minisyms_p_obj = NULL; + int minisyms_p = 0; + unsigned int throttle = 0; + static const char *keywords[] = {"regex","minisyms", "throttle", "symtabs", NULL}; + symtab_list_type symtab_paths; + + if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|O!IO", keywords, + ®ex, &PyBool_Type, + &minisyms_p_obj, &throttle, + &symtab_list)) + return NULL; + + /* Parse minisyms keyword. */ + if (minisyms_p_obj != NULL) + { + int cmp = PyObject_IsTrue (minisyms_p_obj); + if (cmp < 0) + return NULL; + minisyms_p = cmp; + } + + /* The "symtabs" keyword is any Python iterable object that returns + a gdb.Symtab on each iteration. If specified, iterate through + the provided gdb.Symtabs and extract their full path. As + python_string_to_target_string returns a + gdb::unique_xmalloc_ptr<char> and a vector containing these types + cannot be coerced to a const char **p[] via the vector.data call, + release the value from the unique_xmalloc_ptr and place it in a + simple type symtab_list_type (which holds the vector and a + destructor that frees the contents of the allocated strings. */ + if (symtab_list != NULL) + { + gdbpy_ref<> iter (PyObject_GetIter (symtab_list)); + + if (iter == NULL) + return NULL; + + while (true) + { + gdbpy_ref<> next (PyIter_Next (iter.get ())); + + if (next == NULL) + { + if (PyErr_Occurred ()) + return NULL; + break; + } + + gdbpy_ref<> obj_name (PyObject_GetAttrString (next.get (), + "filename")); + + if (obj_name == NULL) + return NULL; + + /* Is the object file still valid? */ + if (obj_name == Py_None) + continue; + + gdb::unique_xmalloc_ptr<char> filename = + python_string_to_target_string (obj_name.get ()); + + if (filename == NULL) + return NULL; + + /* Make sure there is a definite place to store the value of + s before it is released. */ + symtab_paths.vec.push_back (nullptr); + symtab_paths.vec.back () = filename.release (); + } + } + + if (symtab_list) + { + const char **files = symtab_paths.vec.data (); + + symbols = search_symbols (regex, FUNCTIONS_DOMAIN, + symtab_paths.vec.size (), files); + } + else + symbols = search_symbols (regex, FUNCTIONS_DOMAIN, 0, NULL); + + /* Count the number of symbols (both symbols and optionally mini + symbols) so we can correctly size the tuple. */ + for (const symbol_search &p : symbols) + { + /* Mini symbols included? */ + if (minisyms_p) + { + if (p.msymbol.minsym != NULL) + count++; + } + + if (p.symbol != NULL) + count++; + } + + /* Check throttle bounds and exit if in excess. */ + if (throttle != 0 && count > throttle) + { + PyErr_SetString (PyExc_RuntimeError, + _("Number of breakpoints exceeds throttled maximum.")); + return NULL; + } + + gdbpy_ref<> return_tuple (PyTuple_New (count)); + + if (return_tuple == NULL) + return NULL; + + count = 0; + + /* Construct full path names for symbols and call the Python + breakpoint constructor on the resulting names. Be tolerant of + individual breakpoint failures. */ + for (const symbol_search &p : symbols) + { + std::string symbol_name; + + /* Skipping mini-symbols? */ + if (minisyms_p == 0) + if (p.msymbol.minsym != NULL) + continue; + + if (p.msymbol.minsym == NULL) + { + struct symtab *symtab = symbol_symtab (p.symbol); + const char *fullname = symtab_to_fullname (symtab); + + symbol_name = fullname; + symbol_name += ":"; + symbol_name += SYMBOL_LINKAGE_NAME (p.symbol); + } + else + symbol_name = MSYMBOL_LINKAGE_NAME (p.msymbol.minsym); + + gdbpy_ref<> argList (Py_BuildValue("(s)", symbol_name.c_str ())); + gdbpy_ref<> obj (PyObject_CallObject ((PyObject *) + &breakpoint_object_type, + argList.get ())); + + /* Tolerate individual breakpoint failures. */ + if (obj == NULL) + gdbpy_print_stack (); + else + { + PyTuple_SET_ITEM (return_tuple.get (), count, obj.get ()); + count++; + } + } + return return_tuple.release (); +} + /* A Python function which is a wrapper for decode_line_1. */ static PyObject * @@ -1912,7 +2097,9 @@ Return the name of the current target charset." }, { "target_wide_charset", gdbpy_target_wide_charset, METH_NOARGS, "target_wide_charset () -> string.\n\ Return the name of the current target wide charset." }, - + { "rbreak", (PyCFunction) gdbpy_rbreak, METH_VARARGS | METH_KEYWORDS, + "rbreak (Regex) -> Tuple.\n\ +Return a Tuple containing gdb.Breakpoint objects that match the given Regex." }, { "string_to_argv", gdbpy_string_to_argv, METH_VARARGS, "string_to_argv (String) -> Array.\n\ Parse String and return an argv-like array.\n\ diff --git a/gdb/testsuite/gdb.python/py-rbreak-func2.c b/gdb/testsuite/gdb.python/py-rbreak-func2.c new file mode 100644 index 0000000000..1de8bfd5b2 --- /dev/null +++ b/gdb/testsuite/gdb.python/py-rbreak-func2.c @@ -0,0 +1,31 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2017 Free Software Foundation, Inc. + + 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/>. */ + +int efunc1 () +{ + return 1; +} + +int efunc2 () +{ + return 2; +} + +int efunc3 () +{ + return 3; +} diff --git a/gdb/testsuite/gdb.python/py-rbreak.c b/gdb/testsuite/gdb.python/py-rbreak.c new file mode 100644 index 0000000000..287aba6c03 --- /dev/null +++ b/gdb/testsuite/gdb.python/py-rbreak.c @@ -0,0 +1,62 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2013-2017 Free Software Foundation, Inc. + + 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/>. */ + +int func1 () +{ + return 1; +} + +int func2 () +{ + return 2; +} + +int func3 () +{ + return 3; +} + +int func4 () +{ + return 4; +} + +int func5 () +{ + return 5; +} + +void func6 () +{ + return; +} + +void outside_scope () +{ + return; +} + +int main() +{ + func1 (); /* Break func1. */ + func2 (); + func3 (); + func4 (); + func5 (); + func6 (); + outside_scope (); +} diff --git a/gdb/testsuite/gdb.python/py-rbreak.exp b/gdb/testsuite/gdb.python/py-rbreak.exp new file mode 100644 index 0000000000..9385aeae08 --- /dev/null +++ b/gdb/testsuite/gdb.python/py-rbreak.exp @@ -0,0 +1,61 @@ +# Copyright (C) 2017 Free Software Foundation, Inc. +# +# 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/>. + +# This file is part of the GDB testsuite. It tests the mechanism +# exposing values to Python. + +load_lib gdb-python.exp + +standard_testfile py-rbreak.c py-rbreak-func2.c + +if {[prepare_for_testing "failed to prepare" ${testfile} [list $srcfile $srcfile2]] } { + return 1 +} + +# Skip all tests if Python scripting is not enabled. +if { [skip_python_tests] } { continue } + +if ![runto_main] then { + fail "can't run to main" + return 0 +} + +gdb_py_test_silent_cmd "py sl = gdb.rbreak(\"\",minisyms=False)" \ + "Get all function breakpoints" 0 +gdb_test "py print(len(sl))" "11" \ + "Check number of returned breakpoints is 11" +gdb_py_test_silent_cmd "py sl = gdb.rbreak(\"main\.\*\",minisyms=False)" \ + "Get main function breakpoint" 0 +gdb_test "py print(len(sl))" "1" \ + "Check number of returned breakpoints is 1" +gdb_py_test_silent_cmd "py sl = gdb.rbreak(\"func\.\*\",minisyms=False,throttle=10)" \ + "Get functions matching func.*" 0 +gdb_test "py print(len(sl))" "9" \ + "Check number of returned breakpoints is 9" +gdb_test "py gdb.rbreak(\"func\.\*\",minisyms=False,throttle=1)" \ + "Number of breakpoints exceeds throttled maximum.*" \ + "Check throttle errors on too many breakpoints." +gdb_py_test_silent_cmd "py sl = gdb.rbreak(\"func1\",minisyms=True)" \ + "Including mini symbols, get functions matching func.*" 0 +gdb_test "py print(len(sl))" "2" \ + "Check number of returned breakpoints is 2" +gdb_py_test_silent_cmd "python sym = gdb.lookup_symbol(\"efunc1\")" \ + "Find a symbol in objfile" 1 +gdb_py_test_silent_cmd "python symtab = sym\[0\].symtab" \ + "Get backing symbol table" 1 +gdb_py_test_silent_cmd "py sl = gdb.rbreak(\"func\.\*\",minisyms=False,throttle=10,symtabs=\[symtab\])" \ + "Get functions matching func.* in one symtab only." 0 +gdb_test "py print(len(sl))" "3" \ + "Check number of returned breakpoints is 3"
This introduces a Python rbreak function to the Python API. Its functionality was designed to closely match that of the console rbreak but with a number of caveats. That being said, there's a bit of preamble first. My original design was to have the gdb.Breakpoint constructor take a regex pattern. While breakpoints can, and do, have multiple locations they are always tied in with one linespec. I looked at the linespec/location code and the relationship is pretty firmly embedded as a one to many relationship. One linespec can have multiple locations but only one linespec can be represented in the location object. (Or address or probe or whatever the location is representing). A linespec has to be definitive; it has to point to one symbol. I thought about modifying this to have multiple symbols and multiple locations but quickly decided this was really going to violate the design contract of linespecs/location. It would also be considerable and disruptive work. I looked then, again, at a breakpoint having multiple linespecs but again decided the amount of disruption this would generate was not worth a Python rbreak returning one breakpoint representing multiple symbols and multiple locations. So, instead, I've made the Python rbreak functionality a little more tuneable than the console command equivalent. The first tuneable is to allow the user to exclude mini symbols from the pattern matching. A loosely written pattern passed to the console rbreak command can generate 5000+ breakpoints from a simple C "hello world" program. rbreak "" will place a breakpoint on every single mini symbol representing a function. I can't possibly see why this would be desirable. In order to facilitate broadly-written patterns that seek to place a breakpoint on every user-defined function, the equivalent would be: gdb.rbreak ("", minisyms=False) That would place a breakpoint on all functions that are actually defined in the inferior (and not those that are inserted by the compiler, linker, etc). The default for this keyword is False. The second tuneable is a throttle. Beyond the name (which I am unsure about but could not think of a better one), this allows the user to enter a fail-safe limit for breakpoint creation. So, for the following example, an inferior with ten user provided functions: gdb.rbreak ("", minisyms=False, throttle=5) Would set no breakpoints and raise a runtime error. This functionality is more aimed at those who want to utilise Python rbreak from a toolset perspective (say for a GDB UI). The default for this keyword is unlimited. The last tuneable is the ability to limit the pattern matching to a set of gdb.Symtab objects. So, given the example below: gdb.rbreak ("", minisyms=False, throttle=5, symtabs=symTuple) where "symTuple" is a tuple of gdb.Symtab objects, the search/pattern matching would be restricted to those gdb.Symtab objects only. The default for this keyword (i.e, no keyword provided) is to search all symbol tables. All of the examples above have "" as the search pattern but you can, of course, specify any valid GDB regex there. On a review note I seek a little advice on the interface to search_symbols. If the user does provide a symtabs keyword, the code collects the name of the file attached to each symtab specified. It does that through the usual Python method (python_string_to_target_string). That returns a gdb::unique_xmalloc_ptr<char>. I planned to store these in a std::vector, however, I was unable to coerce a std::vector<gdb::unique_xmalloc_ptr<char>> into a char **p[] representation the search_symbols function requires. I ended up releasing the object (that results in a char *) and wrapping the std::vector in a simple type that provides a destructor to properly free the released strings when the object falls out of scope. I thought about just changing the search_symbols interface to accept a vector of (gdb) unique pointers but noted a C++ run had already been performed over the function. Any thoughts here would be appreciated. Long, windy, preamble over the patch follows at the end of the email. Cheers Phil -- 2017-10-11 Phil Muldoon <pmuldoon@redhat.com> * python/python.c (gdbpy_rbreak): New function. 2017-10-11 Phil Muldoon <pmuldoon@redhat.com> * gdb.python/py-rbreak.exp: New file. * gdb.python/py-rbreak.c: New file. * gdb.python/py-rbreak-func2.c: New file. 2017-10-11 Phil Muldoon <pmuldoon@redhat.com> * python.texi (Basic Python): Add rbreak documentation. --