[PATCH 0/3] MR9078: More debugging related fixes & enhancements.
This serie: - drastically reduce winedbg attachement time when lots of modulesi or exported symbols are present (current use case here reduces from 12 seconds to less than a second) - fixes a crash in dbghelp. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9078
From: Eric Pouech <epouech(a)codeweavers.com> Always using GetModuleFileNameExW: - as it doesn't depend on information from debug event that are not always present (pointer to module name, or file handle), - it only uses a single server call (pointer to module name requires two). - and stop using GetModuleFileNameExW which implementation walks the module list in debuggee process, hence request time grows linearly with number of loaded modules. This reduces significantly attachment time in auto mode. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- programs/winedbg/debugger.h | 2 +- programs/winedbg/gdbproxy.c | 3 +-- programs/winedbg/tgt_active.c | 24 ++++++++++-------------- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/programs/winedbg/debugger.h b/programs/winedbg/debugger.h index 45a91e2a6f5..28b60c853fb 100644 --- a/programs/winedbg/debugger.h +++ b/programs/winedbg/debugger.h @@ -485,7 +485,7 @@ extern enum dbg_start dbg_active_auto(int argc, char* argv[]); extern enum dbg_start dbg_active_minidump(int argc, char* argv[]); extern void dbg_active_wait_for_first_exception(void); extern BOOL dbg_attach_debuggee(DWORD pid); -extern void fetch_module_name(void* name_addr, void* mod_addr, WCHAR* buffer, size_t bufsz); +extern void fetch_module_name(void* mod_addr, WCHAR* buffer, size_t bufsz); /* tgt_minidump.c */ extern void minidump_write(const char*, const EXCEPTION_RECORD*); diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index e937c4edf32..cad357a02b7 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -572,8 +572,7 @@ static BOOL handle_debug_event(struct gdb_context* gdbctx, BOOL stop_on_dll_load return TRUE; case LOAD_DLL_DEBUG_EVENT: - fetch_module_name( de->u.LoadDll.lpImageName, de->u.LoadDll.lpBaseOfDll, - u.buffer, ARRAY_SIZE(u.buffer) ); + fetch_module_name( de->u.LoadDll.lpBaseOfDll, u.buffer, ARRAY_SIZE(u.buffer) ); fprintf(stderr, "%04lx:%04lx: loads DLL %ls @%p (%lu<%lu>)\n", de->dwProcessId, de->dwThreadId, u.buffer, diff --git a/programs/winedbg/tgt_active.c b/programs/winedbg/tgt_active.c index a919f70a486..1b396b82c7c 100644 --- a/programs/winedbg/tgt_active.c +++ b/programs/winedbg/tgt_active.c @@ -315,22 +315,19 @@ static DWORD dbg_handle_exception(const EXCEPTION_RECORD* rec, BOOL first_chance static BOOL tgt_process_active_close_process(struct dbg_process* pcs, BOOL kill); -void fetch_module_name(void* name_addr, void* mod_addr, WCHAR* buffer, size_t bufsz) +void fetch_module_name(void* mod_addr, WCHAR* buffer, size_t bufsz) { - memory_get_string_indirect(dbg_curr_process, name_addr, TRUE, buffer, bufsz); - if (!buffer[0] && !GetModuleFileNameExW(dbg_curr_process->handle, mod_addr, buffer, bufsz)) + DWORD len; + if ((len = GetMappedFileNameW( dbg_curr_process->handle, mod_addr, buffer, bufsz ))) { - if (GetMappedFileNameW( dbg_curr_process->handle, mod_addr, buffer, bufsz )) - { - /* FIXME: proper NT->Dos conversion */ - static const WCHAR nt_prefixW[] = {'\\','?','?','\\'}; + /* FIXME: proper NT->Dos conversion */ + static const WCHAR nt_prefixW[] = {'\\','?','?','\\'}; - if (!wcsncmp( buffer, nt_prefixW, 4 )) - memmove( buffer, buffer + 4, (lstrlenW(buffer + 4) + 1) * sizeof(WCHAR) ); - } - else - swprintf(buffer, bufsz, L"DLL_%08lx", (ULONG_PTR)mod_addr); + if (!wcsncmp( buffer, nt_prefixW, 4 )) + memmove( buffer, buffer + 4, (len - 4 + 1) * sizeof(WCHAR) ); } + else + swprintf(buffer, bufsz, L"DLL_%08lx", (ULONG_PTR)mod_addr); } static unsigned dbg_handle_debug_event(DEBUG_EVENT* de) @@ -485,8 +482,7 @@ static unsigned dbg_handle_debug_event(DEBUG_EVENT* de) WINE_ERR("Unknown thread\n"); break; } - fetch_module_name(de->u.LoadDll.lpImageName, de->u.LoadDll.lpBaseOfDll, - u.buffer, ARRAY_SIZE(u.buffer)); + fetch_module_name(de->u.LoadDll.lpBaseOfDll, u.buffer, ARRAY_SIZE(u.buffer)); WINE_TRACE("%04lx:%04lx: loads DLL %s @%p (%lu<%lu>)\n", de->dwProcessId, de->dwThreadId, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9078
From: Eric Pouech <epouech(a)codeweavers.com> Always creating public symbols from export table can be slow. If we already have some debug information, then exported symbols are likely already present. In extreme cases, this can reduce significantly loading time of module. Note: in some (rare) cases, this will change current behavior: - if symbol has no debug inforamtion attached to it (assembly...), then no name will be available for it, - if exported symbol name is different from internal one, then only the internal one will be reported. Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/dbghelp/pe_module.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/dlls/dbghelp/pe_module.c b/dlls/dbghelp/pe_module.c index 6b5a9a4b425..948708ac5a3 100644 --- a/dlls/dbghelp/pe_module.c +++ b/dlls/dbghelp/pe_module.c @@ -477,7 +477,7 @@ static BOOL pe_load_coff_symbol_table(struct module* module) numsym = fmap->u.pe.file_header.NumberOfSymbols; if (!fmap->u.pe.file_header.PointerToSymbolTable || !numsym) - return TRUE; + return FALSE; if (!(mapping = pe_map_full(fmap, NULL))) return FALSE; isym = (const IMAGE_SYMBOL*)(mapping + fmap->u.pe.file_header.PointerToSymbolTable); /* FIXME: no way to get strtable size */ @@ -779,9 +779,11 @@ BOOL pe_load_debug_info(const struct process* pcs, struct module* module) * in which case we'll rely on the export's on the ELF side */ } - /* FIXME shouldn't we check that? if (!module_get_debug(pcs, module)) */ - if (pe_load_export_debug_info(pcs, module) && !ret) - ret = TRUE; + /* FIXME: + * - only loading export debug info in last resort when none of available format succeeded + * (assuming export debug info is a subset of actual debug infomation). + */ + if (!ret) ret = pe_load_export_debug_info(pcs, module); if (!ret) module->module.SymType = SymNone; return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9078
From: Eric Pouech <epouech(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58742 Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/dbghelp/pe_module.c | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/dlls/dbghelp/pe_module.c b/dlls/dbghelp/pe_module.c index 948708ac5a3..eb0954d156a 100644 --- a/dlls/dbghelp/pe_module.c +++ b/dlls/dbghelp/pe_module.c @@ -643,33 +643,35 @@ static BOOL pe_load_msc_debug_info(const struct process* pcs, struct module* mod if (!(mapping = pe_map_full(fmap, &nth))) return FALSE; /* Read in debug directory */ - dbg = RtlImageDirectoryEntryToData( mapping, FALSE, IMAGE_DIRECTORY_ENTRY_DEBUG, &nDbg ); - nDbg = dbg ? nDbg / sizeof(IMAGE_DEBUG_DIRECTORY) : 0; - - /* Parse debug directory */ - if (nth->FileHeader.Characteristics & IMAGE_FILE_DEBUG_STRIPPED) + if ((dbg = RtlImageDirectoryEntryToData( mapping, FALSE, IMAGE_DIRECTORY_ENTRY_DEBUG, &nDbg ))) { - /* Debug info is stripped to .DBG file */ - const IMAGE_DEBUG_MISC* misc = (const IMAGE_DEBUG_MISC*) - ((const char*)mapping + dbg->PointerToRawData); + nDbg /= sizeof(IMAGE_DEBUG_DIRECTORY); - if (nDbg != 1 || dbg->Type != IMAGE_DEBUG_TYPE_MISC || - misc->DataType != IMAGE_DEBUG_MISC_EXENAME) + /* Parse debug directory */ + if (nth->FileHeader.Characteristics & IMAGE_FILE_DEBUG_STRIPPED) { - WARN("-Debug info stripped, but no .DBG file in module %s\n", - debugstr_w(module->modulename)); + /* Debug info is stripped to .DBG file */ + const IMAGE_DEBUG_MISC* misc = (const IMAGE_DEBUG_MISC*) + ((const char*)mapping + dbg->PointerToRawData); + + if (nDbg != 1 || dbg->Type != IMAGE_DEBUG_TYPE_MISC || + misc->DataType != IMAGE_DEBUG_MISC_EXENAME) + { + WARN("-Debug info stripped, but no .DBG file in module %s\n", + debugstr_w(module->modulename)); + } + else + { + ret = pe_load_dbg_file(pcs, module, (const char*)misc->Data, nth->FileHeader.TimeDateStamp); + } } else { - ret = pe_load_dbg_file(pcs, module, (const char*)misc->Data, nth->FileHeader.TimeDateStamp); + /* Debug info is embedded into PE module */ + ret = pe_load_debug_directory(pcs, module, mapping, IMAGE_FIRST_SECTION( nth ), + nth->FileHeader.NumberOfSections, dbg, nDbg); } } - else - { - /* Debug info is embedded into PE module */ - ret = pe_load_debug_directory(pcs, module, mapping, IMAGE_FIRST_SECTION( nth ), - nth->FileHeader.NumberOfSections, dbg, nDbg); - } pe_unmap_full(fmap); return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9078
I don't know anything about this code but the dbghelp test failures might be related? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9078#note_117189
very likely yes, will have a look. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9078#note_117200
participants (3)
-
Eric Pouech -
eric pouech (＠epo)
-
Rémi Bernon