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

[Sergio Durigan Junior <sergiodj@redhat.com>]

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