From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21007 invoked by alias); 4 Sep 2014 15:04:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 20996 invoked by uid 89); 4 Sep 2014 15:04:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 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; Thu, 04 Sep 2014 15:04:14 +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 s84F4Ahd011555 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 4 Sep 2014 11:04:11 -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 s84F48aR023685; Thu, 4 Sep 2014 11:04:09 -0400 Message-ID: <54087F68.8020606@redhat.com> Date: Thu, 04 Sep 2014 15:04:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0 MIME-Version: 1.0 To: Patrick Palka , gdb-patches@sourceware.org Subject: Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments References: <1408563606-24314-1-git-send-email-patrick@parcs.ath.cx> <1408591949-29689-1-git-send-email-patrick@parcs.ath.cx> In-Reply-To: <1408591949-29689-1-git-send-email-patrick@parcs.ath.cx> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-09/txt/msg00122.txt.bz2 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