[PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments

[Patrick Palka <patrick at parcs dot ath dot cx>]

{ v2: Here is my crude attempt at adding a testcase for this changeset.
  I also fixed the bug that I alluded to earlier. }

PR 12526 reports that -location watchpoints against bitfield arguments
trigger false positives when bits around the bitfield, but not the
bitfield itself, are modified.

This happens because -location watchpoints naturally operate at the byte
level, not at the bit level.  When the address of a bitfield lvalue is
taken, information about the bitfield (i.e. its offset and size) is lost
in the process.

This information must first be retained throughout the lifetime of the
-location watchpoint.  This patch achieves this by adding two new fields
to the watchpoint struct: val_bitpos and val_bitsize.  These fields are
set when a watchpoint is first defined in watch_command_1().  They are
both equal to zero if the watchpoint is not a -location watchpoint or if
the argument is not a bitfield.

Then these bitfield parameters are used inside update_watchpoint() and
watchpoint_check() to extract the actual value of the bitfield from the
watchpoint address, with the help of a local helper function
extract_bitfield_from_watchpoint_value().

Finally when creating a HW breakpoint pointing to a bitfield, we
optimize the address and length of the breakpoint.  By skipping over the
bytes that don't cover the bitfield, this step reduces the frequency at
which a read watchpoint for the bitfield is triggered.  It also reduces
the number of times a false-positive call to check_watchpoint() is
triggered for a write watchpoint.

gdb/
	PR breakpoints/12526
	* breakpoint.h (struct watchpoint): New fields val_bitpos and
	val_bitsize.
	* breakpoint.c (watch_command_1): Use these fields to retain
	bitfield information.
	(extract_bitfield_from_watchpoint_value): New function.
	(watchpoint_check): Use it.
	(update_watchpoint): Use it.  Optimize the address and length
	of a HW watchpoint pointing to a bitfield.

gdb/testsuite/
	PR breakpoints/12526
	* gdb.base/pr12526.exp: New file.
	* gdb.base/pr12526.c: New file.
---
 gdb/breakpoint.c                   | 67 +++++++++++++++++++++++++++++++++++++-
 gdb/breakpoint.h                   |  5 +++
 gdb/testsuite/gdb.base/pr12526.c   | 54 ++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/pr12526.exp | 47 ++++++++++++++++++++++++++
 4 files changed, 172 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/pr12526.c
 create mode 100644 gdb/testsuite/gdb.base/pr12526.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 683ed2b..7b7c74b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1703,6 +1703,31 @@ watchpoint_del_at_next_stop (struct watchpoint *w)
   b->disposition = disp_del_at_next_stop;
 }
 
+/* Extract a bitfield value from value VAL using the bit parameters contained in
+   watchpoint W.  */
+
+static struct value *
+extract_bitfield_from_watchpoint_value (struct watchpoint *w, struct value *val)
+{
+  LONGEST bit_val;
+  int ok;
+
+  if (val == NULL)
+    return NULL;
+
+  ok = unpack_value_bits_as_long (value_type (val),
+				  value_contents_for_printing (val),
+				  value_offset (val),
+				  w->val_bitpos,
+				  w->val_bitsize,
+				  val,
+				  &bit_val);
+  if (ok)
+    return value_from_longest (value_type (val), bit_val);
+
+  return NULL;
+}
+
 /* Assuming that B is a watchpoint:
    - Reparse watchpoint expression, if REPARSE is non-zero
    - Evaluate expression and store the result in B->val
@@ -1877,6 +1902,12 @@ update_watchpoint (struct watchpoint *b, int reparse)
 	 watchpoints.  */
       if (!b->val_valid && !is_masked_watchpoint (&b->base))
 	{
+	  if (b->val_bitsize)
+	    {
+	      v = extract_bitfield_from_watchpoint_value (b, v);
+	      if (v)
+		release_value (v);
+	    }
 	  b->val = v;
 	  b->val_valid = 1;
 	}
@@ -1906,8 +1937,24 @@ update_watchpoint (struct watchpoint *b, int reparse)
 		  CORE_ADDR addr;
 		  int type;
 		  struct bp_location *loc, **tmp;
+		  int bitpos = 0, bitsize = 0;
+
+		  if (value_bitsize (v))
+		    {
+		      bitpos = value_bitpos (v);
+		      bitsize = value_bitsize (v);
+		    }
+		  else if (v == result && b->val_bitsize)
+		    {
+		      bitpos = b->val_bitpos;
+		      bitsize = b->val_bitsize;
+		    }
 
 		  addr = value_address (v);
+		  if (bitsize)
+		    /* Skip the bytes that don't contain the bitfield.  */
+		    addr += bitpos / 8;
+
 		  type = hw_write;
 		  if (b->base.type == bp_read_watchpoint)
 		    type = hw_read;
@@ -1922,7 +1969,13 @@ update_watchpoint (struct watchpoint *b, int reparse)
 
 		  loc->pspace = frame_pspace;
 		  loc->address = addr;
-		  loc->length = TYPE_LENGTH (value_type (v));
+
+		  if (bitsize)
+		    /* Just cover the bytes that make up the bitfield.  */
+		    loc->length = ((bitpos % 8) + bitsize + 7) / 8;
+		  else
+		    loc->length = TYPE_LENGTH (value_type (v));
+
 		  loc->watchpoint_type = type;
 		}
 	    }
@@ -5039,6 +5092,9 @@ watchpoint_check (void *p)
       mark = value_mark ();
       fetch_subexp_value (b->exp, &pc, &new_val, NULL, NULL, 0);
 
+      if (b->val_bitsize)
+	new_val = extract_bitfield_from_watchpoint_value (b, new_val);
+
       /* We use value_equal_contents instead of value_equal because
 	 the latter coerces an array to a pointer, thus comparing just
 	 the address of the array instead of its contents.  This is
@@ -11172,6 +11228,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   struct expression *exp;
   const struct block *exp_valid_block = NULL, *cond_exp_valid_block = NULL;
   struct value *val, *mark, *result;
+  int saved_bitpos = 0, saved_bitsize = 0;
   struct frame_info *frame;
   const char *exp_start = NULL;
   const char *exp_end = NULL;
@@ -11305,6 +11362,12 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   mark = value_mark ();
   fetch_subexp_value (exp, &pc, &val, &result, NULL, just_location);
 
+  if (val && just_location)
+    {
+      saved_bitpos = value_bitpos (val);
+      saved_bitsize = value_bitsize (val);
+    }
+
   if (just_location)
     {
       int ret;
@@ -11440,6 +11503,8 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   else
     {
       w->val = val;
+      w->val_bitpos = saved_bitpos;
+      w->val_bitsize = saved_bitsize;
       w->val_valid = 1;
     }
 
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index f6d06ce..2b80af1 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -779,6 +779,11 @@ struct watchpoint
      then an error occurred reading the value.  */
   int val_valid;
 
+  /* When watching the location of a bitfield, contains the offset and size of
+     the bitfield.  Otherwise contains 0.  */
+  int val_bitpos;
+  int val_bitsize;
+
   /* Holds the frame address which identifies the frame this
      watchpoint should be evaluated in, or `null' if the watchpoint
      should be evaluated on the outermost frame.  */
diff --git a/gdb/testsuite/gdb.base/pr12526.c b/gdb/testsuite/gdb.base/pr12526.c
new file mode 100644
index 0000000..b51926d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr12526.c
@@ -0,0 +1,54 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+struct foo
+{
+  unsigned long a:1;
+  unsigned char b:2;
+  unsigned long c:3;
+  char d:4;
+  int e:5;
+  char f:6;
+  int g:7;
+  long h:8;
+} q = { 0 };
+
+int
+main (void)
+{
+  q.a = 1;
+  q.b = 2;
+  q.c = 3;
+  q.d = 4;
+  q.e = 5;
+  q.f = 6;
+  q.g = -7;
+  q.h = -8;
+
+  q.a--;
+  q.b--;
+  q.e--;
+  q.d--;
+  q.c--;
+
+  q.f--;
+  q.g--;
+  q.h--;
+
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/pr12526.exp b/gdb/testsuite/gdb.base/pr12526.exp
new file mode 100644
index 0000000..ed99bdc
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr12526.exp
@@ -0,0 +1,47 @@
+# Copyright 2014 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
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+# Test correctness of watchpoints on bitfields.
+
+gdb_test "watch -l q.c"
+gdb_test "cont" "q\.c.*Old value = 0.*New value = 3.*"
+gdb_test "watch -l q.d"
+gdb_test "cont" "q\.d.*Old value = 0.*New value = 4.*"
+gdb_test "watch q.e"
+gdb_test "cont" "q\.e.*Old value = 0.*New value = 5.*"
+
+gdb_test "cont" "q\.e.*Old value = 5.*New value = 4.*"
+gdb_test "cont" "q\.d.*Old value = 4.*New value = 3.*"
+gdb_test "cont" "q\.c.*Old value = 3.*New value = 2.*"
+
+delete_breakpoints
+gdb_test "watch q.f"
+gdb_test "watch q.g"
+gdb_test "watch -l q.h"
+gdb_test "cont" "q\.f.*Old value = 6.*New value = 5.*"
+gdb_test "cont" "q\.g.*Old value = -7.*New value = -8.*"
+gdb_test "cont" "q\.h.*Old value = -8.*New value = -9.*"
-- 
2.1.0