[PATCH v2 0/1] plugins: Move the windows linking function to qemu

[PATCH v2 0/1] plugins: Move the windows linking function to qemu

[Greg Manning]

v1-v2: Added signed-off-by line.

Greg Manning (1):
  plugins: Move the windows linking function to qemu

 contrib/plugins/win32_linker.c | 23 +++--------------------
 include/sysemu/os-win32.h      | 25 +++++++++++++++++++++++++
 os-win32.c                     | 33 +++++++++++++++++++++++++++++++++
 plugins/loader.c               |  3 +++
 4 files changed, 64 insertions(+), 20 deletions(-)

-- 
2.42.0.windows.1

[PATCH v2 1/1] plugins: Move the windows linking function to qemu

[Greg Manning]

Previously, a plugin author needed an implementation of the
__pfnDliFailureHook2 or __pfnDliNotifyHook2 hook in the plugin. Now all
they need is a null exported pointer with the right name (as in
win32_linker.c). If QEMU finds this, it will set it to the hook
function, which has now moved into qemu (os-win32.c).

Signed-off-by: Greg Manning <gmanning@rapitasystems.com>
---
 contrib/plugins/win32_linker.c | 23 +++--------------------
 include/sysemu/os-win32.h      | 25 +++++++++++++++++++++++++
 os-win32.c                     | 33 +++++++++++++++++++++++++++++++++
 plugins/loader.c               |  3 +++
 4 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/contrib/plugins/win32_linker.c b/contrib/plugins/win32_linker.c
index 7534b2b8bf..619fdd38c8 100644
--- a/contrib/plugins/win32_linker.c
+++ b/contrib/plugins/win32_linker.c
@@ -4,8 +4,8 @@
  * This hook, __pfnDliFailureHook2, is documented in the microsoft documentation here:
  * https://learn.microsoft.com/en-us/cpp/build/reference/error-handling-and-notification
  * It gets called when a delay-loaded DLL encounters various errors.
- * We handle the specific case of a DLL looking for a "qemu.exe",
- * and give it the running executable (regardless of what it is named).
+ * QEMU will set it to a function which handles the specific case of a DLL looking for
+ * a "qemu.exe", and give it the running executable (regardless of what it is named).
  *
  * This work is licensed under the terms of the GNU LGPL, version 2 or later.
  * See the COPYING.LIB file in the top-level directory.
@@ -14,21 +14,4 @@
 #include <windows.h>
 #include <delayimp.h>
 
-FARPROC WINAPI dll_failure_hook(unsigned dliNotify, PDelayLoadInfo pdli);
-
-
-PfnDliHook __pfnDliFailureHook2 = dll_failure_hook;
-
-FARPROC WINAPI dll_failure_hook(unsigned dliNotify, PDelayLoadInfo pdli) {
-    if (dliNotify == dliFailLoadLib) {
-        /* If the failing request was for qemu.exe, ... */
-        if (strcmp(pdli->szDll, "qemu.exe") == 0) {
-            /* Then pass back a pointer to the top level module. */
-            HMODULE top = GetModuleHandle(NULL);
-            return (FARPROC) top;
-        }
-    }
-    /* Otherwise we can't do anything special. */
-    return 0;
-}
-
+__declspec(dllexport) PfnDliHook __pfnDliNotifyHook2 = NULL;
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 1047d260cb..0f698554b2 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -30,6 +30,8 @@
 #include <windows.h>
 #include <ws2tcpip.h>
 #include "qemu/typedefs.h"
+#include <delayimp.h>
+#include <gmodule.h>
 
 #ifdef HAVE_AFUNIX_H
 #include <afunix.h>
@@ -265,6 +267,29 @@ win32_close_exception_handler(struct _EXCEPTION_RECORD*, void*,
 void *qemu_win32_map_alloc(size_t size, HANDLE *h, Error **errp);
 void qemu_win32_map_free(void *ptr, HANDLE h, Error **errp);
 
+
+/* dll_delaylink_hook:
+ * @dliNotify: Type of event that caused this callback.
+ * @pdli: Extra data about the DLL being loaded
+ *
+ * For more info on the arguments and when this gets invoked see
+ * https://learn.microsoft.com/en-us/cpp/build/reference/understanding-the-helper-function
+ *
+ * This is to be used on windows as the target of a __pfnDliNotifyHook2 or __pfnDliFailureHook2
+ * hook. If the DLL being loaded is 'qemu.exe', it instead passes back a pointer to the main
+ * executable This gets set into plugins to assist with the plugins delay-linking back to the main
+ * executable, if they define an appropriate symbol. */
+FARPROC WINAPI dll_delaylink_hook(unsigned dliNotify, PDelayLoadInfo pdli);
+
+/* set_dll_delaylink_hook:
+ * @mod: pointer to the DLL being loaded
+ *
+ * takes a pointer to a loaded plugin DLL, and tries to find a __pfnDliNotifyHook2 or
+ * __pfnDliFailureHook2 hook. If it finds either one, and its value is null, it sets it to the
+ * address fo dll_delaylink_hook.
+ */
+void set_dll_delaylink_hook(GModule *mod);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/os-win32.c b/os-win32.c
index 725ad652e8..4a113d1d10 100644
--- a/os-win32.c
+++ b/os-win32.c
@@ -60,3 +60,36 @@ void os_set_line_buffering(void)
     setbuf(stdout, NULL);
     setbuf(stderr, NULL);
 }
+
+FARPROC WINAPI dll_delaylink_hook(unsigned dliNotify, PDelayLoadInfo pdli) {
+    /* If we just tried, or are about to try to load a DLL ... */
+    if (dliNotify == dliFailLoadLib || dliNotify == dliNotePreLoadLibrary) {
+        /* ... if the failing request was for qemu.exe, ... */
+        if (strcmp(pdli->szDll, "qemu.exe") == 0) {
+            /* ... then pass back a pointer to the top level module. */
+            HMODULE top = GetModuleHandle(NULL);
+            return (FARPROC) top;
+        }
+    }
+    /* Otherwise we can't do anything special. */
+    return 0;
+}
+void set_dll_delaylink_hook(GModule *mod) {
+    /* find the __pfnDliFailureHook2 symbol in the DLL.
+     * if found, set it to our handler.
+     */
+    gpointer sym;
+    PfnDliHook *hook;
+    if (g_module_symbol(mod, "__pfnDliFailureHook2", &sym)) {
+        hook = (PfnDliHook*) sym;
+        if (hook != NULL && *hook == NULL) {
+            *hook = &dll_delaylink_hook;
+        }
+    }
+    if (g_module_symbol(mod, "__pfnDliNotifyHook2", &sym)) {
+        hook = (PfnDliHook*) sym;
+        if (hook != NULL && *hook == NULL) {
+            *hook = &dll_delaylink_hook;
+        }
+    }
+}
diff --git a/plugins/loader.c b/plugins/loader.c
index 734c11cae0..7ead9b26e4 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -241,6 +241,9 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info, E
     }
     QTAILQ_INSERT_TAIL(&plugin.ctxs, ctx, entry);
     ctx->installing = true;
+    #ifdef CONFIG_WIN32
+        set_dll_delaylink_hook(ctx->handle);
+    #endif
     rc = install(ctx->id, info, desc->argc, desc->argv);
     ctx->installing = false;
     if (rc) {
-- 
2.42.0.windows.1

Re: [PATCH v2 1/1] plugins: Move the windows linking function to qemu

[Paolo Bonzini]

On 11/9/23 10:25, Greg Manning wrote:
> Previously, a plugin author needed an implementation of the
> __pfnDliFailureHook2 or __pfnDliNotifyHook2 hook in the plugin. Now all
> they need is a null exported pointer with the right name (as in
> win32_linker.c). If QEMU finds this, it will set it to the hook
> function, which has now moved into qemu (os-win32.c).
> 
> Signed-off-by: Greg Manning <gmanning@rapitasystems.com>
> ---

Thanks for trying! :)

I'm not sure how big an improvement this is, but I'll let Alex judge.

The main issue I had with win32_linker.c is the additional complexity 
for external plugins, which is now decreased but to some extent remains. 
  I should have made that clearer.  One possibility is to put the 
definition in a macro, and have the plugin sources simply expand the macro.

Paolo

Re: [PATCH v2 1/1] plugins: Move the windows linking function to qemu

[Greg Manning]

> Previously, a plugin author needed an implementation of the
> __pfnDliFailureHook2 or __pfnDliNotifyHook2 hook in the plugin. Now all
> they need is a null exported pointer with the right name (as in
> win32_linker.c). If QEMU finds this, it will set it to the hook
> function, which has now moved into qemu (os-win32.c).

I have a new idea for this. We've made the qemu_plugin_api.lib file which 
is a delaylib with all the symbol names of all the api functions, so windows
can do the whole delay-linking thing. We could also put into that archive
the object win32_linker.o:

ar -q qemu_plugin.api.lib ../whatever/win32_linker.o

Then hopefully when a plugin links to this, it gets the __pfnDliFailureHook2
symbol defined and set up and everything would work. Except gcc strips
out any unreferenced symbols from static libs when linking. So the plugin
would have to be linked thusly:

gcc -shared -o my_plugin.dll -Wl,-u,__pfnDliFailureHook2 my_plugin.o qemu_plugin_api.lib

But no other qemu-fiddling-with-things or extra code in plugins required.

Hmm. Feels like half a solution. I wonder if there's a way to mark symbols as 
"always required despite what dead code analysis says".

Greg.

Re: [PATCH v2 1/1] plugins: Move the windows linking function to qemu

[Paolo Bonzini]

On Fri, Nov 10, 2023 at 6:36 PM Greg Manning <gmanning@rapitasystems.com> wrote:
> Then hopefully when a plugin links to this, it gets the __pfnDliFailureHook2
> symbol defined and set up and everything would work. Except gcc strips
> out any unreferenced symbols from static libs when linking. So the plugin
> would have to be linked thusly:
>
> gcc -shared -o my_plugin.dll -Wl,-u,__pfnDliFailureHook2 my_plugin.o qemu_plugin_api.lib
>
> But no other qemu-fiddling-with-things or extra code in plugins required.
>
> Hmm. Feels like half a solution. I wonder if there's a way to mark symbols as
> "always required despite what dead code analysis says".

To be clear, I don't dislike at all the simpler solution where you
just add a macro like this:

#ifdef _WIN32
#define QEMU_PLUGIN_HOOK \
   /* contents of win32_linker.c */
#else
#define QEMU_PLUGIN_HOOK
#endif

and add QEMU_PLUGIN_HOOK to a source file of every plugin. But if you
would like to use a library, you can pass a linker script on the
command line as if it was a library, and the paths within the linker
script are resolved relative to the linker script itself. So you can
place in tests/plugins/meson.build something like:

# uses dlltool like it does now... can also use custom_target
delaylib = configure_file(output: 'qemu_plugin_api.lib',
   ...)
delayhook = static_library('qemu_delay_hook', sources: 'qemu_delay_hook.c')
plugin_api = configure_file(output : 'libqemu_plugin_api.a', input:
'libqemu_plugin_api.ld', copy: true, install_dir:
get_option('libdir'))

where the last configure_file creates a file with contents such as

INPUT(qemu_plugin_api.lib) # from dlltool
INPUT(libqemu_delay_hook.a) # compiled from qemu_delay_hook.c
EXTERN(__pfnDliNotifyHook2)  # equivalent to -Wl,-u

And then it should work to add link_depends: [delayhook, delaylib,
plugin_api], link_args: plugin_api as in your previous version.

Finally, since the hook will be built by ninja, you also need

diff --git a/Makefile b/Makefile
index 676a4a54f48..7b42d85f1dc 100644
--- a/Makefile
+++ b/Makefile
@@ -184,6 +184,7 @@ $(SUBDIR_RULES):
 ifneq ($(filter contrib/plugins, $(SUBDIRS)),)
 .PHONY: plugins
 plugins: contrib/plugins/all
+contrib/plugins/all: | run-ninja
 endif

 .PHONY: recurse-all recurse-clean

to ensure that the plugins are built after libqemu_delay_hook.a (of
course feel free to change the file names!).

The main disadvantage is that the Microsoft linker does not know
linker scripts, so that's a point in favor of the macro solution.

Paolo