When `import_dll` function gets `STATUS_DLL_NOT_FOUND` from `load_dll()`, it has no way of distinguishing whether it's a dependency for the dll wasn't found, or the dll itself is missing.
That results in confusing ERR messages, where a dll is found, but wine claims it isn't. Fix this by moving the print to the actual place that is aware of whether it's the parent or the child dll haven't been found.
Log before:
0024:err:module:import_dll Library lib2.dll (which is needed by L"Z:\tmp\lib1.dll") not found 0024:err:module:import_dll Library lib1.dll (which is needed by L"Z:\tmp\a.exe") not found
Log after:
0024:err:module:load_dll Library L"lib2.dll" (which is needed by L"Z:\tmp\foo\lib1.dll") not found 0024:err:module:import_dll Loading library lib2.dll (which is needed by L"Z:\tmp\foo\lib1.dll") failed (error c0000135). 0024:err:module:import_dll Loading library lib1.dll (which is needed by L"Z:\tmp\foo\a.exe") failed (error c0000135).
--------
While it's just about prints, but it costed me hours of wandering *(because if I know where to find the dependency dll, but I see that the main dll is also "not found" despite being "right there", I have to research the latter problem that looks more complicated. But given enough resources applied, it then turns out to be a faux)*.
As a fun fact, I am apparently not the only one to have stumbled upon. Here's a screenshot of a `#english` chat where I asked about a commit title correctness. It's the last place where you'd expect to find someone familiar with WINE, and yet here's a person that had to deal with that exact error as well:
So, I think it's kinda serious problem.
From: Konstantin Kharlamov Hi-Angel@yandex.ru
Signed-off-by: Konstantin Kharlamov Hi-Angel@yandex.ru --- dlls/ntdll/loader.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index 043bce67ea9..ae7d0ecaa73 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -935,7 +935,7 @@ static FARPROC find_ordinal_export( HMODULE module, const IMAGE_EXPORT_DIRECTORY proc = get_rva( module, functions[ordinal] );
/* if the address falls into the export dir, it's a forward */ - if (((const char *)proc >= (const char *)exports) && + 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 );
@@ -1734,7 +1734,7 @@ static void process_detach(void)
/* Call detach notification */ mod->Flags &= ~LDR_PROCESS_ATTACHED; - MODULE_InitDLL( CONTAINING_RECORD(mod, WINE_MODREF, ldr), + MODULE_InitDLL( CONTAINING_RECORD(mod, WINE_MODREF, ldr), DLL_PROCESS_DETACH, ULongToPtr(process_detaching) ); call_ldr_notifications( LDR_DLL_NOTIFICATION_REASON_UNLOADED, mod );
@@ -3699,7 +3699,7 @@ void WINAPI LdrShutdownThread(void) if ( mod->Flags & LDR_NO_DLL_CALLS ) continue;
- MODULE_InitDLL( CONTAINING_RECORD(mod, WINE_MODREF, ldr), + MODULE_InitDLL( CONTAINING_RECORD(mod, WINE_MODREF, ldr), DLL_THREAD_DETACH, NULL ); }
From: Konstantin Kharlamov Hi-Angel@yandex.ru
When `import_dll` function gets `STATUS_DLL_NOT_FOUND` from `load_dll()`, it has no way of distinguishing whether it's a dependency for the dll wasn't found, or the dll itself is missing.
That results in confusing ERR messages, where a dll is found, but wine claims it isn't. Fix this by moving the print to the actual place that is aware of whether it's the parent or the child dll haven't been found.
Log before:
0024:err:module:import_dll Library lib2.dll (which is needed by L"Z:\tmp\lib1.dll") not found 0024:err:module:import_dll Library lib1.dll (which is needed by L"Z:\tmp\a.exe") not found
Log after:
0024:err:module:load_dll Library L"lib2.dll" (which is needed by L"Z:\tmp\foo\lib1.dll") not found 0024:err:module:import_dll Loading library lib2.dll (which is needed by L"Z:\tmp\foo\lib1.dll") failed (error c0000135). 0024:err:module:import_dll Loading library lib1.dll (which is needed by L"Z:\tmp\foo\a.exe") failed (error c0000135).
Signed-off-by: Konstantin Kharlamov Hi-Angel@yandex.ru --- dlls/ntdll/loader.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index ae7d0ecaa73..266be08e700 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -1066,12 +1066,8 @@ 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)); - else - ERR("Loading library %s (which is needed by %s) failed (error %x).\n", - name, debugstr_w(current_modref->ldr.FullDllName.Buffer), status); + ERR("Loading library %s (which is needed by %s) failed (error %x).\n", + name, debugstr_w(current_modref->ldr.FullDllName.Buffer), status); return FALSE; }
@@ -3134,7 +3130,19 @@ static NTSTATUS load_dll( const WCHAR *load_path, const WCHAR *libname, DWORD fl return STATUS_SUCCESS; }
- if (nts && nts != STATUS_INVALID_IMAGE_NOT_MZ) goto done; + if (nts && nts != STATUS_INVALID_IMAGE_NOT_MZ) + { + if (nts == STATUS_DLL_NOT_FOUND) + { + if (current_modref) + ERR("Library %s (which is needed by %s) not found\n", + debugstr_w(libname), + debugstr_w(current_modref->ldr.FullDllName.Buffer)); + else + ERR("Library %s not found\n", debugstr_w(libname)); + } + goto done; + }
if (NtCurrentTeb64()) {
Worth noting, with this patch I'm also getting a new message
``` 0074:err:module:load_dll Library L"winemac.drv" not found ```
But that's irrelevant problem: indeed, why would WINE try to load `winemac.drv` on Linux. The new message is on point here, so I hope that wouldn't be a blocker to this MR.
On Wed Oct 26 06:37:42 2022 +0000, Konstantin Kharlamov wrote:
Worth noting, with this patch I'm also getting a new message
0074:err:module:load_dll Library L"winemac.drv" not foundBut that's irrelevant problem: indeed, why would WINE try to load `winemac.drv` on Linux. The new message is on point here, so I hope that wouldn't be a blocker to this MR.
On the other hand, I just realized: `load_dll()` implements a WINAPI `LdrLoadDll()`, whose failure to find a dll may not necessarily be a problem *(e.g. if an app simply searches for a loadable addon or some such)*.
So I presume, I need to modify my patch a little, to only print the error when `if (current_modref)` holds *(i.e. to remove the `else` branch)*. The `current_modref` being set implies we're trying to load the lib because it's required by something else.
I'll update the patch on this evening if there's no opposition.
So I presume, I need to modify my patch a little, to only print the error when `if (current_modref)` holds *(i.e. to remove the `else` branch)*. The `current_modref` being set implies we're trying to load the lib because it's required by something else.
That won't help, it's legitimate to try to load a dll to see if it can be loaded, just like opening a non-existent file. It's also expected for it to fail if some dependency is missing, that's not necessarily a problem. Printing an error at that point is going to cause a lot of false positives.
I like the general idea of the MR, as I had the same issue as well before, but I can't tell if it's correct this way.
On Wed Oct 26 09:43:41 2022 +0000, Alexandre Julliard wrote:
So I presume, I need to modify my patch a little, to only print the error when `if (current_modref)` holds *(i.e. to remove the `else` branch)*. The `current_modref` being set implies we're trying to load the lib because it's required by something else.
That won't help, it's legitimate to try to load a dll to see if it can be loaded, just like opening a non-existent file. It's also expected for it to fail if some dependency is missing, that's not necessarily a problem. Printing an error at that point is going to cause a lot of false positives.
Hmm, thanks, I see. Welp, in this case the only possible fix is to add to `load_dll()` a new `BOOL *err_for_parent_dll` argument to specifically indicate, whether the error is for the parent or the child one.
I've spent lots of time attempting various refactorings in this code, and if we can't move the error from `import_dll` down to `load_dll`, adding a new argument is the only way to fix this AFAICS.
I've spent lots of time attempting various refactorings in this code,
More specifically, alternatives I've attempted were:
* breaking `load_dll` to two halfs — didn't work well because a bunch of similar arguments need to be passed between them. It's doable, just didn't seem pretty to me at all * replacing `STATUS_DLL_NOT_FOUND` with another error code: didn't really work because error codes from `load_dll` may return higher the stack, so… Yeah, well, it's doable as well, but isn't pretty.
So, yeah, given the circumstances, adding another argument to `load_dll()` is the most viable way AFAICS.
I'd suggest simply tweaking the error message (something like "not found or dependency missing").
On Wed Oct 26 18:50:23 2022 +0000, Alexandre Julliard wrote:
I'd suggest simply tweaking the error message (something like "not found or dependency missing").
So, I tweaked it locally to say a:
Library lib2.dll (which is needed by L"Z:\tmp\foo\lib1.dll") not found or has dependency missing
and asked `<tinwhiskers>` on IRC would that unveil the problem. They say:
That's not ideal as you'll still likely have to go on the wild goose chase to confirm the file is actually there.
Indeed, now that I think of that, such formulation doesn't clarify which dll wasn't found. I imagine that might be a problem if there's many paths where a dlls for the application may be located, so you have to check all of them for each dll.
I don't know, it seems to me, like adding an optional parameter `BOOL *err_for_parent_dll` might be better for end-users. What do you think?
On Wed Oct 26 18:50:23 2022 +0000, Konstantin Kharlamov wrote:
So, I tweaked it locally to say a:
Library lib2.dll (which is needed by L"Z:\tmp\foo\lib1.dll") not
found or has dependency missing and asked `<tinwhiskers>` on IRC would that unveil the problem. They say:
That's not ideal as you'll still likely have to go on the wild goose
chase to confirm the file is actually there. Indeed, now that I think of that, such formulation doesn't clarify which dll wasn't found. I imagine that might be a problem if there's many paths where a dlls for the application may be located, so you have to check all of them for each dll. I don't know, it seems to me, like adding an optional parameter `BOOL *err_for_parent_dll` might be better for end-users. What do you think?
Alright, well, maybe: `Library lib2.dll (which is needed by L"Z:\tmp\foo\lib1.dll") or a library it depends upon not found`? Though a bit longer than a `…or has dependency missing`, but a "missing dependency" sounds a bit too abstract. I see no better wording.
On Thu Oct 27 23:40:26 2022 +0000, Konstantin Kharlamov wrote:
Alright, well, maybe: `Library lib2.dll (which is needed by L"Z:\tmp\foo\lib1.dll") or a library it depends upon not found`? Though a bit longer than a `…or has dependency missing`, but a "missing dependency" sounds a bit too abstract. I see no better wording.
Ping. Which way should I go with? I'd personally prefer to have more explicit message at the cost of excess argument, but if maintainers prefer a vague message instead but no additional parameter, I will implement that. Probably a text like in my prev. comment.
Ping. Any news? Would be useful to have some version of this merged before feature freeze, so the next version to be supported for an year would have less confusing error messages.
-
Alexandre Julliard (＠julliard) -
Bernhard Kölbl (＠besentv)
-
Konstantin Kharlamov
-
Konstantin Kharlamov (＠Hi-Angel)