From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27238 invoked by alias); 7 Sep 2014 18:36:07 -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 27219 invoked by uid 89); 7 Sep 2014 18:36:04 -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-f45.google.com Received: from mail-pa0-f45.google.com (HELO mail-pa0-f45.google.com) (209.85.220.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 07 Sep 2014 18:36:02 +0000 Received: by mail-pa0-f45.google.com with SMTP id rd3so930133pab.32 for ; Sun, 07 Sep 2014 11:36:00 -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=sb141tzkS19ids4cONhQZk0oQihPfDcTdFTdFI/+Eco=; b=X0zQYq77sKcyk5rcsq/pNUYNXBSbzxUMaCEK2ikcZ6410jccC7U6ql1PSoHnPBerhk ZY8X/WOqvadvi8dkG4jnqHbGsPVKWcU5KW5M0BEAEf80a3HKaDAqQAHQsDceb1XquFTr XyBZ1ezM1GyN+qTlAoT8sL2NDaI3MDd/xUtMzy2fCRW8w4swd7qcOPRheMwtwZcThJpt RjhO+a1QaVltgad/8aRjp60MMkxPOL6ioNS026OuZdATYyd0EMJOKoUMlfOusswC3Awj ua/8tyzwcq6Z/am2m7MZd/LarTVYaws+Bz0qdw2MpBU4IImswcsI2M8U55qZT9XkJ6Sv oNSA== X-Gm-Message-State: ALoCoQk/iNnAiN/+LKFEsgkCE78bjGpUYcebvbDwOfelRhxPIBqfIicMBwqwWKzUnXojDmyjWSck MIME-Version: 1.0 X-Received: by 10.66.139.200 with SMTP id ra8mr40476577pab.69.1410114960559; Sun, 07 Sep 2014 11:36:00 -0700 (PDT) Received: by 10.70.64.137 with HTTP; Sun, 7 Sep 2014 11:36:00 -0700 (PDT) In-Reply-To: <54087F68.8020606@redhat.com> References: <1408563606-24314-1-git-send-email-patrick@parcs.ath.cx> <1408591949-29689-1-git-send-email-patrick@parcs.ath.cx> <54087F68.8020606@redhat.com> Date: Sun, 07 Sep 2014 18:36:00 -0000 Message-ID: Subject: Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments From: Patrick Palka To: Pedro Alves Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00172.txt.bz2 On Thu, Sep 4, 2014 at 11:04 AM, Pedro Alves 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