Fix assert in gdbserver for watchpoints watching the same address

GDB can create watchpoints watching the same address.
This was causing assertion failures.
To handle this, hash table (with key watched address) is replaced
by an xarray of address/lengh/kind.
Fully identical watches are ignored (either not inserted, and
not causing a problem if already deleted).

gdbserver_tests/mcwatchpoint enhanced to test duplicated watchpoints

git-svn-id: svn://svn.valgrind.org/valgrind/trunk@12637 a5019735-40e9-0310-863c-91ae7b9d1cf9

diff --git a/NEWS b/NEWS
index 737ce1d7ffb2a48351dddc7500b9c4048af9d629..7ed0f0b0369b8bde778e51171272171e4690680f 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -108,6 +108,7 @@ n-i-bz  s390x: Shadow registers can now be examined using vgdb
 n-i-bz  Bypass gcc4.4/4.5 wrong code generation causing out of memory or asserts
 n-i-bz  Add missing gdbserver xml files for shadow registers for ppc32
 n-i-bz  Fix false positive in sys_clone on amd64 when optional args are not given (e.g. child_tidptr)
+n-i-bz  Fix assert in gdbserver for watchpoints watching the same address
 
 Release 3.7.0 (5 November 2011)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/coregrind/m_gdbserver/m_gdbserver.c b/coregrind/m_gdbserver/m_gdbserver.c
index 3bf7c02ce4bafba10b69aefeae1815a09c1b7bea..9851167d62ed36e2510a86d0f7ae3e8a181704d2 100644 (file)
--- a/coregrind/m_gdbserver/m_gdbserver.c
+++ b/coregrind/m_gdbserver/m_gdbserver.c
@@ -39,6 +39,7 @@
 #include "pub_core_threadstate.h"
 #include "pub_core_transtab.h"
 #include "pub_tool_hashtable.h"
+#include "pub_tool_xarray.h"
 #include "pub_core_libcassert.h"
 #include "pub_tool_libcbase.h"
 #include "pub_core_libcsignal.h"
@@ -139,6 +140,7 @@ static char* sym (Addr addr, Bool is_code)
    static int w = 0;
    PtrdiffT offset;
    if (w == 2) w = 0;
+   buf[w][0] = '\0';
    if (is_code) {
       VG_(describe_IP) (addr, buf[w], 200);
    } else {
@@ -152,6 +154,18 @@ static char* sym (Addr addr, Bool is_code)
 static int gdbserver_called = 0;
 static int gdbserver_exited = 0;
 
+/* alloc and free functions for xarray and similar. */
+static void* gs_alloc (HChar* cc, SizeT sz)
+{
+   void* res = VG_(arena_malloc)(VG_AR_CORE, cc, sz);
+   vg_assert (res);
+   return res;
+}
+static void gs_free (void* ptr)
+{
+   VG_(arena_free)(VG_AR_CORE, ptr);
+}
+
 typedef
    enum {
      GS_break,
@@ -221,21 +235,52 @@ char* VG_(ppPointKind) (PointKind kind)
    case write_watchpoint:    return "write_watchpoint";
    case read_watchpoint:     return "read_watchpoint";
    case access_watchpoint:   return "access_watchpoint";
-   default: vg_assert(0);
+   default:                  return "???wrong PointKind";
    }
 }
 
 typedef
    struct _GS_Watch {
-      struct _GS_Watch* next;
       Addr    addr;
       SizeT   len;
       PointKind kind;
    }
    GS_Watch;
 
-/* gs_watches contains a list of all addresses+len that are being watched. */
-static VgHashTable gs_watches = NULL;
+/* gs_watches contains a list of all addresses+len+kind that are being
+   watched. */
+static XArray* gs_watches = NULL;
+
+static inline GS_Watch* index_gs_watches(Word i)
+{
+   return *(GS_Watch **) VG_(indexXA) (gs_watches, i);
+}
+
+/* Returns the GS_Watch matching addr/len/kind and sets *g_ix to its
+   position in gs_watches.
+   If no matching GS_Watch is found, returns NULL and sets g_ix to -1. */
+static GS_Watch* lookup_gs_watch (Addr addr, SizeT len, PointKind kind,
+                                  Word* g_ix)
+{
+   const Word n_elems = VG_(sizeXA) (gs_watches);
+   Word i;
+   GS_Watch *g;
+
+   /* Linear search. If we have many watches, this might be optimised
+      by having the array sorted and using VG_(lookupXA) */
+   for (i = 0; i < n_elems; i++) {
+      g = index_gs_watches(i);
+      if (g->addr == addr && g->len == len && g->kind == kind) {
+         // Found.
+         *g_ix = i;
+         return g;
+      }
+   }
+
+   // Not found.
+   *g_ix = -1;
+   return NULL;
+}
 
 
 /* protocol spec tells the below must be idempotent. */
@@ -290,6 +335,7 @@ Bool VG_(gdbserver_point) (PointKind kind, Bool insert,
 {
    Bool res;
    GS_Watch *g;
+   Word g_ix;
    Bool is_code = kind == software_breakpoint || kind == hardware_breakpoint;
 
    dlog(1, "%s %s at addr %p %s\n",
@@ -314,7 +360,10 @@ Bool VG_(gdbserver_point) (PointKind kind, Bool insert,
    if (!res) 
       return False; /* error or unsupported */
 
-   g = VG_(HT_lookup) (gs_watches, (UWord)addr);
+   // Protocol says insert/remove must be idempotent.
+   // So, we just ignore double insert or (supposed) double delete.
+
+   g = lookup_gs_watch (addr, len, kind, &g_ix);
    if (insert) {
       if (g == NULL) {
          g = VG_(arena_malloc)(VG_AR_CORE, "gdbserver_point watchpoint",
@@ -322,27 +371,38 @@ Bool VG_(gdbserver_point) (PointKind kind, Bool insert,
          g->addr = addr;
          g->len  = len;
          g->kind = kind;
-         VG_(HT_add_node)(gs_watches, g);
+         VG_(addToXA)(gs_watches, &g);
       } else {
-         g->kind = kind;
+         dlog(1, 
+              "VG_(gdbserver_point) addr %p len %d kind %s already inserted\n",
+               C2v(addr), len, VG_(ppPointKind) (kind));
       }
    } else {
-      vg_assert (g != NULL);
-      VG_(HT_remove) (gs_watches, g->addr);
-      VG_(arena_free) (VG_AR_CORE, g);
+      if (g != NULL) {
+         VG_(removeIndexXA) (gs_watches, g_ix);
+         VG_(arena_free) (VG_AR_CORE, g);
+      } else {
+         dlog(1, 
+              "VG_(gdbserver_point) addr %p len %d kind %s already deleted?\n",
+              C2v(addr), len, VG_(ppPointKind) (kind));
+      }
    }  
    return True;
 }
 
 Bool VG_(is_watched)(PointKind kind, Addr addr, Int szB)
 {
+   Word n_elems;
    GS_Watch* g;
+   Word i;
    Bool watched = False;
    const ThreadId tid = VG_(running_tid);
 
    if (!gdbserver_called)
       return False;
 
+   n_elems = VG_(sizeXA) (gs_watches);
+
    Addr to = addr + szB; // semi-open interval [addr, to[
 
    vg_assert (kind == access_watchpoint 
@@ -350,8 +410,9 @@ Bool VG_(is_watched)(PointKind kind, Addr addr, Int szB)
               || kind == write_watchpoint);
    dlog(1, "tid %d VG_(is_watched) %s addr %p szB %d\n",
         tid, VG_(ppPointKind) (kind), C2v(addr), szB);
-   VG_(HT_ResetIter) (gs_watches);
-   while ((g = VG_(HT_Next) (gs_watches))) {
+
+   for (i = 0; i < n_elems; i++) {
+      g = index_gs_watches(i);
       switch (g->kind) {
       case software_breakpoint:
       case hardware_breakpoint:
@@ -473,26 +534,29 @@ static void clear_gdbserved_addresses(Bool clear_only_jumps)
    VG_(free) (ag);
 }
 
-// Clear watched addressed in gs_watches
+// Clear watched addressed in gs_watches, delete gs_watches.
 static void clear_watched_addresses(void)
 {
-   GS_Watch** ag;
-   UInt n_elems;
-   int i;
+   GS_Watch* g;
+   const Word n_elems = VG_(sizeXA) (gs_watches);
+   Word i;
 
    dlog(1,
-        "clear_watched_addresses: scanning hash table nodes %d\n", 
-        VG_(HT_count_nodes) (gs_watches));
-   ag = (GS_Watch**) VG_(HT_to_array) (gs_watches, &n_elems);
+        "clear_watched_addresses: %ld elements\n", 
+        n_elems);
+   
    for (i = 0; i < n_elems; i++) {
-      if (!VG_(gdbserver_point) (ag[i]->kind,
+      g = index_gs_watches(i);
+      if (!VG_(gdbserver_point) (g->kind,
                                  /* insert */ False,
-                                 ag[i]->addr,
-                                 ag[i]->len)) {
+                                 g->addr,
+                                 g->len)) {
          vg_assert (0);
       }
    }
-   VG_(free) (ag);
+
+   VG_(deleteXA) (gs_watches);
+   gs_watches = NULL;
 }
 
 static void invalidate_if_jump_not_yet_gdbserved (Addr addr, char* from)
@@ -546,8 +610,6 @@ static void gdbserver_cleanup_in_child_after_fork(ThreadId me)
       VG_(HT_destruct) (gs_addresses, VG_(free));
       gs_addresses = NULL;
       clear_watched_addresses();
-      VG_(HT_destruct) (gs_watches, VG_(free));
-      gs_watches = NULL;
    } else {
       vg_assert (gs_addresses == NULL);
       vg_assert (gs_watches == NULL);
@@ -592,7 +654,10 @@ static void call_gdbserver ( ThreadId tid , CallReason reason)
       vg_assert (gs_addresses == NULL);
       vg_assert (gs_watches == NULL);
       gs_addresses = VG_(HT_construct)( "gdbserved_addresses" );
-      gs_watches = VG_(HT_construct)( "gdbserved_watches" );
+      gs_watches = VG_(newXA)(gs_alloc,
+                              "gdbserved_watches",
+                              gs_free,
+                              sizeof(GS_Watch*));
       VG_(atfork)(NULL, NULL, gdbserver_cleanup_in_child_after_fork);
    }
    vg_assert (gs_addresses != NULL);
@@ -1326,7 +1391,7 @@ void VG_(gdbserver_status_output)(void)
    const int nr_gdbserved_addresses 
       = (gs_addresses == NULL ? -1 : VG_(HT_count_nodes) (gs_addresses));
    const int nr_watchpoints
-      = (gs_watches == NULL ? -1 : VG_(HT_count_nodes) (gs_watches));
+      = (gs_watches == NULL ? -1 : (int) VG_(sizeXA) (gs_watches));
    remote_utils_output_status();
    VG_(umsg)
       ("nr of calls to gdbserver: %d\n"

diff --git a/gdbserver_tests/mcwatchpoints.stdinB.gdb b/gdbserver_tests/mcwatchpoints.stdinB.gdb
index 9411ef013c5b947a38e1c494b099d55918e73892..c2750980d1d9c1ad08ec056a6de13b862f183220 100644 (file)
--- a/gdbserver_tests/mcwatchpoints.stdinB.gdb
+++ b/gdbserver_tests/mcwatchpoints.stdinB.gdb
@@ -14,6 +14,9 @@ continue
 rwatch undefined[0]
 awatch undefined[4]
 watch  undefined[8]
+rwatch undefined[9]
+awatch undefined[9]
+watch  undefined[9]
 #
 # now we should encounter 4 break points
 continue

diff --git a/gdbserver_tests/mcwatchpoints.stdoutB.exp b/gdbserver_tests/mcwatchpoints.stdoutB.exp
index 0604aebcbce5f443cb87541102df999654ccb6bf..b0232850161d86d782d525bc47cea382f714e39c 100644 (file)
--- a/gdbserver_tests/mcwatchpoints.stdoutB.exp
+++ b/gdbserver_tests/mcwatchpoints.stdoutB.exp
@@ -5,6 +5,9 @@ Breakpoint 1, breakme (line=19) at watchpoints.c:7
 Hardware read watchpoint 2: undefined[0]
 Hardware access (read/write) watchpoint 3: undefined[4]
 Hardware watchpoint 4: undefined[8]
+Hardware read watchpoint 5: undefined[9]
+Hardware access (read/write) watchpoint 6: undefined[9]
+Hardware watchpoint 7: undefined[9]
 Continuing.
 Hardware read watchpoint 2: undefined[0]
 Value = 117 'u'