[gdbserver 2/2] x86-linux Z0 support, and support multiple breakpoints at the same address.

[Pedro Alves <pedro at codesourcery dot com>]

With the previous patch installed, the testsuite shows one
regression, in gdb.base/unload.exp.

This test revealed that mixing Z0 and a target that
supports shared libraries has one wrinkle not considered before.

The test does:

   1. dlopen library1.
   2. dlsym function from library1, set breakpoint there, run to it.
   3. dlclose library1.

   4. dlopen library2.
   5. dlsym function from library2, set breakpoint there, run to it.
   6. dlclose library2.

library2 is loaded at the same address as library1.

The problem is, when in step #3, the library1 is closed, any
breakpoints that were installed on the target within that library
are gone from memory.  But, GDBserver has no idea the library
was closed --- SVR4 libraries are all handled from within GDB,
with shared library event breakpoints, reading DSO lists off
of inferior memory and such.  GDBserver has no idea the breakpoints
set in library1 are gone.  In this particular test, step #5 ends up
creating a breakpoint at example the same address as step #2.
Since GDBserver at this point still thinks the breakpoint at
#2 is inserted, a second attempt to set a breakpoint at the same
address just increments the reference count of the old breakpoint.
But, remember that the corresponding trap is no longer planted
in the inferior, wiped when library1 was unloaded.

I've thought of several ways to fix this.  My first reaction was
for GDB to tell the target to delete all breakpoints set in
library1, when library1 is found gone.  All removals would naturally
fail, since the shared library's address space has been unmapped.
But, then I realized this has one problem.  It would be possible
for library2 to be loaded before GDB knows that library1 is
gone.  In that case, the target could try to put a shadow built
from library1 back into library2.

This was when I thought of trying to handle this all
within the target side, and came up with this patch.
It does a few things:

 - it ensures the breakpoint still looks inserted in
   a couple of places --- memory reads and memory writes.
   This is so that if the shared library where the breakpoint
   was inserted is gone, and another library is loaded
   in its place, read_inferior_memory doesn't return a buffer
   with the shadows of the old breakpoints on top of the
   new loaded library.

 - when it sees a second GDB breakpoint being installed at
   the same address as a previous GDB breakpoint, it assumes
   the first breakpoint must be gone by now.

 - adds a new function that validates inserted breakpoints.
   That is, it checks to see if the trap still looks
   inserted in the inferior.  If not, we better get rid of
   the breakpoint.

 - for extra safety, when GDBserver sees a qSymbols packet,
   it assumes memory may have changed, and revalidates all
   breakpoints.

As alternatives, I though of coming up with a new
packet that would allow GDB to say memory region
FOO to BAR has changed behind your back.  The target would
then forget about breakpoints in this region.  Alternatively,
I thought that GDBserver could track the mmap syscall, detect
when memory is mapped/unmaped.

Then, I thought that this mechanism of validating memory
as in the patch is something that is needed for
breakpoints in self modifiable code, so it's probably a
good thing to have anyway.  We had to do something similar
for our other target that supports tracepoints, because
it explicitly required supporting breakpoints in JIT
compiled code.

So, I suggest we go with this patch, and try to come up
with something clever further down the road, when we
have a bit more experience with this, and if we find it's
actually needed.

With both patches applied, I have no regressions x86_64-linux
gdbserver.

WDYT?

-- 
Pedro Alves

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

	gdb/gdbserver/
	* mem-break.c (struct raw_breakpoint): New field shlib_disabled.
	(set_gdb_breakpoint_at): If GDB is inserting a breakpoint on top
	of another, then delete the previous, and validate all
	breakpoints.
	(validate_inserted_breakpoint): New.
	(delete_disabled_breakpoints): New.
	(validate_breakpoints): New.
	(check_mem_read): Validate breakpoints before trusting their
	shadow.  Delete disabled breakpoints.
	(check_mem_write): Validate breakpoints before trusting they
	should be inserted.  Delete disabled breakpoints.
	* mem-break.h (validate_breakpoints):
	* server.c (handle_query): Validate breakpoints when we see a
	qSymbol query.

---
 gdb/gdbserver/mem-break.c |  101 +++++++++++++++++++++++++++++++++++++++++++++-
 gdb/gdbserver/mem-break.h |    4 +
 gdb/gdbserver/server.c    |   11 +++++
 3 files changed, 114 insertions(+), 2 deletions(-)

Index: src/gdb/gdbserver/mem-break.c
===================================================================
--- src.orig/gdb/gdbserver/mem-break.c	2010-03-26 15:20:51.000000000 +0000
+++ src/gdb/gdbserver/mem-break.c	2010-03-26 15:49:16.000000000 +0000
@@ -65,6 +65,10 @@ struct raw_breakpoint
   /* Non-zero if this breakpoint is currently inserted in the
      inferior.  */
   int inserted;
+
+  /* Non-zero if this breakpoint is currently disabled because we no
+     longer detect it as inserted.  */
+  int shlib_disabled;
 };
 
 /* The type of a breakpoint.  */
@@ -326,6 +330,24 @@ set_gdb_breakpoint_at (CORE_ADDR where)
   if (breakpoint_data == NULL)
     return 1;
 
+  /* If we see GDB inserting a second breakpoint at the same address,
+     then the first breakpoint must have disappeared due to a shared
+     library unload.  On targets where the shared libraries are
+     handled by userspace, like SVR4, for example, GDBserver can't
+     tell if a library was loaded or unloaded.  Since we refcount
+     breakpoints, if we didn't do this, we'd just increase the
+     refcount of the previous breakpoint at this address, but the trap
+     was not planted in the inferior anymore, thus the breakpoint
+     would never be hit.  */
+  bp = find_gdb_breakpoint_at (where);
+  if (bp != NULL)
+    {
+      delete_gdb_breakpoint_at (where);
+
+      /* Might as well validate all other breakpoints.  */
+      validate_breakpoints ();
+    }
+
   bp = set_breakpoint_at (where, NULL);
   if (bp == NULL)
     return -1;
@@ -543,12 +565,70 @@ breakpoints_supported (void)
   return breakpoint_data != NULL;
 }
 
+static int
+validate_inserted_breakpoint (struct raw_breakpoint *bp)
+{
+  unsigned char *buf;
+  int err;
+
+  gdb_assert (bp->inserted == 1);
+
+  buf = alloca (breakpoint_len);
+  err = (*the_target->read_memory) (bp->pc, buf, breakpoint_len);
+  if (err || memcmp (buf, breakpoint_data, breakpoint_len) != 0)
+    {
+      /* Tag it as gone.  */
+      bp->inserted = 0;
+      bp->shlib_disabled = 1;
+      return 0;
+    }
+
+  return 1;
+}
+
+static void
+delete_disabled_breakpoints (void)
+{
+  struct process_info *proc = current_process ();
+  struct breakpoint *bp, *next;
+
+  for (bp = proc->breakpoints; bp != NULL; bp = next)
+    {
+      next = bp->next;
+      if (bp->raw->shlib_disabled)
+	delete_breakpoint_1 (proc, bp);
+    }
+}
+
+/* Check if breakpoints we inserted still appear to be inserted.  They
+   may disappear due to a shared library unload, and worse, a new
+   shared library may be reloaded at the same address as the
+   previously unloaded one.  If that happens, we should make sure that
+   the shadows memory of the old breakpoints isn't used when reading
+   or writing memory.  */
+
+void
+validate_breakpoints (void)
+{
+  struct process_info *proc = current_process ();
+  struct breakpoint *bp;
+
+  for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
+    {
+      if (bp->raw->inserted)
+	validate_inserted_breakpoint (bp->raw);
+    }
+
+  delete_disabled_breakpoints ();
+}
+
 void
 check_mem_read (CORE_ADDR mem_addr, unsigned char *buf, int mem_len)
 {
   struct process_info *proc = current_process ();
   struct raw_breakpoint *bp = proc->raw_breakpoints;
   CORE_ADDR mem_end = mem_addr + mem_len;
+  int disabled_one = 0;
 
   for (; bp != NULL; bp = bp->next)
     {
@@ -574,8 +654,16 @@ check_mem_read (CORE_ADDR mem_addr, unsi
       buf_offset = start - mem_addr;
 
       if (bp->inserted)
-	memcpy (buf + buf_offset, bp->old_data + copy_offset, copy_len);
+	{
+	  if (validate_inserted_breakpoint (bp))
+	    memcpy (buf + buf_offset, bp->old_data + copy_offset, copy_len);
+	  else
+	    disabled_one = 1;
+	}
     }
+
+  if (disabled_one)
+    delete_disabled_breakpoints ();
 }
 
 void
@@ -584,6 +672,7 @@ check_mem_write (CORE_ADDR mem_addr, uns
   struct process_info *proc = current_process ();
   struct raw_breakpoint *bp = proc->raw_breakpoints;
   CORE_ADDR mem_end = mem_addr + mem_len;
+  int disabled_one = 0;
 
   for (; bp != NULL; bp = bp->next)
     {
@@ -610,8 +699,16 @@ check_mem_write (CORE_ADDR mem_addr, uns
 
       memcpy (bp->old_data + copy_offset, buf + buf_offset, copy_len);
       if (bp->inserted)
-	memcpy (buf + buf_offset, breakpoint_data + copy_offset, copy_len);
+	{
+	  if (validate_inserted_breakpoint (bp))
+	    memcpy (buf + buf_offset, breakpoint_data + copy_offset, copy_len);
+	  else
+	    disabled_one = 1;
+	}
     }
+
+  if (disabled_one)
+    delete_disabled_breakpoints ();
 }
 
 /* Delete all breakpoints, and un-insert them from the inferior.  */
Index: src/gdb/gdbserver/mem-break.h
===================================================================
--- src.orig/gdb/gdbserver/mem-break.h	2010-03-26 15:16:52.000000000 +0000
+++ src/gdb/gdbserver/mem-break.h	2010-03-26 15:49:16.000000000 +0000
@@ -108,4 +108,8 @@ void delete_all_breakpoints (void);
 
 void free_all_breakpoints (struct process_info *proc);
 
+/* Check if breakpoints still seem to be inserted in the inferior.  */
+
+void validate_breakpoints (void);
+
 #endif /* MEM_BREAK_H */
Index: src/gdb/gdbserver/server.c
===================================================================
--- src.orig/gdb/gdbserver/server.c	2010-03-26 15:15:36.000000000 +0000
+++ src/gdb/gdbserver/server.c	2010-03-26 15:49:16.000000000 +0000
@@ -858,6 +858,17 @@ handle_query (char *own_buf, int packet_
 
   if (strcmp ("qSymbol::", own_buf) == 0)
     {
+      /* GDB is suggesting new symbols have been loaded.  This may
+	 mean a new shared library has been detected as loaded, so
+	 take the opportunity to check if breakpoints we think are
+	 inserted, still are.  Note that it isn't guaranteed that
+	 we'll see this when a shared library is loaded, and nor will
+	 we see this for unloads (although breakpoints in unloaded
+	 libraries shouldn't trigger), as GDB may not find symbols for
+	 the library at all.  We also re-validate breakpoints when we
+	 see a second GDB breakpoint for the same address.  */
+      validate_breakpoints ();
+
       if (target_running () && the_target->look_up_symbols != NULL)
 	(*the_target->look_up_symbols) ();