[PATCH 0/2] linux-nat: Fix watchpoint issues across exec calls

[PATCH 0/2] linux-nat: Fix watchpoint issues across exec calls

[Pedro Franco de Carvalho]

These two patches are intended to fix an issue caused by calls to exec
when hardware breakpoints and watchpoints are in use.  Currently, the
linux native targets don't correctly keep track of debug register usage
across exec calls.

I've implemented this for the x86 linux target, but this likely affects
other native linux targets that keep track of debug registers (e.g. s390
and ARM).

I found these issues while fixing other issues with watchpoints for the
native linux on Power target.  I'm still working on those, and will
incorporate the changes needed for using watchpoints with exec with them.

Pedro Franco de Carvalho (2):
  linux-nat: Callback for exec events
  x86 linux: Update debug register state after exec events

 gdb/linux-nat.c                            |  4 ++
 gdb/linux-nat.h                            | 13 ++++++
 gdb/testsuite/gdb.base/watchpoint-exec.c   | 48 ++++++++++++++++++++
 gdb/testsuite/gdb.base/watchpoint-exec.exp | 53 ++++++++++++++++++++++
 gdb/x86-linux-nat.h                        |  3 ++
 gdb/x86-nat.c                              | 15 ++++--
 gdb/x86-nat.h                              |  8 +++-
 7 files changed, 140 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-exec.c
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-exec.exp

-- 
2.20.1

[PATCH 2/2] x86 linux: Update debug register state after exec events

[Pedro Franco de Carvalho]

This patch changes the linux native x86 target so that it updates its
debug register state after an exec call is detected, by overriding the
architecture-specific low target method low_post_exec from
linux_nat_target.

Previously, an exec event could cause the debug register state to
become inconsistent.

The infrun logic tries to re-insert symbolic watchpoints and breakpoints
after an exec event, in case that they are still meaningful in the new
program.  However, it assumes watchpoints and breakpoints are cleared
across the exec event, including those that use hardware debug registers,
so it doesn't try to remove them before re-inserting them.

From the target's perspective, this is seen as two successive calls to
insert_watchpoint, with no remove_watchpoint in between, as it is
sometimes done when the inferior stops and is resumed again.  The first
insertion comes before the inferior is resumed and eventually calls exec,
the second happens after the exec call when infrun updates the symbolic
breakpoints and watchpoints that are still meaningful.  This double
insertion can cause the state of the debug registers tracked by the low
target to become inconsistent.

In some cases this can cause a watchpoint not to be installed.  The x86
linux native target keeps track of requests for equivalent hardware
watchpoints with a reference count so that it doesn't need to duplicate
debug register usage.  This type of request doesn't cause the debug
registers in the threads to be updated, since the target assumes the
thread already has the debug registers properly configured.

If a program self-execs, a global symbol can have the same address and
length in its new instance.  This will cause the double insertion across
an exec call to not have any effect, so the watchpoint won't trigger if
the second instance of the program writes to the watched region.  This
issue is reflected by the testcase included in this patch.

This specific issue probably doesn't happen in ARM and s390, and possibly
other targets, since they don't seem to handle duplicated hardware
watchpoints specially, but any debug register state tracked by these
targets is likely to be incorrect, and could cause other issues, so they
should also redefine low_post_exec to reset the debug register state that
they track.  The new testcase will likely pass for these.  The Power
linux native target currently has other issues that need to be addressed
first.

This patch makes no changes to the x86 linux-low gdbserver stub, because
when the generic linux stub detects an exec event, and if exec events are
configured to be reported to the client ("exec-events+" in the query
packet), it deletes the process state and recreates one, which causes the
debug register data of the linux x86 stub to be cleared.  Linux stubs for
other arches that support hardware watchpoints can use this same
mechanism.

When the generic linux stub detects an exec event, but these are not
configured to be reported back to the client, the process state is not
re-initialized.  However, if infrun isn't notified of the exec event
in the first place, it won't try to insert the watchpoint twice
without a removal in-between, so the debug register state in the stub
will match what infrun knows about the watchpoints, even though it
doesn't match what is actually installed in the threads, if the
watchpoint was cleared by the exec call.

gdb/ChangeLog:
2019-05-16  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* x86-nat.h (x86_cleanup_dregs): Declare overloaded function with
	a pid argument.  Adjust the comment for the original function.
	* x86-nat.c (x86_cleanup_dregs): Define overloaded function.
	Change the original version to use it.
	* x86-linux-nat.h (x86_linux_nat_target) <low_post_exec>:
	Override and define.

gdb/testsuite/ChangeLog:
2019-05-16  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* gdb.base/watchpoint-exec.exp: New testcase.
	* gdb.base/watchpoint-exec.c: New file.
---
 gdb/testsuite/gdb.base/watchpoint-exec.c   | 48 ++++++++++++++++++++
 gdb/testsuite/gdb.base/watchpoint-exec.exp | 53 ++++++++++++++++++++++
 gdb/x86-linux-nat.h                        |  3 ++
 gdb/x86-nat.c                              | 15 ++++--
 gdb/x86-nat.h                              |  8 +++-
 5 files changed, 123 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-exec.c
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-exec.exp

diff --git a/gdb/testsuite/gdb.base/watchpoint-exec.c b/gdb/testsuite/gdb.base/watchpoint-exec.c
new file mode 100644
index 0000000000..56d3dbe69f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-exec.c
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2019 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/>.  */
+
+/* This program exits if its first argument is --stop, otherwise it
+   tries to exec the first argument, passing --stop.  In both cases it
+   writes to a global variable, which GDB will watch in this test.  It
+   is meant to be called with its own pathname, so that it writes to
+   the variable, execs itself once, writes to the variable again, and
+   exits.  */
+
+#include <unistd.h>
+#include <string.h>
+
+int watched_var = 0;
+
+int main (int argc, char *argv[])
+{
+  char *new_argv[3] = {"", "--stop", NULL};
+
+  if (argc != 2)
+    return 1;
+
+  new_argv[0] = argv[0];
+
+  watched_var = 1;
+
+  if (strcmp (argv[1], "--stop") == 0)
+    return 0;
+
+  if (execvp (argv[1], &new_argv[0]) < 0)
+    return 1;
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-exec.exp b/gdb/testsuite/gdb.base/watchpoint-exec.exp
new file mode 100644
index 0000000000..d366add292
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-exec.exp
@@ -0,0 +1,53 @@
+# Copyright (C) 2019 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/>.
+
+# This file is part of the gdb testsuite.
+
+# Test if a hardware watchpoint triggers before and after a program
+# self-execs.  This should happen since the watched global symbol must
+# be valid after the exec.
+
+if {[skip_hw_watchpoint_tests]} {
+    return
+}
+
+# The testsuite doesn't currently allow sending args to the stub.
+if [use_gdb_stub] {
+	unsupported "using gdb stub"
+	return
+}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return
+}
+
+gdb_test_no_output "set args $binfile" ""
+
+runto_main
+
+# We don't want to hit the breakpoint from runto_main after the exec.
+delete_breakpoints
+
+gdb_test "watch watched_var" "Hardware watchpoint.*: watched_var.*"
+
+set watchpoint_hit_re \
+    "Hardware watchpoint.*: watched_var.*Old value = 0.*New value = 1"
+
+gdb_test "continue" "$watchpoint_hit_re.*" "continue before exec"
+
+gdb_test "continue" "is executing new program.*$watchpoint_hit_re.*" \
+    "continue after exec"
diff --git a/gdb/x86-linux-nat.h b/gdb/x86-linux-nat.h
index 887e30eecd..9f024d4649 100644
--- a/gdb/x86-linux-nat.h
+++ b/gdb/x86-linux-nat.h
@@ -65,6 +65,9 @@ struct x86_linux_nat_target : public x86_nat_target<linux_nat_target>
   void low_forget_process (pid_t pid) override
   { x86_forget_process (pid); }
 
+  void low_post_exec (struct lwp_info *lwp) override
+  { x86_cleanup_dregs (lwp->ptid.pid ()); }
+
   void low_prepare_to_resume (struct lwp_info *lwp) override
   { x86_linux_prepare_to_resume (lwp); }
 
diff --git a/gdb/x86-nat.c b/gdb/x86-nat.c
index cd9ce17e8d..71f7c35ca9 100644
--- a/gdb/x86-nat.c
+++ b/gdb/x86-nat.c
@@ -133,13 +133,22 @@ x86_forget_process (pid_t pid)
 }
 
 /* Clear the reference counts and forget everything we knew about the
-   debug registers.  */
+   debug registers of process PID.  */
 
 void
-x86_cleanup_dregs (void)
+x86_cleanup_dregs (pid_t pid)
 {
   /* Starting from scratch has the same effect.  */
-  x86_forget_process (inferior_ptid.pid ());
+  x86_forget_process (pid);
+}
+
+/* Convenience overloaded function that calls x86_cleanup_dregs with
+   the pid of inferior_ptid.  */
+
+void
+x86_cleanup_dregs (void)
+{
+  x86_cleanup_dregs (inferior_ptid.pid ());
 }
 
 /* Insert a watchpoint to watch a memory region which starts at
diff --git a/gdb/x86-nat.h b/gdb/x86-nat.h
index baa4218a87..08df339a33 100644
--- a/gdb/x86-nat.h
+++ b/gdb/x86-nat.h
@@ -36,7 +36,13 @@
 
 extern void x86_set_debug_register_length (int len);
 
-/* Use this function to reset the x86-nat.c debug register state.  */
+/* Use this function to reset the x86-nat.c debug register state for
+   PID.  */
+
+extern void x86_cleanup_dregs (pid_t pid);
+
+/* Use this function to reset the x86-nat.c debug register state for
+   the pid of inferior_ptid.  */
 
 extern void x86_cleanup_dregs (void);
 
-- 
2.20.1

[PATCH 1/2] linux-nat: Callback for exec events

[Pedro Franco de Carvalho]

This patch adds a new virtual low method to the linux native target,
called when an exec event is detected (PTRACE_EVENT_EXEC).

The main motivation for this is for arch-specific code to be able to
update its debug register state, since infrun assumes that an exec
will cause the hardware watchpoints in the inferior to be cleared.

Because this callback is called before infrun and other GDB layers
become aware of the exec event, care must be taken when overriding it.

2019-05-16  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>

	* linux-nat.h (class linux_nat_target) <low_post_exec>: New
	method.
	* linux-nat.c (linux_handle_extended_wait): Call low_post_exec.
---
 gdb/linux-nat.c |  4 ++++
 gdb/linux-nat.h | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 65165a2d46..188b1696a8 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2092,6 +2092,10 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
 	 thread execs, it changes its tid to the tgid, and the old
 	 tgid thread might have not been resumed.  */
       lp->resumed = 1;
+
+      /* Let the arch know that lp called exec.  */
+      linux_target->low_post_exec (lp);
+
       return 0;
     }
 
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index 0c1695ad10..569683ba33 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -168,6 +168,19 @@ public:
   virtual void low_forget_process (pid_t pid)
   {}
 
+  /* The method to call, if any, when an exec event is detected on
+     LWP.
+
+     This is meant to be used by the low target to update its own
+     internal state of debug registers, given that infrun assumes that
+     watchpoints are cleared accross an exec.
+
+     For other needs, be careful of assumptions about the state of
+     other layers of GDB, which haven't been notified of the exec
+     event at this point.  */
+  virtual void low_post_exec (struct lwp_info * lwp)
+  {}
+
   /* Hook to call prior to resuming a thread.  */
   virtual void low_prepare_to_resume (struct lwp_info *)
   {}
-- 
2.20.1