RE: [PATCH] Fix for gdb.base/break-idempotent.exp

[Carl Love <cel@us.ibm.com>]

Andrew:

Thanks for looking at the patch.  I am fine with you using your patch
and merging in anything from mine that is useful.  Thanks for sharing
you insight on the issues.  Thanks for the help in getting this fixed.

Take care.

                  Carl 

On Mon, 2022-06-27 at 14:40 +0100, Andrew Burgess wrote:
> Hi Carl,
> 
> First, an apology, I posted this patch:
> 
>   
> https://sourceware.org/pipermail/gdb-patches/2022-June/190257.html
>  
> 
> without first checking the mailing list, so I missed that you had
> already posted this.
> 
> You'll see that our fixes are pretty similar.  Given you posted this
> first, I'm happy for you to merge your patch, however, I do have some
> feedback...
> 
> Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > GDB maintainers:
> > 
> > The gdb.base/break-idempotent.exp test generates errors trying to
> > set
> > and remove breakpoints.  The issue is gdb is trying to set and
> > remove
> > breakpoints in skip_hw_watchpoint_tests not in break-idempotent.
> 
> I'm not sure I agree with this.  What I saw was problems when
> delete_breakpoints was called from break-idempotent.exp AFTER the
> skip_hw_watchpoint_tests call has caused GDB to exit.
> 
> >                                                                   T
> > he
> > issue is the skip_hw_watchpoint_test restarts gdb on the test hw
> > watchpoint test binary after gdb has been started for the break-
> > idempotent.exp test messing up the test.
> 
> It's the fact that GDB is exited at the end of has_hw_wp_support that
> causes the immediate problem.  Of couse, if we changed
> has_hw_wp_support
> so that GDB was left running then the restart would become the
> problem...
> 
> > This patch moves the check for hardware breakpoints before running
> > the
> > break-idempotent tests.  The move also improves the performance as
> > the
> > skip_hw_watchpoint_test only needs to be run once rather for each
> > test
> > case.
> 
> The has_hw_wp_support check is cached, so there's no (significant)
> performance improvement; has_hw_wp_support is only ever called once.
> The key to the fix is thta skip_hw_watchpoint_tests, and hence
> has_hw_wp_support is now called before GDB is started as part of the
> main test.
> 
> > The patch has been tested on Power 10 and fixes the failures seen
> > on
> > PowerPC.
> > 
> > Please let me know if this patch is acceptable for mainline.
> >                 Carl Love
> > 
> > 
> > -------------------------------------------------
> > Fix for gdb.base/break-idempotent.exp
> > 
> > ---
> >  gdb/testsuite/gdb.base/break-idempotent.exp | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/testsuite/gdb.base/break-idempotent.exp
> > b/gdb/testsuite/gdb.base/break-idempotent.exp
> > index 29002f103a8..38b7632a7fc 100644
> > --- a/gdb/testsuite/gdb.base/break-idempotent.exp
> > +++ b/gdb/testsuite/gdb.base/break-idempotent.exp
> > @@ -145,6 +145,16 @@ proc test_break { always_inserted
> > break_command } {
> >      }
> >  }
> >  
> > +# The skip_hw_watchpoint_tests generates a small test source file
> > to test if HW
> > +# watchpoints are supported.  Do not want the compile and test for
> > the skip
> > +# hw watchpoint to restart gdb after the break-idempotemt test has
> > is started
> > +# on gdb.  Do the skip_hw_watchpoint_tests first.
> 
> How about this updated comment?
> 
>   # The skip_hw_watchpoint_tests starts GDB on a small test program
> to
>   # 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.
> 
> > +if {[skip_hw_watchpoint_tests]} {
> > +    set skip_hw_wp 1
> > +} else {
> > +    set skip_hw_wp 0
> > +}
> 
> In my patch I wrote something similar, but, mentioned that
> skip_hw_watchpoint_tests caches its result, so just called
> skip_hw_watchpoint_tests without changing anything else.
> 
> If you prefer your version, then that's fine, but please use
> true/false
> instead of 1/0 when setting skip_hw_wp.
> 
> Thanks,
> Andrew
> 
> > +
> >  # The testcase uses the "file" command to force breakpoint re-set
> > in
> >  # GDB.  Test both with and without PIE, as GDB used to mishandle
> >  # breakpoint re-set when reloading PIEs.
> > @@ -174,7 +184,7 @@ foreach_with_prefix pie { "nopie" "pie" } {
> >  	    test_break $always_inserted "hbreak"
> >  	}
> >  
> > -	if {![skip_hw_watchpoint_tests]} {
> > +	if {!$skip_hw_wp} {
> >  	    test_break $always_inserted "watch"
> >  	}
> >  
> > -- 
> > 2.31.1