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

[Joel Brobecker <brobecker@adacore.com>]

[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]

> 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):
[...]
> 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.
[...]
> What about this alternative below as preparatory for your
> patch?  It makes gdbserver closer to GDB here.
> 2013-12-10  Pedro Alves  <palves@redhat.com>
[updated patch]
> 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.

I tested the patch on x86-windows, with no regression.

On top of that patch, I was able to implement the same post-init
trick of looking for ntdll.dll, but not without a couple of surprises:
we don't have FILENAME_CMP nor a "basename" function in gdbserver
at the moment. I worked around the first issue by using strcasecmp,
which is good enough for our purposes. But for the second issue,
I only had a handful of bad options for our current situation:
  1. Import the module from gnulib; but that's never an innocent
     change, and also the documentation says that it does not work
     for Windows paths;
  2. Import the libiberty module by hand, which itself depends on
     their safe-ctype.h module.
  3. Write a quick ad hoc function that implements basename.

(1) is a non-starter, and I didn't like either of (2) or (3).
In the end, I went for (2) as the quickest option towards testing
the change and sending an RFC patch. Should we go with this approach,
we'll probably want to add the libiberty dependencies through
configure.srv instead of inside OBS. That addition should be
temporary, as the minute we stop looking specifically for ntdll,
and load all mapped dlls through that loop, we'll stop needing
lbasename, and will be able to remove the dependency.

But the good news is that the patch does fix the problem and adds
the missing ntdll.dll. I've tested the resulting gdbserver through
our testsuite as best as I could, and the results are more than
decent, so I think the change should be relatively good.

I'll also add comments and documentation, if we decide to move forward.

gdb/gdbserver/ChangeLog:

        * Makefile.in (OBS): Add safe-ctype.o and lbasename.o.
        (safe-ctype.o, lbasename.o): New rules.
        * win32-low.c (win32_ensure_ntdll_loaded): New function.
        (do_initial_child_stuff): Add call to win32_ensure_ntdll_loaded.

WDYT? It almost makes you want to take the risk of moving forward
with the post-branch proposal now rather than waiting for the branch
;-).

Thanks,
-- 
Joel

[-- Attachment #2: 0001-gdbserver-nameless-LOAD_DLL_DEBUG_EVENT-causes-ntdll.patch --]
[-- Type: text/x-diff, Size: 4847 bytes --]

From 11887cf93aa8f046610136e2ce3afd3100efc9a5 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 12 Dec 2013 12:53:45 -0500
Subject: [PATCH] [gdbserver] nameless LOAD_DLL_DEBUG_EVENT causes ntdll.dll
 to be missing

This is the gdbserver-equivalent of the change made in GDB to handle
the case, in x64 windows version 2012, where the kernel produces
a LOAD_DLL_DEBUG_EVENT where the name of the associated DLL cannot
be determined at that time, and thus has to be processed later.

The visible symptom is that ntdll.dll is missing from the list of
shared libraries known to be mapped by the inferior, with other
side-effects such as failure to unwind through code provided by
that DLL (such as exception handling routines).

gdb/gdbserver/ChangeLog:

	* Makefile.in (OBS): Add safe-ctype.o and lbasename.o.
	(safe-ctype.o, lbasename.o): New rules.
	* win32-low.c (win32_ensure_ntdll_loaded): New function.
	(do_initial_child_stuff): Add call to win32_ensure_ntdll_loaded.
---
 gdb/gdbserver/Makefile.in |    8 ++++-
 gdb/gdbserver/win32-low.c |   68 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 1 deletions(-)

diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 641ea17..67d3a09 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -176,7 +176,7 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
       target.o waitstatus.o utils.o version.o vec.o gdb_vecs.o \
       mem-break.o hostio.o event-loop.o tracepoint.o xml-utils.o \
       common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
-      tdesc.o $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
+      tdesc.o safe-ctype.o lbasename.o $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
 GDBREPLAY_OBS = gdbreplay.o version.o
 GDBSERVER_LIBS = @GDBSERVER_LIBS@
 XM_CLIBS = @LIBS@
@@ -543,6 +543,12 @@ vasprintf.o: $(srcdir)/../../libiberty/vasprintf.c
 vsnprintf.o: $(srcdir)/../../libiberty/vsnprintf.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
+safe-ctype.o: $(srcdir)/../../libiberty/safe-ctype.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
+lbasename.o: $(srcdir)/../../libiberty/lbasename.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
 
 aarch64.c : $(srcdir)/../regformats/aarch64.dat $(regdat_sh)
 	$(SHELL) $(regdat_sh) $(srcdir)/../regformats/aarch64.dat aarch64.c
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index a4c9e77..a5f9b9d 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -105,6 +105,7 @@ 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);
+static void win32_ensure_ntdll_loaded (void);
 
 /* Get the thread ID from the current selected inferior (the current
    thread).  */
@@ -371,6 +372,8 @@ do_initial_child_stuff (HANDLE proch, DWORD pid, int attached)
 	win32_resume (&resume, 1);
       }
     }
+
+  win32_ensure_ntdll_loaded ();
 }
 
 /* Resume all artificially suspended threads if we are continuing
@@ -1134,6 +1137,71 @@ failed:
   return 0;
 }
 
+static void
+win32_ensure_ntdll_loaded (void)
+{
+  struct inferior_list_entry *dll_e;
+  size_t i;
+  HMODULE dh_buf[1];
+  HMODULE *DllHandle = dh_buf;
+  DWORD cbNeeded;
+  BOOL ok;
+
+  for (dll_e = all_dlls.head; dll_e != NULL; dll_e = dll_e->next)
+    {
+      struct dll_info *dll = (struct dll_info *) dll_e;
+
+      if (strcasecmp (lbasename (dll->name), "ntdll.dll") == 0)
+	return;
+    }
+
+  if (!load_psapi ())
+    return;
+
+  cbNeeded = 0;
+  ok = (*win32_EnumProcessModules) (current_process_handle,
+				    DllHandle,
+				    sizeof (HMODULE),
+				    &cbNeeded);
+
+  if (!ok || !cbNeeded)
+    return;
+
+  DllHandle = (HMODULE *) alloca (cbNeeded);
+  if (!DllHandle)
+    return;
+
+  ok = (*win32_EnumProcessModules) (current_process_handle,
+				    DllHandle,
+				    cbNeeded,
+				    &cbNeeded);
+  if (!ok)
+    return;
+
+  for (i = 0; i < ((size_t) cbNeeded / sizeof (HMODULE)); i++)
+    {
+      MODULEINFO mi;
+      char dll_name[MAX_PATH];
+
+      if (!(*win32_GetModuleInformation) (current_process_handle,
+					  DllHandle[i],
+					  &mi,
+					  sizeof (mi)))
+	continue;
+      if ((*win32_GetModuleFileNameExA) (current_process_handle,
+					 DllHandle[i],
+					 dll_name,
+					 MAX_PATH) == 0)
+	continue;
+      if (strcasecmp (lbasename (dll_name), "ntdll.dll") == 0)
+	{
+	  win32_add_one_solib (dll_name,
+			       (CORE_ADDR) (uintptr_t) mi.lpBaseOfDll);
+	  return;
+	}
+    }
+}
+
 typedef HANDLE (WINAPI *winapi_CreateToolhelp32Snapshot) (DWORD, DWORD);
 typedef BOOL (WINAPI *winapi_Module32First) (HANDLE, LPMODULEENTRY32);
 typedef BOOL (WINAPI *winapi_Module32Next) (HANDLE, LPMODULEENTRY32);
-- 
1.7.9