[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
> >  }