Re: [python][patch] PR python/19151 Hardware breakpoints in GDB Python.

[Pedro Alves <palves at redhat dot com>]

Hi Phil,

I agree with Eli, this should be mentioned in NEWS, as all
Python API additions/changes do.

Some nits below, but otherwise looks fine.

Please post a v2 with a NEWS entry, including the proposed
git commit log, and it should be good to go.

On 04/30/2018 01:37 PM, Phil Muldoon wrote:
> 
> 2018-04-30  Phil Muldoon  <pmuldoon@redhat.com>
>  
>  	PR python/19151
>  	* python/py-breakpoint.c: Add hardware breakpoint constant
>  	gdb.BP_HARDWARE_BREAKPOINT.

Mention "(pybp_codes)":

  	* python/py-breakpoint.c (pybp_codes): Add hardware
        breakpoint constant gdb.BP_HARDWARE_BREAKPOINT.

>  	(bppy_init): Add bp_hardware_breakpoint case. Use the enum bptype
>  	variable

Double space after '.' and missing '.' at end of second sentence.

> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -681,6 +681,33 @@ proc_with_prefix test_bkpt_qualified {} {
>  	"-q in spec string and qualified false"
>  }
>  
> +# Test hardware assisted breakpoints
> +proc_with_prefix test_hardware_breakpoints { } {
> +    global srcfile testfile decimal
> +
> +    # Start with a fresh gdb.
> +    clean_restart ${testfile}
> +
> +    if {[skip_hw_breakpoint_tests]} {
> +	unsupported "Hardware breakpoints."

Missing "return"

> +    }

> +
> +    if ![runto_main] then {
> +	fail "cannot run to main."
> +	return 0
> +    }
> +
> +    set hardware_location [gdb_get_line_number "Break at multiply."]
> +    gdb_test  "python hbp = gdb.Breakpoint (\"$hardware_location\", type=gdb.BP_HARDWARE_BREAKPOINT)" \

               ^^ spurious double space.

> +	".*Hardware assisted breakpoint ($decimal)+ at .*$srcfile, line ($decimal)+\." \

Leading ".*" not necessary, it's implied.

> +	"Set hardware breakpoint"

Lowercase "Set".

> +    gdb_continue_to_breakpoint "Break at multiply." \
> +	".*$srcfile:$hardware_location.*"

Leading ".*" not necessary, it's implied.

> +    gdb_test "info breakpoints" \
> +	"2.*hw breakpoint.*$srcfile:$hardware_location.*" \
> +	"Check info breakpoints shows a hardware breakpoint"

Lowercase "Check".  I'd remove "check, even, since all tests
are checking something:

	"info breakpoints shows a hardware breakpoint"

Thanks,
Pedro Alves