Re: [python][patch] Python rbreak

[Phil Muldoon <pmuldoon at redhat dot com>]

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