[PATCH v9 0/5] MR7: ntdll: Properly track refcount with forwarded exports.
This fixes https://bugs.winehq.org/show_bug.cgi?id=52094. For reference, here's how current_modref is being passed around (before this patch): ```mermaid graph TB fii["fixup_imports_ilonly<br><em style='color:#ff0'>(directly sets current_modref)</em>"] fi["fixup_imports<br><em style='color:#ff0'>(directly sets current_modref)</em>"] pa["process_attach<br><em style='color:#ff0'>(directly sets current_modref)</em>"] ld[load_dll] id["import_dll<br><em style='color:#0f0'>(directly uses current_modref)</em>"] bin["build_import_name<br><em style='color:#0f0'>(directly uses current_modref)</em>"] foe["find_ordinal_export<br><em style='color:#0f0'>(uses current_modref for relay)</em>"] ffe["find_forwarded_export<br><em style='color:#0f0'>(directly uses current_modref)</em>"] fne[find_named_export] MI[MODULE_InitDLL] fii --> ld fi --> id pa --> MI -.-> DllMain id --> bin id --> ld id --> foe id --> fne --> foe --> ffe --> foe ffe --> fne ffe --> bin style DllMain color:red; ``` -- v9: ntdll: Properly track refcount with forwarded exports. ntdll: Don't re-add a module dependency if it already exists. https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Prepare for adding context information (e.g. static or dynamic) about the current importer, which is needed for correct RelayFromExclude handling. --- dlls/ntdll/loader.c | 70 ++++++++++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 0c25fe14133..ffee8e8d68c 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -184,9 +184,16 @@ static RTL_BITMAP tls_bitmap; static RTL_BITMAP tls_expansion_bitmap; static WINE_MODREF *cached_modref; -static WINE_MODREF *current_modref; static WINE_MODREF *last_failed_modref; +struct importer +{ + struct importer *prev; + WINE_MODREF *modref; +}; + +static struct importer *current_importer; + static LDR_DDAG_NODE *node_ntdll, *node_kernel32; static NTSTATUS load_dll( const WCHAR *load_path, const WCHAR *libname, DWORD flags, WINE_MODREF** pwm, BOOL system ); @@ -504,6 +511,20 @@ static ULONG_PTR allocate_stub( const char *dll, const char *name ) static inline ULONG_PTR allocate_stub( const char *dll, const char *name ) { return 0xdeadbeef; } #endif /* __i386__ */ +/* The loader_section must be locked while calling this function. */ +static void push_importer( struct importer *importer, WINE_MODREF *modref ) +{ + importer->modref = modref; + importer->prev = current_importer; + current_importer = importer; +} + +/* The loader_section must be locked while calling this function. */ +static void pop_importer( struct importer *importer ) +{ + current_importer = importer->prev; +} + /* call ldr notifications */ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) { @@ -738,7 +759,7 @@ static NTSTATUS build_import_name( WCHAR buffer[256], const char *import, int le { const API_SET_NAMESPACE *map = NtCurrentTeb()->Peb->ApiSetMap; const API_SET_NAMESPACE_ENTRY *entry; - const WCHAR *host = current_modref ? current_modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *host = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; UNICODE_STRING str; while (len && import[len-1] == ' ') len--; /* remove trailing spaces */ @@ -922,9 +943,9 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { - if (!imports_fixup_done && current_modref) + if (!imports_fixup_done && current_importer) { - add_module_dependency( current_modref->ldr.DdagNode, wm->ldr.DdagNode ); + add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); } else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { @@ -992,12 +1013,12 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY if (TRACE_ON(snoop)) { - const WCHAR *user = current_modref ? current_modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; proc = SNOOP_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } if (TRACE_ON(relay)) { - const WCHAR *user = current_modref ? current_modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } return proc; @@ -1090,7 +1111,8 @@ void * WINAPI RtlFindExportedRoutineByName( HMODULE module, const char *name ) */ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LPCWSTR load_path, WINE_MODREF **pwm ) { - BOOL system = current_modref->system || (current_modref->ldr.Flags & LDR_WINE_INTERNAL); + struct importer *importer = current_importer; + BOOL system = importer->modref->system || (importer->modref->ldr.Flags & LDR_WINE_INTERNAL); NTSTATUS status; WINE_MODREF *wmImp; HMODULE imp_mod; @@ -1125,10 +1147,10 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP { if (status == STATUS_DLL_NOT_FOUND) ERR("Library %s (which is needed by %s) not found\n", - name, debugstr_w(current_modref->ldr.FullDllName.Buffer)); + name, debugstr_w(importer->modref->ldr.FullDllName.Buffer)); else ERR("Loading library %s (which is needed by %s) failed (error %lx).\n", - name, debugstr_w(current_modref->ldr.FullDllName.Buffer), status); + name, debugstr_w(importer->modref->ldr.FullDllName.Buffer), status); return FALSE; } @@ -1161,7 +1183,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP thunk_list->u1.Function = allocate_stub( name, (const char*)pe_name->Name ); } WARN(" imported from %s, allocating stub %p\n", - debugstr_w(current_modref->ldr.FullDllName.Buffer), + debugstr_w(importer->modref->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); import_list++; thunk_list++; @@ -1181,7 +1203,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP { thunk_list->u1.Function = allocate_stub( name, IntToPtr(ordinal) ); WARN("No implementation for %s.%d imported from %s, setting to %p\n", - name, ordinal, debugstr_w(current_modref->ldr.FullDllName.Buffer), + name, ordinal, debugstr_w(importer->modref->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); } TRACE_(imports)("--- Ordinal %s.%d = %p\n", name, ordinal, (void *)thunk_list->u1.Function ); @@ -1197,7 +1219,7 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP { thunk_list->u1.Function = allocate_stub( name, (const char*)pe_name->Name ); WARN("No implementation for %s.%s imported from %s, setting to %p\n", - name, pe_name->Name, debugstr_w(current_modref->ldr.FullDllName.Buffer), + name, pe_name->Name, debugstr_w(importer->modref->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); } TRACE_(imports)("--- %s %s.%d = %p\n", @@ -1396,21 +1418,21 @@ static void free_tls_slot( LDR_DATA_TABLE_ENTRY *mod ) */ static NTSTATUS fixup_imports_ilonly( WINE_MODREF *wm, LPCWSTR load_path, void **entry ) { + struct importer importer; NTSTATUS status; void *proc; const char *name; - WINE_MODREF *prev, *imp; + WINE_MODREF *imp; if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) return STATUS_SUCCESS; /* already done */ wm->ldr.Flags &= ~LDR_DONT_RESOLVE_REFS; - prev = current_modref; - current_modref = wm; + push_importer( &importer, wm ); assert( !wm->ldr.DdagNode->Dependencies.Tail ); if (!(status = load_dll( load_path, L"mscoree.dll", 0, &imp, FALSE )) && !add_module_dependency_after( wm->ldr.DdagNode, imp->ldr.DdagNode, NULL )) status = STATUS_NO_MEMORY; - current_modref = prev; + pop_importer( &importer ); if (status) { ERR( "mscoree.dll not found, IL-only binary %s cannot be loaded\n", @@ -1437,7 +1459,8 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) { const IMAGE_IMPORT_DESCRIPTOR *imports; SINGLE_LIST_ENTRY *dep_after; - WINE_MODREF *prev, *imp; + struct importer importer; + WINE_MODREF *imp; int i, nb_imports; DWORD size; NTSTATUS status; @@ -1463,8 +1486,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) /* load the imported modules. They are automatically * added to the modref list of the process. */ - prev = current_modref; - current_modref = wm; + push_importer( &importer, wm ); status = STATUS_SUCCESS; for (i = 0; i < nb_imports; i++) { @@ -1474,7 +1496,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) else if (imp && imp->ldr.DdagNode != node_ntdll && imp->ldr.DdagNode != node_kernel32) add_module_dependency_after( wm->ldr.DdagNode, imp->ldr.DdagNode, dep_after ); } - current_modref = prev; + pop_importer( &importer ); if (wm->ldr.ActivationContext) RtlDeactivateActivationContext( 0, cookie ); return status; } @@ -1755,8 +1777,9 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ) /* Call DLL entry point */ if (status == STATUS_SUCCESS) { - WINE_MODREF *prev = current_modref; - current_modref = wm; + struct importer importer; + + push_importer( &importer, wm ); call_ldr_notifications( LDR_DLL_NOTIFICATION_REASON_LOADED, &wm->ldr ); status = MODULE_InitDLL( wm, DLL_PROCESS_ATTACH, lpReserved ); @@ -1773,7 +1796,8 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ) last_failed_modref = wm; WARN("Initialization of %s failed\n", debugstr_w(wm->ldr.BaseDllName.Buffer)); } - current_modref = prev; + + pop_importer( &importer ); } if (wm->ldr.ActivationContext) RtlDeactivateActivationContext( 0, cookie ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> This matches the Windows GetProcAddress() behavior when handling dynamic module dependencies. To avoid breaking WINEDEBUG=+relay, flag dynamic importers (`is_dynamic = TRUE`) and explicitly ignore them when testing for RelayFromExclude modules. Otherwise, GetProcAddress() on kernel32 export forwarders (which comprise most of the exports) will be recognized as relaying *from* kernel32 itself (instead of the actual importer) and will be subsequently excluded from WINEDEBUG=+relay due to the default `RelayFromExclude` which includes `kernel32`. The current behavior is to treat it as relaying from the actual importer, not kernel32, so this doen't become a problem. This bit is true for all export forwarder DLLs in general, and also affects RelayFromInclude as well as SnoopFrom{Exclude,Include} (+snoop). --- dlls/ntdll/loader.c | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index ffee8e8d68c..2fa31b34d4d 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -190,6 +190,7 @@ struct importer { struct importer *prev; WINE_MODREF *modref; + BOOL is_dynamic; }; static struct importer *current_importer; @@ -512,9 +513,10 @@ static inline ULONG_PTR allocate_stub( const char *dll, const char *name ) { ret #endif /* __i386__ */ /* The loader_section must be locked while calling this function. */ -static void push_importer( struct importer *importer, WINE_MODREF *modref ) +static void push_importer( struct importer *importer, WINE_MODREF *modref, BOOL is_dynamic ) { importer->modref = modref; + importer->is_dynamic = is_dynamic; importer->prev = current_importer; current_importer = importer; } @@ -525,6 +527,20 @@ static void pop_importer( struct importer *importer ) current_importer = importer->prev; } +/* The loader_section must be locked while calling this function. */ +static const WCHAR *get_last_static_importer_name(void) +{ + struct importer *importer; + for (importer = current_importer; importer != NULL; importer = importer->prev) + { + if (!importer->is_dynamic) + { + return importer->modref->ldr.BaseDllName.Buffer; + } + } + return NULL; +} + /* call ldr notifications */ static void call_ldr_notifications( ULONG reason, LDR_DATA_TABLE_ENTRY *module ) { @@ -1013,12 +1029,12 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY if (TRACE_ON(snoop)) { - const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = get_last_static_importer_name(); proc = SNOOP_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } if (TRACE_ON(relay)) { - const WCHAR *user = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *user = get_last_static_importer_name(); proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } return proc; @@ -1427,7 +1443,7 @@ static NTSTATUS fixup_imports_ilonly( WINE_MODREF *wm, LPCWSTR load_path, void * if (!(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) return STATUS_SUCCESS; /* already done */ wm->ldr.Flags &= ~LDR_DONT_RESOLVE_REFS; - push_importer( &importer, wm ); + push_importer( &importer, wm, FALSE ); assert( !wm->ldr.DdagNode->Dependencies.Tail ); if (!(status = load_dll( load_path, L"mscoree.dll", 0, &imp, FALSE )) && !add_module_dependency_after( wm->ldr.DdagNode, imp->ldr.DdagNode, NULL )) @@ -1486,7 +1502,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) /* load the imported modules. They are automatically * added to the modref list of the process. */ - push_importer( &importer, wm ); + push_importer( &importer, wm, FALSE ); status = STATUS_SUCCESS; for (i = 0; i < nb_imports; i++) { @@ -1779,7 +1795,7 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ) { struct importer importer; - push_importer( &importer, wm ); + push_importer( &importer, wm, FALSE ); call_ldr_notifications( LDR_DLL_NOTIFICATION_REASON_LOADED, &wm->ldr ); status = MODULE_InitDLL( wm, DLL_PROCESS_ATTACH, lpReserved ); @@ -2063,8 +2079,14 @@ NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name, else if ((exports = RtlImageDirectoryEntryToData( module, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ))) { - void *proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL ) - : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL ); + struct importer importer; + void *proc; + + push_importer( &importer, wm, TRUE ); + proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL ) + : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL ); + pop_importer( &importer ); + if (proc) { *address = proc; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> current_importer is now set by all callers of build_import_name, find_forwarded_export, and find_ordinal_export. --- dlls/ntdll/loader.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 2fa31b34d4d..9e7da8ae1b5 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -775,7 +775,7 @@ static NTSTATUS build_import_name( WCHAR buffer[256], const char *import, int le { const API_SET_NAMESPACE *map = NtCurrentTeb()->Peb->ApiSetMap; const API_SET_NAMESPACE_ENTRY *entry; - const WCHAR *host = current_importer ? current_importer->modref->ldr.BaseDllName.Buffer : NULL; + const WCHAR *host = current_importer->modref->ldr.BaseDllName.Buffer; UNICODE_STRING str; while (len && import[len-1] == ' ') len--; /* remove trailing spaces */ @@ -959,7 +959,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { - if (!imports_fixup_done && current_importer) + if (!imports_fixup_done) { add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Today, calling add_module_dependency() multiple times with the same arguments results in duplicate edges. Duplicate edges are harmless, but bloats memory usage. The number of duplicate edges does not affect the dependency graph; the graph is determined by the set of unique edges. Consciously avoid duplicates by checking for them in add_module_dependency_after(). This allows us to generate a unique dependency edge for all imports of export forwarders that belong to the same DLL. --- dlls/ntdll/loader.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 9e7da8ae1b5..29fbac97cc8 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -875,6 +875,21 @@ static void remove_single_list_entry( LDRP_CSLIST *list, SINGLE_LIST_ENTRY *entr entry->Next = NULL; } +static LDR_DEPENDENCY *find_module_dependency( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to ) +{ + SINGLE_LIST_ENTRY *entry, *mark = from->Dependencies.Tail; + + if (!mark) return NULL; + + for (entry = mark->Next; entry != mark; entry = entry->Next) + { + LDR_DEPENDENCY *dep = CONTAINING_RECORD( entry, LDR_DEPENDENCY, dependency_to_entry ); + if (dep->dependency_from == from && dep->dependency_to == to) return dep; + } + + return NULL; +} + /********************************************************************** * add_module_dependency_after */ @@ -883,6 +898,16 @@ static BOOL add_module_dependency_after( LDR_DDAG_NODE *from, LDR_DDAG_NODE *to, { LDR_DEPENDENCY *dep; + if ((dep = find_module_dependency( from, to ))) + { + /* Dependency already exists; consume the module reference stolen from the caller */ + LDR_DATA_TABLE_ENTRY *mod = CONTAINING_RECORD( to->Modules.Flink, LDR_DATA_TABLE_ENTRY, NodeModuleLink ); + WINE_MODREF *wm = CONTAINING_RECORD( mod, WINE_MODREF, ldr ); + assert( wm->ldr.LoadCount != 1 ); + if (wm->ldr.LoadCount != -1) wm->ldr.LoadCount--; + return TRUE; + } + if (!(dep = RtlAllocateHeap( GetProcessHeap(), 0, sizeof(*dep) ))) return FALSE; dep->dependency_from = from; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52094 --- dlls/kernel32/tests/loader.c | 2 -- dlls/ntdll/loader.c | 17 ++++++++++++----- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 2c7cc784be4..5ea169beb51 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2777,7 +2777,6 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module /* FreeLibrary() should *not* unload the DLL immediately */ module = GetModuleHandleA( temp_paths[i] ); - todo_wine_if(i < ultimate_depender_index && i + 1 != importer_index) ok( module == modules[i], "modules[%Iu] expected %p, got %p (unloaded?) err=%lu\n", i, modules[i], module, GetLastError() ); } @@ -2789,7 +2788,6 @@ static void subtest_export_forwarder_dep_chain( size_t num_chained_export_module { HMODULE module = GetModuleHandleA( temp_paths[i] ); - todo_wine_if(i < ultimate_depender_index && i + 1 != importer_index) ok( module == modules[i], "modules[%Iu] expected %p, got %p (unloaded?) err=%lu\n", i, modules[i], module, GetLastError() ); } diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 29fbac97cc8..2f4e2dd0674 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -984,11 +984,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (load_dll( load_path, mod_name, 0, &wm, imp->system ) == STATUS_SUCCESS && !(wm->ldr.Flags & LDR_DONT_RESOLVE_REFS)) { - if (!imports_fixup_done) - { - add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); - } - else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) + if (imports_fixup_done && process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { LdrUnloadDll( wm->ldr.DllBase ); wm = NULL; @@ -1002,6 +998,11 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS return NULL; } } + else + { + if (wm->ldr.LoadCount != -1) wm->ldr.LoadCount++; + } + if ((exports = RtlImageDirectoryEntryToData( wm->ldr.DllBase, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size ))) { @@ -1021,6 +1022,12 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS forward, debugstr_w(get_modref(module)->ldr.FullDllName.Buffer), debugstr_w(get_modref(module)->ldr.BaseDllName.Buffer) ); } + else if (wm->ldr.DdagNode != node_ntdll && wm->ldr.DdagNode != node_kernel32) + { + add_module_dependency( current_importer->modref->ldr.DdagNode, wm->ldr.DdagNode ); + wm = NULL; + } + if (wm) LdrUnloadDll( wm->ldr.DllBase ); return proc; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7
On Thu Feb 6 14:48:30 2025 +0000, Jacek Caban wrote:
So, in a sense, `add_module_dependency` holds the final authority over whether we want to actually increase LoadCount or not. I see, thanks for explanation. It would be easier to follow if `add_module_dependency` would be responsible for increasing `LoadCount` instead, but it would mean that we'd initially create a module with 0 `LoadCount`, creating more corner cases, so in the end something like would likely not be worth it. `LdrUnloadDll` is rather a heavy tool just to decrease the count in that patch. Since we know that we have another edge, we know that we won't actually need to unload the DLL, so just a direct decrement of `LoadCount` should be enough? I'm mentioning that for consideration, it's not a strong opinion. Done in v9.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93901
On Thu Feb 6 14:44:50 2025 +0000, Jinoh Kang wrote: > > Since this mechanism can't be fully precise (`DllMain` might call > functions from a different module) and we don't use such > `GetProcAddress` calls inside Wine DLLs, where this feature is typically > applied, I'm wondering if we could simplify it by treating `is_dynamic` > as a `NULL` user here. > On second look, there seems to be multiple Wine builtin DLLs that *do* > call GetProcAddress() in DllMain: > - shlwapi: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328db1379d/dlls/shlwapi/shlwapi_main.c#L155-172 > - wintab32: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328db1379d/dlls/wintab32/wintab32.c#L96-127 > There may be other DLLs that I have missed (e.g., because GetProcAddress > is called indirectly via some helper function in the same DLL). Maybe > something needs to be done with these DLLs before we can get rid of > dynamic dependency handling? shlwapi looks like a relict from before PE conversion, we should be able to link those functions directly now. wintab32 will need a different solution anyway, ideally we wouldn't use PE exports in display drivers. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93902
On Thu Feb 6 14:49:16 2025 +0000, Jacek Caban wrote:
shlwapi looks like a relict from before PE conversion, we should be able to link those functions directly now. wintab32 will need a different solution anyway, ideally we wouldn't use PE exports in display drivers. There are more:
- concrt140: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... - Seems like the same for msvcirt, msvcp140_1, msvcp60, and msvcp90, although I haven't verified. - mapi32: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... calls https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... - secur32: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... calls https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... calls https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... - Same for mpr: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... calls https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... calls https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... At this point it's plausible that I may be missing even more. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93910
On Thu Feb 6 15:44:00 2025 +0000, Jinoh Kang wrote:
There are more: - concrt140: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... - Seems like the same for msvcirt, msvcp140_1, msvcp60, and msvcp90, although I haven't verified. - mapi32: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... calls https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... - secur32: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... calls https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... calls https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... - Same for mpr: https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... calls https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... calls https://gitlab.winehq.org/wine/wine/-/blob/7b57598dcfc9198aa9cf6ac4ce17e2328... At this point it's plausible that I may be missing even more. Besides, since you mentioned uncertainty around apisets, I wonder if such refactoring would merit a separate MR. I'm uncomfortable with consolidating too much possibility of regression into one MR, I think it's best spread over time.
Maybe go only the `!current_modref->is_dynamic` part applied, although I'm not sure if I'm still breaking someone else's workflow with any of the DLLs above. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_93913
Besides, since you mentioned uncertainty around apisets,
I mentioned it because it required a closer look, and I ran out of time that day. That said, I think the change is fine. It needed a closer look to review for this MR anyway.
I wonder if such refactoring would merit a separate MR.
Sure, I created !7287. I think it would be good to settle this first. The whole push/pop approach adds extra effort that wouldn’t be necessary if we decide to go with it.
There are more:
In most cases, I’d say the exclusion shouldn’t apply. A few cases might be better handled as regular imports. Either way, I don’t think changing relay exclude is a big deal. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7#note_94027
participants (3)
-
Jacek Caban (＠jacek) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)