[python] python-inferior add_inferior_object tries to access thread info before it exists with remote target (VMware remote stub)

[python] python-inferior add_inferior_object tries to access thread info before it exists with remote target (VMware remote stub)

[Andrew Sutherland]

Using the python branch tip, attempting to connect to a VMware 
(Workstation 6.5.x) replay remote stub, I get a "thread.c:581: 
internal-error: is_thread_state: Assertion `tp' failed." assertion.

I presume this is a unique problem because the stub refuses the "Hc-1" 
command with "E00" which means there are no magic_null_ptid protections 
in place.  So gdb asks what the current thread is, and gets an answer. 
In the process of creating the inferior via remote_add_inferior (and 
before it gets to call add_thread_silent), things go off the rails when 
the python add_inferior_object notification calls get_current_arch in:
   cleanup = ensure_python_env (get_current_arch (), current_language);

The communication leading up to this is:
+$qC#b4
+$QC1#c5
+$qAttached#8f
+$#00
+

The death stack trace is:
#5  0x080991d6 in internal_error (file=0x830c8a8 "thread.c", line=581, 
string=0x82cfcd2 "%s: Assertion `%s' failed.") at utils.c:1003
#6  0x08171f75 in is_thread_state (ptid=..., state=THREAD_EXITED) at 
thread.c:581
#7  0x08171fbb in is_exited (ptid=...) at thread.c:594
#8  0x0809f12f in has_stack_frames () at frame.c:1113
#9  0x0818758b in get_current_arch () at arch-utils.c:748
#10 0x08107c9b in add_inferior_object (pid=42000) at 
.././gdb/python/python-inferior.c:107
#11 0x080931af in generic_observer_notify (subject=<value optimized 
out>, args=0xbfeff6d4) at observer.c:166
#12 0x080935a8 in observer_notify_new_inferior (pid=42000) at 
observer.inc:833
#13 0x080a4f1d in add_inferior_silent (pid=42000) at inferior.c:88
#14 0x080a4f47 in add_inferior (pid=42000) at inferior.c:96
#15 0x080d33bd in remote_add_inferior (pid=42000, attached=0) at 
remote.c:1181
#16 0x080db0c0 in remote_start_remote (uiout=0x84a4470, 
opaque=0xbfeff874) at remote.c:2707
#17 0x08174b4a in catch_exception (uiout=0x84a4470, func=0x80dad00 
<remote_start_remote>, func_args=0xbfeff874, mask=6) at exceptions.c:462
#18 0x080d6574 in remote_open_1 (name=<value optimized out>, from_tty=1, 
target=0x83b2a40, extended_p=0) at remote.c:3333


My local fix is to cause has_stack_frames to treat a null thread_info 
the same as not having an inferior:

index 67e0607..8698563 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1106,7 +1106,8 @@ has_stack_frames (void)
      return 0;

    /* No current inferior, no frame.  */
-  if (ptid_equal (inferior_ptid, null_ptid))
+  if (ptid_equal (inferior_ptid, null_ptid) ||
+      !find_thread_ptid(inferior_ptid))
      return 0;

    /* Don't try to read from a dead thread.  */


Please let me know what the next step, if any, I should perform in order 
to get some form of fix in the tree.

Thanks,
Andrew

Re: [python] python-inferior add_inferior_object tries to access thread info before it exists with remote target (VMware remote stub)

[Jan Kratochvil]

On Wed, 09 Sep 2009 05:34:25 +0200, Andrew Sutherland wrote:
> Using the python branch tip, attempting to connect to a VMware  
> (Workstation 6.5.x) replay remote stub, I get a "thread.c:581:  
> internal-error: is_thread_state: Assertion `tp' failed." assertion.

This is workarounded a similiar way in archer-jankratochvil-fedora12 by
d9c6e0c7789438aeabf053556f39a838536400e4.

FSF GDB has a chicked-and-egg problem that the handlers registered on
observer_notify_new_inferior need already a valid thread list but the handlers
registered on observer_notify_new_thread need already a valid inferior.

The initial thread for inferior should be initialized before the new-inferior
notification is being sent.  Tried to patch it before (attached) but got out
of time, I should finish it soon.


Thanks,
Jan


diff --git a/gdb/corelow.c b/gdb/corelow.c
index 49de82d..092c16d 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -371,21 +371,11 @@ core_open (char *filename, int from_tty)
   push_target (&core_ops);
   discard_cleanups (old_chain);
 
-  add_inferior_silent (corelow_pid);
-
-  /* Do this before acknowledging the inferior, so if
-     post_create_inferior throws (can happen easilly if you're loading
-     a core file with the wrong exec), we aren't left with threads
-     from the previous inferior.  */
-  init_thread_list ();
-
   /* Set INFERIOR_PTID early, so an upper layer can rely on it being
      set while in the target_find_new_threads call below.  */
   inferior_ptid = pid_to_ptid (corelow_pid);
 
-  /* Assume ST --- Add a main task.  We'll later detect when we go
-     from ST to MT.  */
-  add_thread_silent (inferior_ptid);
+  add_inferior_silent (inferior_ptid);
 
   /* Need to flush the register cache (and the frame cache) from a
      previous debug session.  If inferior_ptid ends up the same as the
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 9eadbc9..aa814c5 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -395,11 +395,11 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
   if (!have_inferiors ())
     init_thread_list ();
 
-  add_inferior (pid);
-
   /* Needed for wait_for_inferior stuff below.  */
   inferior_ptid = pid_to_ptid (pid);
 
+  add_inferior (inferior_ptid);
+
   new_tty_postfork ();
 
   /* We have something that executes now.  We'll be running through
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index aecd820..8872a18 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -202,6 +202,8 @@ extern struct thread_info *add_thread (ptid_t ptid);
    about new thread.  */
 extern struct thread_info *add_thread_silent (ptid_t ptid);
 
+extern struct thread_info *add_thread_silent_no_notify (ptid_t ptid);
+
 /* Same as add_thread, and sets the private info.  */
 extern struct thread_info *add_thread_with_info (ptid_t ptid,
 						 struct private_thread_info *);
diff --git a/gdb/go32-nat.c b/gdb/go32-nat.c
index 836a819..e981ed3 100644
--- a/gdb/go32-nat.c
+++ b/gdb/go32-nat.c
@@ -713,12 +713,10 @@ go32_create_inferior (struct target_ops *ops, char *exec_file,
   save_npx ();
 #endif
 
-  inferior_ptid = pid_to_ptid (SOME_PID);
-  add_inferior_silent (SOME_PID);
-
   push_target (&go32_ops);
 
-  add_thread_silent (inferior_ptid);
+  inferior_ptid = pid_to_ptid (SOME_PID);
+  add_inferior_silent (SOME_PID);
 
   clear_proceed_status ();
   insert_breakpoints ();
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 89a37a6..275ab55 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -225,7 +225,7 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
 
   inferior_ptid = pid_to_ptid (pid);
 
-  inf = add_inferior (pid);
+  inf = add_inferior (inferior_ptid);
   inf->attach_flag = 1;
 
   /* Always add a main thread.  If some target extends the ptrace
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 43eacda..4286c71 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -71,13 +71,14 @@ init_inferior_list (void)
 }
 
 struct inferior *
-add_inferior_silent (int pid)
+add_inferior_silent (ptid_t ptid)
 {
   struct inferior *inf;
+  struct thread_info *tp;
 
   inf = xmalloc (sizeof (*inf));
   memset (inf, 0, sizeof (*inf));
-  inf->pid = pid;
+  inf->pid = ptid_get_pid (ptid);
 
   inf->stop_soon = NO_STOP_QUIETLY;
 
@@ -85,18 +86,30 @@ add_inferior_silent (int pid)
   inf->next = inferior_list;
   inferior_list = inf;
 
-  observer_notify_new_inferior (pid);
+  /* Do this before acknowledging the inferior, so if
+     post_create_inferior throws (can happen easilly if you're loading
+     a core file with the wrong exec), we aren't left with threads
+     from the previous inferior.  */
+  init_thread_list ();
+
+  /* Assume ST --- Add a main task.  We'll later detect when we go
+     from ST to MT.  */
+  tp = add_thread_silent_no_notify (ptid);
+
+  observer_notify_new_inferior (ptid_get_pid (ptid));
+
+  observer_notify_new_thread (tp);
 
   return inf;
 }
 
 struct inferior *
-add_inferior (int pid)
+add_inferior (ptid_t ptid)
 {
-  struct inferior *inf = add_inferior_silent (pid);
+  struct inferior *inf = add_inferior_silent (ptid);
 
   if (print_inferior_events)
-    printf_unfiltered (_("[New inferior %d]\n"), pid);
+    printf_unfiltered (_("[New inferior %d]\n"), ptid_get_pid (ptid));
 
   return inf;
 }
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 7312e51..057129a 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -434,11 +434,11 @@ extern void init_inferior_list (void);
    inferior is found, and return the pointer to the new inferior.
    Caller may use this pointer to initialize the private inferior
    data.  */
-extern struct inferior *add_inferior (int pid);
+extern struct inferior *add_inferior (ptid_t ptid);
 
 /* Same as add_inferior, but don't print new inferior notifications to
    the CLI.  */
-extern struct inferior *add_inferior_silent (int pid);
+extern struct inferior *add_inferior_silent (ptid_t ptid);
 
 /* Delete an existing inferior list entry, due to inferior exit.  */
 extern void delete_inferior (int pid);
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 1308844..24d2307 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -636,7 +636,7 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child)
 	  struct cleanup *old_chain;
 
 	  /* Add process to GDB's tables.  */
-	  child_inf = add_inferior (child_pid);
+	  child_inf = add_inferior (pid_to_ptid (child_pid));
 
 	  parent_inf = current_inferior ();
 	  child_inf->attach_flag = parent_inf->attach_flag;
@@ -728,7 +728,7 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child)
       /* Add the new inferior first, so that the target_detach below
 	 doesn't unpush the target.  */
 
-      child_inf = add_inferior (child_pid);
+      child_inf = add_inferior (pid_to_ptid (child_pid));
 
       parent_inf = current_inferior ();
       child_inf->attach_flag = parent_inf->attach_flag;
diff --git a/gdb/monitor.c b/gdb/monitor.c
index 1e61afd..c845f55 100644
--- a/gdb/monitor.c
+++ b/gdb/monitor.c
@@ -822,8 +822,7 @@ monitor_open (char *args, struct monitor_ops *mon_ops, int from_tty)
 
   /* Make run command think we are busy...  */
   inferior_ptid = monitor_ptid;
-  add_inferior_silent (ptid_get_pid (inferior_ptid));
-  add_thread_silent (inferior_ptid);
+  add_inferior_silent (inferior_ptid);
 
   /* Give monitor_wait something to read */
 
diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index 7ca16c7..2587c77 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -472,7 +472,6 @@ gdbsim_create_inferior (struct target_ops *target, char *exec_file, char *args,
 
   inferior_ptid = remote_sim_ptid;
   add_inferior_silent (ptid_get_pid (inferior_ptid));
-  add_thread_silent (inferior_ptid);
 
   insert_breakpoints ();	/* Needed to get correct instruction in cache */
 
diff --git a/gdb/remote.c b/gdb/remote.c
index e30e699..1587d9e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1168,7 +1168,7 @@ remote_query_attached (int pid)
    not.  */
 
 static struct inferior *
-remote_add_inferior (int pid, int attached)
+remote_add_inferior (ptid_t ptid, int attached)
 {
   struct remote_state *rs = get_remote_state ();
   struct inferior *inf;
@@ -1177,9 +1177,9 @@ remote_add_inferior (int pid, int attached)
      considered attached, or if is to be considered to have been
      spawned by the stub.  */
   if (attached == -1)
-    attached = remote_query_attached (pid);
+    attached = remote_query_attached (ptid_get_pid (ptid));
 
-  inf = add_inferior (pid);
+  inf = add_inferior (ptid);
 
   inf->attach_flag = attached;
 
@@ -1259,7 +1259,7 @@ remote_notice_new_inferior (ptid_t currthread, int running)
 	 may not know about it yet.  Add it before adding its child
 	 thread, so notifications are emitted in a sensible order.  */
       if (!in_inferior_list (ptid_get_pid (currthread)))
-	inf = remote_add_inferior (ptid_get_pid (currthread), -1);
+	inf = remote_add_inferior (currthread, -1);
 
       /* This is really a new thread.  Add it.  */
       remote_add_thread (currthread, running);
@@ -2712,7 +2712,7 @@ remote_start_remote (struct ui_out *uiout, void *opaque)
       /* Now, if we have thread information, update inferior_ptid.  */
       inferior_ptid = remote_current_thread (inferior_ptid);
 
-      remote_add_inferior (ptid_get_pid (inferior_ptid), -1);
+      remote_add_inferior (inferior_ptid, -1);
 
       /* Always add the main thread.  */
       add_thread_silent (inferior_ptid);
@@ -3485,10 +3485,10 @@ extended_remote_attach_1 (struct target_ops *target, char *args, int from_tty)
     error (_("Attaching to %s failed"),
 	   target_pid_to_str (pid_to_ptid (pid)));
 
-  remote_add_inferior (pid, 1);
-
   inferior_ptid = pid_to_ptid (pid);
 
+  remote_add_inferior (inferior_ptid, 1);
+
   if (non_stop)
     {
       struct thread_info *thread;
@@ -6798,8 +6798,7 @@ extended_remote_create_inferior_1 (char *exec_file, char *args,
   /* Now, if we have thread information, update inferior_ptid.  */
   inferior_ptid = remote_current_thread (inferior_ptid);
 
-  remote_add_inferior (ptid_get_pid (inferior_ptid), 0);
-  add_thread_silent (inferior_ptid);
+  remote_add_inferior (inferior_ptid, 0);
 
   /* Get updated offsets, if the stub uses qOffsets.  */
   get_offsets ();
diff --git a/gdb/thread.c b/gdb/thread.c
index 815c82d..7c15098 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -163,7 +163,7 @@ new_thread (ptid_t ptid)
 }
 
 struct thread_info *
-add_thread_silent (ptid_t ptid)
+add_thread_silent_no_notify (ptid_t ptid)
 {
   struct thread_info *tp;
 
@@ -197,8 +197,6 @@ add_thread_silent (ptid_t ptid)
 	  tp->state_ = THREAD_STOPPED;
 	  switch_to_thread (ptid);
 
-	  observer_notify_new_thread (tp);
-
 	  /* All done.  */
 	  return tp;
 	}
@@ -207,7 +205,14 @@ add_thread_silent (ptid_t ptid)
 	delete_thread (ptid);
     }
 
-  tp = new_thread (ptid);
+  return new_thread (ptid);
+}
+
+struct thread_info *
+add_thread_silent (ptid_t ptid)
+{
+  struct thread_info *tp = add_thread_silent_no_notify (ptid);
+
   observer_notify_new_thread (tp);
 
   return tp;
@@ -499,6 +504,9 @@ thread_alive (struct thread_info *tp)
 void
 prune_threads (void)
 {
+/* Regressing on: gdb.server/server-run.exp
+   thread.c:581: internal-error: is_thread_state: Assertion `tp' failed.  */
+#if 0
   struct thread_info *tp;
   struct thread_info **prevp = &thread_list;
 
@@ -514,6 +522,7 @@ prune_threads (void)
       else
 	prevp = &tp->next;
     }
+#endif
 }
 
 void

[python] [patch] Fix crash on remote targets [Re: [python] python-inferior add_inferior_object tries to access thread info before it exists with remote target (VMware remote stub)]

[Jan Kratochvil]

Hi,

tried first to get working get_current_arch at the add_inferior_object time
but found it is probably right it does not work at that time.

gdb.server/server-run.exp currently crashes with:
thread.c:581: internal-error: is_thread_state: Assertion `tp' failed.

No regressions on {x86_64,x86_64-m32,i686}-fedora11-linux-gnu.


Thanks,
Jan


2009-09-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* python/py-inferior.c (add_inferior_object): Call ensure_python_env
	just with target_gdbarch, new comment.

--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -104,7 +104,10 @@ add_inferior_object (int pid)
       return;
     }
 
-  cleanup = ensure_python_env (get_current_arch (), current_language);
+  /* While creating new inferior no inferior thread is available.  Therefore
+     get_current_arch has no valid current frame (and it would crash).  */
+
+  cleanup = ensure_python_env (target_gdbarch, current_language);
 
   inf_obj = PyObject_New (inferior_object, &inferior_object_type);
   if (!inf_obj)

ping: Re: [python] [patch] Fix crash on remote targets [Re: [python] python-inferior add_inferior_object tries to access thread info before it exists with remote target (VMware remote stub)]

[Jan Kratochvil]

Hi Tom,

ping, this bug costs daily many watthours of testbox. :-)


Thanks,
Jan

------------------------------------------------------------------------------

Hi,

tried first to get working get_current_arch at the add_inferior_object time
but found it is probably right it does not work at that time.

gdb.server/server-run.exp currently crashes with:
thread.c:581: internal-error: is_thread_state: Assertion `tp' failed.

No regressions on {x86_64,x86_64-m32,i686}-fedora11-linux-gnu.


Thanks,
Jan


2009-09-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* python/py-inferior.c (add_inferior_object): Call ensure_python_env
	just with target_gdbarch, new comment.

--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -104,7 +104,10 @@ add_inferior_object (int pid)
       return;
     }
 
-  cleanup = ensure_python_env (get_current_arch (), current_language);
+  /* While creating new inferior no inferior thread is available.  Therefore
+     get_current_arch has no valid current frame (and it would crash).  */
+
+  cleanup = ensure_python_env (target_gdbarch, current_language);
 
   inf_obj = PyObject_New (inferior_object, &inferior_object_type);
   if (!inf_obj)

Re: ping: Re: [python] [patch] Fix crash on remote targets [Re: [python] python-inferior add_inferior_object tries to access thread info before it exists with remote target (VMware remote stub)]

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> ping, this bug costs daily many watthours of testbox. :-)

Thanks for the ping, I had missed or completely forgotten about this.
It is ok, please push.

Tom

Re: ping: Re: [python] [patch] Fix crash on remote targets [Re: [python] python-inferior add_inferior_object tries to access thread info before it exists with remote target (VMware remote stub)]

[Jan Kratochvil]

On Tue, 13 Oct 2009 22:40:08 +0200, Tom Tromey wrote:
> It is ok, please push.

Done:
	b6fd5e85924d71f28bb7e9d463e97bfd26a25f80


Thanks,
Jan