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

  { 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
  

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.
  

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.
  

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
  

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