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

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

[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

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

[Simon Marchi]

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

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

[Philippe Waroquiers]

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

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

[Simon Marchi]

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

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

[Philippe Waroquiers]

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