This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [python][patch] Python rbreak
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