From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27556 invoked by alias); 5 Apr 2010 16:27:06 -0000 Received: (qmail 27206 invoked by uid 22791); 5 Apr 2010 16:27:01 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 05 Apr 2010 16:26:54 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 18A092BAB4D; Mon, 5 Apr 2010 12:26:53 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id Xe4CqK2N4MzI; Mon, 5 Apr 2010 12:26:53 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id D42482BAB35; Mon, 5 Apr 2010 12:26:52 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id BB18AF58C2; Mon, 5 Apr 2010 09:26:48 -0700 (PDT) Date: Mon, 05 Apr 2010 16:27:00 -0000 From: Joel Brobecker To: Phil Muldoon Cc: gdb-patches ml Subject: Re: [patch][python] Add breakpoint support. Message-ID: <20100405162648.GD19194@adacore.com> References: <4BB0B063.6000600@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4BB0B063.6000600@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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 X-SW-Source: 2010-04/txt/msg00077.txt.bz2 General question: How come the python error strings are not i18n'ed? For instance: > + PyErr_SetString (PyExc_TypeError, "cannot delete `silent' attribute"); This is definitely the standard, currently. Is that normal? > 2010-03-29 Phil Muldoon > Thiago Jung Bauermann > Tom Tromey > > * breakpoint.c (condition_command): Simplify. Move condition > setting code too ... > (set_breakpoint_condition): ... here. New function. > * breakpoint.h (set_breakpoint_condition): Declare. > * Makefile.in (SUBDIR_PYTHON_OBS): Add py-breakpoint. > (SUBDIR_PYTHON_SRCS): Likewise. > (py-breakpoint.o): New rule. > * python/py-breakpoint.c: New file. > * python/python-internal.h (gdbpy_breakpoints) > (gdbpy_get_hook_function, gdbpy_initialize_breakpoints): Declare. > (GDB_PY_SET_HANDLE_EXCEPTION) Define. > * python/python.c (gdbpy_get_hook_function): New function. Overall, it looks fine to me. Just a few comments... > +/* From breakpoint.c. */ > +extern struct breakpoint *breakpoint_chain; This shouldn't be needed. The only use I found was: > + for (bp = breakpoint_chain; bp; bp = bp->next) > + if (bp->number == num) > + break; But there is a public function that should return the breakpoint you are looking for: get_breakpoint. > +/* Evaluate to true if the breakpoint NUM is valid, false otherwise. */ > +#define BPPY_VALID_P(Num) \ > + ((Num) >= 0 \ > + && (Num) < bppy_slots \ > + && bppy_breakpoints[Num] != NULL) Personally (and I litterally mean it, it's a personal preference), I'd rather we did not use macros, but a real function... Being a static function, I'm sure the compiler will be capable of performing whatever is best for performance (and that way, we can avoid the parens around "Num", and the "do/while(0)" dance in the macro that follows). > + breakpoint_object *self_bp = (breakpoint_object *) self; > + if (self_bp->bp) Can you add an empty line after the declaration(s)? > +/* Python function to set the thread of a breakpoint. */ > +static int > +bppy_set_thread (PyObject *self, PyObject *newvalue, void *closure) Small request: Can we have the same for tasks (Ada tasks)? The function that allows you to verify that a given task number is valid is: extern int valid_task_id (int); Similarly, we can also have a getter for the breakpoint task ID? > + if (bp->type != bp_breakpoint && bp->type != bp_watchpoint > + && bp->type != bp_hardware_watchpoint > + && bp->type != bp_read_watchpoint > + && bp->type != bp_access_watchpoint) Small nit-pick? Since we're aligning all conditions, can we split the first line in two? if (bp->type != bp_breakpoint && bp->type != bp_watchpoint && bp->type != bp_hardware_watchpoint && bp->type != bp_read_watchpoint && bp->type != bp_access_watchpoint) I think it'll be easier to not miss the "!= bp_watchpoint" condition. > +PyObject * > +gdbpy_get_hook_function (const char *name) This function needs to be documented... > +set testfile "py-breakpoint" > +set srcfile ${testfile}.c > +set binfile ${objdir}/${subdir}/${testfile} > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { > + untested "Couldn't compile ${srcfile}" > + return -1 > +} > +# Start with a fresh gdb. > +gdb_exit > +gdb_start > +gdb_reinitialize_dir $srcdir/$subdir > +gdb_load ${binfile} All the above can be simplified into: set testfile "py-breakpoint" set srcfile ${testfile}.c if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } { return -1 } I mention this, because people rarely seem to know about this... > +gdb_test "python print blist\[0\].location" "main." "Check breakpoint location" ^^^ Can you check whether you might need to escape the period, here? > +# Start with a fresh gdb. > +gdb_exit > +gdb_start > +gdb_reinitialize_dir $srcdir/$subdir > +gdb_load ${binfile} You can replace this with a call to clean_restart. -- Joel