* [PATCH] Fix for gdb.base/break-idempotent.exp @ 2022-06-14 22:38 Carl Love 2022-06-22 15:11 ` Carl Love 2022-06-27 13:40 ` Andrew Burgess 0 siblings, 2 replies; 6+ messages in thread From: Carl Love @ 2022-06-14 22:38 UTC (permalink / raw) To: gdb-patches, will schmidt, cel, Ulrich Weigand 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re:[Ping] [PATCH] Fix for gdb.base/break-idempotent.exp 2022-06-14 22:38 [PATCH] Fix for gdb.base/break-idempotent.exp Carl Love @ 2022-06-22 15:11 ` Carl Love 2022-06-27 13:40 ` Andrew Burgess 1 sibling, 0 replies; 6+ messages in thread From: Carl Love @ 2022-06-22 15:11 UTC (permalink / raw) To: gdb-patches, will schmidt, Ulrich Weigand, cel 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" > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix for gdb.base/break-idempotent.exp 2022-06-14 22:38 [PATCH] Fix for gdb.base/break-idempotent.exp Carl Love 2022-06-22 15:11 ` Carl Love @ 2022-06-27 13:40 ` Andrew Burgess 2022-06-27 15:14 ` Carl Love 1 sibling, 1 reply; 6+ messages in thread From: Andrew Burgess @ 2022-06-27 13:40 UTC (permalink / raw) To: Carl Love via Gdb-patches, gdb-patches, will schmidt, cel, Ulrich Weigand 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] Fix for gdb.base/break-idempotent.exp 2022-06-27 13:40 ` Andrew Burgess @ 2022-06-27 15:14 ` Carl Love 2022-06-29 9:58 ` Andrew Burgess 0 siblings, 1 reply; 6+ messages in thread From: Carl Love @ 2022-06-27 15:14 UTC (permalink / raw) To: Andrew Burgess, Carl Love via Gdb-patches, will schmidt, Ulrich Weigand 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] Fix for gdb.base/break-idempotent.exp 2022-06-27 15:14 ` Carl Love @ 2022-06-29 9:58 ` Andrew Burgess 2022-06-29 15:34 ` Carl Love 0 siblings, 1 reply; 6+ messages in thread From: Andrew Burgess @ 2022-06-29 9:58 UTC (permalink / raw) To: Carl Love, Carl Love via Gdb-patches, will schmidt, Ulrich Weigand 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" } ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] Fix for gdb.base/break-idempotent.exp 2022-06-29 9:58 ` Andrew Burgess @ 2022-06-29 15:34 ` Carl Love 0 siblings, 0 replies; 6+ messages in thread From: Carl Love @ 2022-06-29 15:34 UTC (permalink / raw) To: Andrew Burgess, Carl Love via Gdb-patches, will schmidt, Ulrich Weigand 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" > } > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-29 15:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-14 22:38 [PATCH] Fix for gdb.base/break-idempotent.exp Carl Love 2022-06-22 15:11 ` Carl Love 2022-06-27 13:40 ` Andrew Burgess 2022-06-27 15:14 ` Carl Love 2022-06-29 9:58 ` Andrew Burgess 2022-06-29 15:34 ` Carl Love
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).