public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] gdb/testsuite: Run test when software watchpoints are used
@ 2018-07-04 16:40 Andrew Burgess
  2018-07-06  2:53 ` Simon Marchi
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Burgess @ 2018-07-04 16:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

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(-)

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" \
-- 
2.14.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gdb/testsuite: Run test when software watchpoints are used
  2018-07-04 16:40 [PATCH] gdb/testsuite: Run test when software watchpoints are used Andrew Burgess
@ 2018-07-06  2:53 ` Simon Marchi
  2018-07-06 13:59   ` Andrew Burgess
  0 siblings, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2018-07-06  2:53 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gdb/testsuite: Run test when software watchpoints are used
  2018-07-06  2:53 ` Simon Marchi
@ 2018-07-06 13:59   ` Andrew Burgess
  2018-07-06 14:43     ` Simon Marchi
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Burgess @ 2018-07-06 13:59 UTC (permalink / raw)
  To: Simon Marchi; +Cc: gdb-patches

* 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gdb/testsuite: Run test when software watchpoints are used
  2018-07-06 13:59   ` Andrew Burgess
@ 2018-07-06 14:43     ` Simon Marchi
  2018-07-06 23:21       ` Andrew Burgess
  0 siblings, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2018-07-06 14:43 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gdb/testsuite: Run test when software watchpoints are used
  2018-07-06 14:43     ` Simon Marchi
@ 2018-07-06 23:21       ` Andrew Burgess
  2018-07-07  0:47         ` Simon Marchi
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Burgess @ 2018-07-06 23:21 UTC (permalink / raw)
  To: Simon Marchi; +Cc: gdb-patches

* Simon Marchi <simark@simark.ca> [2018-07-06 10:43:49 -0400]:

> 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.

OK, got you, something like this then:

Thanks,
Andrew

---

[PATCH] gdb/testsuite: Run test with software and hardware watchpoints

Expand the gdb.base/watchpoint-reuse-slot.exp test to be run twice,
once with hardware watchpoints disabled (this is new) and then with
hardware watchpoints enabled (the old way).

Running with hardware watchpoints enabled is skipped if the board file
says that hardware watchpoints are not supported on this target.

gdb/testsuite/ChangeLog:

	* gdb.base/watchpoint-reuse-slot.exp: Test with hardware
	watchpoints disabled and then enabled.
---
 gdb/testsuite/ChangeLog                          |   5 +
 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 170 ++++++++++++++---------
 2 files changed, 108 insertions(+), 67 deletions(-)

diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
index f196b89eab7..b1d7b14206e 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]} {
@@ -85,54 +80,62 @@ proc stepi {} {
 
 gdb_breakpoint $srcline
 gdb_continue_to_breakpoint "stepi line"
+set cur_addr [get_pc]
 
 # The test tries various sequences of different types of watchpoints.
 # Probe for support first.
+proc build_cmds_list {} {
+    global gdb_prompt
+
+    # So we get an immediate warning/error if the target doesn't support a
+    # given watchpoint type.
+    gdb_test_no_output "set breakpoint always-inserted on" \
+	"Set breakpoints always inserted while building cmds list"
+
+    # The list of supported commands.  Below we'll probe for support and
+    # add elements to this list.
+    set cmds {}
+
+    foreach cmd {"watch" "awatch" "rwatch"} {
+	set test $cmd
+	gdb_test_multiple "$cmd buf.byte\[0\]" $test {
+	    -re "You may have requested too many.*$gdb_prompt $" {
+		unsupported $test
+	    }
+	    -re "Target does not support.*$gdb_prompt $" {
+		unsupported $test
+	    }
+	    -re "Can't set read/access watchpoint when hardware watchpoints are disabled.*$gdb_prompt $" {
+		unsupported $test
+	    }
+	    -re "$gdb_prompt $" {
+		pass $test
+		lappend cmds $cmd
+	    }
+	}
 
-# So we get an immediate warning/error if the target doesn't support a
-# given watchpoint type.
-gdb_test_no_output "set breakpoint always-inserted on"
-
-# The list of supported commands.  Below we'll probe for support and
-# add elements to this list.
-set cmds {}
+	delete_breakpoints
+    }
 
-foreach cmd {"watch" "awatch" "rwatch"} {
-    set test $cmd
-    gdb_test_multiple "$cmd buf.byte\[0\]" $test {
+    set test "hbreak"
+    gdb_test_multiple "hbreak main" $test {
 	-re "You may have requested too many.*$gdb_prompt $" {
 	    unsupported $test
 	}
-	-re "Target does not support.*$gdb_prompt $" {
+	-re "No hardware breakpoint support.*$gdb_prompt $" {
 	    unsupported $test
 	}
 	-re "$gdb_prompt $" {
 	    pass $test
-	    lappend cmds $cmd
+	    lappend cmds "hbreak"
 	}
     }
 
-   delete_breakpoints
-}
+    delete_breakpoints
 
-set test "hbreak"
-gdb_test_multiple "hbreak main" $test {
-    -re "You may have requested too many.*$gdb_prompt $" {
-	unsupported $test
-    }
-    -re "No hardware breakpoint support.*$gdb_prompt $" {
-	unsupported $test
-    }
-    -re "$gdb_prompt $" {
-	pass $test
-	lappend cmds "hbreak"
-    }
+    return $cmds
 }
 
-delete_breakpoints
-
-set cur_addr [get_pc]
-
 # Return true if the memory range [buf.byte + OFFSET, +WIDTH] can be
 # monitored by CMD, otherwise return false.
 
@@ -183,9 +186,10 @@ proc valid_addr_p {cmd offset width} {
 }
 
 # Watch WIDTH bytes at BASE + OFFSET.  CMD specifices the specific
-# type of watchpoint to use.  If CMD is "hbreak", WIDTH is ignored.
+# type of watchpoint to use.  If CMD is "hbreak", WIDTH is ignored.  The
+# HW_WP_P flag tells us if harware watchpoints are enabled or not.
 
-proc watch_command {cmd base offset width} {
+proc watch_command {cmd base offset width hw_wp_p} {
     global srcfile srcline hex
 
     if {$cmd == "hbreak"} {
@@ -193,8 +197,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 { ! $hw_wp_p } {
+	    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" \
@@ -206,42 +217,48 @@ proc watch_command {cmd base offset width} {
     }
 }
 
-# Run test proper.  See intro for description.
-
-foreach always_inserted {"off" "on" } {
-    gdb_test_no_output "set breakpoint always-inserted $always_inserted"
-    foreach cmd1 $cmds {
-	foreach cmd2 $cmds {
-	    for {set width 1} {$width < 4} {incr width} {
+# Run the watchpoint tests (see the description at the top for details), the
+# HW_WP_P flag tells us if hardware watchpoints are enabled or not.
+proc run_watchpoints_tests {hw_wp_p} {
 
-		if {$cmd1 == "hbreak" && $cmd2 == "hbreak" && $width > 1} {
-		    # hbreak ignores WIDTH, no use testing more than
-		    # once.
-		    continue
-		}
+    set cmds [build_cmds_list]
 
-		for {set x 0} {$x < 4} {incr x} {
+    foreach always_inserted {"off" "on" } {
+	gdb_test_no_output "set breakpoint always-inserted $always_inserted"
+	foreach cmd1 $cmds {
+	    foreach cmd2 $cmds {
+		for {set width 1} {$width < 4} {incr width} {
 
-		    if { ![valid_addr_p $cmd1 $x $width]
-			 || ![valid_addr_p $cmd2 $x+1 $width] } {
-			# Skip tests if requested address or length
-			# of breakpoint or watchpoint don't meet
-			# target or kernel requirements.
+		    if {$cmd1 == "hbreak" && $cmd2 == "hbreak" \
+			    && $width > 1} {
+			# hbreak ignores WIDTH, no use testing more than
+			# once.
 			continue
 		    }
 
-		    set prefix "always-inserted $always_inserted: "
-		    append prefix "$cmd1 x $cmd2: "
-		    with_test_prefix "$prefix: width $width, iter $x" {
-			with_test_prefix "base + 0" {
-			    watch_command $cmd1 $x 0 $width
-			    stepi
-			    gdb_test_no_output "delete \$bpnum"
+		    for {set x 0} {$x < 4} {incr x} {
+
+			if { ![valid_addr_p $cmd1 $x $width]
+			     || ![valid_addr_p $cmd2 $x+1 $width] } {
+			    # Skip tests if requested address or length
+			    # of breakpoint or watchpoint don't meet
+			    # target or kernel requirements.
+			    continue
 			}
-			with_test_prefix "base + 1" {
-			    watch_command $cmd2 $x 1 $width
-			    stepi
-			    gdb_test_no_output "delete \$bpnum"
+
+			set prefix "always-inserted $always_inserted: "
+			append prefix "$cmd1 x $cmd2: "
+			with_test_prefix "$prefix: width $width, iter $x" {
+			    with_test_prefix "base + 0" {
+				watch_command $cmd1 $x 0 $width $hw_wp_p
+				stepi
+				gdb_test_no_output "delete \$bpnum"
+			    }
+			    with_test_prefix "base + 1" {
+				watch_command $cmd2 $x 1 $width $hw_wp_p
+				stepi
+				gdb_test_no_output "delete \$bpnum"
+			    }
 			}
 		    }
 		}
@@ -249,3 +266,22 @@ foreach always_inserted {"off" "on" } {
 	}
     }
 }
+
+# Run tests with hardware watchpoints disabled, then again with them
+# enabled (if this target supports hardware watchpoints).
+foreach hw_wp_p { 0 1 } {
+
+    if { $hw_wp_p } {
+	# Does this target support h/w watchpoints?
+	if [target_info exists gdb,no_hardware_watchpoints] { continue }
+	set prefix "hw-watch"
+    } else {
+	set prefix "sw-watch"
+    }
+
+    gdb_test_no_output "set can-use-hw-watchpoints ${hw_wp_p}"
+
+    with_test_prefix $prefix {
+	run_watchpoints_tests $hw_wp_p
+    }
+}
-- 
2.14.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gdb/testsuite: Run test when software watchpoints are used
  2018-07-06 23:21       ` Andrew Burgess
@ 2018-07-07  0:47         ` Simon Marchi
  2018-07-10 14:01           ` Andrew Burgess
  0 siblings, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2018-07-07  0:47 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

On 2018-07-06 07:21 PM, Andrew Burgess wrote:
> OK, got you, something like this then:

Yes, that looks good.  However, the test doesn't really work for me here,
even without this patch, I get a ton of failures.  Does it work well for you
on native x86_64?  If so, I'm fine if you push the patch.  Finding it why it
fails on my machine is another story.  Here's the gdb.log, for reference.

https://pastebin.com/raw/izy5c0Eh

I just noted two nits:

> @@ -183,9 +186,10 @@ proc valid_addr_p {cmd offset width} {
>  }
>  
>  # Watch WIDTH bytes at BASE + OFFSET.  CMD specifices the specific
> -# type of watchpoint to use.  If CMD is "hbreak", WIDTH is ignored.
> +# type of watchpoint to use.  If CMD is "hbreak", WIDTH is ignored.  The
> +# HW_WP_P flag tells us if harware watchpoints are enabled or not.

"harware"

> @@ -249,3 +266,22 @@ foreach always_inserted {"off" "on" } {
>  	}
>      }
>  }
> +
> +# Run tests with hardware watchpoints disabled, then again with them
> +# enabled (if this target supports hardware watchpoints).
> +foreach hw_wp_p { 0 1 } {
> +
> +    if { $hw_wp_p } {
> +	# Does this target support h/w watchpoints?
> +	if [target_info exists gdb,no_hardware_watchpoints] { continue }
> +	set prefix "hw-watch"
> +    } else {
> +	set prefix "sw-watch"
> +    }
> +
> +    gdb_test_no_output "set can-use-hw-watchpoints ${hw_wp_p}"
> +
> +    with_test_prefix $prefix {
> +	run_watchpoints_tests $hw_wp_p
> +    }
> +}

For just two variations, I am not sure we gain much in readability by doing a loop
here.  It might be clearer to do:

run_tests_software_watchpoints

if supports_hardware_watchpoints:
  run_tests_hardware_watchpoints


I don't really mind, it's your choice.

Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gdb/testsuite: Run test when software watchpoints are used
  2018-07-07  0:47         ` Simon Marchi
@ 2018-07-10 14:01           ` Andrew Burgess
  2018-10-31 13:52             ` Phil Muldoon
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Burgess @ 2018-07-10 14:01 UTC (permalink / raw)
  To: Simon Marchi; +Cc: gdb-patches

* Simon Marchi <simark@simark.ca> [2018-07-06 20:47:16 -0400]:

> On 2018-07-06 07:21 PM, Andrew Burgess wrote:
> > OK, got you, something like this then:
> 
> Yes, that looks good.  However, the test doesn't really work for me here,
> even without this patch, I get a ton of failures.  Does it work well for you
> on native x86_64?  If so, I'm fine if you push the patch.  Finding it why it
> fails on my machine is another story.  Here's the gdb.log, for reference.
> 
> https://pastebin.com/raw/izy5c0Eh

So... here's what I know so far....

This _used_ to work for me until _very_ recently, but no longer does.

My setup at the moment is x86-64, Fedora 27, -uname -sr' output:

    Linux 4.17.3-100.fc27.x86_64

I'm pretty sure I performed a machine update, including kernel
version, in the last few days (since posting this patch) so my
suspicion is that this might be to blame.

To further back this up, my other machine (which is badly in need of
an update) is x86-64, but running Fedora 24, 'uname -sr' output is:

    Linux 4.11.12-100.fc24.x86_64

And on this machine this test script runs with no unexpected
failures.

My current guess is that a recent update has changed some aspect of
the environment such that the hardware watchpoint support is no longer
working as expected.  This is a little irritating but I don't think
was made worse with my patch, so, with your feedback below fixed, I've
pushed this in.

Thanks,
Andrew




> 
> I just noted two nits:
> 
> > @@ -183,9 +186,10 @@ proc valid_addr_p {cmd offset width} {
> >  }
> >  
> >  # Watch WIDTH bytes at BASE + OFFSET.  CMD specifices the specific
> > -# type of watchpoint to use.  If CMD is "hbreak", WIDTH is ignored.
> > +# type of watchpoint to use.  If CMD is "hbreak", WIDTH is ignored.  The
> > +# HW_WP_P flag tells us if harware watchpoints are enabled or not.
> 
> "harware"
> 
> > @@ -249,3 +266,22 @@ foreach always_inserted {"off" "on" } {
> >  	}
> >      }
> >  }
> > +
> > +# Run tests with hardware watchpoints disabled, then again with them
> > +# enabled (if this target supports hardware watchpoints).
> > +foreach hw_wp_p { 0 1 } {
> > +
> > +    if { $hw_wp_p } {
> > +	# Does this target support h/w watchpoints?
> > +	if [target_info exists gdb,no_hardware_watchpoints] { continue }
> > +	set prefix "hw-watch"
> > +    } else {
> > +	set prefix "sw-watch"
> > +    }
> > +
> > +    gdb_test_no_output "set can-use-hw-watchpoints ${hw_wp_p}"
> > +
> > +    with_test_prefix $prefix {
> > +	run_watchpoints_tests $hw_wp_p
> > +    }
> > +}
> 
> For just two variations, I am not sure we gain much in readability by doing a loop
> here.  It might be clearer to do:
> 
> run_tests_software_watchpoints
> 
> if supports_hardware_watchpoints:
>   run_tests_hardware_watchpoints
> 
> 
> I don't really mind, it's your choice.
> 
> Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gdb/testsuite: Run test when software watchpoints are used
  2018-07-10 14:01           ` Andrew Burgess
@ 2018-10-31 13:52             ` Phil Muldoon
  0 siblings, 0 replies; 8+ messages in thread
From: Phil Muldoon @ 2018-10-31 13:52 UTC (permalink / raw)
  To: Andrew Burgess, Simon Marchi; +Cc: gdb-patches

On 10/07/2018 15:01, Andrew Burgess wrote:
> * Simon Marchi <simark@simark.ca> [2018-07-06 20:47:16 -0400]:

>> Yes, that looks good.  However, the test doesn't really work for me here,
>> even without this patch, I get a ton of failures.  Does it work well for you
>> on native x86_64?  If so, I'm fine if you push the patch.  Finding it why it
>> fails on my machine is another story.  Here's the gdb.log, for reference.
>>
>> https://pastebin.com/raw/izy5c0Eh

I've tracked this bug, with what I know, in:

https://sourceware.org/bugzilla/show_bug.cgi?id=23845

Cheers,

Phil

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-10-31 13:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-04 16:40 [PATCH] gdb/testsuite: Run test when software watchpoints are used Andrew Burgess
2018-07-06  2:53 ` Simon Marchi
2018-07-06 13:59   ` Andrew Burgess
2018-07-06 14:43     ` Simon Marchi
2018-07-06 23:21       ` Andrew Burgess
2018-07-07  0:47         ` Simon Marchi
2018-07-10 14:01           ` Andrew Burgess
2018-10-31 13:52             ` Phil Muldoon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).