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

Patrick Palka
2014-09-07T18:36:07Z

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