From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x530.google.com (mail-ed1-x530.google.com [IPv6:2a00:1450:4864:20::530]) by sourceware.org (Postfix) with ESMTPS id A5B483955C8E for ; Tue, 31 May 2022 14:14:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A5B483955C8E Received: by mail-ed1-x530.google.com with SMTP id t5so17737228edc.2 for ; Tue, 31 May 2022 07:14:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=uzsBqsubEflRcCMd0TeDJ1VP7C3+Zf7DRaHzW0v4LNs=; b=iockPuSuHbElyER197/kQhnpS7vrZDS5QRC+MoNi0b4Jud0slaBA4LyT+u7oAPN1vE jvdU2qrUEANIUfwRWCShT+pzwvhO5ELuJwXQ/RPOYGAUV2/DD1qiYz4IvC7QrPH/Jdpr 9Wg4OPWy3AihlLDVjL87Th5m9pCC1Bq+Guk1SkcwTLVOpBaN5Lx9NQ7IUmV1P9nxzXQ5 PEZG92EmdXRgc6h2lEcDQYwcDZIli0thjRHNGs4AzBVEgxdjKfIvJfWmavLq3/aCbgU9 MZjd1ZLtMmLorM6BYLEIQ6e4dw9aLTjMzLleHsbQ8k0YbmzxWGQ3Kppnq8jxwYPRAygD T4aA== X-Gm-Message-State: AOAM531dTgZd2QeEdUQxt9cE7QLsFApoMOQ0h7tZ615DCAm2927vJVGR DwgZtI2y65ZbVIpcGXzHcNeU66gQdCu3sFsFqNN0KrIjXk4= X-Google-Smtp-Source: ABdhPJx+fpjiyonRALEQW4GBYLCx2IyQchk8ViiB1wRPjKVUBGi2iypLA0PZ5nLSLRByx2ptBXf6oKhrroaYsqsPd+Q= X-Received: by 2002:a05:6402:42d4:b0:412:c26b:789 with SMTP id i20-20020a05640242d400b00412c26b0789mr63642907edc.232.1654006467404; Tue, 31 May 2022 07:14:27 -0700 (PDT) MIME-Version: 1.0 References: <20220420083757.763625-1-simon.farre.cx@gmail.com> <0af6cb3c-43c9-0083-2b57-7a5d338c0af7@palves.net> In-Reply-To: <0af6cb3c-43c9-0083-2b57-7a5d338c0af7@palves.net> From: Simon Farre Date: Tue, 31 May 2022 16:14:16 +0200 Message-ID: Subject: Re: [PATCH v5] gdb/python: Add BreakpointLocation type To: Pedro Alves Cc: Simon Farre via Gdb-patches X-Spam-Status: No, score=-9.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, HTML_MESSAGE, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 31 May 2022 14:14:31 -0000 Thanks Pedro for taking your time to go over my patch! Is there a good reason we're not exposing all the attributes MI exposes? > For example, with: > > (top-gdb) b main > (top-gdb) interpreter-exec mi "-break-list" > > we see: > > ... > locations=[{number="3.1", > enabled="y", > addr="0x00000000000ed06c", > func="main(int, char**)", > file="/home/pedro/gdb/binutils-gdb/src/gdb/gdb.c", > > fullname="/net/cascais.nfs/brno/pedro/gdb/binutils-gdb/src/gdb/gdb.c", > line="25", > thread-groups=["i1"]}, > So at least "func", "fullname" and "thread-groups" (inferiors) seem to be > missing. > > See here: > > https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Breakpoint-Information.html > > I will make sure to add those for v6 - there was actually no particular reason they were left out actually. I'll also fix the grammatical and spelling errors. Or perhaps: > error (_("Breakpoint location does not have an owner breakpoint.")); > > But note you've named the location's owner field "parent", for some reason, > though the documentation for that field says it is the "owner"... > Better to use the same terminology everywhere, IMO. Why not call the field > "owner" instead of inventing a new term, if GDB ends up calling it owner, > and the manual describes it as such, anyhow? > You're absolutely right. This was a mistake. This should be "owner" and will be changed. > > +static PyObject * > > +bplocpy_get_source_location (PyObject *py_self, void *closure) > > +{ > > + gdbpy_breakpoint_location_object *self > > + = (gdbpy_breakpoint_location_object *) py_self; > > + BPPY_REQUIRE_VALID (self->parent); > > + BPLOCPY_REQUIRE_VALID (self->parent, self); > > + if (self->bp_loc->symtab) > > + { > > + gdbpy_ref<> tup (PyTuple_New (2)); > > + /* symtab->filename is never NULL. */ > > + gdbpy_ref<> filename > > + = host_string_to_python_string (self->bp_loc->symtab->filename); > > + > > + if (PyTuple_SetItem (tup.get (), 0, filename.release ()) == -1 > > If this returns -1, doesn't filename leak? > PyTuple_SetItem steals a reference from filename, if it fails it decrements it. (https://stackoverflow.com/questions/6111843/limitations-of-pytuple-setitem) > > + You should have received a copy of the GNU General Public License > > + along with this program. If not, see . > */ > > + > > +double add (double DoubleA, double DoubleB) > > We try to follow the GDB conventions in the testcases as well. > > It would be better to use a non-floating type for this, as some > targets don't support floating point (see gdb_skip_float_test), and > there's no > reason the test shouldn't run on such targets. > > > > +{ > > + return DoubleA + DoubleB; > > +} > > + > > +int add (int IntA, int IntB) > > +{ > > + return IntA + IntB; > > +} > > + > > +int main (void) > > +{ > > + double d = add (1.0, 2.0); > > + int i = add (1, 2); > > +} > > diff --git a/gdb/testsuite/gdb.python/py-bp-locations.exp > b/gdb/testsuite/gdb.python/py-bp-locations.exp > > new file mode 100644 > > index 00000000000..b06b62de971 > > --- /dev/null > > +++ b/gdb/testsuite/gdb.python/py-bp-locations.exp > > @@ -0,0 +1,62 @@ > > +# Copyright (C) 2022-2022 Free Software Foundation, Inc. > > + > > + > > +if { ![support_displaced_stepping] } { > > + unsupported "displaced stepping" > > + return -1 > > +} > > This looks leftover from some copied-from testcase. I don't see a relation > to this testcase. > > > + > > +load_lib gdb-python.exp > > + > > +standard_testfile > > + > > +if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" > executable {c++ debug}] != "" } { > > + return -1 > > +} > > + > > +save_vars { GDBFLAGS } { > > + clean_restart $testfile > > +} > > + > > +if { [skip_python_tests] } { continue } > > + > > +if ![runto_main] { > > + return -1 > > +} > > + > > +# set breakpoint with 2 locations > > Start sentences with uppercase, and finish them with period. > > > +gdb_breakpoint "add" > > + > > +# test that the locations return correct source locations > > Ditto, and more below. > > > +gdb_test "python print(gdb.breakpoints()\[1\].locations\[0\].source)" \ > > + ".*('.*py-bp-locations.c', 20).*" > > +gdb_test "python print(gdb.breakpoints()\[1\].locations\[1\].source)" \ > > + ".*('.*py-bp-locations.c', 25).*" > > It would be better to use gdb_get_line_number instead of hardcoding line > numbers. > > > + > > +# disable first location and make sure we don't hit it > > +gdb_test "python gdb.breakpoints()\[1\].locations\[0\].enabled = False" > "" > > +gdb_continue_to_breakpoint "" ".*25.*" > > + > > +if ![runto_main] { > > + return -1 > > +} > > + > > +gdb_breakpoint "add" > > +# disable "add" master breakpoint and verify all locations are disabled. > > owner, parent, now master? :-P > Consistency in naming is quite obviously not my strongest skill :P. I'll get to changing them.