Re: [patch][python] Add breakpoint support.

[Joel Brobecker <brobecker@adacore.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