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

[Andrew Burgess <aburgess@redhat.com>]

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.

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