From patchwork Wed Aug 20 19:40:06 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Palka X-Patchwork-Id: 2458 Received: (qmail 13895 invoked by alias); 20 Aug 2014 19:40:31 -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 13884 invoked by uid 89); 20 Aug 2014 19:40:30 -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-f47.google.com Received: from mail-qa0-f47.google.com (HELO mail-qa0-f47.google.com) (209.85.216.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 20 Aug 2014 19:40:29 +0000 Received: by mail-qa0-f47.google.com with SMTP id i13so7353912qae.6 for ; Wed, 20 Aug 2014 12:40:24 -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; bh=/yWCA/RFz7F5mSIJZCjwJpJx/9YCWWmT8EHXaIGePMA=; b=PhbfFehx5kRpZV3hYP/Nz++VHIpX6kzZcbTZE4zqjWNevsu3Ou+K72vc3K5tJ/jz1r SO6Ofx6SMZMzQMZa1KJsR2RxKgFyZoNagOghlzvwbLTYbKLUtAM3AqiUW9cAmo4RvWwt NHSmVZVNdhYkuVW84TYL+G2Fz8wA6Z8TNcB6PIXWjVcL9SDcQxT4D9sdJPCbKpApJO7K 6BW6CjslzECyqq51XY02QyKWYq2HXCTvUX2jA4Lc/vOng9V20Q4s3j0u0YBSkmY1NYi+ SyJgEMOXggvObyvhOahOBBZcFNV+xoutiV4fD7RS38h3pKfwjL+K+3EhwrElDm0W3NGs uRnQ== X-Gm-Message-State: ALoCoQlyZovaBXfoKLkSyQ0gIhvY2oqDQkyrKbkkB6rrBLdQT1SCfKTOdxtcAstdIyNbvSeQK4hk X-Received: by 10.224.61.144 with SMTP id t16mr82593473qah.2.1408563624412; Wed, 20 Aug 2014 12:40:24 -0700 (PDT) Received: from localhost.localdomain (ool-4353af5c.dyn.optonline.net. [67.83.175.92]) by mx.google.com with ESMTPSA id w3sm42141611qap.37.2014.08.20.12.40.23 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 20 Aug 2014 12:40:23 -0700 (PDT) From: Patrick Palka To: gdb-patches@sourceware.org Cc: Patrick Palka Subject: [PATCH] Fix PR12526: -location watchpoints for bitfield arguments Date: Wed, 20 Aug 2014 15:40:06 -0400 Message-Id: <1408563606-24314-1-git-send-email-patrick@parcs.ath.cx> 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. PR gdb/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/breakpoint.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- gdb/breakpoint.h | 5 +++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 683ed2b..7e8fbd1 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 += bitsize / 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. */