Re: [RFA] nameless LOAD_DLL_DEBUG_EVENT causes ntdll.dll to be missing

[Pedro Alves <palves at redhat dot com>]

On 12/10/2013 10:56 AM, Joel Brobecker wrote:
> Hi Pedro,
> 
>> [regarding gdbserver]
>>> I think the fix is very low risk, and could go in with minimal
>>> testing.
>>
>> OK - will work on that ASAP.
> 
> I just had a look, and unfortunately, by the time we get to the end of
> do_initial_child_stuff, gdbserver has not done the wait/resume cycle
> and so DLLs have not been mapped yet. This is because this part is
> currently handled by the generic code, as opposed to the "create_inferior"
> or "attach" methods.

Hmm, I had forgotten that.  I always though that gdbserver's
"create inferior" sequence of calling mywait after create_inferior
to be a little odd, leading to this issue (the FIXME):

  signal_pid = create_inferior (new_argv[0], new_argv);

  /* FIXME: we don't actually know at this point that the create
     actually succeeded.  We won't know that until we wait.  */
  fprintf (stderr, "Process %s created; pid = %ld\n", argv[0],
	   signal_pid);

Changing that would mean changing more than we're willing at
the moment.  We can still work in that direction, and actually
make gdbserver's win32 initial event handling more similar to
GDB's.

> It seems to me that, if we want to fix this issue in GDBserver,
> we'll need to add a new method, something like inferior_created.
> We'd then call this new method, if defined, at the end of both
> start_inferior and attach_inferior.
> 
> Does this sound right to you?

What about this alternative below as preparatory for your
patch?  It makes gdbserver closer to GDB here.

(Actually, I've though before of making gdb/windows-nat.c's initial
event handling in do_initial_windows_stuff call windows_wait directly
instead of wait_for_inferior, to get rid of stop_after_trap.
That would also go in the direction of your earlier suggestion
of fetching the dll list after the initial events,
as then the initial LOAD_DLL_DEBUG_EVENT events wouldn't cause
the solib_add calls in infrun.c -- infrun wouldn't see them at
all.)

"gdbserver.exe :9999 gdbserver.exe" under wine, and then
connecting with an --enable-targets=all GNU/Linux GDB works,
but I'm not really able to test beyond that.

2013-12-10  Pedro Alves  <palves@redhat.com>

	* target.c (mywait): Convert TARGET_WAITKIND_LOADED to
	TARGET_WAITKIND_STOPPED.
	* win32-low.c (stopped_at_initial_breakpoint): New global.
	(do_initial_child_stuff): Consume events up to the initial
	breakpoint here.
	(win32_wait): Return the last event if starting up.
	Don't ignore TARGET_WAITKIND_LOADED here.
---

 gdb/gdbserver/target.c    |    5 ++++
 gdb/gdbserver/win32-low.c |   62 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index d4a2a98..d229933 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -82,6 +82,11 @@ mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,

   ret = (*the_target->wait) (ptid, ourstatus, options);

+  /* We don't expose _LOADED events to gdbserver core.  See the
+     `dlls_changed' global.  */
+  if (ourstatus->kind == TARGET_WAITKIND_LOADED)
+    ourstatus->kind = TARGET_WAITKIND_STOPPED;
+
   /* If GDB is connected through TCP/serial, then GDBserver will most
      probably be running on its own terminal/console, so it's nice to
      print there why is GDBserver exiting.  If however, GDB is
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 979eedd..17dd5bc 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -80,6 +80,11 @@ static enum gdb_signal last_sig = GDB_SIGNAL_0;
 /* The current debug event from WaitForDebugEvent.  */
 static DEBUG_EVENT current_event;

+/* A status that hasn't been reported to the core yet, and so
+   win32_wait should return it next, instead of fetching the next
+   debug event off the win32 API.  */
+static struct target_waitstatus cached_status;
+
 /* Non zero if an interrupt request is to be satisfied by suspending
    all threads.  */
 static int soft_interrupt_requested = 0;
@@ -97,6 +102,8 @@ typedef BOOL (WINAPI *winapi_DebugSetProcessKillOnExit) (BOOL KillOnExit);
 typedef BOOL (WINAPI *winapi_DebugBreakProcess) (HANDLE);
 typedef BOOL (WINAPI *winapi_GenerateConsoleCtrlEvent) (DWORD, DWORD);

+static ptid_t win32_wait (ptid_t ptid, struct target_waitstatus *ourstatus,
+			  int options);
 static void win32_resume (struct thread_resume *resume_info, size_t n);

 /* Get the thread ID from the current selected inferior (the current
@@ -336,6 +343,34 @@ do_initial_child_stuff (HANDLE proch, DWORD pid, int attached)

   if (the_low_target.initial_stuff != NULL)
     (*the_low_target.initial_stuff) ();
+
+  cached_status.kind = TARGET_WAITKIND_IGNORE;
+
+  /* Flush all currently pending debug events (thread and dll list) up
+     to the initial breakpoint.  */
+  while (1)
+    {
+      struct target_waitstatus status;
+
+      win32_wait (minus_one_ptid, &status, 0);
+
+      /* Note win32_wait doesn't return thread events.  */
+      if (status.kind != TARGET_WAITKIND_LOADED)
+	{
+	  cached_status = status;
+	  break;
+	}
+
+      {
+	struct thread_resume resume;
+
+	resume.thread = minus_one_ptid;
+	resume.kind = resume_continue;
+	resume.sig = 0;
+
+	win32_resume (&resume, 1);
+      }
+    }
 }

 /* Resume all artificially suspended threads if we are continuing
@@ -1593,6 +1628,18 @@ win32_wait (ptid_t ptid, struct target_waitstatus *ourstatus, int options)
 {
   struct regcache *regcache;

+  if (cached_status.kind != TARGET_WAITKIND_IGNORE)
+    {
+      /* The core always does a wait after creating the inferior, and
+	 do_initial_child_stuff already ran the inferior to the
+	 initial breakpoint (or an exit, if creating the process
+	 fails).  Report it now.  */
+      cached_status.kind = TARGET_WAITKIND_IGNORE;
+
+      *ourstatus = cached_status;
+      return debug_event_ptid (&current_event);
+    }
+
   while (1)
     {
       if (!get_child_debug_event (ourstatus))
@@ -1612,21 +1659,6 @@ win32_wait (ptid_t ptid, struct target_waitstatus *ourstatus, int options)

 	  regcache = get_thread_regcache (current_inferior, 1);
 	  child_fetch_inferior_registers (regcache, -1);
-
-	  if (ourstatus->kind == TARGET_WAITKIND_LOADED
-	      && !server_waiting)
-	    {
-	      /* When gdb connects, we want to be stopped at the
-		 initial breakpoint, not in some dll load event.  */
-	      child_continue (DBG_CONTINUE, -1);
-	      break;
-	    }
-
-	  /* We don't expose _LOADED events to gdbserver core.  See
-	     the `dlls_changed' global.  */
-	  if (ourstatus->kind == TARGET_WAITKIND_LOADED)
-	    ourstatus->kind = TARGET_WAITKIND_STOPPED;
-
 	  return debug_event_ptid (&current_event);
 	default:
 	  OUTMSG (("Ignoring unknown internal event, %d\n", ourstatus->kind));