* [PATCH] Fix PR12526: -location watchpoints for bitfield arguments @ 2014-08-20 19:40 Patrick Palka 2014-08-20 20:59 ` Patrick Palka 2014-08-21 3:32 ` [PATCH v2] " Patrick Palka 0 siblings, 2 replies; 15+ messages in thread From: Patrick Palka @ 2014-08-20 19:40 UTC (permalink / raw) To: gdb-patches; +Cc: Patrick Palka 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. */ -- 2.1.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR12526: -location watchpoints for bitfield arguments 2014-08-20 19:40 [PATCH] Fix PR12526: -location watchpoints for bitfield arguments Patrick Palka @ 2014-08-20 20:59 ` Patrick Palka 2014-08-21 3:32 ` [PATCH v2] " Patrick Palka 1 sibling, 0 replies; 15+ messages in thread From: Patrick Palka @ 2014-08-20 20:59 UTC (permalink / raw) To: gdb-patches; +Cc: Patrick Palka Oops, found a bug: the line addr += bitsize / 8; ought to be addr += bitpos / 8; ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-08-20 19:40 [PATCH] Fix PR12526: -location watchpoints for bitfield arguments Patrick Palka 2014-08-20 20:59 ` Patrick Palka @ 2014-08-21 3:32 ` Patrick Palka 2014-08-27 14:28 ` Patrick Palka 2014-09-04 15:04 ` Pedro Alves 1 sibling, 2 replies; 15+ messages in thread From: Patrick Palka @ 2014-08-21 3:32 UTC (permalink / raw) To: gdb-patches; +Cc: Patrick Palka { 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 <http://www.gnu.org/licenses/>. */ + +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 <http://www.gnu.org/licenses/>. + +# 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-08-21 3:32 ` [PATCH v2] " Patrick Palka @ 2014-08-27 14:28 ` Patrick Palka 2014-09-03 13:18 ` Patrick Palka 2014-09-04 15:04 ` Pedro Alves 1 sibling, 1 reply; 15+ messages in thread From: Patrick Palka @ 2014-08-27 14:28 UTC (permalink / raw) To: gdb-patches; +Cc: Patrick Palka On Wed, Aug 20, 2014 at 11:32 PM, Patrick Palka <patrick@parcs.ath.cx> 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 <http://www.gnu.org/licenses/>. */ > + > +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 <http://www.gnu.org/licenses/>. > + > +# 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-08-27 14:28 ` Patrick Palka @ 2014-09-03 13:18 ` Patrick Palka 0 siblings, 0 replies; 15+ messages in thread From: Patrick Palka @ 2014-09-03 13:18 UTC (permalink / raw) To: gdb-patches; +Cc: Patrick Palka On Wed, Aug 27, 2014 at 10:28 AM, Patrick Palka <patrick@parcs.ath.cx> wrote: > On Wed, Aug 20, 2014 at 11:32 PM, Patrick Palka <patrick@parcs.ath.cx> 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 <http://www.gnu.org/licenses/>. */ >> + >> +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 <http://www.gnu.org/licenses/>. >> + >> +# 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-08-21 3:32 ` [PATCH v2] " Patrick Palka 2014-08-27 14:28 ` Patrick Palka @ 2014-09-04 15:04 ` Pedro Alves 2014-09-07 18:36 ` Patrick Palka 1 sibling, 1 reply; 15+ messages in thread From: Pedro Alves @ 2014-09-04 15:04 UTC (permalink / raw) To: Patrick Palka, gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-09-04 15:04 ` Pedro Alves @ 2014-09-07 18:36 ` Patrick Palka 2014-09-07 18:37 ` [PATCH] " Patrick Palka 2014-09-07 23:57 ` [PATCH v2] " Sergio Durigan Junior 0 siblings, 2 replies; 15+ messages in thread From: Patrick Palka @ 2014-09-07 18:36 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On Thu, Sep 4, 2014 at 11:04 AM, Pedro Alves <palves@redhat.com> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] Fix PR12526: -location watchpoints for bitfield arguments 2014-09-07 18:36 ` Patrick Palka @ 2014-09-07 18:37 ` Patrick Palka 2014-09-16 16:46 ` Pedro Alves 2014-09-07 23:57 ` [PATCH v2] " Sergio Durigan Junior 1 sibling, 1 reply; 15+ messages in thread From: Patrick Palka @ 2014-09-07 18:37 UTC (permalink / raw) To: gdb-patches; +Cc: Patrick Palka { v2: Here is my crude attempt at adding a testcase for this changeset. I also fixed the bug that I mentioned 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. * value.h (unpack_value_bitfield): New prototype. * value.c (unpack_value_bitfield): Make extern. gdb/testsuite/ PR breakpoints/12526 * gdb.base/watch-bitfields.exp: New file. * gdb.base/watch-bitfields.c: New file. --- gdb/breakpoint.c | 74 +++++++++++++++++++++++++++++- gdb/breakpoint.h | 5 ++ gdb/testsuite/gdb.base/watch-bitfields.c | 54 ++++++++++++++++++++++ gdb/testsuite/gdb.base/watch-bitfields.exp | 56 ++++++++++++++++++++++ gdb/value.c | 2 +- gdb/value.h | 5 ++ 6 files changed, 194 insertions(+), 2 deletions(-) create mode 100644 gdb/testsuite/gdb.base/watch-bitfields.c create mode 100644 gdb/testsuite/gdb.base/watch-bitfields.exp diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 683ed2b..2d33ff7 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1703,6 +1703,29 @@ 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) +{ + struct value *bit_val; + + if (val == NULL) + return NULL; + + bit_val = allocate_value (value_type (val)); + + unpack_value_bitfield (bit_val, + w->val_bitpos, + w->val_bitsize, + value_contents_for_printing (val), + value_offset (val), + val); + + return bit_val; +} + /* 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 +1900,12 @@ update_watchpoint (struct watchpoint *b, int reparse) watchpoints. */ if (!b->val_valid && !is_masked_watchpoint (&b->base)) { + if (b->val_bitsize != 0) + { + v = extract_bitfield_from_watchpoint_value (b, v); + if (v != NULL) + release_value (v); + } b->val = v; b->val_valid = 1; } @@ -1906,8 +1935,31 @@ 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) != 0) + { + /* Extract the bit parameters out from the bitfield + sub-expression. */ + bitpos = value_bitpos (v); + bitsize = value_bitsize (v); + } + else if (v == result && b->val_bitsize != 0) + { + /* If VAL_BITSIZE != 0 then RESULT is actually a bitfield + lvalue whose bit parameters are saved in the fields + VAL_BITPOS and VAL_BITSIZE. */ + bitpos = b->val_bitpos; + bitsize = b->val_bitsize; + } addr = value_address (v); + if (bitsize != 0) + { + /* 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 +1974,15 @@ update_watchpoint (struct watchpoint *b, int reparse) loc->pspace = frame_pspace; loc->address = addr; - loc->length = TYPE_LENGTH (value_type (v)); + + if (bitsize != 0) + { + /* 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 +5099,9 @@ watchpoint_check (void *p) mark = value_mark (); fetch_subexp_value (b->exp, &pc, &new_val, NULL, NULL, 0); + if (b->val_bitsize != 0) + 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 +11235,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 +11369,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 +11510,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/watch-bitfields.c b/gdb/testsuite/gdb.base/watch-bitfields.c new file mode 100644 index 0000000..fb57885 --- /dev/null +++ b/gdb/testsuite/gdb.base/watch-bitfields.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 <http://www.gnu.org/licenses/>. */ + +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.h--; + q.c--; + q.b--; + q.e--; + q.d--; + q.c--; + q.f--; + q.g--; + q.h--; + + + return 0; +} diff --git a/gdb/testsuite/gdb.base/watch-bitfields.exp b/gdb/testsuite/gdb.base/watch-bitfields.exp new file mode 100644 index 0000000..3f25384 --- /dev/null +++ b/gdb/testsuite/gdb.base/watch-bitfields.exp @@ -0,0 +1,56 @@ +# 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 <http://www.gnu.org/licenses/>. + +# 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 +} + +# Continue inferior execution, expecting the watchpoint EXPR to be triggered +# having old value OLD and new value NEW. +proc expect_watchpoint { expr old new } { + set expr_re [string_to_regexp $expr] + gdb_test "print $expr" "\\$\\d+ = $old\\s" + gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" + gdb_test "print $expr" "\\$\\d+ = $new\\s" +} + +# Check that -location watchpoints against bitfields trigger properly. +gdb_test "watch -l q.a" +gdb_test "watch -l q.e" +expect_watchpoint "q.a" 0 1 +expect_watchpoint "q.e" 0 5 +expect_watchpoint "q.a" 1 0 +expect_watchpoint "q.e" 5 4 +gdb_test "cont" ".*exited normally.*" + +# Check that regular watchpoints against expressions involving bitfields +# trigger properly. +runto_main +gdb_test "watch q.d + q.f + q.g" +expect_watchpoint "q.d + q.f + q.g" 0 4 +expect_watchpoint "q.d + q.f + q.g" 4 10 +expect_watchpoint "q.d + q.f + q.g" 10 3 +expect_watchpoint "q.d + q.f + q.g" 3 2 +expect_watchpoint "q.d + q.f + q.g" 2 1 +expect_watchpoint "q.d + q.f + q.g" 1 0 +gdb_test "cont" ".*exited normally.*" diff --git a/gdb/value.c b/gdb/value.c index 6620f96..fdc8858d 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -3231,7 +3231,7 @@ unpack_field_as_long (struct type *type, const gdb_byte *valaddr, int fieldno) are unavailable/optimized out, DEST_VAL is correspondingly marked unavailable/optimized out. */ -static void +void unpack_value_bitfield (struct value *dest_val, int bitpos, int bitsize, const gdb_byte *valaddr, int embedded_offset, diff --git a/gdb/value.h b/gdb/value.h index 4cdbf21..e3603c3 100644 --- a/gdb/value.h +++ b/gdb/value.h @@ -613,6 +613,11 @@ extern int unpack_value_field_as_long (struct type *type, const gdb_byte *valadd int embedded_offset, int fieldno, const struct value *val, LONGEST *result); +extern void unpack_value_bitfield (struct value *dest_val, + int bitpos, int bitsize, + const gdb_byte *valaddr, int embedded_offset, + const struct value *val); + extern struct value *value_field_bitfield (struct type *type, int fieldno, const gdb_byte *valaddr, int embedded_offset, -- 2.1.0.60.g85f0837 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Fix PR12526: -location watchpoints for bitfield arguments 2014-09-07 18:37 ` [PATCH] " Patrick Palka @ 2014-09-16 16:46 ` Pedro Alves 0 siblings, 0 replies; 15+ messages in thread From: Pedro Alves @ 2014-09-16 16:46 UTC (permalink / raw) To: Patrick Palka, gdb-patches On 09/07/2014 07:37 PM, Patrick Palka wrote: > { v2: Here is my crude attempt at adding a testcase for this changeset. I > also fixed the bug that I mentioned earlier. } Thank you! > > 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. I've pushed this in, with a couple minor tweaks. Dropped ()'s in function references from the commit log (per GNU convention), and tweaked this one: > + if (val && just_location) too to be explicit, like: if (val != NULL && just_location) Below's what I pushed. Handling the duplicate test messages issue as follow up, to keep this moving. Thanks again for fixing this! Nice work. -------------- From bb9d5f81c36ecc61e3d4a70ce7e41348c8b12fef Mon Sep 17 00:00:00 2001 From: Patrick Palka <patrick@parcs.ath.cx> Date: Tue, 16 Sep 2014 17:40:06 +0100 Subject: [PATCH] Fix PR12526: -location watchpoints for bitfield arguments 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. * value.h (unpack_value_bitfield): New prototype. * value.c (unpack_value_bitfield): Make extern. gdb/testsuite/ PR breakpoints/12526 * gdb.base/watch-bitfields.exp: New file. * gdb.base/watch-bitfields.c: New file. --- gdb/ChangeLog | 14 ++++++ gdb/testsuite/ChangeLog | 6 +++ gdb/breakpoint.c | 74 +++++++++++++++++++++++++++++- gdb/breakpoint.h | 5 ++ gdb/testsuite/gdb.base/watch-bitfields.c | 54 ++++++++++++++++++++++ gdb/testsuite/gdb.base/watch-bitfields.exp | 56 ++++++++++++++++++++++ gdb/value.c | 2 +- gdb/value.h | 5 ++ 8 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 gdb/testsuite/gdb.base/watch-bitfields.c create mode 100644 gdb/testsuite/gdb.base/watch-bitfields.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 125ab92..5234a50 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,17 @@ +2014-09-16 Patrick Palka <patrick@parcs.ath.cx> + + 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. + * value.h (unpack_value_bitfield): New prototype. + * value.c (unpack_value_bitfield): Make extern. + 2014-09-16 Samuel Thibault <samuel.thibault@ens-lyon.org> * config/i386/i386gnu.mh (NATDEPFILES): Add x86-nat.o and diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index c06ba4d..655301e 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,9 @@ +2014-09-16 Patrick Palka <patrick@parcs.ath.cx> + + PR breakpoints/12526 + * gdb.base/watch-bitfields.exp: New file. + * gdb.base/watch-bitfields.c: New file. + 2014-09-16 Pedro Alves <palves@redhat.com> * gdb.base/watchpoint-stops-at-right-insn.exp (test): Compare diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index f990d97..94b55c3 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1703,6 +1703,29 @@ 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) +{ + struct value *bit_val; + + if (val == NULL) + return NULL; + + bit_val = allocate_value (value_type (val)); + + unpack_value_bitfield (bit_val, + w->val_bitpos, + w->val_bitsize, + value_contents_for_printing (val), + value_offset (val), + val); + + return bit_val; +} + /* 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 +1900,12 @@ update_watchpoint (struct watchpoint *b, int reparse) watchpoints. */ if (!b->val_valid && !is_masked_watchpoint (&b->base)) { + if (b->val_bitsize != 0) + { + v = extract_bitfield_from_watchpoint_value (b, v); + if (v != NULL) + release_value (v); + } b->val = v; b->val_valid = 1; } @@ -1906,8 +1935,31 @@ 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) != 0) + { + /* Extract the bit parameters out from the bitfield + sub-expression. */ + bitpos = value_bitpos (v); + bitsize = value_bitsize (v); + } + else if (v == result && b->val_bitsize != 0) + { + /* If VAL_BITSIZE != 0 then RESULT is actually a bitfield + lvalue whose bit parameters are saved in the fields + VAL_BITPOS and VAL_BITSIZE. */ + bitpos = b->val_bitpos; + bitsize = b->val_bitsize; + } addr = value_address (v); + if (bitsize != 0) + { + /* 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 +1974,15 @@ update_watchpoint (struct watchpoint *b, int reparse) loc->pspace = frame_pspace; loc->address = addr; - loc->length = TYPE_LENGTH (value_type (v)); + + if (bitsize != 0) + { + /* 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 +5099,9 @@ watchpoint_check (void *p) mark = value_mark (); fetch_subexp_value (b->exp, &pc, &new_val, NULL, NULL, 0); + if (b->val_bitsize != 0) + 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 @@ -11203,6 +11266,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; @@ -11336,6 +11400,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 != NULL && just_location) + { + saved_bitpos = value_bitpos (val); + saved_bitsize = value_bitsize (val); + } + if (just_location) { int ret; @@ -11471,6 +11541,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 8abb5ea..00c8802 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/watch-bitfields.c b/gdb/testsuite/gdb.base/watch-bitfields.c new file mode 100644 index 0000000..fb57885 --- /dev/null +++ b/gdb/testsuite/gdb.base/watch-bitfields.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 <http://www.gnu.org/licenses/>. */ + +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.h--; + q.c--; + q.b--; + q.e--; + q.d--; + q.c--; + q.f--; + q.g--; + q.h--; + + + return 0; +} diff --git a/gdb/testsuite/gdb.base/watch-bitfields.exp b/gdb/testsuite/gdb.base/watch-bitfields.exp new file mode 100644 index 0000000..3f25384 --- /dev/null +++ b/gdb/testsuite/gdb.base/watch-bitfields.exp @@ -0,0 +1,56 @@ +# 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 <http://www.gnu.org/licenses/>. + +# 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 +} + +# Continue inferior execution, expecting the watchpoint EXPR to be triggered +# having old value OLD and new value NEW. +proc expect_watchpoint { expr old new } { + set expr_re [string_to_regexp $expr] + gdb_test "print $expr" "\\$\\d+ = $old\\s" + gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" + gdb_test "print $expr" "\\$\\d+ = $new\\s" +} + +# Check that -location watchpoints against bitfields trigger properly. +gdb_test "watch -l q.a" +gdb_test "watch -l q.e" +expect_watchpoint "q.a" 0 1 +expect_watchpoint "q.e" 0 5 +expect_watchpoint "q.a" 1 0 +expect_watchpoint "q.e" 5 4 +gdb_test "cont" ".*exited normally.*" + +# Check that regular watchpoints against expressions involving bitfields +# trigger properly. +runto_main +gdb_test "watch q.d + q.f + q.g" +expect_watchpoint "q.d + q.f + q.g" 0 4 +expect_watchpoint "q.d + q.f + q.g" 4 10 +expect_watchpoint "q.d + q.f + q.g" 10 3 +expect_watchpoint "q.d + q.f + q.g" 3 2 +expect_watchpoint "q.d + q.f + q.g" 2 1 +expect_watchpoint "q.d + q.f + q.g" 1 0 +gdb_test "cont" ".*exited normally.*" diff --git a/gdb/value.c b/gdb/value.c index 6620f96..fdc8858d 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -3231,7 +3231,7 @@ unpack_field_as_long (struct type *type, const gdb_byte *valaddr, int fieldno) are unavailable/optimized out, DEST_VAL is correspondingly marked unavailable/optimized out. */ -static void +void unpack_value_bitfield (struct value *dest_val, int bitpos, int bitsize, const gdb_byte *valaddr, int embedded_offset, diff --git a/gdb/value.h b/gdb/value.h index 4cdbf21..e3603c3 100644 --- a/gdb/value.h +++ b/gdb/value.h @@ -613,6 +613,11 @@ extern int unpack_value_field_as_long (struct type *type, const gdb_byte *valadd int embedded_offset, int fieldno, const struct value *val, LONGEST *result); +extern void unpack_value_bitfield (struct value *dest_val, + int bitpos, int bitsize, + const gdb_byte *valaddr, int embedded_offset, + const struct value *val); + extern struct value *value_field_bitfield (struct type *type, int fieldno, const gdb_byte *valaddr, int embedded_offset, -- 1.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-09-07 18:36 ` Patrick Palka 2014-09-07 18:37 ` [PATCH] " Patrick Palka @ 2014-09-07 23:57 ` Sergio Durigan Junior 2014-09-08 0:10 ` Sergio Durigan Junior 1 sibling, 1 reply; 15+ messages in thread From: Sergio Durigan Junior @ 2014-09-07 23:57 UTC (permalink / raw) To: Patrick Palka; +Cc: Pedro Alves, gdb-patches Hi Patrick, Thanks for the patch. Comments about the testcase below. On Sunday, September 07 2014, Patrick Palka wrote: >> 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? Ahh, the art of testcase writing :-). Yes, Pedro meant the third argument of gdb_test. You are free to omit it if you are doing something simple (e.g., "cont"), but when you do so, the test message becomes the first argument of the gdb_test (i.e., the command itself). And since you are doing lots of "cont", you will see lots of "cont" in the messages as well. Take a look: $ cat gdb/testsuite/gdb.sum | grep "PASS" | sort | uniq -c | sort -n 1 PASS: gdb.base/watch-bitfields.exp: watch -l q.a 1 PASS: gdb.base/watch-bitfields.exp: watch -l q.e 1 PASS: gdb.base/watch-bitfields.exp: watch q.d + q.f + q.g 4 PASS: gdb.base/watch-bitfields.exp: print q.a 4 PASS: gdb.base/watch-bitfields.exp: print q.e 12 PASS: gdb.base/watch-bitfields.exp: cont 12 PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g See that you have 12 tests whose messages are "cont"? One thing you could do is to actually write the test messages. Another thing is to use with_test_prefix, as mentioned in the wiki. For example: On Sunday, September 07 2014, Patrick Palka wrote: > +# Continue inferior execution, expecting the watchpoint EXPR to be triggered > +# having old value OLD and new value NEW. > +proc expect_watchpoint { expr old new } { > + set expr_re [string_to_regexp $expr] > + gdb_test "print $expr" "\\$\\d+ = $old\\s" gdb_test "print $expr" "\\$\\d+ = $old\\s" \ "check if expr == $old" You also don't need to check for the beginning starting "$<N> = " (but if you want, you can use $decimal instead of \d+). No need to check for \s also. > + gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" \ "check if watch on $expr triggers" > + gdb_test "print $expr" "\\$\\d+ = $new\\s" gdb_test "print $expr" "\\$\\d+ = $new\\s" \ "check if expr == $new" > +} > + > +# Check that -location watchpoints against bitfields trigger properly. > +gdb_test "watch -l q.a" > +gdb_test "watch -l q.e" > +expect_watchpoint "q.a" 0 1 > +expect_watchpoint "q.e" 0 5 > +expect_watchpoint "q.a" 1 0 > +expect_watchpoint "q.e" 5 4 > +gdb_test "cont" ".*exited normally.*" # Check that -location watchpoints against bitfields trigger properly. with_test_prefix "-location watch triggers with bitfields" { gdb_test "watch -l q.a" "insert watchpoint in q.a" gdb_test "watch -l q.e" "insert watchpoint in q.e" expect_watchpoint "q.a" 0 1 ... gdb_test "cont" ".*exited normally.*" "check if program exited normally" } (BTW, it's better to use anchored regex when testing for something. See the last test above). And you would a similar dance with the other set of tests below. > +# Check that regular watchpoints against expressions involving bitfields > +# trigger properly. > +runto_main > +gdb_test "watch q.d + q.f + q.g" > +expect_watchpoint "q.d + q.f + q.g" 0 4 > +expect_watchpoint "q.d + q.f + q.g" 4 10 > +expect_watchpoint "q.d + q.f + q.g" 10 3 > +expect_watchpoint "q.d + q.f + q.g" 3 2 > +expect_watchpoint "q.d + q.f + q.g" 2 1 > +expect_watchpoint "q.d + q.f + q.g" 1 0 > +gdb_test "cont" ".*exited normally.*" Crap, I had to tweak the testcase so much that I figured I'd send a patch. This applies on top of yours. Cheers, -- Sergio GPG key ID: 0x65FC5E36 Please send encrypted e-mail if possible http://sergiodj.net/ Index: binutils-gdb/gdb/testsuite/gdb.base/watch-bitfields.exp =================================================================== --- binutils-gdb.orig/gdb/testsuite/gdb.base/watch-bitfields.exp +++ binutils-gdb/gdb/testsuite/gdb.base/watch-bitfields.exp @@ -25,32 +25,61 @@ if {![runto_main]} { return -1 } +proc insert_watchpoint { expr } { + global decimal + + set expr_re [string_to_regexp $expr] + gdb_test "watch $expr" "\(Hardware \)?\[Ww\]atchpoint $decimal: $expr_re" \ + "insert watchpoint on $expr" +} + +proc insert_location_watchpoint { expr } { + global decimal + + set expr_re [string_to_regexp $expr] + gdb_test "watch -l $expr" \ + "\(Hardware \)?\[Ww\]atchpoint $decimal: -location $expr_re" \ + "insert location watchpoint on $expr" +} + # Continue inferior execution, expecting the watchpoint EXPR to be triggered # having old value OLD and new value NEW. proc expect_watchpoint { expr old new } { set expr_re [string_to_regexp $expr] - gdb_test "print $expr" "\\$\\d+ = $old\\s" - gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" - gdb_test "print $expr" "\\$\\d+ = $new\\s" + gdb_test "print $expr" " = $old" "check if $expr == $old" + gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" \ + "check if watch on $expr triggers (old val = $old, new val = $new)" + gdb_test "print $expr" " = $new" "check if expr == $new" } # Check that -location watchpoints against bitfields trigger properly. -gdb_test "watch -l q.a" -gdb_test "watch -l q.e" -expect_watchpoint "q.a" 0 1 -expect_watchpoint "q.e" 0 5 -expect_watchpoint "q.a" 1 0 -expect_watchpoint "q.e" 5 4 -gdb_test "cont" ".*exited normally.*" +with_test_prefix "-location watch triggers with bitfields" { + insert_location_watchpoint "q.a" + insert_location_watchpoint "q.e" + expect_watchpoint "q.a" 0 1 + expect_watchpoint "q.e" 0 5 + expect_watchpoint "q.a" 1 0 + expect_watchpoint "q.e" 5 4 + gdb_test "cont" \ + "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\].*" \ + "check if program exited normally" +} # Check that regular watchpoints against expressions involving bitfields # trigger properly. -runto_main -gdb_test "watch q.d + q.f + q.g" -expect_watchpoint "q.d + q.f + q.g" 0 4 -expect_watchpoint "q.d + q.f + q.g" 4 10 -expect_watchpoint "q.d + q.f + q.g" 10 3 -expect_watchpoint "q.d + q.f + q.g" 3 2 -expect_watchpoint "q.d + q.f + q.g" 2 1 -expect_watchpoint "q.d + q.f + q.g" 1 0 -gdb_test "cont" ".*exited normally.*" +if {![runto_main]} { + return -1 +} + +with_test_prefix "regular watch triggers against bitfields" { + insert_watchpoint "q.d + q.f + q.g" + expect_watchpoint "q.d + q.f + q.g" 0 4 + expect_watchpoint "q.d + q.f + q.g" 4 10 + expect_watchpoint "q.d + q.f + q.g" 10 3 + expect_watchpoint "q.d + q.f + q.g" 3 2 + expect_watchpoint "q.d + q.f + q.g" 2 1 + expect_watchpoint "q.d + q.f + q.g" 1 0 + gdb_test "cont" \ + "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\].*" \ + "check if program exited normally" +} ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-09-07 23:57 ` [PATCH v2] " Sergio Durigan Junior @ 2014-09-08 0:10 ` Sergio Durigan Junior 2014-09-16 16:54 ` Pedro Alves 0 siblings, 1 reply; 15+ messages in thread From: Sergio Durigan Junior @ 2014-09-08 0:10 UTC (permalink / raw) To: Patrick Palka; +Cc: Pedro Alves, gdb-patches On Sunday, September 07 2014, I wrote: >> + gdb_test "print $expr" "\\$\\d+ = $new\\s" > > gdb_test "print $expr" "\\$\\d+ = $new\\s" \ > "check if expr == $new" I forgot to mention that this is not enough to make the messages unique, so I added a "(old val = $old, new val = $new)" after the proposed message. -- Sergio GPG key ID: 0x65FC5E36 Please send encrypted e-mail if possible http://sergiodj.net/ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-09-08 0:10 ` Sergio Durigan Junior @ 2014-09-16 16:54 ` Pedro Alves 2014-09-16 17:05 ` Sergio Durigan Junior 0 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2014-09-16 16:54 UTC (permalink / raw) To: Sergio Durigan Junior, Patrick Palka; +Cc: gdb-patches On 09/08/2014 01:10 AM, Sergio Durigan Junior wrote: > On Sunday, September 07 2014, I wrote: > >>> + gdb_test "print $expr" "\\$\\d+ = $new\\s" >> >> gdb_test "print $expr" "\\$\\d+ = $new\\s" \ >> "check if expr == $new" > > I forgot to mention that this is not enough to make the messages unique, > so I added a "(old val = $old, new val = $new)" after the proposed > message. Hey Sergio, For some odd reason, I completely missed your replies to Patrick, and wanting to keep this moving, ended up redoing most of your patch from scratch. Gah. /me bangs head in wall :-P. As penance, I salvaged pieces from both patches, which I think ends up being better than either version was. - function names a bit more concise - gdb.sum output is more concise too and shows patterns more clearly. - putting things in procedures allows run_to_main returning with canceling the other branch of the test (and allows easy hacking away part of the test when debugging it). - uses gdb_continue_to_end This is what we have currently: Running /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.base/watch-bitfields.exp ... PASS: gdb.base/watch-bitfields.exp: watch -l q.a PASS: gdb.base/watch-bitfields.exp: watch -l q.e PASS: gdb.base/watch-bitfields.exp: print q.a PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: print q.a PASS: gdb.base/watch-bitfields.exp: print q.e PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: print q.e PASS: gdb.base/watch-bitfields.exp: print q.a PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: print q.a PASS: gdb.base/watch-bitfields.exp: print q.e PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: print q.e PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: watch q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: cont PASS: gdb.base/watch-bitfields.exp: print q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: cont This is the end result: Running /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.base/watch-bitfields.exp ... PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: watch -location q.a PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: watch -location q.e PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 0->1: print expression before PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 0->1: continue PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 0->1: print expression after PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: print expression before PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: print expression after PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 1->0: print expression before PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 1->0: continue PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.a: 1->0: print expression after PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 5->4: print expression before PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 5->4: continue PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 5->4: print expression after PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields: continue until exit PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: watch q.d + q.f + q.g PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 0->4: print expression before PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 0->4: continue PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 0->4: print expression after PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 4->10: print expression before PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 4->10: continue PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 4->10: print expression after PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 10->3: print expression before PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 10->3: continue PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 10->3: print expression after PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 3->2: print expression before PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 3->2: continue PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 3->2: print expression after PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 2->1: print expression before PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 2->1: continue PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 2->1: print expression after PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 1->0: print expression before PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 1->0: continue PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: q.d + q.f + q.g: 1->0: print expression after PASS: gdb.base/watch-bitfields.exp: regular watch against bitfields: continue until exit WDYT? ---------- From d3f6e7f2ab3238ed98133d1f484da623dc55e8aa Mon Sep 17 00:00:00 2001 From: Sergio Durigan Junior <sergiodj@redhat.com> Date: Tue, 16 Sep 2014 17:32:04 +0100 Subject: [PATCH] gdb.base/watch-bitfields.exp: Improve test Make test messages unique, and a couple other tweaks. gdb/testsuite/ 2014-09-16 Sergio Durigan Junior <sergiodj@redhat.com> Pedro Alves <palves@redhat.com> * gdb.base/watch-bitfields.exp: Pass string other than test file name to prepare_for_testing. (watch): New procedure. (expect_watchpoint): Use with_test_prefix. (top level): Factor out tests to ... (test_watch_location, test_regular_watch): ... these new procedures, and use with_test_prefix and gdb_continue_to_end. --- gdb/testsuite/gdb.base/watch-bitfields.exp | 77 ++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/gdb/testsuite/gdb.base/watch-bitfields.exp b/gdb/testsuite/gdb.base/watch-bitfields.exp index 3f25384..7b7fa22 100644 --- a/gdb/testsuite/gdb.base/watch-bitfields.exp +++ b/gdb/testsuite/gdb.base/watch-bitfields.exp @@ -17,40 +17,65 @@ standard_testfile -if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} { +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { return -1 } -if {![runto_main]} { - return -1 +# Set a watchpoint watching EXPR. +proc watch { expr } { + global decimal + + set expr_re [string_to_regexp $expr] + gdb_test "watch $expr" \ + "\(Hardware \)?\[Ww\]atchpoint $decimal: $expr_re" } # Continue inferior execution, expecting the watchpoint EXPR to be triggered # having old value OLD and new value NEW. proc expect_watchpoint { expr old new } { - set expr_re [string_to_regexp $expr] - gdb_test "print $expr" "\\$\\d+ = $old\\s" - gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" - gdb_test "print $expr" "\\$\\d+ = $new\\s" + with_test_prefix "$expr: $old->$new" { + set expr_re [string_to_regexp $expr] + gdb_test "print $expr" "\\$\\d+ = $old\\s" "print expression before" + gdb_test "continue" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" + gdb_test "print $expr" "\\$\\d+ = $new\\s" "print expression after" + } } # Check that -location watchpoints against bitfields trigger properly. -gdb_test "watch -l q.a" -gdb_test "watch -l q.e" -expect_watchpoint "q.a" 0 1 -expect_watchpoint "q.e" 0 5 -expect_watchpoint "q.a" 1 0 -expect_watchpoint "q.e" 5 4 -gdb_test "cont" ".*exited normally.*" - -# Check that regular watchpoints against expressions involving bitfields -# trigger properly. -runto_main -gdb_test "watch q.d + q.f + q.g" -expect_watchpoint "q.d + q.f + q.g" 0 4 -expect_watchpoint "q.d + q.f + q.g" 4 10 -expect_watchpoint "q.d + q.f + q.g" 10 3 -expect_watchpoint "q.d + q.f + q.g" 3 2 -expect_watchpoint "q.d + q.f + q.g" 2 1 -expect_watchpoint "q.d + q.f + q.g" 1 0 -gdb_test "cont" ".*exited normally.*" +proc test_watch_location {} { + with_test_prefix "-location watch against bitfields" { + if {![runto_main]} { + return -1 + } + + watch "-location q.a" + watch "-location q.e" + expect_watchpoint "q.a" 0 1 + expect_watchpoint "q.e" 0 5 + expect_watchpoint "q.a" 1 0 + expect_watchpoint "q.e" 5 4 + gdb_continue_to_end + } +} + +# Check that regular watchpoints against expressions involving +# bitfields trigger properly. +proc test_regular_watch {} { + with_test_prefix "regular watch against bitfields" { + if {![runto_main]} { + return -1 + } + + watch "q.d + q.f + q.g" + expect_watchpoint "q.d + q.f + q.g" 0 4 + expect_watchpoint "q.d + q.f + q.g" 4 10 + expect_watchpoint "q.d + q.f + q.g" 10 3 + expect_watchpoint "q.d + q.f + q.g" 3 2 + expect_watchpoint "q.d + q.f + q.g" 2 1 + expect_watchpoint "q.d + q.f + q.g" 1 0 + gdb_continue_to_end + } +} + +test_watch_location +test_regular_watch -- 1.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-09-16 16:54 ` Pedro Alves @ 2014-09-16 17:05 ` Sergio Durigan Junior 2014-09-16 17:09 ` Pedro Alves 0 siblings, 1 reply; 15+ messages in thread From: Sergio Durigan Junior @ 2014-09-16 17:05 UTC (permalink / raw) To: Pedro Alves; +Cc: Patrick Palka, gdb-patches On Tuesday, September 16 2014, Pedro Alves wrote: > Hey Sergio, > > For some odd reason, I completely missed your replies to Patrick, > and wanting to keep this moving, ended up redoing most of your > patch from scratch. Gah. /me bangs head in wall :-P. No worries :-). > As penance, I salvaged pieces from both patches, which I think > ends up being better than either version was. > > - function names a bit more concise > - gdb.sum output is more concise too and shows patterns more clearly. > - putting things in procedures allows run_to_main returning with > canceling the other branch of the test (and allows easy hacking > away part of the test when debugging it). > - uses gdb_continue_to_end > > This is what we have currently: > [...] > > WDYT? Looks neat, thanks for doing that! > ---------- > From d3f6e7f2ab3238ed98133d1f484da623dc55e8aa Mon Sep 17 00:00:00 2001 > From: Sergio Durigan Junior <sergiodj@redhat.com> > Date: Tue, 16 Sep 2014 17:32:04 +0100 > Subject: [PATCH] gdb.base/watch-bitfields.exp: Improve test > > Make test messages unique, and a couple other tweaks. > > gdb/testsuite/ > 2014-09-16 Sergio Durigan Junior <sergiodj@redhat.com> > Pedro Alves <palves@redhat.com> > > * gdb.base/watch-bitfields.exp: Pass string other than test file > name to prepare_for_testing. > (watch): New procedure. > (expect_watchpoint): Use with_test_prefix. > (top level): Factor out tests to ... > (test_watch_location, test_regular_watch): ... these new > procedures, and use with_test_prefix and gdb_continue_to_end. > --- > gdb/testsuite/gdb.base/watch-bitfields.exp | 77 ++++++++++++++++++++---------- > 1 file changed, 51 insertions(+), 26 deletions(-) > > diff --git a/gdb/testsuite/gdb.base/watch-bitfields.exp b/gdb/testsuite/gdb.base/watch-bitfields.exp > index 3f25384..7b7fa22 100644 > --- a/gdb/testsuite/gdb.base/watch-bitfields.exp > +++ b/gdb/testsuite/gdb.base/watch-bitfields.exp > @@ -17,40 +17,65 @@ > > standard_testfile > > -if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} { > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { > return -1 > } > > -if {![runto_main]} { > - return -1 > +# Set a watchpoint watching EXPR. > +proc watch { expr } { > + global decimal > + > + set expr_re [string_to_regexp $expr] > + gdb_test "watch $expr" \ > + "\(Hardware \)?\[Ww\]atchpoint $decimal: $expr_re" > } > > # Continue inferior execution, expecting the watchpoint EXPR to be triggered > # having old value OLD and new value NEW. > proc expect_watchpoint { expr old new } { > - set expr_re [string_to_regexp $expr] > - gdb_test "print $expr" "\\$\\d+ = $old\\s" > - gdb_test "cont" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" > - gdb_test "print $expr" "\\$\\d+ = $new\\s" > + with_test_prefix "$expr: $old->$new" { > + set expr_re [string_to_regexp $expr] > + gdb_test "print $expr" "\\$\\d+ = $old\\s" "print expression before" > + gdb_test "continue" "$expr_re\\s.*Old value = $old\\s+New value = $new\\s.*" > + gdb_test "print $expr" "\\$\\d+ = $new\\s" "print expression after" > + } > } > > # Check that -location watchpoints against bitfields trigger properly. > -gdb_test "watch -l q.a" > -gdb_test "watch -l q.e" > -expect_watchpoint "q.a" 0 1 > -expect_watchpoint "q.e" 0 5 > -expect_watchpoint "q.a" 1 0 > -expect_watchpoint "q.e" 5 4 > -gdb_test "cont" ".*exited normally.*" > - > -# Check that regular watchpoints against expressions involving bitfields > -# trigger properly. > -runto_main > -gdb_test "watch q.d + q.f + q.g" > -expect_watchpoint "q.d + q.f + q.g" 0 4 > -expect_watchpoint "q.d + q.f + q.g" 4 10 > -expect_watchpoint "q.d + q.f + q.g" 10 3 > -expect_watchpoint "q.d + q.f + q.g" 3 2 > -expect_watchpoint "q.d + q.f + q.g" 2 1 > -expect_watchpoint "q.d + q.f + q.g" 1 0 > -gdb_test "cont" ".*exited normally.*" > +proc test_watch_location {} { > + with_test_prefix "-location watch against bitfields" { > + if {![runto_main]} { > + return -1 > + } > + > + watch "-location q.a" > + watch "-location q.e" > + expect_watchpoint "q.a" 0 1 > + expect_watchpoint "q.e" 0 5 > + expect_watchpoint "q.a" 1 0 > + expect_watchpoint "q.e" 5 4 > + gdb_continue_to_end > + } > +} > + > +# Check that regular watchpoints against expressions involving > +# bitfields trigger properly. > +proc test_regular_watch {} { > + with_test_prefix "regular watch against bitfields" { > + if {![runto_main]} { > + return -1 > + } > + > + watch "q.d + q.f + q.g" > + expect_watchpoint "q.d + q.f + q.g" 0 4 > + expect_watchpoint "q.d + q.f + q.g" 4 10 > + expect_watchpoint "q.d + q.f + q.g" 10 3 > + expect_watchpoint "q.d + q.f + q.g" 3 2 > + expect_watchpoint "q.d + q.f + q.g" 2 1 > + expect_watchpoint "q.d + q.f + q.g" 1 0 > + gdb_continue_to_end > + } > +} > + > +test_watch_location > +test_regular_watch > -- > 1.9.3 The patch looks good to me. Thanks! -- Sergio GPG key ID: 0x65FC5E36 Please send encrypted e-mail if possible http://sergiodj.net/ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-09-16 17:05 ` Sergio Durigan Junior @ 2014-09-16 17:09 ` Pedro Alves 2014-09-17 13:00 ` Patrick Palka 0 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2014-09-16 17:09 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Patrick Palka, gdb-patches On 09/16/2014 06:05 PM, Sergio Durigan Junior wrote: > On Tuesday, September 16 2014, Pedro Alves wrote: >> For some odd reason, I completely missed your replies to Patrick, >> and wanting to keep this moving, ended up redoing most of your >> patch from scratch. Gah. /me bangs head in wall :-P. > > No worries :-). Phew. :-) > Looks neat, thanks for doing that! ... > The patch looks good to me. > > Thanks! Thank you! Now pushed. Pedro Alves ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments 2014-09-16 17:09 ` Pedro Alves @ 2014-09-17 13:00 ` Patrick Palka 0 siblings, 0 replies; 15+ messages in thread From: Patrick Palka @ 2014-09-17 13:00 UTC (permalink / raw) To: Pedro Alves; +Cc: Sergio Durigan Junior, gdb-patches Awesome! Thanks to the both of you, for your help and guidance. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-09-17 13:00 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-20 19:40 [PATCH] Fix PR12526: -location watchpoints for bitfield arguments Patrick Palka 2014-08-20 20:59 ` Patrick Palka 2014-08-21 3:32 ` [PATCH v2] " Patrick Palka 2014-08-27 14:28 ` Patrick Palka 2014-09-03 13:18 ` Patrick Palka 2014-09-04 15:04 ` Pedro Alves 2014-09-07 18:36 ` Patrick Palka 2014-09-07 18:37 ` [PATCH] " Patrick Palka 2014-09-16 16:46 ` Pedro Alves 2014-09-07 23:57 ` [PATCH v2] " Sergio Durigan Junior 2014-09-08 0:10 ` Sergio Durigan Junior 2014-09-16 16:54 ` Pedro Alves 2014-09-16 17:05 ` Sergio Durigan Junior 2014-09-16 17:09 ` Pedro Alves 2014-09-17 13:00 ` Patrick Palka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox