Re: [python][patch] Python rbreak

[Simon Marchi <simon.marchi@polymtl.ca>]

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