[PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments
Patrick Palka
patrick@parcs.ath.cx
Wed Aug 27 14:28:00 GMT 2014
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.
More information about the Gdb-patches
mailing list