Re: [python][patch] Python rbreak

Phil Muldoon
2017-10-16T23:01:21Z

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