[RFA/commit 3/3] Simplify windows-nat.c::get_module_name

[RFA/commit 3/3] Simplify windows-nat.c::get_module_name

[Joel Brobecker]

Now that get_module_name is no longer called for handling DLL events,
we can simplify it a bit, knowing that the only use is to get the
executable's filename.

While doing so, we adjusted the implementation a bit to avoid
references to DLLs, renamed it to make its more-targeted usage
more explicit, moved it right before the only function that uses it.
We also remove the use of hard-coded length for the buffers being
used.

gdb/ChangeLog:

	* windows-nat.c (get_module_name): Delete.
	(windows_get_exec_module_filename): New function, mostly
	inspired from get_module_name.
	(windows_pid_to_exec_file): Replace call to get_module_name
	by call to windows_get_exec_module_filename.
---
 gdb/windows-nat.c | 129 +++++++++++++++++++++++-------------------------------
 1 file changed, 55 insertions(+), 74 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 9b704d4..3b7182c 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -527,79 +527,6 @@ windows_store_inferior_registers (struct target_ops *ops,
     do_windows_store_inferior_registers (regcache, r);
 }
 
-/* Get the name of a given module at given base address.  If base_address
-   is zero return the first loaded module (which is always the name of the
-   executable).  */
-static int
-get_module_name (LPVOID base_address, char *dll_name_ret)
-{
-  DWORD len;
-  MODULEINFO mi;
-  int i;
-  HMODULE dh_buf[1];
-  HMODULE *DllHandle = dh_buf;	/* Set to temporary storage for
-				   initial query.  */
-  DWORD cbNeeded;
-#ifdef __CYGWIN__
-  cygwin_buf_t pathbuf[__PMAX];	/* Temporary storage prior to converting to
-				   posix form.  __PMAX is always enough
-				   as long as SO_NAME_MAX_PATH_SIZE is defined
-				   as 512.  */
-#endif
-
-  cbNeeded = 0;
-  /* Find size of buffer needed to handle list of modules loaded in
-     inferior.  */
-  if (!EnumProcessModules (current_process_handle, DllHandle,
-			   sizeof (HMODULE), &cbNeeded) || !cbNeeded)
-    goto failed;
-
-  /* Allocate correct amount of space for module list.  */
-  DllHandle = (HMODULE *) alloca (cbNeeded);
-  if (!DllHandle)
-    goto failed;
-
-  /* Get the list of modules.  */
-  if (!EnumProcessModules (current_process_handle, DllHandle, cbNeeded,
-				 &cbNeeded))
-    goto failed;
-
-  for (i = 0; i < (int) (cbNeeded / sizeof (HMODULE)); i++)
-    {
-      /* Get information on this module.  */
-      if (!GetModuleInformation (current_process_handle, DllHandle[i],
-				 &mi, sizeof (mi)))
-	error (_("Can't get module info"));
-
-      if (!base_address || mi.lpBaseOfDll == base_address)
-	{
-	  /* Try to find the name of the given module.  */
-#ifdef __CYGWIN__
-	  /* Cygwin prefers that the path be in /x/y/z format.  */
-	  len = GetModuleFileNameEx (current_process_handle,
-				      DllHandle[i], pathbuf, __PMAX);
-	  if (len == 0)
-	    error (_("Error getting dll name: %u."),
-		   (unsigned) GetLastError ());
-	  if (cygwin_conv_path (CCP_WIN_W_TO_POSIX, pathbuf, dll_name_ret,
-				__PMAX) < 0)
-	    error (_("Error converting dll name to POSIX: %d."), errno);
-#else
-	  len = GetModuleFileNameEx (current_process_handle,
-				      DllHandle[i], dll_name_ret, __PMAX);
-	  if (len == 0)
-	    error (_("Error getting dll name: %u."),
-		   (unsigned) GetLastError ());
-#endif
-	  return 1;	/* success */
-	}
-    }
-
-failed:
-  dll_name_ret[0] = '\0';
-  return 0;		/* failure */
-}
-
 /* Encapsulate the information required in a call to
    symbol_file_add_args.  */
 struct safe_symbol_file_add_args
@@ -1953,6 +1880,60 @@ windows_detach (struct target_ops *ops, const char *args, int from_tty)
   unpush_target (ops);
 }
 
+/* Try to determine the executable filename.
+
+   EXE_NAME_RET is a pointer to a buffer whose size is EXE_NAME_MAX_LEN.
+
+   Upon success, the filename is stored inside EXE_NAME_RET, and
+   this function returns nonzero.
+
+   Otherwise, this function returns zero and the contents of
+   EXE_NAME_RET is undefined.  */
+
+static int
+windows_get_exec_module_filename (char *exe_name_ret, size_t exe_name_max_len)
+{
+  DWORD len;
+  HMODULE dh_buf;
+  DWORD cbNeeded;
+
+  cbNeeded = 0;
+  if (!EnumProcessModules (current_process_handle, &dh_buf,
+			   sizeof (HMODULE), &cbNeeded) || !cbNeeded)
+    return 0;
+
+  /* We know the executable is always first in the list of modules,
+     which we just fetched.  So no need to fetch more.  */
+
+#ifdef __CYGWIN__
+  {
+    /* Cygwin prefers that the path be in /x/y/z format, so extract
+       the filename into a temporary buffer first, and then convert it
+       to POSIX format into the destination buffer.  */
+    cygwin_buf_t *pathbuf = alloca (exe_name_max_len * sizeof (cygwin_buf_t));
+
+    len = GetModuleFileNameEx (current_process_handle,
+			       dh_buf, pathbuf, exe_name_max_len);
+    if (len == 0)
+      error (_("Error getting executable filename: %u."),
+	     (unsigned) GetLastError ());
+    if (cygwin_conv_path (CCP_WIN_W_TO_POSIX, pathbuf, exe_name_ret,
+			  exe_name_max_len) < 0)
+      error (_("Error converting executable filename to POSIX: %d."), errno);
+  }
+#else
+  len = GetModuleFileNameEx (current_process_handle,
+			     dh_buf, exe_name_ret, exe_name_max_len);
+  if (len == 0)
+    error (_("Error getting executable filename: %u."),
+	   (unsigned) GetLastError ());
+#endif
+
+    return 1;	/* success */
+}
+
+/* The pid_to_exec_file target_ops method for this platform.  */
+
 static char *
 windows_pid_to_exec_file (int pid)
 {
@@ -1973,7 +1954,7 @@ windows_pid_to_exec_file (int pid)
 
   /* If we get here then either Cygwin is hosed, this isn't a Cygwin version
      of gdb, or we're trying to debug a non-Cygwin windows executable.  */
-  if (!get_module_name (0, path))
+  if (!windows_get_exec_module_filename (path, sizeof (path)))
     path[0] = '\0';
 
   return path;
-- 
1.8.3.2

[Windows] DLL load/unload events during inferior startup cleanup

[Joel Brobecker]

Hello,

This patch series implements something that we talked about last December,
which we couldn't do then because considered too risky prior to cutting
the gdb-7.7 branch.

  Subject: [RFA] nameless LOAD_DLL_DEBUG_EVENT causes ntdll.dll to be missing
  https://www.sourceware.org/ml/gdb-patches/2013-12/msg00073.html

Quick summary:

  On Windows 2012, the LOAD_DLL_DEBUG_EVENT corresponding to the first
  DLL being mapped, which is ntdll.dll, was happening too early,
  preventing us from being able to add it to our solist. This has
  various consequences, one of them being missing unwind info,
  thus breaking unwinding.

  The fix we applied at the time was to continue as normal during
  the startup phase, and then add some small code in at the of
  that phase to come back and try to locate that DLL so as to add it
  then if we failed to do it earlier.

  But we also remarked that we did not really need to do it this way
  at all. Instead, we should wait for the startup phase to be over,
  and then ask the system about all DLLs, adding them to our solist
  at that point. This involves ignoring load/unload DLL events during
  the startup phase.

This patch series:

  Now that the branch has been cut, this series implements this idea.
  It only applies to GDB at the moment, as I haven't had time to
  look into GDBserver yet. Instead, I've spent the time to test
  this patch on numerous versions of Windows, both 32 and 64 bits,
  with versions going back to XP.

  The meat of the series is in the first patch, with the following two
  being simplifications allowed by the first one.

  I will tackle GDBserver next, but in the meantime, I think this
  patch series should already be useful on its own. If anything,
  I think it's a nice simplification overall, and hey, you even
  get ntdll.dll to show up at the beginning of "info shared" on
  Windows 2012 :-).

[RFA/commit 1/3] Ignore DLL load/unload events during inferior initialization.

[Joel Brobecker]

This patch aims at simplifying DLL handling during the inferior
initialization (process creation during the "run", or during an
"attach"). Instead of processing each DLL load event, which is
sometimes incomplete, we ignore these events until the inferior
has completed its startup phase, and then just iterate over all
DLLs via EnumProcessModules.

gdb/ChangeLog:

        * windows-nat.c (get_windows_debug_event): Ignore
	LOAD_DLL_DEBUG_EVENT and UNLOAD_DLL_DEBUG_EVENT
	if windows_initialization_done == 0.
        (windows_add_all_dlls): Renames windows_ensure_ntdll_loaded.
        Adjust implementation to always load all DLLs.
        (do_initial_windows_stuff): Replace call to
        windows_ensure_ntdll_loaded by call to windows_add_all_dlls.
---
 gdb/windows-nat.c | 45 ++++++++++++---------------------------------
 1 file changed, 12 insertions(+), 33 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 3ce564a..a71e391 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1576,7 +1576,7 @@ get_windows_debug_event (struct target_ops *ops,
 		     (unsigned) current_event.dwThreadId,
 		     "LOAD_DLL_DEBUG_EVENT"));
       CloseHandle (current_event.u.LoadDll.hFile);
-      if (saw_create != 1)
+      if (saw_create != 1 || ! windows_initialization_done)
 	break;
       catch_errors (handle_load_dll, NULL, (char *) "", RETURN_MASK_ALL);
       ourstatus->kind = TARGET_WAITKIND_LOADED;
@@ -1589,7 +1589,7 @@ get_windows_debug_event (struct target_ops *ops,
 		     (unsigned) current_event.dwProcessId,
 		     (unsigned) current_event.dwThreadId,
 		     "UNLOAD_DLL_DEBUG_EVENT"));
-      if (saw_create != 1)
+      if (saw_create != 1 || ! windows_initialization_done)
 	break;
       catch_errors (handle_unload_dll, NULL, (char *) "", RETURN_MASK_ALL);
       ourstatus->kind = TARGET_WAITKIND_LOADED;
@@ -1722,21 +1722,11 @@ windows_wait (struct target_ops *ops,
     }
 }
 
-/* On certain versions of Windows, the information about ntdll.dll
-   is not available yet at the time we get the LOAD_DLL_DEBUG_EVENT,
-   thus preventing us from reporting this DLL as an SO. This has been
-   witnessed on Windows 8.1, for instance.  A possible explanation
-   is that ntdll.dll might be mapped before the SO info gets created
-   by the Windows system -- ntdll.dll is the first DLL to be reported
-   via LOAD_DLL_DEBUG_EVENT and other DLLs do not seem to suffer from
-   that problem.
-
-   If we indeed are missing ntdll.dll, this function tries to recover
-   from this issue, after the fact.  Do nothing if we encounter any
-   issue trying to locate that DLL.  */
+/* Iterate over all DLLs currently mapped by our inferior, and
+   add them to our list of solibs.  */
 
 static void
-windows_ensure_ntdll_loaded (void)
+windows_add_all_dlls (void)
 {
   struct so_list *so;
   HMODULE dummy_hmodule;
@@ -1744,10 +1734,6 @@ windows_ensure_ntdll_loaded (void)
   HMODULE *hmodules;
   int i;
 
-  for (so = solib_start.next; so != NULL; so = so->next)
-    if (FILENAME_CMP (lbasename (so->so_name), "ntdll.dll") == 0)
-      return;  /* ntdll.dll already loaded, nothing to do.  */
-
   if (EnumProcessModules (current_process_handle, &dummy_hmodule,
 			  sizeof (HMODULE), &cb_needed) == 0)
     return;
@@ -1760,7 +1746,7 @@ windows_ensure_ntdll_loaded (void)
 			  cb_needed, &cb_needed) == 0)
     return;
 
-  for (i = 0; i < (int) (cb_needed / sizeof (HMODULE)); i++)
+  for (i = 1; i < (int) (cb_needed / sizeof (HMODULE)); i++)
     {
       MODULEINFO mi;
 #ifdef __USEWIDE
@@ -1781,12 +1767,9 @@ windows_ensure_ntdll_loaded (void)
 #else
       name = dll_name;
 #endif
-      if (FILENAME_CMP (lbasename (name), "ntdll.dll") == 0)
-	{
-	  solib_end->next = windows_make_so (name, mi.lpBaseOfDll);
-	  solib_end = solib_end->next;
-	  return;
-	}
+
+      solib_end->next = windows_make_so (name, mi.lpBaseOfDll);
+      solib_end = solib_end->next;
     }
 }
 
@@ -1843,13 +1826,9 @@ do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching)
 	break;
     }
 
-  /* FIXME: brobecker/2013-12-10: We should try another approach where
-     we first ignore all DLL load/unload events up until this point,
-     and then iterate over all modules to create the associated shared
-     objects.  This is a fairly significant change, however, and we are
-     close to creating a release branch, so we are delaying it a bit,
-     after the branch is created.  */
-  windows_ensure_ntdll_loaded ();
+  /* Now that the inferior has been started and all DLLs have been mapped,
+     we can iterate over all DLLs and load them in.  */
+  windows_add_all_dlls ();
 
   windows_initialization_done = 1;
   inf->control.stop_soon = NO_STOP_QUIETLY;
-- 
1.8.3.2

[RFA/commit 2/3] Windows: Rely purely on event info when handling DLL load event

[Joel Brobecker]

When a DLL gets loaded an the debugger gets a debug event about it,
the currently implementation in handle_load_dll currently tries to
fetch the DLL's name by first iterating over all DLLs known to
the system.  It should be sufficient to rely on the name provided
with the event, however, especially in the situation we are now,
where we now know that we're past the statup phase for our inferior.

This patch therefore simplifies windows-nat.c::handle_load_dll to
only rely on the event's lpImageName.

It also updates the function's comment to document the assumption
regarding not being during the inferior's startup phase. And while
at it, it fixes the function documentation, which was probably
unintentionally inherited from another function (perhaps windows_wait).

gdb/ChangeLog:

	* windows-nat.c (handle_load_dll): Rewrite this function's
	introductory comment.  Remove code using get_module_name
	to get the DLL's name.
---
 gdb/windows-nat.c | 39 ++++++++++-----------------------------
 1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index a71e391..9b704d4 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -835,45 +835,26 @@ get_image_name (HANDLE h, void *address, int unicode)
   return buf;
 }
 
-/* Wait for child to do something.  Return pid of child, or -1 in case
-   of error; store status through argument pointer OURSTATUS.  */
+/* Handle a DLL load event, and return 1.
+
+   This function assumes that this event did not occur during inferior
+   initialization, where their event info may be incomplete (see
+   do_initial_windows_stuff and windows_add_all_dlls for more info
+   on how we handle DLL loading during that phase).  */
+
 static int
 handle_load_dll (void *dummy)
 {
   LOAD_DLL_DEBUG_INFO *event = &current_event.u.LoadDll;
-  char dll_buf[__PMAX];
-  char *dll_name = NULL;
-
-  dll_buf[0] = dll_buf[sizeof (dll_buf) - 1] = '\0';
-
-  /* Try getting the DLL name by searching the list of known modules
-     and matching their base address against this new DLL's base address.
-
-     FIXME: brobecker/2013-12-10:
-     It seems odd to be going through this search if the DLL name could
-     simply be extracted via "event->lpImageName".  Moreover, some
-     experimentation with various versions of Windows seem to indicate
-     that it might still be too early for this DLL to be listed when
-     querying the system about the current list of modules, thus making
-     this attempt pointless.
-
-     This code can therefore probably be removed.  But at the time of
-     this writing, we are too close to creating the GDB 7.7 branch
-     for us to make such a change.  We are therefore defering it.  */
-
-  if (!get_module_name (event->lpBaseOfDll, dll_buf))
-    dll_buf[0] = dll_buf[sizeof (dll_buf) - 1] = '\0';
-
-  dll_name = dll_buf;
+  char *dll_name;
 
   /* Try getting the DLL name via the lpImageName field of the event.
      Note that Microsoft documents this fields as strictly optional,
      in the sense that it might be NULL.  And the first DLL event in
      particular is explicitly documented as "likely not pass[ed]"
      (source: MSDN LOAD_DLL_DEBUG_INFO structure).  */
-  if (*dll_name == '\0')
-    dll_name = get_image_name (current_process_handle,
-			       event->lpImageName, event->fUnicode);
+  dll_name = get_image_name (current_process_handle,
+			     event->lpImageName, event->fUnicode);
   if (!dll_name)
     return 1;
 
-- 
1.8.3.2

pushed: [Windows] DLL load/unload events during inferior startup cleanup

[Joel Brobecker]

Hello,

FYI: I have now pushed this patch series. And I have scheduled some
     time next week to hopefully do the same for GDBserver.

-- 
Joel

Re: [RFA/commit 1/3] Ignore DLL load/unload events during inferior initialization.

[Pedro Alves]

Hi Joel, sorry for not responding to this sooner.

The series looked fine to me, FWIW.  Thanks!

On 02/10/2014 10:50 AM, Joel Brobecker wrote:
> -/* On certain versions of Windows, the information about ntdll.dll
> -   is not available yet at the time we get the LOAD_DLL_DEBUG_EVENT,
> -   thus preventing us from reporting this DLL as an SO. This has been
> -   witnessed on Windows 8.1, for instance.  A possible explanation
> -   is that ntdll.dll might be mapped before the SO info gets created
> -   by the Windows system -- ntdll.dll is the first DLL to be reported
> -   via LOAD_DLL_DEBUG_EVENT and other DLLs do not seem to suffer from
> -   that problem.

I think it's a little unfortunate that we lose this comment (in
some form), because it explains precisely _why_ we defer reading
the dll list to afterwards.  I think someone reading the code
not being familiar with the history will naturally wonder
about this.

-- 
Pedro Alves

Re: [RFA/commit 1/3] Ignore DLL load/unload events during inferior initialization.

[Joel Brobecker]

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

> The series looked fine to me, FWIW.  Thanks!

Thanks for reviewing them too!

> On 02/10/2014 10:50 AM, Joel Brobecker wrote:
> > -/* On certain versions of Windows, the information about ntdll.dll
> > -   is not available yet at the time we get the LOAD_DLL_DEBUG_EVENT,
> > -   thus preventing us from reporting this DLL as an SO. This has been
> > -   witnessed on Windows 8.1, for instance.  A possible explanation
> > -   is that ntdll.dll might be mapped before the SO info gets created
> > -   by the Windows system -- ntdll.dll is the first DLL to be reported
> > -   via LOAD_DLL_DEBUG_EVENT and other DLLs do not seem to suffer from
> > -   that problem.
> 
> I think it's a little unfortunate that we lose this comment (in
> some form), because it explains precisely _why_ we defer reading
> the dll list to afterwards.  I think someone reading the code
> not being familiar with the history will naturally wonder
> about this.

Agreed. In my mind, I had preserved that information, but in looking
at it again, it is, hum, a fairly significantly stripped-down version
of the comment.

Attached is the patch I just checked in.

gdb/ChangeLog:

        * windows-nat.c (handle_unload_dll): Add function documentation.
        (do_initial_windows_stuff): Add comment explaining why we wait
        until after inferior initialization has finished before
        processing all DLLs.

The old comment is resurected at the location where we do the
batch processing of all DLLs, and there are references to it
from both handle_load_dll and handle_unload_dll. Anyone wondering
why we ignore those events when looking at get_windows_debug_event...

    case LOAD_DLL_DEBUG_EVENT:
      [...]
      if (saw_create != 1 || ! windows_initialization_done)
        break;
      catch_errors (handle_load_dll, NULL, (char *) "", RETURN_MASK_ALL);

... can probably search for windows_initialization_done, and
will immediately find do_initial_windows_stuff, where the comment
was added.

Let me know if there are other ways you think I could improve
the documentation further!

-- 
Joel

[-- Attachment #2: 0001-windows-nat.c-Bring-comment-back-regarding-handling-.patch --]
[-- Type: text/x-diff, Size: 3056 bytes --]

From 3be75f87b9a0e5b06175dadedb268c609609c821 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 20 Feb 2014 17:18:47 +0100
Subject: [PATCH] windows-nat.c: Bring comment back regarding handling of DLL
 load events.

This patch brings back a comment that got stripped down a bit too much
during a recent change.

gdb/ChangeLog:

        * windows-nat.c (handle_unload_dll): Add function documentation.
        (do_initial_windows_stuff): Add comment explaining why we wait
        until after inferior initialization has finished before
        processing all DLLs.
---
 gdb/ChangeLog     |  7 +++++++
 gdb/windows-nat.c | 23 ++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2745653..bbb1039 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2014-02-20  Joel Brobecker  <brobecker@adacore.com>
 
+	* windows-nat.c (handle_unload_dll): Add function documentation.
+	(do_initial_windows_stuff): Add comment explaining why we wait
+	until after inferior initialization has finished before
+	processing all DLLs.
+
+2014-02-20  Joel Brobecker  <brobecker@adacore.com>
+
 	* windows-nat.c (get_module_name): Delete.
 	(windows_get_exec_module_filename): New function, mostly
 	inspired from get_module_name.
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index a570a1a..4366aab 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -802,6 +802,14 @@ windows_free_so (struct so_list *so)
   xfree (so);
 }
 
+/* Handle a DLL unload event.
+   Return 1 if successful, or zero otherwise.
+
+   This function assumes that this event did not occur during inferior
+   initialization, where their event info may be incomplete (see
+   do_initial_windows_stuff and windows_add_all_dlls for more info
+   on how we handle DLL loading during that phase).  */
+
 static int
 handle_unload_dll (void *dummy)
 {
@@ -1735,7 +1743,20 @@ do_initial_windows_stuff (struct target_ops *ops, DWORD pid, int attaching)
     }
 
   /* Now that the inferior has been started and all DLLs have been mapped,
-     we can iterate over all DLLs and load them in.  */
+     we can iterate over all DLLs and load them in.
+
+     We avoid doing it any earlier because, on certain versions of Windows,
+     LOAD_DLL_DEBUG_EVENTs are sometimes not complete.  In particular,
+     we have seen on Windows 8.1 that the ntdll.dll load event does not
+     include the DLL name, preventing us from creating an associated SO.
+     A possible explanation is that ntdll.dll might be mapped before
+     the SO info gets created by the Windows system -- ntdll.dll is
+     the first DLL to be reported via LOAD_DLL_DEBUG_EVENT and other DLLs
+     do not seem to suffer from that problem.
+
+     Rather than try to work around this sort of issue, it is much
+     simpler to just ignore DLL load/unload events during the startup
+     phase, and then process them all in one batch now.  */
   windows_add_all_dlls ();
 
   windows_initialization_done = 1;
-- 
1.8.3.2

Re: [RFA/commit 1/3] Ignore DLL load/unload events during inferior initialization.

[Pedro Alves]

On 02/20/2014 04:28 PM, Joel Brobecker wrote:

> Attached is the patch I just checked in.

Thank you.  That's perfect.

-- 
Pedro Alves