* [RFA] When getting the locno of a bpstat, handle the case of bp with null locations. @ 2022-11-20 17:30 Philippe Waroquiers 2022-11-20 19:34 ` Simon Marchi 2022-11-21 15:04 ` Simon Marchi 0 siblings, 2 replies; 5+ messages in thread From: Philippe Waroquiers @ 2022-11-20 17:30 UTC (permalink / raw) To: gdb-patches; +Cc: Philippe Waroquiers The test py-objfile.exp unloads the current file while debugging the process. This results in bpstat bs->b->loc to become nullptr. Handle this case in breakpoint.c:bpstat_locno. Note: GDB crashes on this problem with an internal error, but the end of gdb summary shows: ... === gdb Summary === # of expected passes 36 The output also does not contain a 'FAIL:'. After the dix, the nr of expeted passes increased. In the gdb.log output, one can see: ... Fatal signal: Segmentation fault ----- Backtrace ----- 0x55698905c5b9 gdb_internal_backtrace_1 ../../binutils-gdb/gdb/bt-utils.c:122 0x55698905c5b9 _Z22gdb_internal_backtracev ... ERROR: Couldn't send python print(objfile.filename) to GDB. ERROR: : spawn id exp9 not open while executing "expect { -i exp9 -timeout 10 -re ".*A problem internal to GDB has been detected" { fail "$message (GDB internal error)" gdb_internal_error..." ("uplevel" body line 1) invoked from within .... Wondering if it might be possible to improve gdb_test to have gdb_test "python print(objfile.filename)" "None" \ "objfile.filename after objfile is unloaded" reporting a failed result instead of just producing the internal error. --- gdb/breakpoint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 5b691673a0e..a161b78a8aa 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -4486,7 +4486,7 @@ bpstat_locno (const bpstat *bs) int locno = 0; - if (b != nullptr && b->loc->next != nullptr) + if (b != nullptr && b->loc != nullptr && b->loc->next != nullptr) { const bp_location *bl_i; -- 2.30.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFA] When getting the locno of a bpstat, handle the case of bp with null locations. 2022-11-20 17:30 [RFA] When getting the locno of a bpstat, handle the case of bp with null locations Philippe Waroquiers @ 2022-11-20 19:34 ` Simon Marchi 2022-11-20 23:46 ` Philippe Waroquiers 2022-11-21 15:04 ` Simon Marchi 1 sibling, 1 reply; 5+ messages in thread From: Simon Marchi @ 2022-11-20 19:34 UTC (permalink / raw) To: Philippe Waroquiers, gdb-patches On 11/20/22 12:30, Philippe Waroquiers via Gdb-patches wrote: > The test py-objfile.exp unloads the current file while debugging the process. > This results in bpstat bs->b->loc to become nullptr. > Handle this case in breakpoint.c:bpstat_locno. > > Note: GDB crashes on this problem with an internal error, > but the end of gdb summary shows: > ... > === gdb Summary === > > # of expected passes 36 > > The output also does not contain a 'FAIL:'. > After the dix, the nr of expeted passes increased. > > In the gdb.log output, one can see: > ... > Fatal signal: Segmentation fault > ----- Backtrace ----- > 0x55698905c5b9 gdb_internal_backtrace_1 > ../../binutils-gdb/gdb/bt-utils.c:122 > 0x55698905c5b9 _Z22gdb_internal_backtracev > ... > > ERROR: Couldn't send python print(objfile.filename) to GDB. > ERROR: : spawn id exp9 not open > while executing > "expect { > -i exp9 -timeout 10 > -re ".*A problem internal to GDB has been detected" { > fail "$message (GDB internal error)" > gdb_internal_error..." > ("uplevel" body line 1) > invoked from within > .... > > Wondering if it might be possible to improve gdb_test to have > gdb_test "python print(objfile.filename)" "None" \ > "objfile.filename after objfile is unloaded" > reporting a failed result instead of just producing the internal error. I ran the testsuite with the patch applied, I saw these unexpected failures when running with native-gdbserver or native-extended-gdbserver: FAIL: gdb.base/step-over-syscall.exp: detach-on-fork=off: follow-fork=parent: break cond on target : fork: continue to marker FAIL: gdb.base/step-over-syscall.exp: detach-on-fork=off: follow-fork=child: break cond on target : fork: continue to marker I bisected, and it pointed to the locno patch again. Probably the expected patterns that need to be updated? continue^M Continuing.^M [New inferior 2 (process 1262224)]^M ^M Thread 1.1 "step-over-fork" hit Breakpoint 4.1, marker () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/step-over-fork.c:22^M 22 marker () {}^M (gdb) FAIL: gdb.base/step-over-syscall.exp: detach-on-fork=off: follow-fork=parent: break cond on target : fork: continue to marker However, I confirm it gets rid of the UNRESOLVEDs due to the ASan complaints. But I don't have time to look at the code right now, sorry. Simon ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFA] When getting the locno of a bpstat, handle the case of bp with null locations. 2022-11-20 19:34 ` Simon Marchi @ 2022-11-20 23:46 ` Philippe Waroquiers 0 siblings, 0 replies; 5+ messages in thread From: Philippe Waroquiers @ 2022-11-20 23:46 UTC (permalink / raw) To: Simon Marchi, gdb-patches On Sun, 2022-11-20 at 14:34 -0500, Simon Marchi wrote: > I ran the testsuite with the patch applied, I saw these unexpected > failures when running with native-gdbserver or > native-extended-gdbserver: > > FAIL: gdb.base/step-over-syscall.exp: detach-on-fork=off: follow-fork=parent: break cond on target : fork: continue to marker > FAIL: gdb.base/step-over-syscall.exp: detach-on-fork=off: follow-fork=child: break cond on target : fork: continue to marker > > I bisected, and it pointed to the locno patch again. Probably the > expected patterns that need to be updated? Effectively, this test contains some parts running only with gdbserver, and I missed the needed update of the pattern. I have posted an RFA with a fix: [RFA] Fix step-over-syscall.exp matching regexp for $bpnum.$locno matching https://sourceware.org/pipermail/gdb-patches/2022-November/194020.html > > continue^M > Continuing.^M > [New inferior 2 (process 1262224)]^M > ^M > Thread 1.1 "step-over-fork" hit Breakpoint 4.1, marker () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/step-over-fork.c:22^M > 22 marker () {}^M > (gdb) FAIL: gdb.base/step-over-syscall.exp: detach-on-fork=off: follow-fork=parent: break cond on target : fork: continue to marker > > However, I confirm it gets rid of the UNRESOLVEDs due to the ASan > complaints. But I don't have time to look at the code right now, sorry. Thanks for testing and pointing at these regressions (and sorry about these). There are now 3 RFA fixing some regressions [RFA] Fix use after free introduced by $_hit_bpnum/$_hit_locno variables. [RFA] When getting the locno of a bpstat, handle the case of bp with null locations. [RFA] Fix step-over-syscall.exp matching regexp for $bpnum.$locno matching and a fix for an unrelated problem but discovered when validating the above: [RFA] Fix jump on uninit producer_is_clang of cu.h, rm declared/undefined find_partial_die The week-end was too busy with the above to search for the dwarf2 leaks also found while validating the fixes. I might have time next week-end. Thanks Philippe ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFA] When getting the locno of a bpstat, handle the case of bp with null locations. 2022-11-20 17:30 [RFA] When getting the locno of a bpstat, handle the case of bp with null locations Philippe Waroquiers 2022-11-20 19:34 ` Simon Marchi @ 2022-11-21 15:04 ` Simon Marchi 2022-11-21 21:07 ` Philippe Waroquiers 1 sibling, 1 reply; 5+ messages in thread From: Simon Marchi @ 2022-11-21 15:04 UTC (permalink / raw) To: Philippe Waroquiers, gdb-patches On 11/20/22 12:30, Philippe Waroquiers via Gdb-patches wrote: > The test py-objfile.exp unloads the current file while debugging the process. > This results in bpstat bs->b->loc to become nullptr. > Handle this case in breakpoint.c:bpstat_locno. > > Note: GDB crashes on this problem with an internal error, > but the end of gdb summary shows: > ... > === gdb Summary === > > # of expected passes 36 > > The output also does not contain a 'FAIL:'. > After the dix, the nr of expeted passes increased. dix->fix expeted -> expected > > In the gdb.log output, one can see: > ... > Fatal signal: Segmentation fault > ----- Backtrace ----- > 0x55698905c5b9 gdb_internal_backtrace_1 > ../../binutils-gdb/gdb/bt-utils.c:122 > 0x55698905c5b9 _Z22gdb_internal_backtracev > ... > > ERROR: Couldn't send python print(objfile.filename) to GDB. > ERROR: : spawn id exp9 not open > while executing > "expect { > -i exp9 -timeout 10 > -re ".*A problem internal to GDB has been detected" { > fail "$message (GDB internal error)" > gdb_internal_error..." > ("uplevel" body line 1) > invoked from within > .... > > Wondering if it might be possible to improve gdb_test to have > gdb_test "python print(objfile.filename)" "None" \ > "objfile.filename after objfile is unloaded" > reporting a failed result instead of just producing the internal error. I think an UNRESOLVED would be appropriate here. Normally, it should do that automatically. The perror here: https://gitlab.com/gnutools/binutils-gdb/-/blob/84f9fbe90e5429adb9dee68f04f44c92fa9e2345/gdb/testsuite/lib/gdb.exp#L1183 ... should make it so the next pass/fail becomes an UNRESOLVED. However, we don't even reach a pass / fail, as the expect call throws the error you pasted above, about the spawn id not being open. This mechanism works if GDB hasn't crashed yet when entering gdb_test_multiple, and it's the command gdb_test_multiple sends that crashes GDB. But here, what crashed GDB is the previous gdb_unload call, which uses bare expect, leaving no trace of the crash (in terms of test result). I propose the following changes to handle this situation better: - make gdb_unload use gdb_test_multiple, to make it record a test result and handle the different failure modes - instead of calling perror, manually call unresolved as soone as send_gdb returns an error, and return -1 immediately, to handle more gracefully the case where GDB is already crashed on entry But this is orthogonal with your patch, I will send a separate series. Your patch LGTM: Approved-By: Simon Marchi <simon.marchi@efficios.com> Simon ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFA] When getting the locno of a bpstat, handle the case of bp with null locations. 2022-11-21 15:04 ` Simon Marchi @ 2022-11-21 21:07 ` Philippe Waroquiers 0 siblings, 0 replies; 5+ messages in thread From: Philippe Waroquiers @ 2022-11-21 21:07 UTC (permalink / raw) To: Simon Marchi, gdb-patches On Mon, 2022-11-21 at 10:04 -0500, Simon Marchi wrote: > > On 11/20/22 12:30, Philippe Waroquiers via Gdb-patches wrote: > > The test py-objfile.exp unloads the current file while debugging the process. > > This results in bpstat bs->b->loc to become nullptr. > > Handle this case in breakpoint.c:bpstat_locno. > > > > Note: GDB crashes on this problem with an internal error, > > but the end of gdb summary shows: > > ... > > === gdb Summary === > > > > # of expected passes 36 > > > > The output also does not contain a 'FAIL:'. > > After the dix, the nr of expeted passes increased. > > dix->fix > expeted -> expected > > > > > In the gdb.log output, one can see: > > ... > > Fatal signal: Segmentation fault > > ----- Backtrace ----- > > 0x55698905c5b9 gdb_internal_backtrace_1 > > ../../binutils-gdb/gdb/bt-utils.c:122 > > 0x55698905c5b9 _Z22gdb_internal_backtracev > > ... > > > > ERROR: Couldn't send python print(objfile.filename) to GDB. > > ERROR: : spawn id exp9 not open > > while executing > > "expect { > > -i exp9 -timeout 10 > > -re ".*A problem internal to GDB has been detected" { > > fail "$message (GDB internal error)" > > gdb_internal_error..." > > ("uplevel" body line 1) > > invoked from within > > .... > > > > Wondering if it might be possible to improve gdb_test to have > > gdb_test "python print(objfile.filename)" "None" \ > > "objfile.filename after objfile is unloaded" > > reporting a failed result instead of just producing the internal error. > > I think an UNRESOLVED would be appropriate here. Normally, it should do > that automatically. The perror here: > > https://gitlab.com/gnutools/binutils-gdb/-/blob/84f9fbe90e5429adb9dee68f04f44c92fa9e2345/gdb/testsuite/lib/gdb.exp#L1183 > > ... should make it so the next pass/fail becomes an UNRESOLVED. > However, we don't even reach a pass / fail, as the expect call throws > the error you pasted above, about the spawn id not being open. This > mechanism works if GDB hasn't crashed yet when entering > gdb_test_multiple, and it's the command gdb_test_multiple sends that > crashes GDB. But here, what crashed GDB is the previous gdb_unload > call, which uses bare expect, leaving no trace of the crash (in terms of > test result). > > I propose the following changes to handle this situation better: > > - make gdb_unload use gdb_test_multiple, to make it record a test > result and handle the different failure modes > - instead of calling perror, manually call unresolved as soone as > send_gdb returns an error, and return -1 immediately, to handle > more gracefully the case where GDB is already crashed on entry Note that in the meantime, when comparing gdb.log results, I will now also look for the string ERROR: while up to now I was just searching for FAIL: assuming any new error due to a patch I am testing would be shown as a FAIL:. > > But this is orthogonal with your patch, I will send a separate series. > > Your patch LGTM: > > Approved-By: Simon Marchi <simon.marchi@efficios.com> > > Simon Thanks for the review, pushed. Philippe ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-21 21:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-20 17:30 [RFA] When getting the locno of a bpstat, handle the case of bp with null locations Philippe Waroquiers 2022-11-20 19:34 ` Simon Marchi 2022-11-20 23:46 ` Philippe Waroquiers 2022-11-21 15:04 ` Simon Marchi 2022-11-21 21:07 ` Philippe Waroquiers
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).