Message ID | 1408591949-29689-1-git-send-email-patrick@parcs.ath.cx |
---|---|
State | New, archived |
Headers |
Received: (qmail 30949 invoked by alias); 21 Aug 2014 03:32:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 30936 invoked by uid 89); 21 Aug 2014 03:32:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-qa0-f48.google.com Received: from mail-qa0-f48.google.com (HELO mail-qa0-f48.google.com) (209.85.216.48) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 21 Aug 2014 03:32:38 +0000 Received: by mail-qa0-f48.google.com with SMTP id m5so7717616qaj.21 for <gdb-patches@sourceware.org>; Wed, 20 Aug 2014 20:32:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=W2RzG9/gg1/P8IOY9vxZiV6De5v9VLJGSHAV6kHDMKs=; b=Evl5UJRBiaMcvrO3ERYqWwYEB71x+JigBeE27qllBAjnQ0D2uzy3zluvG/Aa3Ue9GN 6IEugFUtpeTSJLyLdbZuKAsZ1YX810J7gzAgTgwLcmGFRY5ldfvqWWGIPreyogsv1LOx WXC3Z7itIJXio1P/G1TAacrKwm3iz5u470ifXRpRxiFNQXHu7qUfUOUitdnVGEhoEOs2 NTq9G0EK0Qmx/UV3Js39mDEZKXCIl6t/O+UkRS+Cgbr6c91cQnK6qQAeGDRhWUqHGrhb faq+izvTQd69xw+u/mrN1gDKQ7wPAbmzSOATgZyS4MsOVTYoEGXFrtb3TnXoUB0To0dT 1FJg== X-Gm-Message-State: ALoCoQnhg2f7yKcWBpy6Y8YxhuPsn85qY4EcWzvqgvQbLmN2POUzp0gXjsGkXR6bQ6ESSJc6AV8A X-Received: by 10.140.88.243 with SMTP id t106mr16464655qgd.12.1408591956015; Wed, 20 Aug 2014 20:32:36 -0700 (PDT) Received: from localhost.localdomain (ool-4353af5c.dyn.optonline.net. [67.83.175.92]) by mx.google.com with ESMTPSA id c8sm44031353qah.9.2014.08.20.20.32.34 for <multiple recipients> (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 20 Aug 2014 20:32:35 -0700 (PDT) From: Patrick Palka <patrick@parcs.ath.cx> To: gdb-patches@sourceware.org Cc: Patrick Palka <patrick@parcs.ath.cx> Subject: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments Date: Wed, 20 Aug 2014 23:32:29 -0400 Message-Id: <1408591949-29689-1-git-send-email-patrick@parcs.ath.cx> In-Reply-To: <1408563606-24314-1-git-send-email-patrick@parcs.ath.cx> References: <1408563606-24314-1-git-send-email-patrick@parcs.ath.cx> |
Commit Message
Patrick Palka
Aug. 21, 2014, 3:32 a.m. UTC
{ 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
Comments
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
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.*"