From patchwork Tue Sep 16 16:46:04 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 2862 Received: (qmail 11392 invoked by alias); 16 Sep 2014 16:46:17 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 11380 invoked by uid 89); 16 Sep 2014 16:46:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 16 Sep 2014 16:46:10 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s8GGk7CW027257 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 16 Sep 2014 12:46:07 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s8GGk5Zd024575; Tue, 16 Sep 2014 12:46:06 -0400 Message-ID: <5418694C.9070006@redhat.com> Date: Tue, 16 Sep 2014 17:46:04 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.0 MIME-Version: 1.0 To: Patrick Palka , gdb-patches@sourceware.org Subject: Re: [PATCH] Fix PR12526: -location watchpoints for bitfield arguments References: <1410115039-32473-1-git-send-email-patrick@parcs.ath.cx> In-Reply-To: <1410115039-32473-1-git-send-email-patrick@parcs.ath.cx> 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 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 + + 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 * 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 + + PR breakpoints/12526 + * gdb.base/watch-bitfields.exp: New file. + * gdb.base/watch-bitfields.c: New file. + 2014-09-16 Pedro Alves * 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 . */ + +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 . + +# 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,