Re: [RFA] When getting the locno of a bpstat, handle the case of bp with null locations.

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

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