[PATCH 0/5] MR5248: DbgHelp and debug info lookup (Part III)
This is the third part of a larger serie of four parts to cover module loading and debug info file lookup in dbghelp. This third part covers: - searching some sub directories of search-path elements - better support of various forms of module name - change the order of module lookup (improves performance in some games) - various corner case fixes Note: some modifications require new todo marks in the tests, as the full fix will require changing other areas that would make the change too large. The large serie of four parts resolves all the todo:s. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5248
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/dbghelp/path.c | 46 ++++++++++++++++++++++++--------------- dlls/dbghelp/tests/path.c | 4 ---- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/dlls/dbghelp/path.c b/dlls/dbghelp/path.c index 0565442bb8b..db0873f133c 100644 --- a/dlls/dbghelp/path.c +++ b/dlls/dbghelp/path.c @@ -510,7 +510,7 @@ BOOL path_find_symbol_file(const struct process* pcs, const struct module* modul SYMSRV_INDEX_INFOW *info, BOOL* is_unmatched) { struct module_find mf; - WCHAR *ptr; + WCHAR *ptr, *ext; const WCHAR* filename; WCHAR *searchPath = pcs->search_path; WCHAR buffer[MAX_PATH]; @@ -539,22 +539,9 @@ BOOL path_find_symbol_file(const struct process* pcs, const struct module* modul /* FIXME: Use Environment-Variables (see MS docs) _NT_SYMBOL_PATH and _NT_ALT_SYMBOL_PATH - FIXME: Implement "Standard Path Elements" (Path) ... (see MS docs) - do a search for (every?) path-element like this ... - <path> - <path>\dll - <path>\symbols\dll - (dll may be exe, or sys depending on the file extension) */ - - /* 2. check module-path */ - file_pathW(module->module.LoadedImageName, buffer); - if (do_searchW(filename, buffer, FALSE, module_find_cb, &mf)) return TRUE; - if (module->real_path) - { - file_pathW(module->real_path, buffer); - if (do_searchW(filename, buffer, FALSE, module_find_cb, &mf)) return TRUE; - } + */ + ext = wcsrchr(module->module.LoadedImageName, L'.'); while (searchPath) { size_t len; @@ -562,17 +549,40 @@ BOOL path_find_symbol_file(const struct process* pcs, const struct module* modul ptr = wcschr(searchPath, ';'); len = (ptr) ? ptr - searchPath : wcslen(searchPath); - if (len < ARRAY_SIZE(buffer)) + if (len + 1 < ARRAY_SIZE(buffer)) { memcpy(buffer, searchPath, len * sizeof(WCHAR)); - buffer[len] = '\0'; + buffer[len] = L'\0'; /* return first fully matched file */ if (do_searchW(filename, buffer, FALSE, module_find_cb, &mf)) return TRUE; + len = wcslen(buffer); /* do_searchW removes the trailing \ in buffer when present */ + /* check once max size for \symbols\<ext>\ */ + if (ext && len + 9 /* \symbols\ */ + wcslen(ext + 1) + 1 + 1 <= ARRAY_SIZE(buffer)) + { + buffer[len++] = L'\\'; + wcscpy(buffer + len, ext + 1); + wcscat(buffer + len, L"\\"); + if (do_searchW(filename, buffer, FALSE, module_find_cb, &mf)) return TRUE; + wcscpy(buffer + len, L"symbols\\"); + wcscat(buffer + len, ext + 1); + wcscat(buffer + len, L"\\"); + if (do_searchW(filename, buffer, FALSE, module_find_cb, &mf)) return TRUE; + } } else ERR("Too long search element %ls\n", searchPath); searchPath = ptr ? ptr + 1 : NULL; } + + /* check module-path */ + if (module->real_path) + { + file_pathW(module->real_path, buffer); + if (do_searchW(filename, buffer, FALSE, module_find_cb, &mf)) return TRUE; + } + file_pathW(module->module.LoadedImageName, buffer); + if (do_searchW(filename, buffer, FALSE, module_find_cb, &mf)) return TRUE; + /* if no fully matching file is found, return the best matching file if any */ if ((dbghelp_options & SYMOPT_LOAD_ANYTHING) && mf.matched) { diff --git a/dlls/dbghelp/tests/path.c b/dlls/dbghelp/tests/path.c index f45cfd29b58..45b71ed4718 100644 --- a/dlls/dbghelp/tests/path.c +++ b/dlls/dbghelp/tests/path.c @@ -1581,16 +1581,12 @@ static void test_load_modules_path(void) } else { - todo_wine_if(i == 4 || i == 5 || i == 7 || i == 8 || i == 11) ok(im.SymType == SymPdb, "Unexpected symtype %x\n", im.SymType); make_path(filename, topdir, NULL, test_files[test->found_file].module_path); - todo_wine_if(i == 2 || i == 4 || i == 5 || i == 7 || i == 8 || i == 11 || i == 21) ok(!wcscmp(im.LoadedPdbName, filename), "Expected %ls as loaded pdb file, got '%ls' instead\n", test_files[test->found_file].module_path, im.LoadedPdbName); - todo_wine_if(i == 11 || i == 21) ok(im.PdbAge == test_files[test->found_file].age_or_timestamp, "Expected %lx as pdb-age, got %lx instead\n", test_files[test->found_file].age_or_timestamp, im.PdbAge); - todo_wine_if(i == 11) ok(im.PdbUnmatched == !(test_files[test->found_file].age_or_timestamp == 0x0030cafe), "Expecting matched PDB\n"); } ok(IsEqualGUID(&im.PdbSig70, &guid1), "Unexpected PDB GUID\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5248
From: Eric Pouech <epouech(a)codeweavers.com> dbghelp/tests build PDB files, without hash table in TPI stream, but also no types. Native is able to load these PDB files. So, don't fail when loading a PDB without a TPI hash table and without any types. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/dbghelp/msc.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/dlls/dbghelp/msc.c b/dlls/dbghelp/msc.c index 4d310cd0f21..aeaa55cc713 100644 --- a/dlls/dbghelp/msc.c +++ b/dlls/dbghelp/msc.c @@ -3351,12 +3351,14 @@ static BOOL pdb_init_type_parse(const struct msc_debug_info* msc_dbg, ERR("-Unsupported hash of size %u\n", ctp->header.hash_value_size); return FALSE; } - ctp->hash_stream = pdb_read_stream(pdb_file, ctp->header.hash_stream); - /* FIXME always present? if not reconstruct ?*/ - if (!ctp->hash_stream) + if (!(ctp->hash_stream = pdb_read_stream(pdb_file, ctp->header.hash_stream))) { - ERR("-Missing hash table in PDB file\n"); - return FALSE; + if (ctp->header.last_index > ctp->header.first_index) + { + /* may be reconstruct hash table ? */ + FIXME("-No hash table, while types exist\n"); + return FALSE; + } } ctp->module = msc_dbg->module; @@ -3388,7 +3390,7 @@ static BOOL pdb_init_type_parse(const struct msc_debug_info* msc_dbg, /* parse the remap table * => move listed type_id at first position of their hash buckets so that we force remap to them */ - if (ctp->header.type_remap_size) + if (ctp->hash_stream && ctp->header.type_remap_size) { const unsigned* remap = (const unsigned*)((const BYTE*)ctp->hash_stream + ctp->header.type_remap_offset); unsigned i, capa, count_present; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5248
From: Eric Pouech <epouech(a)codeweavers.com> Change from: dll.so > PE image > ELF/Mach-O images into PE image > dll.so > ELF/Mach-O images Main goal is in SymLoadModule*(), to not resynchronize the system modules list when requesting loading of a PE image. This can gain quite some time in some situations. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/dbghelp/module.c | 47 ++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index f21733c7be9..2507e65e169 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -956,10 +956,12 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam } } - pcs->loader->synchronize_module_list(pcs); - /* this is a Wine extension to the API just to redo the synchronisation */ - if (!wImageName && !hFile) return 0; + if (!wImageName && !hFile) + { + pcs->loader->synchronize_module_list(pcs); + return 0; + } if (Flags & SLMFLAG_VIRTUAL) { @@ -968,28 +970,31 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam if (!module) return 0; module->module.SymType = SymVirtual; } - /* check if it's a builtin PE module with a containing ELF module */ - else if (wImageName && module_is_container_loaded(pcs, wImageName, BaseOfDll)) - { - /* force the loading of DLL as builtin */ - module = pe_load_builtin_module(pcs, wImageName, BaseOfDll, SizeOfDll); - } - if (!module) + else { - /* otherwise, try a regular PE module */ - if (!(module = pe_load_native_module(pcs, wImageName, hFile, BaseOfDll, SizeOfDll)) && - wImageName) + /* try PE image */ + module = pe_load_native_module(pcs, wImageName, hFile, BaseOfDll, SizeOfDll); + if (!module && wImageName) { - /* and finally an ELF or Mach-O module */ - module = pcs->loader->load_module(pcs, wImageName, BaseOfDll); + /* It could be either a dll.so file (for which we need the corresponding + * system module) or a system module. + * In both cases, ensure system module list is up-to-date. + */ + pcs->loader->synchronize_module_list(pcs); + if (module_is_container_loaded(pcs, wImageName, BaseOfDll)) + module = pe_load_builtin_module(pcs, wImageName, BaseOfDll, SizeOfDll); + /* at last, try ELF or Mach-O module */ + if (!module) + module = pcs->loader->load_module(pcs, wImageName, BaseOfDll); + } + if (!module) + { + WARN("Couldn't locate %s\n", debugstr_w(wImageName)); + SetLastError(ERROR_NO_MORE_FILES); + return 0; } } - if (!module) - { - WARN("Couldn't locate %s\n", debugstr_w(wImageName)); - SetLastError(ERROR_NO_MORE_FILES); - return 0; - } + /* by default module_new fills module.ModuleName from a derivation * of LoadedImageName. Overwrite it, if we have better information */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5248
From: Eric Pouech <epouech(a)codeweavers.com> Module names appear in three spots in dbghelp: A) SymGetModuleInfo() .ModuleName B) module enumeration (as parameter in callback) C) in symbol/type research in module!name form Tests show that: - A) and B) always use only the derivation of the image name, whatever the passed module name in SymLoadModule(). - C) can use either the form derived from image name {as A) and B)}, but also the passed module name in SymLoadModule(). Note: B) is limited to 64 characters, while A) is limited to 32 characters (not tested here). Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/dbghelp/dbghelp_private.h | 1 + dlls/dbghelp/module.c | 8 ++++---- dlls/dbghelp/tests/path.c | 2 -- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/dlls/dbghelp/dbghelp_private.h b/dlls/dbghelp/dbghelp_private.h index b3a073ff41f..628cb1e6442 100644 --- a/dlls/dbghelp/dbghelp_private.h +++ b/dlls/dbghelp/dbghelp_private.h @@ -438,6 +438,7 @@ struct module struct process* process; IMAGEHLP_MODULEW64 module; WCHAR modulename[64]; /* used for enumeration */ + WCHAR* alt_modulename; /* used in symbol lookup */ struct module* next; enum dhext_module_type type : 16; unsigned short is_virtual : 1, diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index 2507e65e169..a0de4f65056 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -210,6 +210,7 @@ struct module* module_new(struct process* pcs, const WCHAR* name, module->module.BaseOfImage = mod_addr; module->module.ImageSize = size; module_set_module(module, name); + module->alt_modulename = NULL; module->module.ImageName[0] = '\0'; lstrcpynW(module->module.LoadedImageName, name, ARRAY_SIZE(module->module.LoadedImageName)); module->module.SymType = SymDeferred; @@ -288,6 +289,7 @@ struct module* module_find_by_nameW(const struct process* pcs, const WCHAR* name for (module = pcs->lmodules; module; module = module->next) { if (!wcsicmp(name, module->modulename)) return module; + if (module->alt_modulename && !wcsicmp(name, module->alt_modulename)) return module; } SetLastError(ERROR_INVALID_NAME); return NULL; @@ -995,11 +997,9 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam } } - /* by default module_new fills module.ModuleName from a derivation - * of LoadedImageName. Overwrite it, if we have better information - */ + /* Store alternate name for module when provided. */ if (wModuleName) - module_set_module(module, wModuleName); + module->alt_modulename = pool_wcsdup(&module->pool, wModuleName); if (wImageName) lstrcpynW(module->module.ImageName, wImageName, ARRAY_SIZE(module->module.ImageName)); diff --git a/dlls/dbghelp/tests/path.c b/dlls/dbghelp/tests/path.c index 45b71ed4718..fc9a2e13bf9 100644 --- a/dlls/dbghelp/tests/path.c +++ b/dlls/dbghelp/tests/path.c @@ -1789,7 +1789,6 @@ static void test_load_modules_details(void) } else expected_module_name[0] = L'\0'; - todo_wine_if(i >= 2) ok(!wcsicmp(im.ModuleName, expected_module_name), "Unexpected module name '%ls'\n", im.ModuleName); ok(!wcsicmp(im.ImageName, test->in_image_name ? test->in_image_name : L""), "Unexpected image name '%ls'\n", im.ImageName); if ((test->options & SYMOPT_DEFERRED_LOADS) || !test->in_image_name) @@ -1853,7 +1852,6 @@ static void test_load_modules_details(void) ret = SymEnumerateModulesW64(dummy, aggregate_module_details_cb, &md); ok(ret, "SymEnumerateModules64 failed: %lu\n", GetLastError()); ok(md.count == 1, "Unexpected module count %u\n", md.count); - todo_wine_if(i >= 2) ok(!wcscmp(md.name, expected_module_name), "Unexpected module name %ls\n", md.name); free(md.name); /* native will fail loading symbol in deferred state, so force loading of debug symbols */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5248
From: Eric Pouech <epouech(a)codeweavers.com> Seen when debugging some game, one can load a virtual module with a NULL image name. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/dbghelp/module.c | 4 ++-- dlls/dbghelp/tests/path.c | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index a0de4f65056..5021d54c93e 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -959,7 +959,7 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam } /* this is a Wine extension to the API just to redo the synchronisation */ - if (!wImageName && !hFile) + if (!wImageName && !hFile && !Flags) { pcs->loader->synchronize_module_list(pcs); return 0; @@ -967,7 +967,7 @@ DWORD64 WINAPI SymLoadModuleExW(HANDLE hProcess, HANDLE hFile, PCWSTR wImageNam if (Flags & SLMFLAG_VIRTUAL) { - if (!wImageName) return 0; + if (!wImageName) wImageName = L""; module = module_new(pcs, wImageName, DMT_PE, FALSE, TRUE, BaseOfDll, SizeOfDll, 0, 0, IMAGE_FILE_MACHINE_UNKNOWN); if (!module) return 0; module->module.SymType = SymVirtual; diff --git a/dlls/dbghelp/tests/path.c b/dlls/dbghelp/tests/path.c index fc9a2e13bf9..9afefd2d12a 100644 --- a/dlls/dbghelp/tests/path.c +++ b/dlls/dbghelp/tests/path.c @@ -1774,12 +1774,9 @@ static void test_load_modules_details(void) base = SymLoadModuleExW(dummy, NULL, test->in_image_name, test->in_module_name, 0x4000, 0x6666, NULL, test->flags); - todo_wine_if(i == 0 || i == 1) ok(base == 0x4000, "SymLoadModuleExW failed: %lu\n", GetLastError()); ret = SymGetModuleInfoW64(dummy, base, &im); - todo_wine_if(i == 0 || i == 1) ok(ret, "SymGetModuleInfow64 failed: %lu\n", GetLastError()); - if (!ret) goto temp_bail_out; if (test->in_image_name) { WCHAR *dot; @@ -1892,7 +1889,6 @@ static void test_load_modules_details(void) } else ok(0, "Unrecognized file reference %c\n", *ptr); } - temp_bail_out: if (test->in_image_name && !wcscmp(test->in_image_name, L"bar.dll")) { make_path(filename, topdir, NULL, test->in_image_name); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5248
nsi crash looks strange (can't repro locally, and other pipelines from recent mr don't show it) and is not related to the changes context of the crash seems kinda broken (ebp = 0) could likely improve backtrace to not repeat itself with null frame pointer -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5248#note_63725
participants (2)
-
Eric Pouech -
eric pouech (＠epo)