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

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

[Carl Love]

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

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 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.
+if {[skip_hw_watchpoint_tests]} {
+    set skip_hw_wp 1
+} else {
+    set skip_hw_wp 0
+}
+
 # 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

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

[Carl Love]

Ping?

On Tue, 2022-06-14 at 15:38 -0700, Carl Love wrote:
> 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. 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.
> 
> 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 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.
> +if {[skip_hw_watchpoint_tests]} {
> +    set skip_hw_wp 1
> +} else {
> +    set skip_hw_wp 0
> +}
> +
>  # 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"
>  	}
> 

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

[Andrew Burgess]

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

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

[Carl Love]

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

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

[Andrew Burgess]

Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:

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

Carl,

I pushed the patch below.

Thanks,
Andrew

---

commit 13f72372413400410aaa94b7f0e2ff7de663fdcb
Author: Carl Love <cel@us.ibm.com>
Date:   Wed Jun 22 16:14:20 2022 +0100

    gdb/testsuite: fix gdb.base/break-idempotent.exp on ppc
    
    When running the gdb.base/break-idempotent.exp test on ppc, I was
    seeing some test failures (or rather errors), that looked like this:
    
      (gdb) watch local
      Hardware watchpoint 2: local
    
      has_hw_wp_support: Hardware watchpoint detected
      ERROR: no fileid for gcc2-power8
      ERROR: Couldn't send delete breakpoints to GDB.
      ERROR OCCURED: can't read "gdb_spawn_id": no such variable
          while executing
      "expect {
      -i 1000 -timeout 100
              -re ".*A problem internal to GDB has been detected" {
                  fail "$message (GDB internal error)"
                  gdb_internal_erro..."
          ("uplevel" body line 1)
          invoked from within
    
    What happens is that in break-idempotent.exp we basically do this:
    
        if {[prepare_for_testing "failed to prepare" $binfile $srcfile $opts]} {
            continue
        }
    
        # ....
    
        if {![skip_hw_watchpoint_tests]} {
            test_break $always_inserted "watch"
        }
    
    The problem with this is that skip_hw_watchpoint_tests, includes this:
    
        if { [istarget "i?86-*-*"]
             || [istarget "x86_64-*-*"]
             || [istarget "ia64-*-*"]
             || [istarget "arm*-*-*"]
             || [istarget "aarch64*-*-*"]
             || ([istarget "powerpc*-*-linux*"] && [has_hw_wp_support])
             || [istarget "s390*-*-*"] } {
            return 0
        }
    
    For powerpc only we call has_hw_wp_support.  This is a caching proc
    that runs a test within GDB to detect if we have hardware watchpoint
    support or not.
    
    Unfortunately, to run this test we restart GDB, and when the test has
    completed, we exit GDB.  This means that in break-idempotent.exp, when
    we call skip_hw_watchpoint_tests for the first time on powerpc, GDB
    will unexpectedly be exited.  When we later call delete_breakpoints we
    see the errors I reported above.
    
    The fix is to call skip_hw_watchpoint_tests early, before we start GDB
    as part of the break-idempotent.exp script, and store the result in a
    variable, we can then check this variable in the script as needed.
    
    After this change break-idempotent.exp runs fine on powerpc.
    
    Co-authored-by: Andrew Burgess <aburgess@redhat.com>

diff --git a/gdb/testsuite/gdb.base/break-idempotent.exp b/gdb/testsuite/gdb.base/break-idempotent.exp
index 29002f103a8..837ac000b57 100644
--- a/gdb/testsuite/gdb.base/break-idempotent.exp
+++ b/gdb/testsuite/gdb.base/break-idempotent.exp
@@ -36,6 +36,12 @@
 
 standard_testfile
 
+# 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 and cache the result.
+set skip_hw_watchpoint_tests_p [skip_hw_watchpoint_tests]
+
 # Force a breakpoint re-set in GDB.  Currently this is done by
 # reloading symbols with the "file" command.
 
@@ -174,7 +180,7 @@ foreach_with_prefix pie { "nopie" "pie" } {
 	    test_break $always_inserted "hbreak"
 	}
 
-	if {![skip_hw_watchpoint_tests]} {
+	if {!$skip_hw_watchpoint_tests_p} {
 	    test_break $always_inserted "watch"
 	}
 

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

[Carl Love]

Andrew:

Thanks for letting me know.  I did a fresh pull, build and test to
verify everything looks good on my system.

Thanks for the help getting this fixed.  

               Carl 

On Wed, 2022-06-29 at 10:58 +0100, Andrew Burgess wrote:
> Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > 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.
> 
> Carl,
> 
> I pushed the patch below.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 13f72372413400410aaa94b7f0e2ff7de663fdcb
> Author: Carl Love <cel@us.ibm.com>
> Date:   Wed Jun 22 16:14:20 2022 +0100
> 
>     gdb/testsuite: fix gdb.base/break-idempotent.exp on ppc
>     
>     When running the gdb.base/break-idempotent.exp test on ppc, I was
>     seeing some test failures (or rather errors), that looked like
> this:
>     
>       (gdb) watch local
>       Hardware watchpoint 2: local
>     
>       has_hw_wp_support: Hardware watchpoint detected
>       ERROR: no fileid for gcc2-power8
>       ERROR: Couldn't send delete breakpoints to GDB.
>       ERROR OCCURED: can't read "gdb_spawn_id": no such variable
>           while executing
>       "expect {
>       -i 1000 -timeout 100
>               -re ".*A problem internal to GDB has been detected" {
>                   fail "$message (GDB internal error)"
>                   gdb_internal_erro..."
>           ("uplevel" body line 1)
>           invoked from within
>     
>     What happens is that in break-idempotent.exp we basically do
> this:
>     
>         if {[prepare_for_testing "failed to prepare" $binfile
> $srcfile $opts]} {
>             continue
>         }
>     
>         # ....
>     
>         if {![skip_hw_watchpoint_tests]} {
>             test_break $always_inserted "watch"
>         }
>     
>     The problem with this is that skip_hw_watchpoint_tests, includes
> this:
>     
>         if { [istarget "i?86-*-*"]
>              || [istarget "x86_64-*-*"]
>              || [istarget "ia64-*-*"]
>              || [istarget "arm*-*-*"]
>              || [istarget "aarch64*-*-*"]
>              || ([istarget "powerpc*-*-linux*"] &&
> [has_hw_wp_support])
>              || [istarget "s390*-*-*"] } {
>             return 0
>         }
>     
>     For powerpc only we call has_hw_wp_support.  This is a caching
> proc
>     that runs a test within GDB to detect if we have hardware
> watchpoint
>     support or not.
>     
>     Unfortunately, to run this test we restart GDB, and when the test
> has
>     completed, we exit GDB.  This means that in break-idempotent.exp, 
> when
>     we call skip_hw_watchpoint_tests for the first time on powerpc,
> GDB
>     will unexpectedly be exited.  When we later call
> delete_breakpoints we
>     see the errors I reported above.
>     
>     The fix is to call skip_hw_watchpoint_tests early, before we
> start GDB
>     as part of the break-idempotent.exp script, and store the result
> in a
>     variable, we can then check this variable in the script as
> needed.
>     
>     After this change break-idempotent.exp runs fine on powerpc.
>     
>     Co-authored-by: Andrew Burgess <aburgess@redhat.com>
> 
> diff --git a/gdb/testsuite/gdb.base/break-idempotent.exp
> b/gdb/testsuite/gdb.base/break-idempotent.exp
> index 29002f103a8..837ac000b57 100644
> --- a/gdb/testsuite/gdb.base/break-idempotent.exp
> +++ b/gdb/testsuite/gdb.base/break-idempotent.exp
> @@ -36,6 +36,12 @@
> 
>  standard_testfile
> 
> +# 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 and cache the result.
> +set skip_hw_watchpoint_tests_p [skip_hw_watchpoint_tests]
> +
>  # Force a breakpoint re-set in GDB.  Currently this is done by
>  # reloading symbols with the "file" command.
> 
> @@ -174,7 +180,7 @@ foreach_with_prefix pie { "nopie" "pie" } {
>  	    test_break $always_inserted "hbreak"
>  	}
> 
> -	if {![skip_hw_watchpoint_tests]} {
> +	if {!$skip_hw_watchpoint_tests_p} {
>  	    test_break $always_inserted "watch"
>  	}
> 
>