[PATCH v2 0/7] MR7287: ntdll: Avoid using current_modref.
This is a cleanup in preparation for !7. Originally, !7 extended the use of `current_modref`, but I think we could avoid that with a more direct approach. Most of these changes are no-ops. For the relay handler, we no longer treat `GetProcAddress` calls inside `DllMain` as coming from a loaded DLL. I believe this was more of a side effect of how the code was structured rather than an intentional behavior that needs to be preserved. One more use of `current_modref` remains, which will be removed by !7. -- v2: ntdll: Remove no longer needed current_modref. null: Use NULL importer in LdrGetProcedureAddress. ntdll: Pass importer pointer to build_import_name. ntdll: Pass importer pointer to find_forwarded_export. ntdll: Pass importer pointer to find_named_export. ntdll: Pass importer pointer to find_ordinal_export. ntdll: Pass WINE_MODREF to import_dll. https://gitlab.winehq.org/wine/wine/-/merge_requests/7287
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntdll/loader.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 0c25fe14133..b48a950e705 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1088,9 +1088,10 @@ void * WINAPI RtlFindExportedRoutineByName( HMODULE module, const char *name ) * Import the dll specified by the given import descriptor. * The loader_section must be locked while calling this function. */ -static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LPCWSTR load_path, WINE_MODREF **pwm ) +static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, LPCWSTR load_path, WINE_MODREF **pwm ) { - BOOL system = current_modref->system || (current_modref->ldr.Flags & LDR_WINE_INTERNAL); + HMODULE module = wm->ldr.DllBase; + BOOL system = wm->system || (wm->ldr.Flags & LDR_WINE_INTERNAL); NTSTATUS status; WINE_MODREF *wmImp; HMODULE imp_mod; @@ -1124,11 +1125,10 @@ static BOOL import_dll( HMODULE module, const IMAGE_IMPORT_DESCRIPTOR *descr, LP if (status) { 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)); + ERR("Library %s (which is needed by %s) not found\n", name, debugstr_w(wm->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(wm->ldr.FullDllName.Buffer), status); return FALSE; } @@ -1161,7 +1161,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(wm->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); import_list++; thunk_list++; @@ -1181,7 +1181,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(wm->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); } TRACE_(imports)("--- Ordinal %s.%d = %p\n", name, ordinal, (void *)thunk_list->u1.Function ); @@ -1197,7 +1197,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(wm->ldr.FullDllName.Buffer), (void *)thunk_list->u1.Function ); } TRACE_(imports)("--- %s %s.%d = %p\n", @@ -1469,7 +1469,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) for (i = 0; i < nb_imports; i++) { dep_after = wm->ldr.DdagNode->Dependencies.Tail; - if (!import_dll( wm->ldr.DllBase, &imports[i], load_path, &imp )) + if (!import_dll( wm, &imports[i], load_path, &imp )) status = STATUS_DLL_NOT_FOUND; 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 ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7287
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntdll/loader.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index b48a950e705..7be510896d0 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -192,7 +192,7 @@ 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 ); static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ); static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, DWORD ordinal, LPCWSTR load_path ); + DWORD exp_size, DWORD ordinal, LPCWSTR load_path, WINE_MODREF *importer ); static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, const char *name, int hint, LPCWSTR load_path ); @@ -947,7 +947,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (*name == '#') { /* ordinal */ proc = find_ordinal_export( wm->ldr.DllBase, exports, exp_size, - atoi(name+1) - exports->Base, load_path ); + atoi(name+1) - exports->Base, load_path, current_modref ); } else proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path ); } @@ -971,7 +971,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS * The loader_section must be locked while calling this function. */ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, DWORD ordinal, LPCWSTR load_path ) + DWORD exp_size, DWORD ordinal, LPCWSTR load_path, WINE_MODREF *importer ) { FARPROC proc; const DWORD *functions = get_rva( module, exports->AddressOfFunctions ); @@ -992,12 +992,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 = importer ? importer->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 = importer ? importer->ldr.BaseDllName.Buffer : NULL; proc = RELAY_GetProcAddress( module, exports, exp_size, proc, ordinal, user ); } return proc; @@ -1045,12 +1045,12 @@ static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY * { char *ename = get_rva( module, names[hint] ); if (!strcmp( ename, name )) - return find_ordinal_export( module, exports, exp_size, ordinals[hint], load_path ); + return find_ordinal_export( module, exports, exp_size, ordinals[hint], load_path, current_modref ); } /* then do a binary search */ if ((ordinal = find_name_in_exports( module, exports, name )) == -1) return NULL; - return find_ordinal_export( module, exports, exp_size, ordinal, load_path ); + return find_ordinal_export( module, exports, exp_size, ordinal, load_path, current_modref ); } @@ -1176,7 +1176,7 @@ static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, L int ordinal = IMAGE_ORDINAL(import_list->u1.Ordinal); thunk_list->u1.Function = (ULONG_PTR)find_ordinal_export( imp_mod, exports, exp_size, - ordinal - exports->Base, load_path ); + ordinal - exports->Base, load_path, wm ); if (!thunk_list->u1.Function) { thunk_list->u1.Function = allocate_stub( name, IntToPtr(ordinal) ); @@ -2040,7 +2040,7 @@ NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name, 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 ); + : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL, current_modref ); if (proc) { *address = proc; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7287
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntdll/loader.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 7be510896d0..402dcba270f 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -194,7 +194,7 @@ static NTSTATUS process_attach( LDR_DDAG_NODE *node, LPVOID lpReserved ); static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, DWORD ordinal, LPCWSTR load_path, WINE_MODREF *importer ); static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, const char *name, int hint, LPCWSTR load_path ); + DWORD exp_size, const char *name, int hint, LPCWSTR load_path, WINE_MODREF *importer ); /* check whether the file name contains a path */ static inline BOOL contains_path( LPCWSTR name ) @@ -949,7 +949,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS proc = find_ordinal_export( wm->ldr.DllBase, exports, exp_size, atoi(name+1) - exports->Base, load_path, current_modref ); } else - proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path ); + proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path, current_modref ); } if (!proc) @@ -1033,8 +1033,8 @@ static int find_name_in_exports( HMODULE module, const IMAGE_EXPORT_DIRECTORY *e * Find an exported function by name. * The loader_section must be locked while calling this function. */ -static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, const char *name, int hint, LPCWSTR load_path ) +static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, + const char *name, int hint, LPCWSTR load_path, WINE_MODREF *importer ) { const WORD *ordinals = get_rva( module, exports->AddressOfNameOrdinals ); const DWORD *names = get_rva( module, exports->AddressOfNames ); @@ -1045,12 +1045,12 @@ static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY * { char *ename = get_rva( module, names[hint] ); if (!strcmp( ename, name )) - return find_ordinal_export( module, exports, exp_size, ordinals[hint], load_path, current_modref ); + return find_ordinal_export( module, exports, exp_size, ordinals[hint], load_path, importer ); } /* then do a binary search */ if ((ordinal = find_name_in_exports( module, exports, name )) == -1) return NULL; - return find_ordinal_export( module, exports, exp_size, ordinal, load_path, current_modref ); + return find_ordinal_export( module, exports, exp_size, ordinal, load_path, importer ); } @@ -1192,7 +1192,7 @@ static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, L pe_name = get_rva( module, (DWORD)import_list->u1.AddressOfData ); thunk_list->u1.Function = (ULONG_PTR)find_named_export( imp_mod, exports, exp_size, (const char*)pe_name->Name, - pe_name->Hint, load_path ); + pe_name->Hint, load_path, wm ); if (!thunk_list->u1.Function) { thunk_list->u1.Function = allocate_stub( name, (const char*)pe_name->Name ); @@ -2039,7 +2039,7 @@ 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 ) + void *proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL, current_modref ) : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL, current_modref ); if (proc) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7287
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntdll/loader.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 402dcba270f..596cfab4b40 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -903,7 +903,7 @@ static NTSTATUS walk_node_dependencies( LDR_DDAG_NODE *node, void *context, * Find the final function pointer for a forwarded function. * The loader_section must be locked while calling this function. */ -static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWSTR load_path ) +static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWSTR load_path, WINE_MODREF *importer ) { const IMAGE_EXPORT_DIRECTORY *exports; DWORD exp_size; @@ -922,9 +922,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 && importer) { - add_module_dependency( current_modref->ldr.DdagNode, wm->ldr.DdagNode ); + add_module_dependency( importer->ldr.DdagNode, wm->ldr.DdagNode ); } else if (process_attach( wm->ldr.DdagNode, NULL ) != STATUS_SUCCESS) { @@ -947,9 +947,9 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS if (*name == '#') { /* ordinal */ proc = find_ordinal_export( wm->ldr.DllBase, exports, exp_size, - atoi(name+1) - exports->Base, load_path, current_modref ); + atoi(name+1) - exports->Base, load_path, importer ); } else - proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path, current_modref ); + proc = find_named_export( wm->ldr.DllBase, exports, exp_size, name, -1, load_path, importer ); } if (!proc) @@ -988,7 +988,7 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY /* if the address falls into the export dir, it's a forward */ if (((const char *)proc >= (const char *)exports) && ((const char *)proc < (const char *)exports + exp_size)) - return find_forwarded_export( module, (const char *)proc, load_path ); + return find_forwarded_export( module, (const char *)proc, load_path, importer ); if (TRACE_ON(snoop)) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7287
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntdll/loader.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 596cfab4b40..467e8660011 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -734,11 +734,11 @@ static NTSTATUS get_apiset_target( const API_SET_NAMESPACE *map, const API_SET_N /********************************************************************** * build_import_name */ -static NTSTATUS build_import_name( WCHAR buffer[256], const char *import, int len ) +static NTSTATUS build_import_name( WINE_MODREF *importer, WCHAR buffer[256], const char *import, int len ) { 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 = importer ? importer->ldr.BaseDllName.Buffer : NULL; UNICODE_STRING str; while (len && import[len-1] == ' ') len--; /* remove trailing spaces */ @@ -913,7 +913,7 @@ static FARPROC find_forwarded_export( HMODULE module, const char *forward, LPCWS FARPROC proc = NULL; if (!end) return NULL; - if (build_import_name( mod_name, forward, end - forward )) return NULL; + if (build_import_name( importer, mod_name, forward, end - forward )) return NULL; if (!(wm = find_basename_module( mod_name ))) { @@ -1119,7 +1119,7 @@ static BOOL import_dll( WINE_MODREF *wm, const IMAGE_IMPORT_DESCRIPTOR *descr, L return TRUE; } - status = build_import_name( buffer, name, len ); + status = build_import_name( wm, buffer, name, len ); if (!status) status = load_dll( load_path, buffer, 0, &wmImp, system ); if (status) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7287
From: Jacek Caban <jacek(a)codeweavers.com> --- 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 467e8660011..b80b90f69a1 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2039,8 +2039,8 @@ 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, current_modref ) - : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL, current_modref ); + void *proc = name ? find_named_export( module, exports, exp_size, name->Buffer, -1, NULL, NULL ) + : find_ordinal_export( module, exports, exp_size, ord - exports->Base, NULL, NULL ); if (proc) { *address = proc; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7287
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/ntdll/loader.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index b80b90f69a1..00cfd358be1 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -184,7 +184,6 @@ 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; static LDR_DDAG_NODE *node_ntdll, *node_kernel32; @@ -1399,18 +1398,15 @@ static NTSTATUS fixup_imports_ilonly( WINE_MODREF *wm, LPCWSTR load_path, void * 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; 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; if (status) { ERR( "mscoree.dll not found, IL-only binary %s cannot be loaded\n", @@ -1437,7 +1433,7 @@ static NTSTATUS fixup_imports( WINE_MODREF *wm, LPCWSTR load_path ) { const IMAGE_IMPORT_DESCRIPTOR *imports; SINGLE_LIST_ENTRY *dep_after; - WINE_MODREF *prev, *imp; + WINE_MODREF *imp; int i, nb_imports; DWORD size; NTSTATUS status; @@ -1463,8 +1459,6 @@ 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; status = STATUS_SUCCESS; for (i = 0; i < nb_imports; i++) { @@ -1474,7 +1468,6 @@ 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; if (wm->ldr.ActivationContext) RtlDeactivateActivationContext( 0, cookie ); return status; } @@ -1755,9 +1748,6 @@ 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; - call_ldr_notifications( LDR_DLL_NOTIFICATION_REASON_LOADED, &wm->ldr ); status = MODULE_InitDLL( wm, DLL_PROCESS_ATTACH, lpReserved ); if (status == STATUS_SUCCESS) @@ -1773,7 +1763,6 @@ 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; } if (wm->ldr.ActivationContext) RtlDeactivateActivationContext( 0, cookie ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7287
On Sat Feb 8 20:41:10 2025 +0000, Jinoh Kang wrote:
Thanks! I also largely agree that `current_modref` can go away and replaced with explicit parameters.[^1] I think, at least for testing, each commit replacing `current_modref` can be split into the following with much finer granularity: 1. Introduce the `wm` parameter if not already, *but* don't use it just yet. Instead, simply assert() that `wm == current_modref`. That way, if a function writes to `current_modref`, make the same changes to `wm` as well. This will catch any de-sync bugs due to incorrect arguments. 2. Replace all reads to `current_modref` with `wm`.[^2] If (1) passes tests, then this commit will pass them too. 3. If, **and only if,** no callees are known to use `current_modref`, replace all writes to `current_modref` with `current_moderef = (WINE_MODREF *)-0x10000;` or any other poison value. This will catch any unintended use of `current_modref` from one of its callees. Then, when we actually eliminate the `current_modref`, the poison value will finally go away. Once we have assurance that the commits splitted as above are indeed atomic and do no cause (test) regressions, I think we can proceed with the split. If you don't have time, I can volunteer. [^1]: In general, I believe that eliminating dynamically scoped (global or TLS) variabls in favor of lexically scoped variables (function parameters and locals) helps dissect data flow and make function behavior more predictable. [^2]: We just asserted that `current_modref == wm` in all circumstances, barring any callees failing to restore `current_modref` before returning due to e.g., SEH unwinding. I split it differently. I also noticed that I messed up previous version in some cases (mixed importer with imported module), that should be fixed in the new version.
Here is a version that with asserts: https://gitlab.winehq.org/jacek/wine/-/merge_requests/18. I'm not sure what's the point of 3, a simple grep should be enough to ensure that there are no users (or, once it's removed, compiler verifies that for us). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7287#note_94096
On Sat Feb 8 20:41:10 2025 +0000, Jacek Caban wrote:
I split it differently. I also noticed that I messed up previous version in some cases (mixed importer with imported module), that should be fixed in the new version. Here is a version that with asserts: https://gitlab.winehq.org/jacek/wine/-/merge_requests/18. I'm not sure what's the point of 3, a simple grep should be enough to ensure that there are no users (or, once it's removed, compiler verifies that for us). FYI, I'm testing with https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/20, which (hopefully) has more extensive checks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7287#note_94161
On Mon Feb 10 16:08:37 2025 +0000, Jinoh Kang wrote:
FYI, I'm testing with https://gitlab.winehq.org/iamahuman/wine/-/merge_requests/20, which (hopefully) has more extensive checks. Thanks. That looks overkill, but if it helps you, that's obviously fine. You're not suggesting that for upstream, right?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7287#note_94169
On Mon Feb 10 19:25:45 2025 +0000, Jacek Caban wrote:
Thanks. That looks overkill, but if it helps you, that's obviously fine. You're not suggesting that for upstream, right? No, it's for testing only. I'll approve after a few more tests. Thank you for your patience!
I've also looked into fixing GetProcAddress() usages in other DLLs before this PR, but they seem not easily fixable so I guess we could go ahead with this first. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7287#note_94214
Jinoh Kang (@iamahuman) commented about dlls/ntdll/loader.c:
* Find an exported function by name. * The loader_section must be locked while calling this function. */ -static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, - DWORD exp_size, const char *name, int hint, LPCWSTR load_path ) +static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, + const char *name, int hint, LPCWSTR load_path, WINE_MODREF *importer ) Let's keep the style consistent with the prototype declaration.
```suggestion:-1+0 static FARPROC find_named_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY *exports, DWORD exp_size, const char *name, int hint, LPCWSTR load_path, WINE_MODREF *importer ) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7287#note_94272
Jinoh Kang (@iamahuman) commented about dlls/ntdll/loader.c:
return TRUE; }
- status = build_import_name( buffer, name, len ); + status = build_import_name( wm, buffer, name, len ); if (!status) status = load_dll( load_path, buffer, 0, &wmImp, system );
if (status) { 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)); + ERR("Library %s (which is needed by %s) not found\n", name, debugstr_w(wm->ldr.FullDllName.Buffer));
This line is slightly long, can we keep this consistent with the else branch? ```suggestion:-0+0 ERR("Library %s (which is needed by %s) not found\n", name, debugstr_w(wm->ldr.FullDllName.Buffer)); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7287#note_94273
This merge request was approved by Jinoh Kang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7287
A few styling nits, but otherwise looks good. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7287#note_94274
participants (3)
-
Jacek Caban -
Jacek Caban (＠jacek)
-
Jinoh Kang (＠iamahuman)