[PATCH] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[PATCH] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Carl Love]

GDB maintainers:

The gdb.base/watchpoint-reuse-slot.exp uses the check:

if { ![target_info exists gdb,no_hardware_watchpoints]}

to determine if the test should be re-run with HW watchpoints. The test
is TRUE on Power 9 however Power 9 does not support HW watchpoints. 
Not all PowerPC processors support HW watchpoints.  The test needs to
use the skip_hw_watchpoint_access_tests test to correctly determine if
the processor supports HW watchpoints.  

The skip_hw_watchpoint_access_tests starts a small gdb test on the
platform to determine if the processor supports HW watchpoints or not. 
Thus the check must be done before the actual test is run to ensure the
HW watchpoint check does not mess up the actual test.

This patch replaces the [target_info exists gdb,
no_hardware_watchpoints] with the skip_hw_watchpoint_access_tests
before starting the watchpoint-reuse-slot.exp test.

The fix disables the HW watchpoint tests on Power 9 thus eliminating 48
testsuite failures.

The patch has been tested on Power 9, Power 10 and x86-64

Please let me know if this patch is acceptable for mainline.  Thanks.

                              Carl Love

--------------------------------------------------------
[PATCH] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

This test generates 48 failures on Power 9 when testing with HW watchpoints
enabled.  Note Power 9 does not support HW watchpoints.

Not all PowerPC processors support HW watchpoints.  Add the
skip_hw_watchpoint_access_tests to determine if the processor supports HW
watchpoints.  Note this proceedure runs a small test on PowerPC under gdb to
determine if the processor supports HW watchpoints.  Thus the check must be
done before the actual test to prevent disrupting the actual test.

This patch was tested on Power 9, Power 10 and X86-64 with no regressions.
---
 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
index e2ea137424b..829252a65b4 100644
--- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
@@ -22,6 +22,18 @@
 # operation.  (Note that we don't have any of these watchpoints
 # trigger.)
 
+# The skip_hw_watchpoint_tests checks if watchpoints are supported by the
+# processor.  Not all PowerPC proceesors support HW watchpoints.  The
+# skip_hw_watchpoint_access_tests starts GDB on a small test program to
+# dynamically check if HW watchpoints are supported.  We do not want to
+# restart GDB after this test script has itself started GDB, so call
+# skip_hw_watchpoint_tests first and save the result for use later.
+if {[skip_hw_watchpoint_access_tests]} {
+    set supports_hw_wp 0
+} else {
+    set supports_hw_wp 1
+}
+
 standard_testfile
 
 if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
@@ -285,7 +297,7 @@ proc setup_and_run_watchpoints_tests { hw_wp_p } {
 
 # Run tests with hardware watchpoints disabled, then again with them
 # enabled (if this target supports hardware watchpoints).
-if { ![target_info exists gdb,no_hardware_watchpoints]} {
+if { $supports_hw_wp } {
     # Run test with H/W enabled.
     setup_and_run_watchpoints_tests 1
 }
-- 
2.31.1

Re: [PATCH] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[will schmidt]

On Tue, 2022-08-09 at 17:00 -0700, Carl Love wrote:
> GDB maintainers:
> 
> The gdb.base/watchpoint-reuse-slot.exp uses the check:
> 
> if { ![target_info exists gdb,no_hardware_watchpoints]}
> 

Some of the wording of the description made me think this
patch was incomplete.   a few comments below.


> to determine if the test should be re-run with HW watchpoints. The test

re-run or just run?

> is TRUE on Power 9 however Power 9 does not support HW watchpoints. 
> Not all PowerPC processors support HW watchpoints.  The test needs to
> use the skip_hw_watchpoint_access_tests test to correctly determine if
> the processor supports HW watchpoints.  

Power9 DD2 has breakpoints disabled by default, yes.  

> 
> The skip_hw_watchpoint_access_tests starts a small gdb test on the
> platform to determine if the processor supports HW watchpoints or not. 
> Thus the check must be done before the actual test is run to ensure the
> HW watchpoint check does not mess up the actual test.

Per what I see in the sources (snapshot from a couple weeks ago), The
existing skip_hw_watchpoint_access_tests proc calls
skip_hw_watchpoint_tests that checks the targets and the
cached has_hw_wp_support value.   The has_hw_wp_support cached value
holds the results of the small gdb test. 


> 
> This patch replaces the [target_info exists gdb,
> no_hardware_watchpoints] with the skip_hw_watchpoint_access_tests
> before starting the watchpoint-reuse-slot.exp test.

... in order to properly determine whether the watchpoints are
disabled.



> 
> The fix disables the HW watchpoint tests on Power 9 thus eliminating 48
> testsuite failures.
> 
> The patch has been tested on Power 9, Power 10 and x86-64
> 
> Please let me know if this patch is acceptable for mainline.  Thanks.
> 
>                               Carl Love
> 
> --------------------------------------------------------
> [PATCH] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp
> 
> This test generates 48 failures on Power 9 when testing with HW watchpoints
> enabled.  Note Power 9 does not support HW watchpoints.
> 
> Not all PowerPC processors support HW watchpoints.  Add the
> skip_hw_watchpoint_access_tests to determine if the processor supports HW
> watchpoints.  Note this proceedure runs a small test on PowerPC under gdb to
> determine if the processor supports HW watchpoints.  Thus the check must be
> done before the actual test to prevent disrupting the actual test.

Noting that this patch does not "add" the
skip_hw_watchpoint_access_tests proc.
The existing
skip_hw_watchpoint_tests proc includes the comment, and the relevant
stanza

    # These targets support hardware watchpoints natively
    # Note, not all Power 9 processors support hardware watchpoints due to a HW
    # bug.  Use has_hw_wp_support to check do a runtime check for hardware
    # watchpoint support on Powerpc.
...
         || ([istarget "powerpc*-*-linux*"] && [has_hw_wp_support])

The has_hw_wp_support() proc includes the comment
    # Power 9, proc rev 2.2 does not support HW watchpoints due to HW bug.
    # Need to use a runtime test to determine if the Power processor has
    # support for HW watchpoints.




> 
> This patch was tested on Power 9, Power 10 and X86-64 with no regressions.
> ---
>  gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
> index e2ea137424b..829252a65b4 100644
> --- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
> +++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
> @@ -22,6 +22,18 @@
>  # operation.  (Note that we don't have any of these watchpoints
>  # trigger.)
> 
> +# The skip_hw_watchpoint_tests checks if watchpoints are supported by the
> +# processor.  Not all PowerPC proceesors support HW watchpoints.  The

Calling out PowerPC explicitly here doesn't seem correct, unless you
also call out every other target that is not in the list in
skip_hw_watchpoint_tests().  


> +# skip_hw_watchpoint_access_tests starts GDB on a small test program to
> +# dynamically check if HW watchpoints are supported.  We do not want to
> +# restart GDB after this test script has itself started GDB, so call
> +# skip_hw_watchpoint_tests first and save the result for use later.



> +if {[skip_hw_watchpoint_access_tests]} {
> +    set supports_hw_wp 0
> +} else {
> +    set supports_hw_wp 1
> +}
> +
>  standard_testfile
> 
>  if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
> @@ -285,7 +297,7 @@ proc setup_and_run_watchpoints_tests { hw_wp_p } {
> 
>  # Run tests with hardware watchpoints disabled, then again with them
>  # enabled (if this target supports hardware watchpoints).
> -if { ![target_info exists gdb,no_hardware_watchpoints]} {
> +if { $supports_hw_wp } {

So.. could this be simplified with a check against the existing cached
has_hw_wp_support value?


>      # Run test with H/W enabled.
>      setup_and_run_watchpoints_tests 1
>  }

Re: [PATCH] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Carl Love]

On Wed, 2022-08-10 at 10:17 -0500, will schmidt wrote:
> On Tue, 2022-08-09 at 17:00 -0700, Carl Love wrote:
> > GDB maintainers:
> > 
> > The gdb.base/watchpoint-reuse-slot.exp uses the check:
> > 
> > if { ![target_info exists gdb,no_hardware_watchpoints]}
> > 
> 
> Some of the wording of the description made me think this
> patch was incomplete.   a few comments below.
> 
> 
> > to determine if the test should be re-run with HW watchpoints. The
> > test
> 
> re-run or just run?

The tests are run with HW watchpoints disabled.  Then, if the processor
supports HW watchpoints, the expect script runs the tests again with HW
watchpoints enabled.  Will reword to make it clearer.
> 
> > is TRUE on Power 9 however Power 9 does not support HW
> > watchpoints. 
> > Not all PowerPC processors support HW watchpoints.  The test needs
> > to
> > use the skip_hw_watchpoint_access_tests test to correctly determine
> > if
> > the processor supports HW watchpoints.  
> 
> Power9 DD2 has breakpoints disabled by default, yes.  
> 
> > The skip_hw_watchpoint_access_tests starts a small gdb test on the
> > platform to determine if the processor supports HW watchpoints or
> > not. 
> > Thus the check must be done before the actual test is run to ensure
> > the
> > HW watchpoint check does not mess up the actual test.
> 
> Per what I see in the sources (snapshot from a couple weeks ago), The
> existing skip_hw_watchpoint_access_tests proc calls
> skip_hw_watchpoint_tests that checks the targets and the
> cached has_hw_wp_support value.   The has_hw_wp_support cached value
> holds the results of the small gdb test. 

Yes, and it is important that the skip_hw_watchpoint_access_tests check
is run first and not after the test is started because the check will
restart gdb and thus mess up the test.
> 
> 
> > This patch replaces the [target_info exists gdb,
> > no_hardware_watchpoints] with the skip_hw_watchpoint_access_tests
> > before starting the watchpoint-reuse-slot.exp test.
> 
> ... in order to properly determine whether the watchpoints are
> disabled.

OK, will update the wording.
> 
> 
> 
> > The fix disables the HW watchpoint tests on Power 9 thus
> > eliminating 48
> > testsuite failures.
> > 
> > The patch has been tested on Power 9, Power 10 and x86-64
> > 
> > Please let me know if this patch is acceptable for
> > mainline.  Thanks.
> > 
> >                               Carl Love
> > 
> > --------------------------------------------------------
> > [PATCH] Fix hardware watchpoint check in test gdb.base/watchpoint-
> > reuse-slot.exp
> > 
> > This test generates 48 failures on Power 9 when testing with HW
> > watchpoints
> > enabled.  Note Power 9 does not support HW watchpoints.
> > 
> > Not all PowerPC processors support HW watchpoints.  Add the
> > skip_hw_watchpoint_access_tests to determine if the processor
> > supports HW
> > watchpoints.  Note this proceedure runs a small test on PowerPC
> > under gdb to
> > determine if the processor supports HW watchpoints.  Thus the check
> > must be
> > done before the actual test to prevent disrupting the actual test.
> 
> Noting that this patch does not "add" the

True, it is really replacing the existing check with a new check.

> skip_hw_watchpoint_access_tests proc.
> The existing
> skip_hw_watchpoint_tests proc includes the comment, and the relevant
> stanza
> 
>     # These targets support hardware watchpoints natively
>     # Note, not all Power 9 processors support hardware watchpoints
> due to a HW
>     # bug.  Use has_hw_wp_support to check do a runtime check for
> hardware
>     # watchpoint support on Powerpc.
> ...
>          || ([istarget "powerpc*-*-linux*"] && [has_hw_wp_support])
> 
> The has_hw_wp_support() proc includes the comment
>     # Power 9, proc rev 2.2 does not support HW watchpoints due to HW
> bug.
>     # Need to use a runtime test to determine if the Power processor
> has
>     # support for HW watchpoints.
> 
> 
Yes.   
> 
> 
> > This patch was tested on Power 9, Power 10 and X86-64 with no
> > regressions.
> > ---
> >  gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 14
> > +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
> > b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
> > index e2ea137424b..829252a65b4 100644
> > --- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
> > +++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
> > @@ -22,6 +22,18 @@
> >  # operation.  (Note that we don't have any of these watchpoints
> >  # trigger.)
> > 
> > +# The skip_hw_watchpoint_tests checks if watchpoints are supported
> > by the
> > +# processor.  Not all PowerPC proceesors support HW
> > watchpoints.  The
> 
> Calling out PowerPC explicitly here doesn't seem correct, unless you
> also call out every other target that is not in the list in
> skip_hw_watchpoint_tests().  

Yes, we can drop the part specifically calling out Power 9.
> 
> 
> > +# skip_hw_watchpoint_access_tests starts GDB on a small test
> > program to
> > +# dynamically check if HW watchpoints are supported.  We do not
> > want to
> > +# restart GDB after this test script has itself started GDB, so
> > call
> > +# skip_hw_watchpoint_tests first and save the result for use
> > later.
> 
> 
> > +if {[skip_hw_watchpoint_access_tests]} {
> > +    set supports_hw_wp 0
> > +} else {
> > +    set supports_hw_wp 1
> > +}
> > +
> >  standard_testfile
> > 
> >  if {[prepare_for_testing "failed to prepare" $testfile $srcfile
> > debug]} {
> > @@ -285,7 +297,7 @@ proc setup_and_run_watchpoints_tests { hw_wp_p
> > } {
> > 
> >  # Run tests with hardware watchpoints disabled, then again with
> > them
> >  # enabled (if this target supports hardware watchpoints).
> > -if { ![target_info exists gdb,no_hardware_watchpoints]} {
> > +if { $supports_hw_wp } {
> 
> So.. could this be simplified with a check against the existing
> cached
> has_hw_wp_support value?

Yes, I hadn't thought about that.  I updated the patch to just check
the cached value.
> 
> 
> >      # Run test with H/W enabled.
> >      setup_and_run_watchpoints_tests 1
> >  }

Re: [PATCH] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Carl Love]

Will:

On Wed, 2022-08-10 at 10:54 -0700, Carl Love wrote:
> > > +if {[skip_hw_watchpoint_access_tests]} {
> > > +    set supports_hw_wp 0
> > > +} else {
> > > +    set supports_hw_wp 1
> > > +}
> > > +
> > >   standard_testfile
> > > 
> > >   if {[prepare_for_testing "failed to prepare" $testfile $srcfile
> > > debug]} {
> > > @@ -285,7 +297,7 @@ proc setup_and_run_watchpoints_tests {
> > > hw_wp_p
> > > } {
> > > 
> > >   # Run tests with hardware watchpoints disabled, then again with
> > > them
> > >   # enabled (if this target supports hardware watchpoints).
> > > -if { ![target_info exists gdb,no_hardware_watchpoints]} {
> > > +if { $supports_hw_wp } {
> > 
> > So.. could this be simplified with a check against the existing
> > cached
> > has_hw_wp_support value?
> 
> Yes, I hadn't thought about that.  I updated the patch to just check
> the cached value.
> > 
> > >       # Run test with H/W enabled.
> > >       setup_and_run_watchpoints_tests 1
> > >   }

I updated the patch on Power to just check the has_hw_wp_support cached
value.  That works great.  However, when I did my testing on x86-64 it
doesn't work.  The issue is the the runtime check is only done on
PowerPC thus the variable has_hw_wp_support is not initialized on other
platforms.  Thus you do need to set a local variable and use that.  

Sorry, I didn't catch that earlier as I hadn't done all the regression
testing when I replied to your email.

                          Carl 

Re: [PATCH ver 2] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Carl Love]

GDB maintainers, Will:

I have updated the patch per comments from Will.

The gdb.base/watchpoint-reuse-slot.exp uses the check:

if { ![target_info exists gdb,no_hardware_watchpoints]}

to determine if the test should be run with HW watchpoints. The
target_info exists check for HW watchpoints is TRUE on Power 9. 
However HW watchpoints are disabled on Power 9 due to a HW bug. The 
skip_hw_watchpoint_access_tests check needs to be used to properly
determine if HW watchpoints are supported by the processor.  If the
processor is PowerPC, the skip_hw_watchpoint_access_tests will run
small gdb test if the processor supports HW watchpoints.   On other
platforms, the skip_hw_watchpoint_access_tests checks the list of
processors that support HW watchpoints.

This patch replaces the [target_info exists gdb,
no_hardware_watchpoints] with the skip_hw_watchpoint_access_tests
check to properly determine if the processor supports HW watchpoints.

The fix disables the HW watchpoint tests on Power 9 thus eliminating 48
testsuite failures.

The patch has been tested on Power 9, Power 10 and x86-64

Please let me know if this patch is acceptable for mainline.  Thanks.

                              Carl Love
---------------------------------------------------------------
Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

This test generates 48 failures on Power 9 when testing with HW watchpoints
enabled.  Note HW watchpoint support is disabled on Power 9 due to a HW bug.
The skip_hw_watchpoint_access_tests proc must be used to correctly determine
if the processor supports HW watchpoints.

This patch replaces the [target_info exists gdb,no_hardware_watchpoints]
with the skip_hw_watchpoint_access_tests check.

This patch was tested on Power 9, Power 10 and X86-64 with no regressions.
---
 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
index e2ea137424b..3d095d2ce9e 100644
--- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
@@ -22,6 +22,20 @@
 # operation.  (Note that we don't have any of these watchpoints
 # trigger.)
 
+# The skip_hw_watchpoint_tests checks if watchpoints are supported by the
+# processor.  On PowerPC, the check runs a small test program under gdb
+# to determine if the Power processor supports HW watchpoints.  The check
+# must be done before starting the test so as to not disrupt the execution
+# of the actual test.
+
+if {[skip_hw_watchpoint_access_tests]} {
+    set supports_hw_wp 0
+} else {
+    set supports_hw_wp 1
+}
+
+# starting the test.
+
 standard_testfile
 
 if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
@@ -285,7 +299,7 @@ proc setup_and_run_watchpoints_tests { hw_wp_p } {
 
 # Run tests with hardware watchpoints disabled, then again with them
 # enabled (if this target supports hardware watchpoints).
-if { ![target_info exists gdb,no_hardware_watchpoints]} {
+if { $supports_hw_wp } {
     # Run test with H/W enabled.
     setup_and_run_watchpoints_tests 1
 }
-- 
2.31.1

Re: [PATCH] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[will schmidt]

On Wed, 2022-08-10 at 12:17 -0700, Carl Love wrote:
> Will:
> 
> On Wed, 2022-08-10 at 10:54 -0700, Carl Love wrote:
> > > > +if {[skip_hw_watchpoint_access_tests]} {
> > > > +    set supports_hw_wp 0
> > > > +} else {
> > > > +    set supports_hw_wp 1
> > > > +}
> > > > +
> > > >   standard_testfile
> > > > 
> > > >   if {[prepare_for_testing "failed to prepare" $testfile $srcfile
> > > > debug]} {
> > > > @@ -285,7 +297,7 @@ proc setup_and_run_watchpoints_tests {
> > > > hw_wp_p
> > > > } {
> > > > 
> > > >   # Run tests with hardware watchpoints disabled, then again with
> > > > them
> > > >   # enabled (if this target supports hardware watchpoints).
> > > > -if { ![target_info exists gdb,no_hardware_watchpoints]} {
> > > > +if { $supports_hw_wp } {
> > > 
> > > So.. could this be simplified with a check against the existing
> > > cached
> > > has_hw_wp_support value?
> > 
> > Yes, I hadn't thought about that.  I updated the patch to just check
> > the cached value.
> > > >       # Run test with H/W enabled.
> > > >       setup_and_run_watchpoints_tests 1
> > > >   }
> 
> I updated the patch on Power to just check the has_hw_wp_support cached
> value.  That works great.  However, when I did my testing on x86-64 it
> doesn't work.  The issue is the the runtime check is only done on
> PowerPC thus the variable has_hw_wp_support is not initialized on other
> platforms.  Thus you do need to set a local variable and use that.  

Hmm, I don't see anything in has_hw_wp_support gdb.exp that limits the
setting to only for powerpc..  why is that the case?

The current usage
of the variable in skip_hw_watchpoint_tests does include an istarget
reference, is that making the difference here?

         || ([istarget "powerpc*-*-linux*"] && [has_hw_wp_support])


> Sorry, I didn't catch that earlier as I hadn't done all the regression
> testing when I replied to your email.
> 
>                           Carl 
> 

Re: [PATCH] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Carl Love]

On Thu, 2022-08-11 at 09:16 -0500, will schmidt wrote:
> On Wed, 2022-08-10 at 12:17 -0700, Carl Love wrote:
> > Will:
> > 
> > On Wed, 2022-08-10 at 10:54 -0700, Carl Love wrote:
> > > > > +if {[skip_hw_watchpoint_access_tests]} {
> > > > > +    set supports_hw_wp 0
> > > > > +} else {
> > > > > +    set supports_hw_wp 1
> > > > > +}
> > > > > +
> > > > >   standard_testfile
> > > > > 
> > > > >   if {[prepare_for_testing "failed to prepare" $testfile
> > > > > $srcfile
> > > > > debug]} {
> > > > > @@ -285,7 +297,7 @@ proc setup_and_run_watchpoints_tests {
> > > > > hw_wp_p
> > > > > } {
> > > > > 
> > > > >   # Run tests with hardware watchpoints disabled, then again
> > > > > with
> > > > > them
> > > > >   # enabled (if this target supports hardware watchpoints).
> > > > > -if { ![target_info exists gdb,no_hardware_watchpoints]} {
> > > > > +if { $supports_hw_wp } {
> > > > 
> > > > So.. could this be simplified with a check against the existing
> > > > cached
> > > > has_hw_wp_support value?
> > > 
> > > Yes, I hadn't thought about that.  I updated the patch to just
> > > check
> > > the cached value.
> > > > >       # Run test with H/W enabled.
> > > > >       setup_and_run_watchpoints_tests 1
> > > > >   }
> > 
> > I updated the patch on Power to just check the has_hw_wp_support
> > cached
> > value.  That works great.  However, when I did my testing on x86-64 
> > it
> > doesn't work.  The issue is the the runtime check is only done on
> > PowerPC thus the variable has_hw_wp_support is not initialized on
> > other
> > platforms.  Thus you do need to set a local variable and use
> > that.  
> 
> Hmm, I don't see anything in has_hw_wp_support gdb.exp that limits
> the
> setting to only for powerpc..  why is that the case?
> 
> The current usage
> of the variable in skip_hw_watchpoint_tests does include an istarget
> reference, is that making the difference here?
> 
>          || ([istarget "powerpc*-*-linux*"] && [has_hw_wp_support])
> 


The proc skip_hw_watchpoint_access_tests has the code:


proc skip_hw_watchpoint_access_tests {} {
    if { [skip_hw_watchpoint_tests] } {
        return 1
    }
 ...

which calls proc skip_hw_watchpoint_access_tests

which has the code which you referenced above:

proc skip_hw_watchpoint_tests {} {
    # Skip tests if requested by the board
    if { [target_info exists gdb,no_hardware_watchpoints]} {
        return 1
    }

    # These targets support hardware watchpoints natively
    # Note, not all Power 9 processors support hardware watchpoints due to a HW
    # bug.  Use has_hw_wp_support to check do a runtime check for hardware
    # watchpoint support on Powerpc.
    if { [istarget "i?86-*-*"]
         || [istarget "x86_64-*-*"]
         || [istarget "ia64-*-*"]
         || [istarget "arm*-*-*"]
         || [istarget "aarch64*-*-*"]
         || ([istarget "powerpc*-*-linux*"] && [has_hw_wp_support])
         || [istarget "s390*-*-*"] } {
        return 0
    }
...

so if the target is powerpc*-*-linux* we call proc has_hw_wp_support which has the code:

gdb_caching_proc has_hw_wp_support {
    # Power 9, proc rev 2.2 does not support HW watchpoints due to HW bug.
    # Need to use a runtime test to determine if the Power processor has
    # support for HW watchpoints.
    global srcdir subdir gdb_prompt inferior_exited_re

    set compile_flags {debug nowarnings quiet}
    set me "has_hw_wp_support"

    # Compile a test program to test if HW watchpoints are supported
    set src {
        int main (void) {
            volatile int local;
            local = 1;
            if (local == 1)
                return 1;
            return 0;
        }
 ...
    gdb_exit
    remote_file build delete $obj

    verbose "$me: returning $has_hw_wp_support" 2
    return $has_hw_wp_support
}

So here is where has_hw_wp_support is returned and then gets cached.   
Since none of the other targets call these proceedures, the value is
never cached.  In my case, I get the error on X86 that the variable
has_hw_wp_support doesn't exist.

                        Carl 

Re: [PATCH ver 2] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Ulrich Weigand]

Carl Love <cel@us.ibm.com> wrote:

>However HW watchpoints are disabled on Power 9 due to a HW bug. The 
>skip_hw_watchpoint_access_tests check needs to be used to properly
>determine if HW watchpoints are supported by the processor.

This is not quite correct: skip_hw_watchpoint_access_tests checks
whether *access* watchpoints ("awatch") are supported.  This is
not really the issue here, since access watchpoints are treated
as optional by this test case already.

Rather, the problem on Power 9 is that HW watchpoints *in general*
(i.e. all of "watch", "rwatch", and "awatch") are not supported.
To test this, you should use skip_hw_watchpoint_tests, not
skip_hw_watchpoint_access_tests.


>+if {[skip_hw_watchpoint_access_tests]} {
>+    set supports_hw_wp 0
>+} else {
>+    set supports_hw_wp 1
>+}

I guess it would be preferable to use follow break-idempotent.exp
here and simply use:

set skip_hw_watchpoint_tests_p [skip_hw_watchpoint_tests]

Otherwise, this looks good to me.

Bye,
Ulrich

Re: [PATCH ver 2] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Carl Love]

On Fri, 2022-08-12 at 12:23 +0000, Ulrich Weigand wrote:
> Carl Love <cel@us.ibm.com> wrote:
> 
> > However HW watchpoints are disabled on Power 9 due to a HW bug.
> > The 
> > skip_hw_watchpoint_access_tests check needs to be used to properly
> > determine if HW watchpoints are supported by the processor.
> 
> This is not quite correct: skip_hw_watchpoint_access_tests checks
> whether *access* watchpoints ("awatch") are supported.  This is
> not really the issue here, since access watchpoints are treated
> as optional by this test case already.
> 
> Rather, the problem on Power 9 is that HW watchpoints *in general*
> (i.e. all of "watch", "rwatch", and "awatch") are not supported.
> To test this, you should use skip_hw_watchpoint_tests, not
> skip_hw_watchpoint_access_tests.
> 
> 
> > +if {[skip_hw_watchpoint_access_tests]} {
> > +    set supports_hw_wp 0
> > +} else {
> > +    set supports_hw_wp 1
> > +}
> 
> I guess it would be preferable to use follow break-idempotent.exp
> here and simply use:
> 
> set skip_hw_watchpoint_tests_p [skip_hw_watchpoint_tests]
> 
> Otherwise, this looks good to me.

OK, thanks for the clarification on the difference between the to
checks.  I updated the patch to use the same syntax as in the break-
idempotent.exp test.  I retested on X86, Power 9 and Power 10.  I will
repost the patch just to make sure it is OK.

Note, there are two more tests that use the [target_info exists
gdb,no_hardware_watchpoints] and need to be updated. 

Thanks for the help.

                   Carl

Re: [PATCH ver 3] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Carl Love]

GDB maintainers:

I have updated the patch to use the skip_hw_watchpoint_tests check
instead of the skip_hw_watchpoint_access_tests.  Also, I changed the
patch to use the same coding style as used in break-idempotent.exp per
Ulrich's comments.

The gdb.base/watchpoint-reuse-slot.exp uses the check:

if { ![target_info exists gdb,no_hardware_watchpoints]}

to determine if the test should be run with HW watchpoints. The 
target_info exists check for HW watchpoints is TRUE on Power 9. However
HW watchpoints are disabled on Power 9 due to a HW bug. The
skip_hw_watchpoint_tests check needs to be used to properly determine
if HW watchpoints are supported by the processor.  If the processor is
PowerPC, the skip_hw_watchpoint_access_tests will run a small gdb test
if the processor supports HW watchpoints.   On other platforms, the
skip_hw_watchpoint_tests checks the list of processors that support HW
watchpoints.

This patch replaces the [target_info exists gdb,
no_hardware_watchpoints] with the skip_hw_watchpoint_tests
check to properly determine if the processor supports HW watchpoints.

The fix disables the HW watchpoint tests on Power 9 thus eliminating 48
testsuite failures.

The patch has been tested on Power 9, Power 10 and x86-64

Please let me know if this patch is acceptable for mainline.  Thanks.

                              Carl Love

-------------------------------------------------------------
Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

This test generates 48 failures on Power 9 when testing with HW watchpoints
enabled.  Note HW watchpoint support is disabled on Power 9 due to a HW bug.
The skip_hw_watchpoint_tests proc must be used to correctly determine
if the processor supports HW watchpoints.

This patch replaces the [target_info exists gdb,no_hardware_watchpoints]
with the skip_hw_watchpoint_tests check.

This patch was tested on Power 9, Power 10 and X86-64 with no regressions.
---
 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
index e2ea137424b..607d0f8053c 100644
--- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
@@ -22,6 +22,16 @@
 # operation.  (Note that we don't have any of these watchpoints
 # trigger.)
 
+# The skip_hw_watchpoint_tests checks if watchpoints are supported by the
+# processor.  On PowerPC, the check runs a small test program under gdb
+# to determine if the Power processor supports HW watchpoints.  The check
+# must be done before starting the test so as to not disrupt the execution
+# of the actual test.
+
+set skip_hw_watchpoint_tests_p [skip_hw_watchpoint_tests]
+
+# starting the test.
+
 standard_testfile
 
 if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
@@ -285,7 +295,7 @@ proc setup_and_run_watchpoints_tests { hw_wp_p } {
 
 # Run tests with hardware watchpoints disabled, then again with them
 # enabled (if this target supports hardware watchpoints).
-if { ![target_info exists gdb,no_hardware_watchpoints]} {
+if { ![skip_hw_breakpoint_tests] } {
     # Run test with H/W enabled.
     setup_and_run_watchpoints_tests 1
 }
-- 
2.31.1

Re: [PATCH ver 3] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Ulrich Weigand]

Carl Love <cel@us.ibm.com> wrote:

>+# The skip_hw_watchpoint_tests checks if watchpoints are supported by
the
>+# processor.  On PowerPC, the check runs a small test program under
gdb
>+# to determine if the Power processor supports HW watchpoints.  The
check
>+# must be done before starting the test so as to not disrupt the
execution
>+# of the actual test.
>+
>+set skip_hw_watchpoint_tests_p [skip_hw_watchpoint_tests]

>-if { ![target_info exists gdb,no_hardware_watchpoints]} {
>+if { ![skip_hw_breakpoint_tests] } {

I guess this will *work* as the skip_hw_breakpoint_tests
function uses a cache to avoid re-running the test.

But it would seem more logical here to use the 
skip_hw_watchpoint_tests_p variable defined above.

Bye,
Ulrich

Re: [PATCH ver 3] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Carl Love]

Ulrich:

On Thu, 2022-08-18 at 14:31 +0000, Ulrich Weigand wrote:
> > +
> > +set skip_hw_watchpoint_tests_p [skip_hw_watchpoint_tests]
> > -if { ![target_info exists gdb,no_hardware_watchpoints]} {
> > +if { ![skip_hw_breakpoint_tests] } {
> 
> I guess this will *work* as the skip_hw_breakpoint_tests
> function uses a cache to avoid re-running the test.
> 
> But it would seem more logical here to use the 
> skip_hw_watchpoint_tests_p variable defined above.

Yes, I agree I will update this patch and the patch for the other two
patches to use skip_hw_watchpoint_tests_p.  Thanks.

Note, I will post them when I get back from vacation.  

                     Carl 

[PATCH ver 4] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Carl Love]

GDB maintainers:

Version 4:  Changed the test to use the variable
skip_hw_watchpoint_tests_p per Ulrich's comment.

Version 3:  I have updated the patch to use the
skip_hw_watchpoint_tests check instead of the
skip_hw_watchpoint_access_tests.  Also, I changed the patch to use the
same coding style as used in break-idempotent.exp per Ulrich's
comments.

The gdb.base/watchpoint-reuse-slot.exp uses the check:

if { ![target_info exists gdb,no_hardware_watchpoints]}

to determine if the test should be run with HW watchpoints.
The target_info exists check for HW watchpoints is TRUE on Power 9.
However HW watchpoints are disabled on Power 9 due to a HW bug. The
skip_hw_watchpoint_tests check needs to be used to properly determine
if HW watchpoints are supported by the processor.  If the processor is
PowerPC, the skip_hw_watchpoint_access_tests will run a small gdb test
if the processor supports HW watchpoints.   On other platforms, the
skip_hw_watchpoint_tests checks the list of processors that support HW
watchpoints.

This patch replaces the [target_info exists gdb,
no_hardware_watchpoints] with the skip_hw_watchpoint_tests
check to properly determine if the processor supports HW watchpoints.

The fix disables the HW watchpoint tests on Power 9 thus eliminating 48
testsuite failures.

The patch has been tested on Power 9, Power 10 and x86-64

Please let me know if this patch is acceptable for mainline.  Thanks.

                          Carl Love
----------------------

Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

This test generates 48 failures on Power 9 when testing with HW watchpoints
enabled.  Note HW watchpoint support is disabled on Power 9 due to a HW bug.
The skip_hw_watchpoint_tests proc must be used to correctly determine
if the processor supports HW watchpoints.

This patch replaces the [target_info exists gdb,no_hardware_watchpoints]
with the skip_hw_watchpoint_tests check.

This patch was tested on Power 9, Power 10 and X86-64 with no regressions.
---
 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
index e2ea137424b..7ea6a7467c9 100644
--- a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
@@ -22,6 +22,16 @@
 # operation.  (Note that we don't have any of these watchpoints
 # trigger.)
 
+# The skip_hw_watchpoint_tests checks if watchpoints are supported by the
+# processor.  On PowerPC, the check runs a small test program under gdb
+# to determine if the Power processor supports HW watchpoints.  The check
+# must be done before starting the test so as to not disrupt the execution
+# of the actual test.
+
+set skip_hw_watchpoint_tests_p [skip_hw_watchpoint_tests]
+
+# starting the test.
+
 standard_testfile
 
 if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
@@ -285,7 +295,7 @@ proc setup_and_run_watchpoints_tests { hw_wp_p } {
 
 # Run tests with hardware watchpoints disabled, then again with them
 # enabled (if this target supports hardware watchpoints).
-if { ![target_info exists gdb,no_hardware_watchpoints]} {
+if { !$skip_hw_watchpoint_tests_p } {
     # Run test with H/W enabled.
     setup_and_run_watchpoints_tests 1
 }
-- 
2.31.1

Re: [PATCH ver 4] Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-slot.exp

[Ulrich Weigand]

Carl Love <cel@us.ibm.com> wrote:

>Fix hardware watchpoint check in test gdb.base/watchpoint-reuse-
slot.exp

This version is OK.

Thanks,
Ulrich