Re: [PATCH] Fix PR remote/21852: Remote run without specifying a local binary crashes GDB

[Pedro Alves <palves@redhat.com>]

On 08/22/2017 03:04 PM, Sergio Durigan Junior wrote:
> The fix for PR gdb/20609:
> 
>   commit bb805577d2b212411fb7b0a2d01644567fac4e8d
>   Author: Jan Kratochvil <jan.kratochvil@redhat.com>
>   Date:   Thu Sep 29 17:38:16 2016 +0200
> 
> Introduced the concept of deferring the call to breakpoint_re_set on
> certain useful occasions.  However, there is one specific scenario
> where delaying needs to be done and still isn't: the case when we're
> starting a GDB to debug a remote inferior without specifying a local
> binary, as in for example:
> 
>   ./gdb -nx -q --data-directory=data-directory -ex "tar ext :1234" \
>     -ex "set remote exec-file /bin/ls" -ex r
> 
> In this case, when calling exec_file_locate_attach to locate the
> inferior, GDB is incorrectly resetting the breakpoints without a
> thread/inferior even running, which causes an assertion to be
> triggered:
> 
>   binutils-gdb/gdb/thread.c:1609: internal-error: scoped_restore_current_thread::scoped_restore_current_thread(): Assertion `tp != NULL' failed.
>   A problem internal to GDB has been detected,
>   further debugging may prove unreliable.
>   Quit this debugging session? (y or n)
> 
> The right thing to do is to defer resetting the breakpoints when
> locating the binary, which is what this patch does.

Hmm, I think we're missing more rationale.  There may well be
other reasons for doing that, but this case just looks like a
case of remote.c breaking invariants to me -- making inferior_ptid
point to a non-existing thread and then calling common code is
recipe for disaster.  Seems to me that the fix is just to
not do that?  See patch below.  It fixes your test for me
as well, though I haven't run the full testsuite.

From: Pedro Alves <palves@redhat.com>
Date: 2017-08-22 18:42:27 +0100

fix remote.c

---

 gdb/remote.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index ff59a0f..ea21163 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3831,19 +3831,16 @@ add_current_inferior_and_thread (char *wait_status)
 {
   struct remote_state *rs = get_remote_state ();
   int fake_pid_p = 0;
-  ptid_t ptid;
 
   inferior_ptid = null_ptid;
 
   /* Now, if we have thread information, update inferior_ptid.  */
-  ptid = get_current_thread (wait_status);
+  ptid_t curr_ptid = get_current_thread (wait_status);
 
-  if (!ptid_equal (ptid, null_ptid))
+  if (curr_ptid != null_ptid)
     {
       if (!remote_multi_process_p (rs))
 	fake_pid_p = 1;
-
-      inferior_ptid = ptid;
     }
   else
     {
@@ -3851,14 +3848,17 @@ add_current_inferior_and_thread (char *wait_status)
 	 (such as kill) won't work.  This variable serves (at least)
 	 double duty as both the pid of the target process (if it has
 	 such), and as a flag indicating that a target is active.  */
-      inferior_ptid = magic_null_ptid;
+      curr_ptid = magic_null_ptid;
       fake_pid_p = 1;
     }
 
-  remote_add_inferior (fake_pid_p, ptid_get_pid (inferior_ptid), -1, 1);
+  remote_add_inferior (fake_pid_p, ptid_get_pid (curr_ptid), -1, 1);
 
-  /* Add the main thread.  */
-  add_thread_silent (inferior_ptid);
+  /* Add the main thread and switch to it.  Don't try reading
+     registers yes, since we haven't fetched the target description
+     yet.  */
+  thread_info *tp = add_thread_silent (curr_ptid);
+  switch_to_thread_no_regs (tp);
 }
 
 /* Print info about a thread that was found already stopped on