[PATCH] Enable hardware watchpoints on attach for aarch64

[Richard Bunt <richard dot bunt at arm dot com>]

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