Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments

Sergio Durigan Junior
2014-09-07T23:57:31Z

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"
+}