This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[PATCH] Enable hardware watchpoints on attach for aarch64
- From: Richard Bunt <richard dot bunt at arm dot com>
- To: gdb-patches at sourceware dot org
- Cc: Dirk dot Schubert at arm dot com, nd at arm dot com, Richard Bunt <richard dot bunt at arm dot com>
- Date: Mon, 18 Jun 2018 15:40:31 +0100
- Subject: [PATCH] Enable hardware watchpoints on attach for aarch64
- Nodisclaimer: True
- References: <942ba9f74d67b3857396d461353aae1a@polymtl.ca>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
This commit fixes a bug whereby hardware watchpoints are not used on
aarch64 when attaching to a target. The fix adds an aarch64 specialization
of post_attach which records the number of available hardware debug registers
using aarch64_linux_get_debug_reg_capacity. This implementation mirrors that
of aarch64_linux_child_post_startup_inferior which successfully enables the
use of hardware watchpoints when launching the target under the debugger.
2018-05-14 Richard Bunt <richard.bunt@arm.com>
Dirk Schubert <dirk.schubert@arm.com>
* aarch64-linux-nat.c (post_attach): New.
(aarch64_linux_nat_target::post_attach): Override post_attach to
record the number of hardware debug registers.
gdb/testsuite/ChangeLog:
2018-05-14 Richard Bunt <richard.bunt@arm.com>
* gdb.base/watchpoint-hw-attach.c: New test.
* gdb.base/watchpoint-hw-attach.exp: New file.
---
Updated based on Simon's feedback; thanks for the review.
The feedback has been addressed as detailed below.
>On 2018-06-07 09:19, Alan Hayward wrote:
>> Aarch64 changes look good to me.
>> I'm not up to speed enough with .exp files to comment on that part.
>> I tested it on an aarch64 box, and it works fine for me.
>
>Hi,
>
>I looked at the test, if Alan is fine with the code changes then I am
>fine with them. I noted some nits, but otherwise it looks good. Please
>push after addressing them.
>
>>>> +# Get the PID of the test process.
>>>> +set testpid ""
>>>> +set test "get inferior process ID"
>>>> + gdb_test_multiple "p mypid" $test {
>>>> + -re " = ($decimal)\r\n$gdb_prompt $" {
>>>> + set testpid $expect_out(1,string)
>>>> + pass $test
>>>> + }
>>>> +}
>
>You can use get_integer_valueof for this.
>
Fixed, and good to know about this one.
>>>> +set test "attach $testpid"
>>>> +gdb_test_multiple $test $test {
>>>> + -re "Attaching to program.*process $testpid\r\n.*$gdb_prompt $"
>>>> {
>>>> + pass "$test"
>>>> + }
>>>> + timeout {
>>>> + fail "$test"
>>>> + }
>>>> +}
>
>Just wondering, can't simple gdb_test be used here?
>
Changed. There is no reason that I am aware of that this cannot be a
gdb_test. I think this was just the structure used in the attach test that I
borrowed from.
>>>> +
>>>> +gdb_test_no_output "set should_continue = 1"
>>>> +# Ensure the test program is in the top frame so the required
>>>> +# variables are in scope.
>>>> +gdb_breakpoint [gdb_get_line_number "prewatchtrigger"]
>>>> +gdb_continue_to_breakpoint "prewatchtrigger"
>>>> +
>>>> +gdb_test "watch watched_variable" \
>>>> + "Hardware watchpoint $decimal: watched_variable"
>>>> +
>>>> +gdb_test "continue" \
>>>> + "continue.*Continuing.*\.Hardware watchpoint $decimal:\
>>>> + watched_variable.*Old value = 0.*New value = 4.*"
>
>8 spaces -> tab on the last line. Though for test patterns, it's fine
>to go over 80 columns, IMO.
>
Fixed, that certainly makes it easier to read.
>Simon
Will push (as per the review) in a few days if there are no more objections.
Many thanks,
Rich
gdb/aarch64-linux-nat.c | 18 +++++++
gdb/testsuite/gdb.base/watchpoint-hw-attach.c | 44 +++++++++++++++
gdb/testsuite/gdb.base/watchpoint-hw-attach.exp | 71 +++++++++++++++++++++++++
3 files changed, 133 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/watchpoint-hw-attach.c
create mode 100644 gdb/testsuite/gdb.base/watchpoint-hw-attach.exp
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 1e7db29..c9fd062 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -77,6 +77,9 @@ public:
/* Override the GNU/Linux inferior startup hook. */
void post_startup_inferior (ptid_t) override;
+ /* Override the GNU/Linux post attach hook. */
+ void post_attach (int pid) override;
+
/* These three defer to common nat/ code. */
void low_new_thread (struct lwp_info *lp) override
{ aarch64_linux_new_thread (lp); }
@@ -568,6 +571,21 @@ aarch64_linux_nat_target::post_startup_inferior (ptid_t ptid)
linux_nat_target::post_startup_inferior (ptid);
}
+/* Implement the "post_attach" target_ops method. */
+
+void
+aarch64_linux_nat_target::post_attach (int pid)
+{
+ low_forget_process (pid);
+ /* Set the hardware debug register capacity. If
+ aarch64_linux_get_debug_reg_capacity is not called
+ (as it is in aarch64_linux_child_post_startup_inferior) then
+ software watchpoints will be used instead of hardware
+ watchpoints when attaching to a target. */
+ aarch64_linux_get_debug_reg_capacity (pid);
+ linux_nat_target::post_attach (pid);
+}
+
extern struct target_desc *tdesc_arm_with_neon;
/* Implement the "read_description" target_ops method. */
diff --git a/gdb/testsuite/gdb.base/watchpoint-hw-attach.c b/gdb/testsuite/gdb.base/watchpoint-hw-attach.c
new file mode 100644
index 0000000..9d55b28
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-hw-attach.c
@@ -0,0 +1,44 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2018 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+/* This is set to 1 by the debugger post attach to continue to the
+ watchpoint trigger. */
+volatile int should_continue = 0;
+/* The variable to place a watchpoint on. */
+volatile int watched_variable = 0;
+
+int
+main (void)
+{
+ unsigned int counter = 1;
+ int mypid = getpid ();
+
+ /* Wait for the debugger to attach, but not indefinitely so this
+ test program is not left hanging around. */
+ for (counter = 0; !should_continue && counter < 100; counter++)
+ sleep (1); /* pidacquired */
+
+ watched_variable = 0; /* prewatchtrigger */
+ /* Trigger a watchpoint. */
+ watched_variable = 4;
+ printf ("My variable is %d\n", watched_variable);
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-hw-attach.exp b/gdb/testsuite/gdb.base/watchpoint-hw-attach.exp
new file mode 100644
index 0000000..0c5037c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-hw-attach.exp
@@ -0,0 +1,71 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# watchpoint-hw-attach.exp -- Test if hardware watchpoints are used
+# when attaching to a target.
+
+if {[skip_hw_watchpoint_tests]} {
+ return 0
+}
+
+if {![can_spawn_for_attach]} {
+ return 0
+}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+ return -1
+}
+
+clean_restart $binfile
+
+if ![runto_main] {
+ untested "can't run to main"
+ return -1
+}
+
+# Run to the point where mypid in the test program has been
+# populated.
+gdb_breakpoint [gdb_get_line_number "pidacquired"]
+gdb_continue_to_breakpoint "pidacquired"
+
+# Get the PID of the test process.
+set testpid [get_integer_valueof "mypid" 0]
+
+gdb_test "detach" "Detaching from program: .*, process $testpid\r\n\\\[Inferior $decimal \\(process $testpid\\) detached\\\]"
+
+if {$testpid == ""} {
+ return -1
+}
+
+# A clean restart is needed to force the hardware watchpoint setup
+# logic to run post attach rather than post inferior launch.
+clean_restart $binfile
+
+gdb_test "attach $testpid" "Attaching to program: .*, process $testpid.*"
+
+gdb_test_no_output "set should_continue = 1"
+
+# Ensure the test program is in the top frame so the required
+# variables are in scope.
+gdb_breakpoint [gdb_get_line_number "prewatchtrigger"]
+gdb_continue_to_breakpoint "prewatchtrigger"
+
+gdb_test "watch watched_variable" \
+ "Hardware watchpoint $decimal: watched_variable"
+
+gdb_test "continue" \
+ "continue.*Continuing.*\.Hardware watchpoint $decimal: watched_variable.*Old value = 0.*New value = 4.*watched_variable\\);"
--
1.8.5.6