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

Patrick Palka
2014-08-20T19:40:31Z

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