From: Phil Muldoon <pmuldoon@redhat.com>
To: Simon Marchi <simon.marchi@ericsson.com>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [python][patch] Python rbreak
Date: Mon, 16 Oct 2017 23:01:00 -0000 [thread overview]
Message-ID: <5e1ba7e3-5f6e-2478-30a5-7670ec7a9879@redhat.com> (raw)
In-Reply-To: <e3c7c9b4-131a-f237-6392-766a65c336f9@ericsson.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
next prev parent reply other threads:[~2017-10-16 23:01 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-11 11:30 Phil Muldoon
2017-10-11 12:11 ` Eli Zaretskii
2017-10-11 12:27 ` Phil Muldoon
2017-10-11 16:19 ` Kevin Buettner
2017-10-11 16:24 ` Phil Muldoon
2017-10-13 8:08 ` Phil Muldoon
2017-10-16 22:22 ` Simon Marchi
2017-10-16 23:01 ` Phil Muldoon [this message]
2017-10-17 0:24 ` Simon Marchi
2017-11-03 9:46 ` Phil Muldoon
2017-11-03 10:05 ` Eli Zaretskii
2017-11-13 19:29 ` Phil Muldoon
2018-02-01 9:47 ` [RFA/RFC] Clarify contents of NEWS entry re: Python "rbreak" (waa: "Re: [python][patch] Python rbreak") Joel Brobecker
2018-02-01 10:26 ` Phil Muldoon
2018-02-01 16:21 ` Eli Zaretskii
2018-02-01 17:32 ` Joel Brobecker
2018-02-01 18:25 ` Eli Zaretskii
2018-02-02 3:16 ` Joel Brobecker
2018-02-09 4:30 ` Joel Brobecker
2018-02-09 9:26 ` Eli Zaretskii
2018-02-09 12:13 ` Joel Brobecker
2017-11-14 20:23 ` [python][patch] Python rbreak Simon Marchi
2017-11-16 14:19 ` Phil Muldoon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5e1ba7e3-5f6e-2478-30a5-7670ec7a9879@redhat.com \
--to=pmuldoon@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=simon.marchi@ericsson.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).