Print error message when LoadLibrary fails

[cvuchener]

Unlike the posix version, the windows version of OpenPlugin does not print an error message when LoadLibrary fails. This makes it harder to diagnose some user issues (e.g. http://www.bay12forums.com/smf/index.php?topic=138754.msg8227430#msg8227430).

[lethosor]

It's somewhat more complicated on Windows, but I came up with some similar logic for this in another project: https://github.com/lethosor/libdylib/blob/ca9039f41549f0897a4a7c2e154588744936546e/libdylib.c#L123-L137

I don't know if this has ever been properly tested on Windows - are you able to build DFHack on Windows? If not, it could be possible to test a corrupted (e.g. truncated) plugin with the Windows binaries built with our CI.

[cvuchener]

diff --git a/library/PlugLoad-windows.cpp b/library/PlugLoad-windows.cpp
index 8d64e315..2f7632cd 100644
--- a/library/PlugLoad-windows.cpp
+++ b/library/PlugLoad-windows.cpp
@@ -44,7 +44,16 @@ namespace DFHack
     DFLibrary* GLOBAL_NAMES = (DFLibrary*)GetModuleHandle(nullptr);
     DFLibrary * OpenPlugin (const char * filename)
     {
-        return (DFLibrary *) LoadLibrary(filename);
+        DFLibrary *ret = (DFLibrary *) LoadLibrary(filename);
+        if (!ret) {
+            auto error = GetLastError();
+            LPSTR buf = NULL;
+            FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
+                           NULL, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (LPSTR)&buf, 0, NULL);
+            std::cerr << "LoadLibrary " << filename << ": "  << buf << std::endl;
+            LocalFree(buf);
+        }
+       return ret;
     }
     void * LookupPlugin (DFLibrary * plugin ,const char * function)
     {
@@ -54,4 +63,4 @@ namespace DFHack
     {
         FreeLibrary((HMODULE) plugin);
     }
-}
\ No newline at end of file
+}

In stderr.log:

loading plugin 32bit
LoadLibrary E:\Games\df\df_47_04_win64\hack\plugins/32bit.plug.dll: %1 n’est pas une application Win32 valide.

Can't load plugin 32bit
[...]
loading plugin truncated
LoadLibrary E:\Games\df\df_47_04_win64\hack\plugins/truncated.plug.dll: %1 n’est pas une application Win32 valide.

Can't load plugin truncated

The error message are not very nice, there is the %1 I don't know how to handle since removing FORMAT_MESSAGE_IGNORE_INSERTS is not recommended

In particular, it is unsafe to take an arbitrary system error code returned from an API and use FORMAT_MESSAGE_FROM_SYSTEM without FORMAT_MESSAGE_IGNORE_INSERTS.

and there is also the issue with the extra line return.

In dwarf therapist, I use 0 instead of MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), but it uses the current locale too. Is there something equivalent to the "C" locale for posix?

[lethosor]

Yeah, it would be really nice if we could get different messages for the two cases. For reference, on Linux:

loading plugin 32bit
.../hack/plugins/32bit.plug.so: wrong ELF class: ELFCLASS32
Can't load plugin 32bit

loading plugin truncated
.../hack/plugins/truncated.plug.so: file too short
Can't load plugin truncated

These also depend on the system language (e.g. LANG=es_ES.UTF-8 for Spanish results in clase ELF errónea: ELFCLASS32), so I'm not concerned about that (or the extra newline), but I would like to have some distinction between different types of errors. Granted, truncated plugins aren't common.

The only other likely potential issue I can think of is a plugin that attempts to link to a DFHack symbol that isn't actually present in the DFHack library. For instance, building and installing just DFHack (no plugins) with the following patch:

diff --git a/library/include/modules/Kitchen.h b/library/include/modules/Kitchen.h
index 1ddf239b..05c0a6d0 100644
--- a/library/include/modules/Kitchen.h
+++ b/library/include/modules/Kitchen.h
@@ -62,7 +62,7 @@ DFHACK_EXPORT void debug_print(color_ostream &out);
 DFHACK_EXPORT void allowPlantSeedCookery(t_materialIndex materialIndex);

 // add this material to the exclusion list, if it is not already in it
-DFHACK_EXPORT void denyPlantSeedCookery(t_materialIndex materialIndex);
+void denyPlantSeedCookery(t_materialIndex materialIndex);

 // fills a map with info from the limit info storage entries in the exclusion list
 DFHACK_EXPORT void fillWatchMap(std::map<t_materialIndex, unsigned int>& watchMap);

will prevent the seedwatch plugin from loading with:

hack/plugins/seedwatch.plug.so: undefined symbol: _ZN6DFHack7Kitchen20denyPlantSeedCookeryEi

I'm not sure how hard this is to reproduce on Windows (I think you could just build the "DFHack" or "SDL.dll" target and then copy the resulting SDL.dll into the DF folder manually). If it produces a different error than what you posted earlier, I think your patch would be a good improvement as-is.

[cvuchener]

LoadLibrary E:\Games\df\df_47_04_win64\hack\plugins/seedwatch.plug.dll: La procédure spécifiée est introuvable.

ERROR_PROC_NOT_FOUND instead of ERROR_BAD_EXE_FORMAT, so it is useful. And no %1 this time.

I'll try trimming the CR/LF at the end of the error message before sending a PR.

[lethosor]

Sounds good, thanks! Maybe printing the error code would also be useful? (It would be consistent across languages, at least.)