This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch][python] Add breakpoint support.
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Phil Muldoon <pmuldoon at redhat dot com>
- Cc: gdb-patches ml <gdb-patches at sourceware dot org>
- Date: Mon, 5 Apr 2010 09:26:48 -0700
- Subject: Re: [patch][python] Add breakpoint support.
- References: <4BB0B063.6000600@redhat.com>
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 <pmuldoon@redhat.com>
> Thiago Jung Bauermann <bauerman@br.ibm.com>
> Tom Tromey <tromey@redhat.com>
>
> * 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