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

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

[Patrick Palka]

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.

	PR gdb/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/breakpoint.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/breakpoint.h |  5 +++++
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 683ed2b..7e8fbd1 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 += bitsize / 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.  */
-- 
2.1.0

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

[Patrick Palka]

Oops, found a bug: the line

    addr += bitsize / 8;

ought to be

    addr += bitpos / 8;

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

[Patrick Palka]

{ 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

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

[Patrick Palka]

On Wed, Aug 20, 2014 at 11:32 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> { 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
>

Ping.

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

[Patrick Palka]

On Wed, Aug 27, 2014 at 10:28 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Wed, Aug 20, 2014 at 11:32 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> { 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
>>
>
> Ping.

Ping.

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

[Pedro Alves]

Hi Patrick,

Thanks for addressing this!

Overall this looks reasonable.  Comments below.

On 08/21/2014 04:32 AM, Patrick Palka wrote:
> { 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)

Please no implicit boolean conversion, here and elsewhere.
This is a size, so use != 0 or > 0.

> +	    {
> +	      v = extract_bitfield_from_watchpoint_value (b, v);
> +	      if (v)

A pointer, so:

	      if (v != NULL)

etc.

> +		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;
> +		    }

Can you explain these conditions a bit more?  It's not obvious
to me -- even if I hack away the whole "else if" block, the new
test still passes for me?

>  
>  		  addr = value_address (v);
> +		  if (bitsize)
> +		    /* Skip the bytes that don't contain the bitfield.  */
> +		    addr += bitpos / 8;
> +

From:

 https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

"
Any two or more lines in code should be wrapped in braces, even if they are comments, as they look like separate statements:

if (i)
  {
    /* Return success.  */
    return 0;
  }
"

>  		  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;

Likewise here.

> +		  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

Please give the test files a more meaningful name.  Something
like watch-bitfields.{c,exp}, for example.  That way it's much
easier to identify what the test is exercising, and, we can
do things like:

  make check TESTS="gdb.*/*watch*.exp"

to quickly run only (roughly) watchpoint-related tests.

> +# 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.*"

There seems to be "kinds" of patterns being tested here.
The before delete_breakpoints part, and the after part.
Could you add a little comment explaining what they are?
Like "First test that watching foo when bar works".  Etc.

Also, please watch out for duplicate messages:

  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

Might it be good to extend the test a little adding cases that involve
more than one bitfield in an expression, thus covering the optimization
in the loop in update_watchpoint, for more than one iteration?
Like, "watch q.a + q.f", "watch q.a + some_int" and "watch some_int + q.a"
or some such.  What do you think?

Thanks,
Pedro Alves

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

[Patrick Palka]

On Thu, Sep 4, 2014 at 11:04 AM, Pedro Alves <palves@redhat.com> wrote:
> Hi Patrick,
>
> Thanks for addressing this!
>
> Overall this looks reasonable.  Comments below.
>
> On 08/21/2014 04:32 AM, Patrick Palka wrote:
>> { 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)
>
> Please no implicit boolean conversion, here and elsewhere.
> This is a size, so use != 0 or > 0.
>
>> +         {
>> +           v = extract_bitfield_from_watchpoint_value (b, v);
>> +           if (v)
>
> A pointer, so:
>
>               if (v != NULL)
>
> etc.
>
>> +             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;
>> +                 }
>
> Can you explain these conditions a bit more?  It's not obvious
> to me -- even if I hack away the whole "else if" block, the new
> test still passes for me?

The new test still passes because these conditions are only
optimizations.  They optimize the width of a HW watchpoint
cerresponding to a bitfield value as opposed to a full-width value.
Normally such a watchpoint would span the entire width of the
bitfield's base type, but such a watchpoint only has to span the bytes
that contain the bits of the bitfield.  Either way there should be no
change in observed behavior.

The first condition "(value_bitsize (v) != 0)" is for regular
watchpoints, e.g. "watch q.a + q.b + c" (where q.a and q.b are
bitfields), for determining whether a subexpression is a bitfield.

The second condition "(v == result && b->val_bitsize != 0)" is for
-location watchpoints, e.g. "watch -l q.a", for determining whether
the main expression is a bitfield.  The first part of the conjunction
tests whether the current value in the value chain is the result
value.  The second part tests whether the result value is a bitfield
(as determined when we first set val_bitsize in watch_command_1()).
The first condition won't work in this case because we lose the
bitfield information of the main expression as "watch -l q.a"
essentially becomes "watch *(int *)0xaddr".

>
>>
>>                 addr = value_address (v);
>> +               if (bitsize)
>> +                 /* Skip the bytes that don't contain the bitfield.  */
>> +                 addr += bitpos / 8;
>> +
>
> From:
>
>  https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
>
> "
> Any two or more lines in code should be wrapped in braces, even if they are comments, as they look like separate statements:
>
> if (i)
>   {
>     /* Return success.  */
>     return 0;
>   }
> "
>
>>                 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;
>
> Likewise here.
>
>> +               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
>
> Please give the test files a more meaningful name.  Something
> like watch-bitfields.{c,exp}, for example.  That way it's much
> easier to identify what the test is exercising, and, we can
> do things like:
>
>   make check TESTS="gdb.*/*watch*.exp"
>
> to quickly run only (roughly) watchpoint-related tests.

Done.  I also updated the coding style of the patch according to your remarks.

>
>> +# 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.*"
>
> There seems to be "kinds" of patterns being tested here.
> The before delete_breakpoints part, and the after part.
> Could you add a little comment explaining what they are?
> Like "First test that watching foo when bar works".  Etc.
>
> Also, please watch out for duplicate messages:
>
>   https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique
>
> Might it be good to extend the test a little adding cases that involve
> more than one bitfield in an expression, thus covering the optimization
> in the loop in update_watchpoint, for more than one iteration?
> Like, "watch q.a + q.f", "watch q.a + some_int" and "watch some_int + q.a"
> or some such.  What do you think?

Good point.  I rewrote the test case to test a compound watchpoint
expression as you suggested.  I also simplified the test case and
added a few comments.  I'm not sure I understand your comment about
duplicate messages.  What is a "message", in this case?  From what I
understand, the message corresponds to the third argument of gdb_test,
which I'm always omitting.  Also I don't see any of the "@0" or "@1"
stuff that the wiki page alludes to in the output of the test case.
Does that mean my test has no duplicate messages?

Patrick

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

[Patrick Palka]

{ v2: Here is my crude attempt at adding a testcase for this changeset. I
  also fixed the bug that I mentioned 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.
	* value.h (unpack_value_bitfield): New prototype.
	* value.c (unpack_value_bitfield): Make extern.

gdb/testsuite/
	PR breakpoints/12526
	* gdb.base/watch-bitfields.exp: New file.
	* gdb.base/watch-bitfields.c: New file.
---
 gdb/breakpoint.c                           | 74 +++++++++++++++++++++++++++++-
 gdb/breakpoint.h                           |  5 ++
 gdb/testsuite/gdb.base/watch-bitfields.c   | 54 ++++++++++++++++++++++
 gdb/testsuite/gdb.base/watch-bitfields.exp | 56 ++++++++++++++++++++++
 gdb/value.c                                |  2 +-
 gdb/value.h                                |  5 ++
 6 files changed, 194 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/watch-bitfields.c
 create mode 100644 gdb/testsuite/gdb.base/watch-bitfields.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 683ed2b..2d33ff7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1703,6 +1703,29 @@ 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)
+{
+  struct value *bit_val;
+
+  if (val == NULL)
+    return NULL;
+
+  bit_val = allocate_value (value_type (val));
+
+  unpack_value_bitfield (bit_val,
+			 w->val_bitpos,
+			 w->val_bitsize,
+			 value_contents_for_printing (val),
+			 value_offset (val),
+			 val);
+
+  return bit_val;
+}
+
 /* 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 +1900,12 @@ update_watchpoint (struct watchpoint *b, int reparse)
 	 watchpoints.  */
       if (!b->val_valid && !is_masked_watchpoint (&b->base))
 	{
+	  if (b->val_bitsize != 0)
+	    {
+	      v = extract_bitfield_from_watchpoint_value (b, v);
+	      if (v != NULL)
+		release_value (v);
+	    }
 	  b->val = v;
 	  b->val_valid = 1;
 	}
@@ -1906,8 +1935,31 @@ 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) != 0)
+		    {
+		      /* Extract the bit parameters out from the bitfield
+			 sub-expression.  */
+		      bitpos = value_bitpos (v);
+		      bitsize = value_bitsize (v);
+		    }
+		  else if (v == result && b->val_bitsize != 0)
+		    {
+		     /* If VAL_BITSIZE != 0 then RESULT is actually a bitfield
+			lvalue whose bit parameters are saved in the fields
+			VAL_BITPOS and VAL_BITSIZE.  */
+		      bitpos = b->val_bitpos;
+		      bitsize = b->val_bitsize;
+		    }
 
 		  addr = value_address (v);
+		  if (bitsize != 0)
+		    {
+		      /* 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 +1974,15 @@ update_watchpoint (struct watchpoint *b, int reparse)
 
 		  loc->pspace = frame_pspace;
 		  loc->address = addr;
-		  loc->length = TYPE_LENGTH (value_type (v));
+
+		  if (bitsize != 0)
+		    {
+		      /* 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 +5099,9 @@ watchpoint_check (void *p)
       mark = value_mark ();
       fetch_subexp_value (b->exp, &pc, &new_val, NULL, NULL, 0);
 
+      if (b->val_bitsize != 0)
+	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 +11235,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 +11369,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 +11510,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/watch-bitfields.c b/gdb/testsuite/gdb.base/watch-bitfields.c
new file mode 100644
index 0000000..fb57885
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-bitfields.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.h--;
+  q.c--;
+  q.b--;
+  q.e--;
+  q.d--;
+  q.c--;
+  q.f--;
+  q.g--;
+  q.h--;
+
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watch-bitfields.exp b/gdb/testsuite/gdb.base/watch-bitfields.exp
new file mode 100644
index 0000000..3f25384
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-bitfields.exp
@@ -0,0 +1,56 @@
+# 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
+}
+
+# Continue inferior execution, expecting the watchpoint EXPR to be triggered
+# having old value OLD and new value NEW.
+proc expect_watchpoint { expr old new } {
+    set expr_re [string_to_regexp $expr]
+    gdb_test "print $expr" "\\$\\d+ = $old\\s"
+    gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*"
+    gdb_test "print $expr" "\\$\\d+ = $new\\s"
+}
+
+# Check that -location watchpoints against bitfields trigger properly.
+gdb_test "watch -l q.a"
+gdb_test "watch -l q.e"
+expect_watchpoint "q.a" 0 1
+expect_watchpoint "q.e" 0 5
+expect_watchpoint "q.a" 1 0
+expect_watchpoint "q.e" 5 4
+gdb_test "cont" ".*exited normally.*"
+
+# Check that regular watchpoints against expressions involving bitfields
+# trigger properly.
+runto_main
+gdb_test "watch q.d + q.f + q.g"
+expect_watchpoint "q.d + q.f + q.g" 0 4
+expect_watchpoint "q.d + q.f + q.g" 4 10
+expect_watchpoint "q.d + q.f + q.g" 10 3
+expect_watchpoint "q.d + q.f + q.g" 3 2
+expect_watchpoint "q.d + q.f + q.g" 2 1
+expect_watchpoint "q.d + q.f + q.g" 1 0
+gdb_test "cont" ".*exited normally.*"
diff --git a/gdb/value.c b/gdb/value.c
index 6620f96..fdc8858d 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3231,7 +3231,7 @@ unpack_field_as_long (struct type *type, const gdb_byte *valaddr, int fieldno)
    are unavailable/optimized out, DEST_VAL is correspondingly
    marked unavailable/optimized out.  */
 
-static void
+void
 unpack_value_bitfield (struct value *dest_val,
 		       int bitpos, int bitsize,
 		       const gdb_byte *valaddr, int embedded_offset,
diff --git a/gdb/value.h b/gdb/value.h
index 4cdbf21..e3603c3 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -613,6 +613,11 @@ extern int unpack_value_field_as_long (struct type *type, const gdb_byte *valadd
 				int embedded_offset, int fieldno,
 				const struct value *val, LONGEST *result);
 
+extern void unpack_value_bitfield (struct value *dest_val,
+				   int bitpos, int bitsize,
+				   const gdb_byte *valaddr, int embedded_offset,
+				   const struct value *val);
+
 extern struct value *value_field_bitfield (struct type *type, int fieldno,
 					   const gdb_byte *valaddr,
 					   int embedded_offset,
-- 
2.1.0.60.g85f0837

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

[Sergio Durigan Junior]

Hi Patrick,

Thanks for the patch.  Comments about the testcase below.

On Sunday, September 07 2014, Patrick Palka wrote:

>> Also, please watch out for duplicate messages:
>>
>>   https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique
>>
>> Might it be good to extend the test a little adding cases that involve
>> more than one bitfield in an expression, thus covering the optimization
>> in the loop in update_watchpoint, for more than one iteration?
>> Like, "watch q.a + q.f", "watch q.a + some_int" and "watch some_int + q.a"
>> or some such.  What do you think?
>
> Good point.  I rewrote the test case to test a compound watchpoint
> expression as you suggested.  I also simplified the test case and
> added a few comments.  I'm not sure I understand your comment about
> duplicate messages.  What is a "message", in this case?  From what I
> understand, the message corresponds to the third argument of gdb_test,
> which I'm always omitting.  Also I don't see any of the "@0" or "@1"
> stuff that the wiki page alludes to in the output of the test case.
> Does that mean my test has no duplicate messages?

Ahh, the art of testcase writing :-).

Yes, Pedro meant the third argument of gdb_test.  You are free to omit
it if you are doing something simple (e.g., "cont"), but when you do so,
the test message becomes the first argument of the gdb_test (i.e., the
command itself).  And since you are doing lots of "cont", you will see
lots of "cont" in the messages as well.  Take a look:

  $ cat gdb/testsuite/gdb.sum | grep "PASS" | sort | uniq -c | sort -n                                                                          
        1 PASS: gdb.base/watch-bitfields.exp: watch -l q.a
        1 PASS: gdb.base/watch-bitfields.exp: watch -l q.e
        1 PASS: gdb.base/watch-bitfields.exp: watch q.d + q.f + q.g
        4 PASS: gdb.base/watch-bitfields.exp: print q.a
        4 PASS: gdb.base/watch-bitfields.exp: print q.e
       12 PASS: gdb.base/watch-bitfields.exp: cont
       12 PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g

See that you have 12 tests whose messages are "cont"?

One thing you could do is to actually write the test messages.  Another
thing is to use with_test_prefix, as mentioned in the wiki.  For example:

On Sunday, September 07 2014, Patrick Palka wrote:

> +# Continue inferior execution, expecting the watchpoint EXPR to be triggered
> +# having old value OLD and new value NEW.
> +proc expect_watchpoint { expr old new } {
> +    set expr_re [string_to_regexp $expr]
> +    gdb_test "print $expr" "\\$\\d+ = $old\\s"

  gdb_test "print $expr" "\\$\\d+ = $old\\s" \
      "check if expr == $old"

You also don't need to check for the beginning starting "$<N> = " (but
if you want, you can use $decimal instead of \d+).  No need to check for
\s also.

> +    gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*"

  gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" \
      "check if watch on $expr triggers"

> +    gdb_test "print $expr" "\\$\\d+ = $new\\s"

  gdb_test "print $expr" "\\$\\d+ = $new\\s" \
      "check if expr == $new"

> +}
> +
> +# Check that -location watchpoints against bitfields trigger properly.
> +gdb_test "watch -l q.a"
> +gdb_test "watch -l q.e"
> +expect_watchpoint "q.a" 0 1
> +expect_watchpoint "q.e" 0 5
> +expect_watchpoint "q.a" 1 0
> +expect_watchpoint "q.e" 5 4
> +gdb_test "cont" ".*exited normally.*"

  # Check that -location watchpoints against bitfields trigger properly.
  with_test_prefix "-location watch triggers with bitfields" {
    gdb_test "watch -l q.a" "insert watchpoint in q.a"
    gdb_test "watch -l q.e" "insert watchpoint in q.e"
    expect_watchpoint "q.a" 0 1
    ...
    gdb_test "cont" ".*exited normally.*" "check if program exited normally"
  }

(BTW, it's better to use anchored regex when testing for something.  See
the last test above).

And you would a similar dance with the other set of tests below.

> +# Check that regular watchpoints against expressions involving bitfields
> +# trigger properly.
> +runto_main
> +gdb_test "watch q.d + q.f + q.g"
> +expect_watchpoint "q.d + q.f + q.g" 0 4
> +expect_watchpoint "q.d + q.f + q.g" 4 10
> +expect_watchpoint "q.d + q.f + q.g" 10 3
> +expect_watchpoint "q.d + q.f + q.g" 3 2
> +expect_watchpoint "q.d + q.f + q.g" 2 1
> +expect_watchpoint "q.d + q.f + q.g" 1 0
> +gdb_test "cont" ".*exited normally.*"

Crap, I had to tweak the testcase so much that I figured I'd send a
patch.  This applies on top of yours.

Cheers,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Index: binutils-gdb/gdb/testsuite/gdb.base/watch-bitfields.exp
===================================================================
--- binutils-gdb.orig/gdb/testsuite/gdb.base/watch-bitfields.exp
+++ binutils-gdb/gdb/testsuite/gdb.base/watch-bitfields.exp
@@ -25,32 +25,61 @@ if {![runto_main]} {
     return -1
 }
 
+proc insert_watchpoint { expr } {
+    global decimal
+
+    set expr_re [string_to_regexp $expr]
+    gdb_test "watch $expr" "\(Hardware \)?\[Ww\]atchpoint $decimal: $expr_re" \
+	"insert watchpoint on $expr"
+}
+
+proc insert_location_watchpoint { expr } {
+    global decimal
+
+    set expr_re [string_to_regexp $expr]
+    gdb_test "watch -l $expr" \
+	"\(Hardware \)?\[Ww\]atchpoint $decimal: -location $expr_re" \
+	"insert location watchpoint on $expr"
+}
+
 # Continue inferior execution, expecting the watchpoint EXPR to be triggered
 # having old value OLD and new value NEW.
 proc expect_watchpoint { expr old new } {
     set expr_re [string_to_regexp $expr]
-    gdb_test "print $expr" "\\$\\d+ = $old\\s"
-    gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*"
-    gdb_test "print $expr" "\\$\\d+ = $new\\s"
+    gdb_test "print $expr" " = $old" "check if $expr == $old"
+    gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" \
+	"check if watch on $expr triggers (old val = $old, new val = $new)"
+    gdb_test "print $expr" " = $new" "check if expr == $new"
 }
 
 # Check that -location watchpoints against bitfields trigger properly.
-gdb_test "watch -l q.a"
-gdb_test "watch -l q.e"
-expect_watchpoint "q.a" 0 1
-expect_watchpoint "q.e" 0 5
-expect_watchpoint "q.a" 1 0
-expect_watchpoint "q.e" 5 4
-gdb_test "cont" ".*exited normally.*"
+with_test_prefix "-location watch triggers with bitfields" {
+    insert_location_watchpoint "q.a"
+    insert_location_watchpoint "q.e"
+    expect_watchpoint "q.a" 0 1
+    expect_watchpoint "q.e" 0 5
+    expect_watchpoint "q.a" 1 0
+    expect_watchpoint "q.e" 5 4
+    gdb_test "cont" \
+	"\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\].*" \
+        "check if program exited normally"
+}
 
 # Check that regular watchpoints against expressions involving bitfields
 # trigger properly.
-runto_main
-gdb_test "watch q.d + q.f + q.g"
-expect_watchpoint "q.d + q.f + q.g" 0 4
-expect_watchpoint "q.d + q.f + q.g" 4 10
-expect_watchpoint "q.d + q.f + q.g" 10 3
-expect_watchpoint "q.d + q.f + q.g" 3 2
-expect_watchpoint "q.d + q.f + q.g" 2 1
-expect_watchpoint "q.d + q.f + q.g" 1 0
-gdb_test "cont" ".*exited normally.*"
+if {![runto_main]} {
+    return -1
+}
+
+with_test_prefix "regular watch triggers against bitfields" {
+    insert_watchpoint "q.d + q.f + q.g"
+    expect_watchpoint "q.d + q.f + q.g" 0 4
+    expect_watchpoint "q.d + q.f + q.g" 4 10
+    expect_watchpoint "q.d + q.f + q.g" 10 3
+    expect_watchpoint "q.d + q.f + q.g" 3 2
+    expect_watchpoint "q.d + q.f + q.g" 2 1
+    expect_watchpoint "q.d + q.f + q.g" 1 0
+    gdb_test "cont" \
+	"\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\].*" \
+        "check if program exited normally"
+}

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

[Sergio Durigan Junior]

On Sunday, September 07 2014, I wrote:

>> +    gdb_test "print $expr" "\\$\\d+ = $new\\s"
>
>   gdb_test "print $expr" "\\$\\d+ = $new\\s" \
>       "check if expr == $new"

I forgot to mention that this is not enough to make the messages unique,
so I added a "(old val = $old, new val = $new)" after the proposed
message.

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Pedro Alves]

On 09/07/2014 07:37 PM, Patrick Palka wrote:
> { v2: Here is my crude attempt at adding a testcase for this changeset. I
>   also fixed the bug that I mentioned earlier. }

Thank you!

> 
> 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.

I've pushed this in, with a couple minor tweaks.  Dropped ()'s in
function references from the commit log (per GNU convention), and
tweaked this one:

> +  if (val && just_location)

too to be explicit, like:

  if (val != NULL && just_location)

Below's what I pushed.  Handling the duplicate test messages
issue as follow up, to keep this moving.

Thanks again for fixing this!  Nice work.

--------------

From bb9d5f81c36ecc61e3d4a70ce7e41348c8b12fef Mon Sep 17 00:00:00 2001
From: Patrick Palka <patrick@parcs.ath.cx>
Date: Tue, 16 Sep 2014 17:40:06 +0100
Subject: [PATCH] Fix PR12526: -location watchpoints for bitfield arguments

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.
	* value.h (unpack_value_bitfield): New prototype.
	* value.c (unpack_value_bitfield): Make extern.

gdb/testsuite/
	PR breakpoints/12526
	* gdb.base/watch-bitfields.exp: New file.
	* gdb.base/watch-bitfields.c: New file.
---
 gdb/ChangeLog                              | 14 ++++++
 gdb/testsuite/ChangeLog                    |  6 +++
 gdb/breakpoint.c                           | 74 +++++++++++++++++++++++++++++-
 gdb/breakpoint.h                           |  5 ++
 gdb/testsuite/gdb.base/watch-bitfields.c   | 54 ++++++++++++++++++++++
 gdb/testsuite/gdb.base/watch-bitfields.exp | 56 ++++++++++++++++++++++
 gdb/value.c                                |  2 +-
 gdb/value.h                                |  5 ++
 8 files changed, 214 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/watch-bitfields.c
 create mode 100644 gdb/testsuite/gdb.base/watch-bitfields.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 125ab92..5234a50 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,17 @@
+2014-09-16  Patrick Palka  <patrick@parcs.ath.cx>
+
+	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.
+	* value.h (unpack_value_bitfield): New prototype.
+	* value.c (unpack_value_bitfield): Make extern.
+
 2014-09-16  Samuel Thibault  <samuel.thibault@ens-lyon.org>
 
 	* config/i386/i386gnu.mh (NATDEPFILES): Add x86-nat.o and
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c06ba4d..655301e 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2014-09-16  Patrick Palka  <patrick@parcs.ath.cx>
+
+	PR breakpoints/12526
+	* gdb.base/watch-bitfields.exp: New file.
+	* gdb.base/watch-bitfields.c: New file.
+
 2014-09-16  Pedro Alves  <palves@redhat.com>
 
 	* gdb.base/watchpoint-stops-at-right-insn.exp (test): Compare
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f990d97..94b55c3 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1703,6 +1703,29 @@ 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)
+{
+  struct value *bit_val;
+
+  if (val == NULL)
+    return NULL;
+
+  bit_val = allocate_value (value_type (val));
+
+  unpack_value_bitfield (bit_val,
+			 w->val_bitpos,
+			 w->val_bitsize,
+			 value_contents_for_printing (val),
+			 value_offset (val),
+			 val);
+
+  return bit_val;
+}
+
 /* 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 +1900,12 @@ update_watchpoint (struct watchpoint *b, int reparse)
 	 watchpoints.  */
       if (!b->val_valid && !is_masked_watchpoint (&b->base))
 	{
+	  if (b->val_bitsize != 0)
+	    {
+	      v = extract_bitfield_from_watchpoint_value (b, v);
+	      if (v != NULL)
+		release_value (v);
+	    }
 	  b->val = v;
 	  b->val_valid = 1;
 	}
@@ -1906,8 +1935,31 @@ 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) != 0)
+		    {
+		      /* Extract the bit parameters out from the bitfield
+			 sub-expression.  */
+		      bitpos = value_bitpos (v);
+		      bitsize = value_bitsize (v);
+		    }
+		  else if (v == result && b->val_bitsize != 0)
+		    {
+		     /* If VAL_BITSIZE != 0 then RESULT is actually a bitfield
+			lvalue whose bit parameters are saved in the fields
+			VAL_BITPOS and VAL_BITSIZE.  */
+		      bitpos = b->val_bitpos;
+		      bitsize = b->val_bitsize;
+		    }
 
 		  addr = value_address (v);
+		  if (bitsize != 0)
+		    {
+		      /* 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 +1974,15 @@ update_watchpoint (struct watchpoint *b, int reparse)
 
 		  loc->pspace = frame_pspace;
 		  loc->address = addr;
-		  loc->length = TYPE_LENGTH (value_type (v));
+
+		  if (bitsize != 0)
+		    {
+		      /* 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 +5099,9 @@ watchpoint_check (void *p)
       mark = value_mark ();
       fetch_subexp_value (b->exp, &pc, &new_val, NULL, NULL, 0);
 
+      if (b->val_bitsize != 0)
+	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
@@ -11203,6 +11266,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;
@@ -11336,6 +11400,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 != NULL && just_location)
+    {
+      saved_bitpos = value_bitpos (val);
+      saved_bitsize = value_bitsize (val);
+    }
+
   if (just_location)
     {
       int ret;
@@ -11471,6 +11541,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 8abb5ea..00c8802 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/watch-bitfields.c b/gdb/testsuite/gdb.base/watch-bitfields.c
new file mode 100644
index 0000000..fb57885
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-bitfields.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.h--;
+  q.c--;
+  q.b--;
+  q.e--;
+  q.d--;
+  q.c--;
+  q.f--;
+  q.g--;
+  q.h--;
+
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watch-bitfields.exp b/gdb/testsuite/gdb.base/watch-bitfields.exp
new file mode 100644
index 0000000..3f25384
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-bitfields.exp
@@ -0,0 +1,56 @@
+# 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
+}
+
+# Continue inferior execution, expecting the watchpoint EXPR to be triggered
+# having old value OLD and new value NEW.
+proc expect_watchpoint { expr old new } {
+    set expr_re [string_to_regexp $expr]
+    gdb_test "print $expr" "\\$\\d+ = $old\\s"
+    gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*"
+    gdb_test "print $expr" "\\$\\d+ = $new\\s"
+}
+
+# Check that -location watchpoints against bitfields trigger properly.
+gdb_test "watch -l q.a"
+gdb_test "watch -l q.e"
+expect_watchpoint "q.a" 0 1
+expect_watchpoint "q.e" 0 5
+expect_watchpoint "q.a" 1 0
+expect_watchpoint "q.e" 5 4
+gdb_test "cont" ".*exited normally.*"
+
+# Check that regular watchpoints against expressions involving bitfields
+# trigger properly.
+runto_main
+gdb_test "watch q.d + q.f + q.g"
+expect_watchpoint "q.d + q.f + q.g" 0 4
+expect_watchpoint "q.d + q.f + q.g" 4 10
+expect_watchpoint "q.d + q.f + q.g" 10 3
+expect_watchpoint "q.d + q.f + q.g" 3 2
+expect_watchpoint "q.d + q.f + q.g" 2 1
+expect_watchpoint "q.d + q.f + q.g" 1 0
+gdb_test "cont" ".*exited normally.*"
diff --git a/gdb/value.c b/gdb/value.c
index 6620f96..fdc8858d 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3231,7 +3231,7 @@ unpack_field_as_long (struct type *type, const gdb_byte *valaddr, int fieldno)
    are unavailable/optimized out, DEST_VAL is correspondingly
    marked unavailable/optimized out.  */
 
-static void
+void
 unpack_value_bitfield (struct value *dest_val,
 		       int bitpos, int bitsize,
 		       const gdb_byte *valaddr, int embedded_offset,
diff --git a/gdb/value.h b/gdb/value.h
index 4cdbf21..e3603c3 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -613,6 +613,11 @@ extern int unpack_value_field_as_long (struct type *type, const gdb_byte *valadd
 				int embedded_offset, int fieldno,
 				const struct value *val, LONGEST *result);
 
+extern void unpack_value_bitfield (struct value *dest_val,
+				   int bitpos, int bitsize,
+				   const gdb_byte *valaddr, int embedded_offset,
+				   const struct value *val);
+
 extern struct value *value_field_bitfield (struct type *type, int fieldno,
 					   const gdb_byte *valaddr,
 					   int embedded_offset,
-- 
1.9.3

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

[Pedro Alves]

On 09/08/2014 01:10 AM, Sergio Durigan Junior wrote:
> On Sunday, September 07 2014, I wrote:
> 
>>> +    gdb_test "print $expr" "\\$\\d+ = $new\\s"
>>
>>   gdb_test "print $expr" "\\$\\d+ = $new\\s" \
>>       "check if expr == $new"
> 
> I forgot to mention that this is not enough to make the messages unique,
> so I added a "(old val = $old, new val = $new)" after the proposed
> message.

Hey Sergio,

For some odd reason, I completely missed your replies to Patrick,
and wanting to keep this moving, ended up redoing most of your
patch from scratch.  Gah.  /me bangs head in wall :-P.

As penance, I salvaged pieces from both patches, which I think
ends up being better than either version was.

 - function names a bit more concise
 - gdb.sum output is more concise too and shows patterns more clearly.
 - putting things in procedures allows run_to_main returning with
   canceling the other branch of the test (and allows easy hacking
   away part of the test when debugging it).
 - uses gdb_continue_to_end

This is what we have currently:

Running /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.base/watch-bitfields.exp ...
PASS: gdb.base/watch-bitfields.exp: watch -l q.a
PASS: gdb.base/watch-bitfields.exp: watch -l q.e
PASS: gdb.base/watch-bitfields.exp: print q.a
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: print q.a
PASS: gdb.base/watch-bitfields.exp: print q.e
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: print q.e
PASS: gdb.base/watch-bitfields.exp: print q.a
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: print q.a
PASS: gdb.base/watch-bitfields.exp: print q.e
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: print q.e
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: watch q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: cont
PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: cont

This is the end result:

Running /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.base/watch-bitfields.exp ...
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: watch -location q.a
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: watch -location q.e
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 0->1: print expression before
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 0->1: continue
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 0->1: print expression after
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: print expression before
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: print expression after
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 1->0: print expression before
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 1->0: continue
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 1->0: print expression after
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 5->4: print expression before
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 5->4: continue
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 5->4: print expression after
PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: continue until exit
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: watch q.d + q.f + q.g
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 0->4: print expression before
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 0->4: continue
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 0->4: print expression after
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 4->10: print expression before
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 4->10: continue
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 4->10: print expression after
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 10->3: print expression before
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 10->3: continue
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 10->3: print expression after
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 3->2: print expression before
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 3->2: continue
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 3->2: print expression after
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 2->1: print expression before
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 2->1: continue
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 2->1: print expression after
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 1->0: print expression before
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 1->0: continue
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 1->0: print expression after
PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: continue until exit

WDYT?

----------
From d3f6e7f2ab3238ed98133d1f484da623dc55e8aa Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Tue, 16 Sep 2014 17:32:04 +0100
Subject: [PATCH] gdb.base/watch-bitfields.exp: Improve test

Make test messages unique, and a couple other tweaks.

gdb/testsuite/
2014-09-16  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	* gdb.base/watch-bitfields.exp: Pass string other than test file
	name to prepare_for_testing.
	(watch): New procedure.
	(expect_watchpoint): Use with_test_prefix.
	(top level): Factor out tests to ...
	(test_watch_location, test_regular_watch): ... these new
	procedures, and use with_test_prefix and gdb_continue_to_end.
---
 gdb/testsuite/gdb.base/watch-bitfields.exp | 77 ++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 26 deletions(-)

diff --git a/gdb/testsuite/gdb.base/watch-bitfields.exp b/gdb/testsuite/gdb.base/watch-bitfields.exp
index 3f25384..7b7fa22 100644
--- a/gdb/testsuite/gdb.base/watch-bitfields.exp
+++ b/gdb/testsuite/gdb.base/watch-bitfields.exp
@@ -17,40 +17,65 @@

 standard_testfile

-if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
     return -1
 }

-if {![runto_main]} {
-    return -1
+# Set a watchpoint watching EXPR.
+proc watch { expr } {
+    global decimal
+
+    set expr_re [string_to_regexp $expr]
+    gdb_test "watch $expr" \
+	"\(Hardware \)?\[Ww\]atchpoint $decimal: $expr_re"
 }

 # Continue inferior execution, expecting the watchpoint EXPR to be triggered
 # having old value OLD and new value NEW.
 proc expect_watchpoint { expr old new } {
-    set expr_re [string_to_regexp $expr]
-    gdb_test "print $expr" "\\$\\d+ = $old\\s"
-    gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*"
-    gdb_test "print $expr" "\\$\\d+ = $new\\s"
+    with_test_prefix "$expr: $old->$new" {
+	set expr_re [string_to_regexp $expr]
+	gdb_test "print $expr" "\\$\\d+ = $old\\s" "print expression before"
+	gdb_test "continue" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*"
+	gdb_test "print $expr" "\\$\\d+ = $new\\s" "print expression after"
+    }
 }

 # Check that -location watchpoints against bitfields trigger properly.
-gdb_test "watch -l q.a"
-gdb_test "watch -l q.e"
-expect_watchpoint "q.a" 0 1
-expect_watchpoint "q.e" 0 5
-expect_watchpoint "q.a" 1 0
-expect_watchpoint "q.e" 5 4
-gdb_test "cont" ".*exited normally.*"
-
-# Check that regular watchpoints against expressions involving bitfields
-# trigger properly.
-runto_main
-gdb_test "watch q.d + q.f + q.g"
-expect_watchpoint "q.d + q.f + q.g" 0 4
-expect_watchpoint "q.d + q.f + q.g" 4 10
-expect_watchpoint "q.d + q.f + q.g" 10 3
-expect_watchpoint "q.d + q.f + q.g" 3 2
-expect_watchpoint "q.d + q.f + q.g" 2 1
-expect_watchpoint "q.d + q.f + q.g" 1 0
-gdb_test "cont" ".*exited normally.*"
+proc test_watch_location {} {
+    with_test_prefix "-location watch against bitfields" {
+	if {![runto_main]} {
+	    return -1
+	}
+
+	watch "-location q.a"
+	watch "-location q.e"
+	expect_watchpoint "q.a" 0 1
+	expect_watchpoint "q.e" 0 5
+	expect_watchpoint "q.a" 1 0
+	expect_watchpoint "q.e" 5 4
+	gdb_continue_to_end
+    }
+}
+
+# Check that regular watchpoints against expressions involving
+# bitfields trigger properly.
+proc test_regular_watch {} {
+    with_test_prefix "regular watch against bitfields" {
+	if {![runto_main]} {
+	    return -1
+	}
+
+	watch "q.d + q.f + q.g"
+	expect_watchpoint "q.d + q.f + q.g" 0 4
+	expect_watchpoint "q.d + q.f + q.g" 4 10
+	expect_watchpoint "q.d + q.f + q.g" 10 3
+	expect_watchpoint "q.d + q.f + q.g" 3 2
+	expect_watchpoint "q.d + q.f + q.g" 2 1
+	expect_watchpoint "q.d + q.f + q.g" 1 0
+	gdb_continue_to_end
+    }
+}
+
+test_watch_location
+test_regular_watch
-- 
1.9.3

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

[Sergio Durigan Junior]

On Tuesday, September 16 2014, Pedro Alves wrote:

> Hey Sergio,
>
> For some odd reason, I completely missed your replies to Patrick,
> and wanting to keep this moving, ended up redoing most of your
> patch from scratch.  Gah.  /me bangs head in wall :-P.

No worries :-).

> As penance, I salvaged pieces from both patches, which I think
> ends up being better than either version was.
>
>  - function names a bit more concise
>  - gdb.sum output is more concise too and shows patterns more clearly.
>  - putting things in procedures allows run_to_main returning with
>    canceling the other branch of the test (and allows easy hacking
>    away part of the test when debugging it).
>  - uses gdb_continue_to_end
>
> This is what we have currently:
>
[...]
>
> WDYT?

Looks neat, thanks for doing that!

> ----------
> From d3f6e7f2ab3238ed98133d1f484da623dc55e8aa Mon Sep 17 00:00:00 2001
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Tue, 16 Sep 2014 17:32:04 +0100
> Subject: [PATCH] gdb.base/watch-bitfields.exp: Improve test
>
> Make test messages unique, and a couple other tweaks.
>
> gdb/testsuite/
> 2014-09-16  Sergio Durigan Junior  <sergiodj@redhat.com>
> 	    Pedro Alves  <palves@redhat.com>
>
> 	* gdb.base/watch-bitfields.exp: Pass string other than test file
> 	name to prepare_for_testing.
> 	(watch): New procedure.
> 	(expect_watchpoint): Use with_test_prefix.
> 	(top level): Factor out tests to ...
> 	(test_watch_location, test_regular_watch): ... these new
> 	procedures, and use with_test_prefix and gdb_continue_to_end.
> ---
>  gdb/testsuite/gdb.base/watch-bitfields.exp | 77 ++++++++++++++++++++----------
>  1 file changed, 51 insertions(+), 26 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/watch-bitfields.exp b/gdb/testsuite/gdb.base/watch-bitfields.exp
> index 3f25384..7b7fa22 100644
> --- a/gdb/testsuite/gdb.base/watch-bitfields.exp
> +++ b/gdb/testsuite/gdb.base/watch-bitfields.exp
> @@ -17,40 +17,65 @@
>
>  standard_testfile
>
> -if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
>      return -1
>  }
>
> -if {![runto_main]} {
> -    return -1
> +# Set a watchpoint watching EXPR.
> +proc watch { expr } {
> +    global decimal
> +
> +    set expr_re [string_to_regexp $expr]
> +    gdb_test "watch $expr" \
> +	"\(Hardware \)?\[Ww\]atchpoint $decimal: $expr_re"
>  }
>
>  # Continue inferior execution, expecting the watchpoint EXPR to be triggered
>  # having old value OLD and new value NEW.
>  proc expect_watchpoint { expr old new } {
> -    set expr_re [string_to_regexp $expr]
> -    gdb_test "print $expr" "\\$\\d+ = $old\\s"
> -    gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*"
> -    gdb_test "print $expr" "\\$\\d+ = $new\\s"
> +    with_test_prefix "$expr: $old->$new" {
> +	set expr_re [string_to_regexp $expr]
> +	gdb_test "print $expr" "\\$\\d+ = $old\\s" "print expression before"
> +	gdb_test "continue" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*"
> +	gdb_test "print $expr" "\\$\\d+ = $new\\s" "print expression after"
> +    }
>  }
>
>  # Check that -location watchpoints against bitfields trigger properly.
> -gdb_test "watch -l q.a"
> -gdb_test "watch -l q.e"
> -expect_watchpoint "q.a" 0 1
> -expect_watchpoint "q.e" 0 5
> -expect_watchpoint "q.a" 1 0
> -expect_watchpoint "q.e" 5 4
> -gdb_test "cont" ".*exited normally.*"
> -
> -# Check that regular watchpoints against expressions involving bitfields
> -# trigger properly.
> -runto_main
> -gdb_test "watch q.d + q.f + q.g"
> -expect_watchpoint "q.d + q.f + q.g" 0 4
> -expect_watchpoint "q.d + q.f + q.g" 4 10
> -expect_watchpoint "q.d + q.f + q.g" 10 3
> -expect_watchpoint "q.d + q.f + q.g" 3 2
> -expect_watchpoint "q.d + q.f + q.g" 2 1
> -expect_watchpoint "q.d + q.f + q.g" 1 0
> -gdb_test "cont" ".*exited normally.*"
> +proc test_watch_location {} {
> +    with_test_prefix "-location watch against bitfields" {
> +	if {![runto_main]} {
> +	    return -1
> +	}
> +
> +	watch "-location q.a"
> +	watch "-location q.e"
> +	expect_watchpoint "q.a" 0 1
> +	expect_watchpoint "q.e" 0 5
> +	expect_watchpoint "q.a" 1 0
> +	expect_watchpoint "q.e" 5 4
> +	gdb_continue_to_end
> +    }
> +}
> +
> +# Check that regular watchpoints against expressions involving
> +# bitfields trigger properly.
> +proc test_regular_watch {} {
> +    with_test_prefix "regular watch against bitfields" {
> +	if {![runto_main]} {
> +	    return -1
> +	}
> +
> +	watch "q.d + q.f + q.g"
> +	expect_watchpoint "q.d + q.f + q.g" 0 4
> +	expect_watchpoint "q.d + q.f + q.g" 4 10
> +	expect_watchpoint "q.d + q.f + q.g" 10 3
> +	expect_watchpoint "q.d + q.f + q.g" 3 2
> +	expect_watchpoint "q.d + q.f + q.g" 2 1
> +	expect_watchpoint "q.d + q.f + q.g" 1 0
> +	gdb_continue_to_end
> +    }
> +}
> +
> +test_watch_location
> +test_regular_watch
> -- 
> 1.9.3

The patch looks good to me.

Thanks!

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Pedro Alves]

On 09/16/2014 06:05 PM, Sergio Durigan Junior wrote:
> On Tuesday, September 16 2014, Pedro Alves wrote:

>> For some odd reason, I completely missed your replies to Patrick,
>> and wanting to keep this moving, ended up redoing most of your
>> patch from scratch.  Gah.  /me bangs head in wall :-P.
> 
> No worries :-).

Phew.  :-)

> Looks neat, thanks for doing that!

...

> The patch looks good to me.
> 
> Thanks!

Thank you!  Now pushed.

Pedro Alves

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

[Patrick Palka]

Awesome!  Thanks to the both of you, for your help and guidance.