From: Pedro Alves <palves@redhat.com>
To: Phil Muldoon <pmuldoon@redhat.com>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [python][patch] PR python/19151 Hardware breakpoints in GDB Python.
Date: Wed, 09 May 2018 14:31:00 -0000 [thread overview]
Message-ID: <e88342f7-0fed-fc40-0620-1fd360f60608@redhat.com> (raw)
In-Reply-To: <68969e58-85d9-5cb7-d2a5-14930d08f799@redhat.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
next prev parent reply other threads:[~2018-05-09 14:31 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-30 11:47 Phil Muldoon
2018-04-30 12:37 ` Phil Muldoon
2018-04-30 14:33 ` Eli Zaretskii
2018-05-09 14:31 ` Pedro Alves [this message]
2018-04-30 14:31 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e88342f7-0fed-fc40-0620-1fd360f60608@redhat.com \
--to=palves@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=pmuldoon@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).