Re: [python][patch] Python rbreak

[Simon Marchi <simon dot marchi at ericsson dot com>]

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,
> +					&regex, &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