Patchwork gdb/testsuite: Run test when software watchpoints are used

login
register
mail settings
Submitter Andrew Burgess
Date July 4, 2018, 4:40 p.m.
Message ID <20180704164017.5849-1-andrew.burgess@embecosm.com>
Download mbox | patch
Permalink /patch/28237/
State New
Headers show

Comments

Andrew Burgess - July 4, 2018, 4:40 p.m.
The test gdb.base/watchpoint-reuse-slot.exp can be run when software
watchpoints are in use, we just need to update one test pattern to
look for 'Watchpoint' instead of 'Hardware watchpoint' in one case.

gdb/testsuite/ChangeLog:

	* gdb.base/watchpoint-reuse-slot.exp: Test can be run using
	software watchpoints, we just need to update a test pattern in one
	place.
---
 gdb/testsuite/ChangeLog                          |  6 ++++++
 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 14 ++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)
Simon Marchi - July 6, 2018, 2:53 a.m.
On 2018-07-04 12:40 PM, Andrew Burgess wrote:
> The test gdb.base/watchpoint-reuse-slot.exp can be run when software
> watchpoints are in use, we just need to update one test pattern to
> look for 'Watchpoint' instead of 'Hardware watchpoint' in one case.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/watchpoint-reuse-slot.exp: Test can be run using
> 	software watchpoints, we just need to update a test pattern in one
> 	place.

Hi Andrew,

Reading the test description, it seems like it exists specifically to
check the hardware watchpoint mechanisms of the targets.  Did you
find it would also be useful to run it with software watchpoints?
What was the idea that motivated to do this change in the first place?

Simon
Andrew Burgess - July 6, 2018, 1:59 p.m.
* Simon Marchi <simark@simark.ca> [2018-07-05 22:53:54 -0400]:

> On 2018-07-04 12:40 PM, Andrew Burgess wrote:
> > The test gdb.base/watchpoint-reuse-slot.exp can be run when software
> > watchpoints are in use, we just need to update one test pattern to
> > look for 'Watchpoint' instead of 'Hardware watchpoint' in one case.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* gdb.base/watchpoint-reuse-slot.exp: Test can be run using
> > 	software watchpoints, we just need to update a test pattern in one
> > 	place.
> 
> Hi Andrew,
> 
> Reading the test description, it seems like it exists specifically to
> check the hardware watchpoint mechanisms of the targets.  Did you
> find it would also be useful to run it with software watchpoints?
> What was the idea that motivated to do this change in the first
> place?

I think you give me too much credit!

What happened was I had a target without h/w watchpoints, I ran the
GDB testsuite and had a set of passes and fails.  After some
investigation I realised that I'd neglected to mark the target as not
supporting h/w watchpoints in the board file.

Once I'd added the no h/w watchpoint flag in the board file I reran
the tests, and mostly things looked better.  Failures, or unresolved
tests had become unsupported.

However.... in watchpoint-reuse-slot.exp a number of tests that used
to pass had gone away, so I went looking at the test script.

What I saw was that though the test declared a need for h/w
watchpoints, the test would run perfectly fine without them.

You'll notice that with my change if the board file says that h/w
watchpoints are supported then we still look for the full "Hardware
watchpoint" pattern in the output, that is, my change does not mean
that if GDB broke and h/w watchpoints changed to s/w watchpoints (when
they shouldn't) the test would pass.  I think that after my change all
targets that previously ran this test are just as well tested as they
ever were.

But, we have additional s/w watchpoint testing for targets that don't
support h/w watchpoints.  Is this testing anything that's not covered
elsewhere?  Honestly, I don't know.  There probably is a lot of test
duplication, but I can't guarantee that there's nothing unique in
here.

I guess my question is, what's the harm from broadening the test in
this way?  If I've missed something and this change could mean a bug
can now slip into GDB then absolutely, this is not acceptable.  But, I
can't see how (yet)...

Thanks,
Andrew
Simon Marchi - July 6, 2018, 2:43 p.m.
On 2018-07-06 09:59, Andrew Burgess wrote:
> I think you give me too much credit!
> 
> What happened was I had a target without h/w watchpoints, I ran the
> GDB testsuite and had a set of passes and fails.  After some
> investigation I realised that I'd neglected to mark the target as not
> supporting h/w watchpoints in the board file.
> 
> Once I'd added the no h/w watchpoint flag in the board file I reran
> the tests, and mostly things looked better.  Failures, or unresolved
> tests had become unsupported.
> 
> However.... in watchpoint-reuse-slot.exp a number of tests that used
> to pass had gone away, so I went looking at the test script.
> 
> What I saw was that though the test declared a need for h/w
> watchpoints, the test would run perfectly fine without them.
> 
> You'll notice that with my change if the board file says that h/w
> watchpoints are supported then we still look for the full "Hardware
> watchpoint" pattern in the output, that is, my change does not mean
> that if GDB broke and h/w watchpoints changed to s/w watchpoints (when
> they shouldn't) the test would pass.  I think that after my change all
> targets that previously ran this test are just as well tested as they
> ever were.
> 
> But, we have additional s/w watchpoint testing for targets that don't
> support h/w watchpoints.  Is this testing anything that's not covered
> elsewhere?  Honestly, I don't know.  There probably is a lot of test
> duplication, but I can't guarantee that there's nothing unique in
> here.
> 
> I guess my question is, what's the harm from broadening the test in
> this way?  If I've missed something and this change could mean a bug
> can now slip into GDB then absolutely, this is not acceptable.  But, I
> can't see how (yet)...

Indeed, I see no harm in running more tests, at worst it's useless.  
Maybe
it will find some bug in the software watchpoint code.

I was thinking that we might as well run it with software watchpoints 
even
with targets that support hardware watchpoints.  We could run 
unconditionally
with "set can-use-hw-watchpoints 0" and run it with "1" on targets that
support hw watchpoints.

Simon

Patch

diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
index f196b89eab..445c350ef2 100644
--- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
@@ -22,11 +22,6 @@ 
 # operation.  (Note that we don't have any of these watchpoints
 # trigger.)
 
-if [target_info exists gdb,no_hardware_watchpoints] {
-    unsupported "no target support"
-    return
-}
-
 standard_testfile
 
 if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
@@ -193,8 +188,15 @@  proc watch_command {cmd base offset width} {
 	gdb_test "hbreak $expr" "Hardware assisted breakpoint \[0-9\]+ at $hex"
     } elseif {$cmd == "watch"} {
 	set expr "*(buf.byte + $base + $offset)@$width"
+
+	if [target_info exists gdb,no_hardware_watchpoints] {
+	    set wp_prefix "Watchpoint"
+	} else {
+	    set wp_prefix "Hardware watchpoint"
+	}
+
 	gdb_test "$cmd $expr" \
-	    "Hardware watchpoint \[0-9\]+: [string_to_regexp $expr]"
+	    "${wp_prefix} \[0-9\]+: [string_to_regexp $expr]"
     } elseif {$cmd == "awatch"} {
 	set expr "*(buf.byte + $base + $offset)@$width"
 	gdb_test "$cmd $expr" \