From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18306 invoked by alias); 27 Aug 2014 14:28:17 -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 18269 invoked by uid 89); 27 Aug 2014 14:28:16 -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-pa0-f42.google.com Received: from mail-pa0-f42.google.com (HELO mail-pa0-f42.google.com) (209.85.220.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 27 Aug 2014 14:28:14 +0000 Received: by mail-pa0-f42.google.com with SMTP id lf10so380371pab.29 for ; Wed, 27 Aug 2014 07:28:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=EPachFa/F+3v/UxFo/FBlN5o21n11SYyPfGs4yvnqUs=; b=ju1hgX58LwKRKKG7tv4KLdRqcpbocF0D49jpGGu/4rfKEUnhvPnvDfjjMP6cfXIgX7 rcSlb/nxzNqmWVgU+sGcLwjw+UHWEWnveqAzK8jaikrSIkAXLh7G8Fzc1mwYkHGkZjkH QXY5VrGfePySljSeB8qAzSolEW8Qy3wBPUdvQy0wgvT7JfSh/9Wv4Zip17aRJviRugfO bsagGbyT6vGs+MxxJrRC97wzPfH3UjXKkqSZ29du4+AsvUFSj5UF3iC7oTHT7aLpYZVf 5kKOI7TYA1dEZf1pbK2klNoQPDzH7wUwPJel31xhwEYgxheMlpduFeZh1e+u5n5vDPge wTeQ== X-Gm-Message-State: ALoCoQkN3hxTlvmBZsn+KmDGcINjRQBAO3iud1ZcLIf16/fn2JfXMIAMjUnzAIm0BZsmseXVk4JA MIME-Version: 1.0 X-Received: by 10.69.26.33 with SMTP id iv1mr46558102pbd.62.1409149688250; Wed, 27 Aug 2014 07:28:08 -0700 (PDT) Received: by 10.70.67.228 with HTTP; Wed, 27 Aug 2014 07:28:08 -0700 (PDT) In-Reply-To: <1408591949-29689-1-git-send-email-patrick@parcs.ath.cx> References: <1408563606-24314-1-git-send-email-patrick@parcs.ath.cx> <1408591949-29689-1-git-send-email-patrick@parcs.ath.cx> Date: Wed, 27 Aug 2014 14:28:00 -0000 Message-ID: Subject: Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments From: Patrick Palka To: gdb-patches@sourceware.org Cc: Patrick Palka Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg00571.txt.bz2 On Wed, Aug 20, 2014 at 11:32 PM, 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) > + { > + 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 . */ > + > +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 . > + > +# 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.