Fix watchthreads-reorder.exp fails in linux gdbserver

Fix watchthreads-reorder.exp fails in linux gdbserver

[Pedro Alves]

I've applied the patch below, to address in linux gdbserver the problem
watchthreads-reorder.exp uncovers, similarly to how it was fixed
in linux-nat.c.

Fixes:

-FAIL: gdb.threads/watchthreads-reorder.exp: reorder1: continue b
+PASS: gdb.threads/watchthreads-reorder.exp: reorder1: continue b

-- 
Pedro Alves

2010-03-14  Pedro Alves  <pedro@codesourcery.com>

	gdb/gdbserver/
	* linux-low.h (struct lwp_info): New fields
	`stopped_by_watchpoint' and `stopped_data_address'.
	* linux-low.c (linux_wait_for_lwp): Check for watchpoint triggers
	here, and cache them in the lwp object.
	(wait_for_sigstop): Check stopped_by_watchpoint lwp field
	directly.
	(linux_resume_one_lwp): Clear the lwp's stopped_by_watchpoint
	field.
	(linux_stopped_by_watchpoint): Rewrite.
	(linux_stopped_data_address): Rewrite.

---
 gdb/gdbserver/linux-low.c |   62 +++++++++++++++++++++++++++++++++++++++-------
 gdb/gdbserver/linux-low.h |   10 +++++++
 2 files changed, 63 insertions(+), 9 deletions(-)

Index: src/gdb/gdbserver/linux-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-low.c	2010-03-14 18:31:41.000000000 +0000
+++ src/gdb/gdbserver/linux-low.c	2010-03-14 18:40:46.000000000 +0000
@@ -1062,6 +1062,51 @@ retry:
       new_inferior = 0;
     }
 
+  /* Fetch the possibly triggered data watchpoint info and store it in
+     CHILD.
+
+     On some archs, like x86, that use debug registers to set
+     watchpoints, it's possible that the way to know which watched
+     address trapped, is to check the register that is used to select
+     which address to watch.  Problem is, between setting the
+     watchpoint and reading back which data address trapped, the user
+     may change the set of watchpoints, and, as a consequence, GDB
+     changes the debug registers in the inferior.  To avoid reading
+     back a stale stopped-data-address when that happens, we cache in
+     LP the fact that a watchpoint trapped, and the corresponding data
+     address, as soon as we see CHILD stop with a SIGTRAP.  If GDB
+     changes the debug registers meanwhile, we have the cached data we
+     can rely on.  */
+
+  if (WIFSTOPPED (*wstatp) && WSTOPSIG (*wstatp) == SIGTRAP)
+    {
+      if (the_low_target.stopped_by_watchpoint == NULL)
+	{
+	  child->stopped_by_watchpoint = 0;
+	}
+      else
+	{
+	  struct thread_info *saved_inferior;
+
+	  saved_inferior = current_inferior;
+	  current_inferior = get_lwp_thread (child);
+
+	  child->stopped_by_watchpoint
+	    = the_low_target.stopped_by_watchpoint ();
+
+	  if (child->stopped_by_watchpoint)
+	    {
+	      if (the_low_target.stopped_data_address != NULL)
+		child->stopped_data_address
+		  = the_low_target.stopped_data_address ();
+	      else
+		child->stopped_data_address = 0;
+	    }
+
+	  current_inferior = saved_inferior;
+	}
+    }
+
   if (debug_threads
       && WIFSTOPPED (*wstatp)
       && the_low_target.get_pc != NULL)
@@ -1724,7 +1769,7 @@ wait_for_sigstop (struct inferior_list_e
 	 signal.  */
       if (WSTOPSIG (wstat) == SIGTRAP
 	  && lwp->stepping
-	  && !linux_stopped_by_watchpoint ())
+	  && !lwp->stopped_by_watchpoint)
 	{
 	  if (debug_threads)
 	    fprintf (stderr, "  single-step SIGTRAP ignored\n");
@@ -1878,6 +1923,7 @@ linux_resume_one_lwp (struct lwp_info *l
 			   get_lwp_thread (lwp));
   errno = 0;
   lwp->stopped = 0;
+  lwp->stopped_by_watchpoint = 0;
   lwp->stepping = step;
   ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (lwp), 0,
 	  /* Coerce to a uintptr_t first to avoid potential gcc warning
@@ -2817,19 +2863,17 @@ linux_remove_point (char type, CORE_ADDR
 static int
 linux_stopped_by_watchpoint (void)
 {
-  if (the_low_target.stopped_by_watchpoint != NULL)
-    return the_low_target.stopped_by_watchpoint ();
-  else
-    return 0;
+  struct lwp_info *lwp = get_thread_lwp (current_inferior);
+
+  return lwp->stopped_by_watchpoint;
 }
 
 static CORE_ADDR
 linux_stopped_data_address (void)
 {
-  if (the_low_target.stopped_data_address != NULL)
-    return the_low_target.stopped_data_address ();
-  else
-    return 0;
+  struct lwp_info *lwp = get_thread_lwp (current_inferior);
+
+  return lwp->stopped_data_address;
 }
 
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)
Index: src/gdb/gdbserver/linux-low.h
===================================================================
--- src.orig/gdb/gdbserver/linux-low.h	2010-03-14 18:31:41.000000000 +0000
+++ src/gdb/gdbserver/linux-low.h	2010-03-14 18:32:00.000000000 +0000
@@ -161,6 +161,16 @@ struct lwp_info
   int pending_is_breakpoint;
   CORE_ADDR pending_stop_pc;
 
+  /* STOPPED_BY_WATCHPOINT is non-zero if this LWP stopped with a data
+     watchpoint trap.  */
+  int stopped_by_watchpoint;
+
+  /* On architectures where it is possible to know the data address of
+     a triggered watchpoint, STOPPED_DATA_ADDRESS is non-zero, and
+     contains such data address.  Only valid if STOPPED_BY_WATCHPOINT
+     is true.  */
+  CORE_ADDR stopped_data_address;
+
   /* If this is non-zero, it is a breakpoint to be reinserted at our next
      stop (SIGTRAP stops only).  */
   CORE_ADDR bp_reinsert;

Re: Fix watchthreads-reorder.exp fails in linux gdbserver

[Joel Brobecker]

> I've applied the patch below, to address in linux gdbserver the problem
> watchthreads-reorder.exp uncovers, similarly to how it was fixed
> in linux-nat.c.

This is a bit off topic, and even maybe a dream-that-will-never-come-true,
but I've been wondering about sharing the wait loop between GDB and gdbserver.
Sounds like a big job at the very least (in order to extract the code
from GDB for the platform currently supported), but do you think that
it's actually doable / worth the effort? It's just that we're (actually,
mostly you!) are very regularly fixing the same problem twice; once
in GDB, and then once in gdbserver. Given the complexity of this code
on some platforms, it's a real shame.

-- 
Joel

Re: Fix watchthreads-reorder.exp fails in linux gdbserver

[Pedro Alves]

On Monday 15 March 2010 00:50:06, Joel Brobecker wrote:
> > I've applied the patch below, to address in linux gdbserver the problem
> > watchthreads-reorder.exp uncovers, similarly to how it was fixed
> > in linux-nat.c.
> 
> This is a bit off topic, and even maybe a dream-that-will-never-come-true,
> but I've been wondering about sharing the wait loop between GDB and gdbserver.

This topic comes up all the time.

> Sounds like a big job at the very least (in order to extract the code
> from GDB for the platform currently supported), but do you think that
> it's actually doable / worth the effort?

It's doable, and in fact, I've been keeping that in mind over
the months, in the direction the non-stop and multi-process
gdbserver support changes have taken.

But, as you guessed, it would take a lot more effort.

> It's just that we're (actually,
> mostly you!) are very regularly fixing the same problem twice; once
> in GDB, and then once in gdbserver. Given the complexity of this code
> on some platforms, it's a real shame.

I believe that at some point this will end up happening, and it
will be GDBserver's side that will prevail.  It used to be that
native gdb was much more featureful than gdbserver, but the balance
is now starting to tip to the other direction.

-- 
Pedro Alves

Re: Fix watchthreads-reorder.exp fails in linux gdbserver

[Joel Brobecker]

> I believe that at some point this will end up happening, and it
> will be GDBserver's side that will prevail.  It used to be that
> native gdb was much more featureful than gdbserver, but the balance
> is now starting to tip to the other direction.

That, plus the fact that I think that the gdbserver side might be less
entangled - so the code might be easier to pull out and isolate?

In any case, thanks for sharing your thoughts...

-- 
Joel, gdbserver novice ;-)