From patchwork Sun Sep 7 23:57:25 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 2670 Received: (qmail 28623 invoked by alias); 7 Sep 2014 23:57:31 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 28614 invoked by uid 89); 7 Sep 2014 23:57:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.0 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Sun, 07 Sep 2014 23:57:29 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s87NvQtk030197 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Sun, 7 Sep 2014 19:57:26 -0400 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s87NvPHd020732 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Sun, 7 Sep 2014 19:57:26 -0400 From: Sergio Durigan Junior To: Patrick Palka Cc: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH v2] Fix PR12526: -location watchpoints for bitfield arguments References: <1408563606-24314-1-git-send-email-patrick@parcs.ath.cx> <1408591949-29689-1-git-send-email-patrick@parcs.ath.cx> <54087F68.8020606@redhat.com> X-URL: http://www.redhat.com Date: Sun, 07 Sep 2014 19:57:25 -0400 In-Reply-To: (Patrick Palka's message of "Sun, 7 Sep 2014 14:36:00 -0400") Message-ID: <87zjebyoey.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes 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 "$ = " (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, 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" +}