Re: [python][patch] Python rbreak

Simon Marchi
2017-10-17T00:24:38Z

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