From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8771 invoked by alias); 3 Sep 2014 13:18:18 -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 8757 invoked by uid 89); 3 Sep 2014 13:18:17 -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-pd0-f174.google.com Received: from mail-pd0-f174.google.com (HELO mail-pd0-f174.google.com) (209.85.192.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 03 Sep 2014 13:18:15 +0000 Received: by mail-pd0-f174.google.com with SMTP id ft15so10997708pdb.5 for ; Wed, 03 Sep 2014 06:18:10 -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=lDmqVIfrBEyiSHMrOz1MqcIfJe0KzQkrdcPKNEhErS4=; b=Kaf6YkNKtnPVsiLnSbHWO8gkU2y4UDxTUDgHn9ouA5eQx/PkWEBqO82isn9IzVeYr9 lbLz4UFTS8xrKNsHpIWctrD8B6Ill3xJpodqZXMYt3LEG1Ei3Q8Px3Dhycz7Z3LMo/wa PDykj3zyfBQ9Kw0olBHRN7Ipr1wJ1YsnZvDQqOO8pvN7rXjo05v7p0mbLBA3NmcwmMUt xbZ2z0IIsulqTXSeVOEeUoNYKi47Yy24tEs+vROhGJzOKkAJ6IfS2Z/f40qNb1777hCU bGADsMwSHsIPVpQ3I1/UnqHVy+V3TwV7hgvptOC7xJNTqrhc006ZCR+QFeBeD3FI1KCB qIAA== X-Gm-Message-State: ALoCoQnkaALVqrSZRxb9Ur/1G2DuhwYxP0MxkqoTiQqdimIBp4U58Rh6e9+t0z5vZ3wQ5X1YuY4R MIME-Version: 1.0 X-Received: by 10.66.139.200 with SMTP id ra8mr58246962pab.69.1409750290558; Wed, 03 Sep 2014 06:18:10 -0700 (PDT) Received: by 10.70.118.134 with HTTP; Wed, 3 Sep 2014 06:18:10 -0700 (PDT) In-Reply-To: References: <1408563606-24314-1-git-send-email-patrick@parcs.ath.cx> <1408591949-29689-1-git-send-email-patrick@parcs.ath.cx> Date: Wed, 03 Sep 2014 13:18: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-09/txt/msg00091.txt.bz2 On Wed, Aug 27, 2014 at 10:28 AM, Patrick Palka wrote: > 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. Ping.