From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 91105 invoked by alias); 17 Oct 2017 00:24:38 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 91071 invoked by uid 89); 17 Oct 2017 00:24:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=caps, life X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 17 Oct 2017 00:24:32 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v9H0OP34007961 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 16 Oct 2017 20:24:30 -0400 Received: by simark.ca (Postfix, from userid 112) id 5555B1E540; Mon, 16 Oct 2017 20:24:25 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 93F8E1E517; Mon, 16 Oct 2017 20:24:04 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 17 Oct 2017 00:24:00 -0000 From: Simon Marchi To: Phil Muldoon Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [python][patch] Python rbreak In-Reply-To: <5e1ba7e3-5f6e-2478-30a5-7670ec7a9879@redhat.com> References: <5e1ba7e3-5f6e-2478-30a5-7670ec7a9879@redhat.com> Message-ID: <3193f5c7a0c98c548722bb6c143f347e@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 17 Oct 2017 00:24:25 +0000 X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00493.txt.bz2 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