From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 86695 invoked by alias); 16 Oct 2017 23:01:21 -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 86680 invoked by uid 89); 16 Oct 2017 23:01:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=degree X-HELO: mail-wr0-f177.google.com Received: from mail-wr0-f177.google.com (HELO mail-wr0-f177.google.com) (209.85.128.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 16 Oct 2017 23:01:19 +0000 Received: by mail-wr0-f177.google.com with SMTP id y44so4447437wry.10 for ; Mon, 16 Oct 2017 16:01:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=k4NsRMLYvCj5RKZNN6FrwnNUgk/cLgrR/BfkMjv4zL0=; b=mfv6yk59vFpuy5qGr8iShnf51xjnALl6AsUQDTZSnp1rSxss8PpPkOF3pvVMMEnDiw RJVUyUCEAYPgHJTSYkdqJyn+2Q74Ou3pGjRikDGQTkdOHIxGKQbJKBUndVCZcGhXpq1g 1EiTmY1bU4HR/38qeLE6RT3zOwKhP3s8BFkULBoWKZovOUe+wCPI7tCAvL3EpbwfWsPe iyQqGffGgoTl7FPiJ3v1QplSL7Zj9ili5eBAV9cWSbiczZOUqoiHVZDtiWtw/bIJZqUY 5NUVNPIojhilW24A8svxLF2kTX3P3++aCzPN5njDwVLx+KDBZS9Abpg6tyYyVjeg14KV YNFw== X-Gm-Message-State: AMCzsaX5PAC/WXLZv5oOvgKs7pPcZBK3rpC+HgulENEzRBHe+bl3xoC8 GJDrWd15RvDBOp00Nqtba6oFtYitrjU= X-Google-Smtp-Source: ABhQp+SvK9DVOAAQYpCUvORlFoPb3SeioZfD5Tc8Go2EDndaf1t4w1AhaGc89B68bQX2FhZyvpaiQg== X-Received: by 10.223.198.82 with SMTP id u18mr1931076wrg.5.1508194876405; Mon, 16 Oct 2017 16:01:16 -0700 (PDT) Received: from ?IPv6:2a02:c7f:ae6a:ed00:4685:ff:fe66:9f4? ([2a02:c7f:ae6a:ed00:4685:ff:fe66:9f4]) by smtp.gmail.com with ESMTPSA id i16sm14392393wrf.19.2017.10.16.16.01.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Oct 2017 16:01:15 -0700 (PDT) Subject: Re: [python][patch] Python rbreak To: Simon Marchi , "gdb-patches@sourceware.org" References: From: Phil Muldoon Message-ID: <5e1ba7e3-5f6e-2478-30a5-7670ec7a9879@redhat.com> Date: Mon, 16 Oct 2017 23:01:00 -0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00488.txt.bz2 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