[PATCH] gdb/testsuite: gdb.server/server-kill.exp 'info frame' before kill_server

[PATCH] gdb/testsuite: gdb.server/server-kill.exp 'info frame' before kill_server

[Andrew Burgess]

This commit follows on from the following two commits:

  commit 80dc83fd0e70f4d522a534bc601df5e05b81d564
  Date:   Fri Jun 11 11:30:47 2021 +0100

      gdb/remote: handle target dying just before a stepi

And:

  commit 079f190d4cfc6aa9c934b00a9134bc0fcc172d53
  Date:   Thu Mar 9 10:45:03 2023 +0100

      [gdb/testsuite] Fix gdb.server/server-kill.exp for remote target

The first of these issues commits fixes an issue in GDB and tried to
extend the gdb.server/server-kill.exp test to cover the GDB fix.

Unfortunately, the changes to gdb.server/server-kill.exp were not
correct, and were causing problems when trying to run with the
remote-gdbserver-on-localhost board file.

The second commit reverts some of the gdb.server/server-kill.exp
changes introduced in the first commit so that the test will now work
correctly with the remote-gdbserver-on-localhost board file.

The second commit is just about GDB's testing infrastructure -- it's
not about the original fix to GDB from the first commit.

While reviewing the second commit I wanted to check that the problem
fixed in the first commit is still being tested by the
gdb.server/server-kill.exp script, so I reverted the change to
breakpoint.c that is the core of the first commit and ran the test
script ..... and saw no failures.

The first commit is about GDB discovering that gdbserver has dies
while trying to insert a breakpoint.  As soon as GDB spots that
gdbserver is gone we mourn the remote inferior, which ends up deleting
all the breakpoints associated with the remote inferiors.  We then
throw an exception which is caught in the insert breakpoints code, and
we try to display an error that includes the breakpoint number
.... but the breakpoint has already been deleted ... and so GDB
crashes.

After digging a little, what I found is that today, when the test does
'stepi' the first thing we end up doing is calculating the frame-id as
part of the stepi logic, it is during this frame-id calculation that
we mourn the remote inferior, delete the breakpoints, and throw an
exception.  The exception is caught by the top level interpreter loop,
and so we never try to print the breakpoint number which is what
caused the original crash.

If I add an 'info frame' command to the test script, prior to killing
gdbserver, then now when we 'stepi' GDB already has the frame-id
calculated, and the first thing we do is try to insert the
breakpoints, this will trigger the original bug.

In order to reproduce this experiment you'll need to change a function
in breakpoint.c, like this:

  static void
  rethrow_on_target_close_error (const gdb_exception &e)
  {
    return;
  }

Then run gdb.server/server-kill.exp with and without this patch.  You
should find that without this patch there are zero test failures,
while with this patch there will be one failure like this:

  (gdb) PASS: gdb.server/server-kill.exp: test_stepi: info frame
  Executing on target: kill -9 4513    (timeout = 300)
  builtin_spawn -ignore SIGHUP kill -9 4513
  stepi
  ../../src/gdb/breakpoint.c:2863: internal-error: insert_bp_location: Assertion `bl->owner != nullptr' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  ----- Backtrace -----
  ...
---
 gdb/testsuite/gdb.server/server-kill.exp | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
index 0e9df6e1a5f..4b40913fff6 100644
--- a/gdb/testsuite/gdb.server/server-kill.exp
+++ b/gdb/testsuite/gdb.server/server-kill.exp
@@ -137,6 +137,12 @@ proc_with_prefix test_stepi {} {
 	return
     }
 
+    # Ensure GDB has computed the frame-id for the current frame
+    # before we kill the gdbserver.  With the frame-id cached when we
+    # stepi below the first packets we try to send to gdbserver will
+    # be from within the breakpoint insertion process.
+    gdb_test "info frame" "Stack level 0, .*"
+
     kill_server
 
     gdb_test "stepi" "(Target disconnected|Remote connection closed|Remote communication error).*"

base-commit: deb65a3cd86462cb19d10a867ee474b3f4cf7012
-- 
2.25.4

Re: [PATCH] gdb/testsuite: gdb.server/server-kill.exp 'info frame' before kill_server

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> This commit follows on from the following two commits:
>
>   commit 80dc83fd0e70f4d522a534bc601df5e05b81d564
>   Date:   Fri Jun 11 11:30:47 2021 +0100
>
>       gdb/remote: handle target dying just before a stepi
>
> And:
>
>   commit 079f190d4cfc6aa9c934b00a9134bc0fcc172d53
>   Date:   Thu Mar 9 10:45:03 2023 +0100
>
>       [gdb/testsuite] Fix gdb.server/server-kill.exp for remote target
>
> The first of these issues commits fixes an issue in GDB and tried to
> extend the gdb.server/server-kill.exp test to cover the GDB fix.
>
> Unfortunately, the changes to gdb.server/server-kill.exp were not
> correct, and were causing problems when trying to run with the
> remote-gdbserver-on-localhost board file.
>
> The second commit reverts some of the gdb.server/server-kill.exp
> changes introduced in the first commit so that the test will now work
> correctly with the remote-gdbserver-on-localhost board file.
>
> The second commit is just about GDB's testing infrastructure -- it's
> not about the original fix to GDB from the first commit.
>
> While reviewing the second commit I wanted to check that the problem
> fixed in the first commit is still being tested by the
> gdb.server/server-kill.exp script, so I reverted the change to
> breakpoint.c that is the core of the first commit and ran the test
> script ..... and saw no failures.
>
> The first commit is about GDB discovering that gdbserver has dies
> while trying to insert a breakpoint.  As soon as GDB spots that
> gdbserver is gone we mourn the remote inferior, which ends up deleting
> all the breakpoints associated with the remote inferiors.  We then
> throw an exception which is caught in the insert breakpoints code, and
> we try to display an error that includes the breakpoint number
> .... but the breakpoint has already been deleted ... and so GDB
> crashes.
>
> After digging a little, what I found is that today, when the test does
> 'stepi' the first thing we end up doing is calculating the frame-id as
> part of the stepi logic, it is during this frame-id calculation that
> we mourn the remote inferior, delete the breakpoints, and throw an
> exception.  The exception is caught by the top level interpreter loop,
> and so we never try to print the breakpoint number which is what
> caused the original crash.
>
> If I add an 'info frame' command to the test script, prior to killing
> gdbserver, then now when we 'stepi' GDB already has the frame-id
> calculated, and the first thing we do is try to insert the
> breakpoints, this will trigger the original bug.
>
> In order to reproduce this experiment you'll need to change a function
> in breakpoint.c, like this:
>
>   static void
>   rethrow_on_target_close_error (const gdb_exception &e)
>   {
>     return;
>   }
>
> Then run gdb.server/server-kill.exp with and without this patch.  You
> should find that without this patch there are zero test failures,
> while with this patch there will be one failure like this:
>
>   (gdb) PASS: gdb.server/server-kill.exp: test_stepi: info frame
>   Executing on target: kill -9 4513    (timeout = 300)
>   builtin_spawn -ignore SIGHUP kill -9 4513
>   stepi
>   ../../src/gdb/breakpoint.c:2863: internal-error: insert_bp_location: Assertion `bl->owner != nullptr' failed.
>   A problem internal to GDB has been detected,
>   further debugging may prove unreliable.
>   ----- Backtrace -----
>   ...
> ---
>  gdb/testsuite/gdb.server/server-kill.exp | 6 ++++++
>  1 file changed, 6 insertions(+)

I fixed a few grammatical errors in the commit message and pushed this
patch.  The updated version is below.

Let me know if this causes anyone any problems.

Thanks,
Andrew

---

commit 71f18376db954e95a44a9281d05699a228070f77
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Mar 15 14:18:37 2023 +0000

    gdb/testsuite: gdb.server/server-kill.exp 'info frame' before kill_server
    
    This commit follows on from the following two commits:
    
      commit 80dc83fd0e70f4d522a534bc601df5e05b81d564
      Date:   Fri Jun 11 11:30:47 2021 +0100
    
          gdb/remote: handle target dying just before a stepi
    
    And:
    
      commit 079f190d4cfc6aa9c934b00a9134bc0fcc172d53
      Date:   Thu Mar 9 10:45:03 2023 +0100
    
          [gdb/testsuite] Fix gdb.server/server-kill.exp for remote target
    
    The first of these commits fixed an issue in GDB and tried to extend
    the gdb.server/server-kill.exp test to cover the GDB fix.
    
    Unfortunately, the changes to gdb.server/server-kill.exp were not
    correct, and were causing problems when trying to run with the
    remote-gdbserver-on-localhost board file.
    
    The second commit reverts some of the gdb.server/server-kill.exp
    changes introduced in the first commit so that the test will now work
    correctly with the remote-gdbserver-on-localhost board file.
    
    The second commit is just about GDB's testing infrastructure -- it's
    not about the original fix to GDB from the first commit, the actual
    GDB change was fine.
    
    While reviewing the second commit I wanted to check that the problem
    fixed in the first commit is still being tested by the
    gdb.server/server-kill.exp script, so I reverted the change to
    breakpoint.c that is the core of the first commit and ran the test
    script ..... and saw no failures.
    
    The first commit is about GDB discovering that gdbserver has died
    while trying to insert a breakpoint.  As soon as GDB spots that
    gdbserver is gone we mourn the remote inferior, which ends up deleting
    all the breakpoints associated with the remote inferiors.  We then
    throw an exception which is caught in the insert breakpoints code, and
    we try to display an error that includes the breakpoint number
    .... but the breakpoint has already been deleted ... and so GDB
    crashes.
    
    After digging a little, what I found is that today, when the test does
    'stepi' the first thing we end up doing is calculating the frame-id as
    part of the stepi logic, it is during this frame-id calculation that
    we mourn the remote inferior, delete the breakpoints, and throw an
    exception.  The exception is caught by the top level interpreter loop,
    and so we never try to print the breakpoint number which is what
    caused the original crash.
    
    If I add an 'info frame' command to the test script, prior to killing
    gdbserver, then now when we 'stepi' GDB already has the frame-id
    calculated, and the first thing we do is try to insert the
    breakpoints, this will trigger the original bug.
    
    In order to reproduce this experiment you'll need to change a function
    in breakpoint.c, like this:
    
      static void
      rethrow_on_target_close_error (const gdb_exception &e)
      {
        return;
      }
    
    Then run gdb.server/server-kill.exp with and without this patch.  You
    should find that without this patch there are zero test failures,
    while with this patch there will be one failure like this:
    
      (gdb) PASS: gdb.server/server-kill.exp: test_stepi: info frame
      Executing on target: kill -9 4513    (timeout = 300)
      builtin_spawn -ignore SIGHUP kill -9 4513
      stepi
      ../../src/gdb/breakpoint.c:2863: internal-error: insert_bp_location: Assertion `bl->owner != nullptr' failed.
      A problem internal to GDB has been detected,
      further debugging may prove unreliable.
      ----- Backtrace -----
      ...

diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
index 0e9df6e1a5f..4b40913fff6 100644
--- a/gdb/testsuite/gdb.server/server-kill.exp
+++ b/gdb/testsuite/gdb.server/server-kill.exp
@@ -137,6 +137,12 @@ proc_with_prefix test_stepi {} {
 	return
     }
 
+    # Ensure GDB has computed the frame-id for the current frame
+    # before we kill the gdbserver.  With the frame-id cached when we
+    # stepi below the first packets we try to send to gdbserver will
+    # be from within the breakpoint insertion process.
+    gdb_test "info frame" "Stack level 0, .*"
+
     kill_server
 
     gdb_test "stepi" "(Target disconnected|Remote connection closed|Remote communication error).*"