[PATCH 0/1] MR10016: dbghelp: Rewrite EnumerateLoadedModulesW64 in terms of CreateToolhelp32Snapshot.

[Yuxuan Shui (＠yshui)]

Follow up to !9371.

Did some quick, unscientific performance tests, EnumerateLoadedModulesW64 is massively faster after this change, so I think everything is working.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10016

[PATCH 1/1] dbghelp: Rewrite EnumerateLoadedModulesW64 in terms of CreateToolhelp32Snapshot.

[Yuxuan Shui]

From: Yuxuan Shui <[e-mail]>

---
 dlls/dbghelp/module.c | 125 ++++++++----------------------------------
 1 file changed, 24 insertions(+), 101 deletions(-)

diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c
index 6428c539420..d0e27a44d28 100644
--- a/dlls/dbghelp/module.c
+++ b/dlls/dbghelp/module.c
@@ -29,6 +29,7 @@
 #include "dbghelp_private.h"
 #include "image_private.h"
 #include "psapi.h"
+#include "tlhelp32.h"
 #include "winternl.h"
 #include "wine/debug.h"
 
@@ -1254,23 +1255,6 @@ BOOL  WINAPI EnumerateLoadedModules(HANDLE hProcess,
 }
 #endif
 
-static unsigned int load_and_grow_modules(HANDLE process, HMODULE** hmods, unsigned start, unsigned* alloc, DWORD filter)
-{
-    DWORD needed;
-    BOOL ret;
-
-    while ((ret = EnumProcessModulesEx(process, *hmods + start, (*alloc - start) * sizeof(HMODULE),
-                                       &needed, filter)) &&
-           needed > (*alloc - start) * sizeof(HMODULE))
-    {
-        HMODULE* new = HeapReAlloc(GetProcessHeap(), 0, *hmods, (*alloc) * 2 * sizeof(HMODULE));
-        if (!new) return 0;
-        *hmods = new;
-        *alloc *= 2;
-    }
-    return ret ? needed / sizeof(HMODULE) : 0;
-}
-
 /******************************************************************
  *		EnumerateLoadedModulesW64 (DBGHELP.@)
  *
@@ -1280,15 +1264,16 @@ BOOL  WINAPI EnumerateLoadedModulesW64(HANDLE process,
                                        PVOID user)
 {
     OBJECT_BASIC_INFORMATION obi;
-    HMODULE*            hmods;
-    unsigned            alloc = 256, count, count32, i;
-    USHORT              pcs_machine, native_machine;
-    BOOL                with_32bit_modules;
-    WCHAR               imagenameW[MAX_PATH];
-    MODULEINFO          mi;
-    WCHAR*              sysdir = NULL;
-    WCHAR*              wowdir = NULL;
-    size_t              sysdir_len = 0, wowdir_len = 0;
+    DWORD               snapshot_flags;
+    DWORD               pid = GetProcessId( process );
+    HANDLE              snapshot;
+    MODULEENTRY32W      me;
+
+    if (!pid)
+    {
+        SetLastError(STATUS_INVALID_CID);
+        return FALSE;
+    }
 
     if (process != GetCurrentProcess() &&
         RtlIsCurrentProcess( process ) &&
@@ -1296,90 +1281,28 @@ BOOL  WINAPI EnumerateLoadedModulesW64(HANDLE process,
         obi.GrantedAccess & PROCESS_VM_READ)
     {
         TRACE("same process.\n");
-        process = GetCurrentProcess();
+        pid = 0;
     }
 
-    /* process might not be a handle to a live process */
-    if (!IsWow64Process2(process, &pcs_machine, &native_machine))
-    {
-        SetLastError(STATUS_INVALID_CID);
-        return FALSE;
-    }
-    with_32bit_modules = sizeof(void*) > sizeof(int) &&
-        pcs_machine != IMAGE_FILE_MACHINE_UNKNOWN &&
-        (dbghelp_options & SYMOPT_INCLUDE_32BIT_MODULES);
+    snapshot_flags = TH32CS_SNAPMODULE;
+    if (dbghelp_options & SYMOPT_INCLUDE_32BIT_MODULES)
+        snapshot_flags |= TH32CS_SNAPMODULE32;
 
-    if (!(hmods = HeapAlloc(GetProcessHeap(), 0, alloc * sizeof(hmods[0]))))
+    snapshot = CreateToolhelp32Snapshot( snapshot_flags, pid );
+    if (snapshot == INVALID_HANDLE_VALUE)
         return FALSE;
 
-    /* Note:
-     * - we report modules returned from kernelbase.EnumProcessModulesEx
-     * - appending 32bit modules when possible and requested
-     *
-     * When considering 32bit modules in a wow64 child process, required from
-     * a 64bit process:
-     * - native returns from kernelbase.EnumProcessModulesEx
-     *   redirected paths (that is in system32 directory), while
-     *   dbghelp.EnumerateLoadedModulesWine returns the effective path
-     *   (eg. syswow64 for x86_64).
-     * - (Except for the main module, if gotten from syswow64, where kernelbase
-     *    will return the effective path)
-     * - Wine kernelbase (and ntdll) incorrectly return these modules from
-     *   syswow64 (except for ntdll which is returned from system32).
-     * => for these modules, always perform a system32 => syswow64 path
-     *    conversion (it'll work even if ntdll/kernelbase is fixed).
-     */
-    if ((count = load_and_grow_modules(process, &hmods, 0, &alloc, LIST_MODULES_DEFAULT)) && with_32bit_modules)
+    me.dwSize = sizeof(me);
+    if (Module32FirstW( snapshot, &me ))
     {
-        /* append 32bit modules when required */
-        if ((count32 = load_and_grow_modules(process, &hmods, count, &alloc, LIST_MODULES_32BIT)))
+        do
         {
-            sysdir_len = GetSystemDirectoryW(NULL, 0);
-            wowdir_len = GetSystemWow64Directory2W(NULL, 0, pcs_machine);
-
-            if (!sysdir_len || !wowdir_len ||
-                !(sysdir = HeapAlloc(GetProcessHeap(), 0, (sysdir_len + 1 + wowdir_len + 1) * sizeof(WCHAR))))
-            {
-                HeapFree(GetProcessHeap(), 0, hmods);
-                return FALSE;
-            }
-            wowdir = sysdir + sysdir_len + 1;
-            if (GetSystemDirectoryW(sysdir, sysdir_len) >= sysdir_len)
-                FIXME("shouldn't happen\n");
-            if (GetSystemWow64Directory2W(wowdir, wowdir_len, pcs_machine) >= wowdir_len)
-                FIXME("shouldn't happen\n");
-            wcscat(sysdir, L"\\");
-            wcscat(wowdir, L"\\");
-        }
-    }
-    else count32 = 0;
-
-    for (i = 0; i < count + count32; i++)
-    {
-        if (GetModuleInformation(process, hmods[i], &mi, sizeof(mi)) &&
-            GetModuleFileNameExW(process, hmods[i], imagenameW, ARRAY_SIZE(imagenameW)))
-        {
-            /* rewrite path in system32 into syswow64 for 32bit modules */
-            if (i >= count)
-            {
-                size_t len = wcslen(imagenameW);
-
-                if (!wcsnicmp(imagenameW, sysdir, sysdir_len) &&
-                    (len - sysdir_len + wowdir_len) + 1 <= ARRAY_SIZE(imagenameW))
-                {
-                    memmove(&imagenameW[wowdir_len], &imagenameW[sysdir_len], (len - sysdir_len) * sizeof(WCHAR));
-                    memcpy(imagenameW, wowdir, wowdir_len * sizeof(WCHAR));
-                }
-            }
-            if (!enum_cb(imagenameW, (DWORD_PTR)mi.lpBaseOfDll, mi.SizeOfImage, user))
-                break;
-        }
+            if (!enum_cb( me.szExePath, (DWORD_PTR)me.modBaseAddr, me.modBaseSize, user )) break;
+        } while (Module32NextW( snapshot, &me ));
     }
 
-    HeapFree(GetProcessHeap(), 0, hmods);
-    HeapFree(GetProcessHeap(), 0, sysdir);
-
-    return count != 0;
+    CloseHandle( snapshot );
+    return TRUE;
 }
 
 static void dbghelp_str_WtoA(const WCHAR *src, char *dst, int dst_len)
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/10016

[eric pouech (＠epo)]

I get this error on old wow configuration when testing dbghelp:dbghelp

dbghelp.c:1475: Test failed: \[32/32 enum:default wine_old_wow64\]: Unexpected event.count_wowdir 5

but that's generated from the debug LOAD_DLL event:s, not your patch; so we can leave it aside for now

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10016#note_128636

[PATCH 0/1] MR10016: dbghelp: Rewrite EnumerateLoadedModulesW64 in terms of CreateToolhelp32Snapshot. - approved

[eric pouech (＠epo)]

This merge request was approved by eric pouech.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10016