Re: [PATCH v3 6/6] plugin: Getting qemu-plugin works under win32.

["Alex Bennée" <alex.bennee@linaro.org>]

Yonggang Luo <luoyonggang@gmail.com> writes:

> We removed the need of .symbols file, so is the
> configure script, if we one expose a function to qemu-plugin
> just need prefix the function with QEMU_PLUGIN_EXPORT
>
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> ---
>  Makefile                     |   1 -
>  configure                    |  71 -------------
>  contrib/plugins/hotblocks.c  |   1 +
>  contrib/plugins/hotpages.c   |   1 +
>  contrib/plugins/howvec.c     |   1 +
>  contrib/plugins/lockstep.c   |   1 +
>  include/qemu/qemu-plugin.h   | 197 +++++++++++++++++++++++++++--------
>  meson.build                  |   6 +-
>  plugins/api.c                |  62 +++++------
>  plugins/core.c               |  10 +-
>  plugins/loader.c             |  50 ++++++++-
>  plugins/meson.build          |  10 +-
>  plugins/plugin.h             |   1 +
>  plugins/qemu-plugins.symbols |  40 -------
>  tests/plugin/bb.c            |   1 +
>  tests/plugin/empty.c         |   1 +
>  tests/plugin/insn.c          |   1 +
>  tests/plugin/mem.c           |   1 +
>  18 files changed, 251 insertions(+), 205 deletions(-)
>  delete mode 100644 plugins/qemu-plugins.symbols
>
> diff --git a/Makefile b/Makefile
> index 54fc1a9d10..9981dd5209 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -105,7 +105,6 @@ config-host.mak: $(SRC_PATH)/configure $(SRC_PATH)/pc-bios $(SRC_PATH)/VERSION
>  
>  # Force configure to re-run if the API symbols are updated
>  ifeq ($(CONFIG_PLUGIN),y)
> -config-host.mak: $(SRC_PATH)/plugins/qemu-plugins.symbols
>  
>  .PHONY: plugins
>  plugins:
> diff --git a/configure b/configure
> index 1c21a73c3b..ea447919fc 100755
> --- a/configure
> +++ b/configure
> @@ -5435,61 +5435,6 @@ if compile_prog "" "" ; then
>    atomic64=yes
>  fi
>  
> -#########################################
> -# See if --dynamic-list is supported by the linker
> -ld_dynamic_list="no"
> -if test "$static" = "no" ; then
> -    cat > $TMPTXT <<EOF
> -{
> -  foo;
> -};
> -EOF
> -
> -    cat > $TMPC <<EOF
> -#include <stdio.h>
> -void foo(void);
> -
> -void foo(void)
> -{
> -  printf("foo\n");
> -}
> -
> -int main(void)
> -{
> -  foo();
> -  return 0;
> -}
> -EOF
> -
> -    if compile_prog "" "-Wl,--dynamic-list=$TMPTXT" ; then
> -        ld_dynamic_list="yes"
> -    fi
> -fi
> -
> -#########################################
> -# See if -exported_symbols_list is supported by the linker
> -
> -ld_exported_symbols_list="no"
> -if test "$static" = "no" ; then
> -    cat > $TMPTXT <<EOF
> -  _foo
> -EOF
> -
> -    if compile_prog "" "-Wl,-exported_symbols_list,$TMPTXT" ; then
> -        ld_exported_symbols_list="yes"
> -    fi
> -fi
> -
> -if  test "$plugins" = "yes" &&
> -    test "$ld_dynamic_list" = "no" &&
> -    test "$ld_exported_symbols_list" = "no" ; then
> -  error_exit \
> -      "Plugin support requires dynamic linking and specifying a set of symbols " \
> -      "that are exported to plugins. Unfortunately your linker doesn't " \
> -      "support the flag (--dynamic-list or -exported_symbols_list) used " \
> -      "for this purpose. You can't build with --static."
> -fi
> -
>  ########################################
>  # See if __attribute__((alias)) is supported.
>  # This false for Xcode 9, but has been remedied for Xcode 10.
> @@ -7074,22 +7019,6 @@ fi
>  
>  if test "$plugins" = "yes" ; then
>      echo "CONFIG_PLUGIN=y" >> $config_host_mak
> -    # Copy the export object list to the build dir
> -    if test "$ld_dynamic_list" = "yes" ; then
> -	echo "CONFIG_HAS_LD_DYNAMIC_LIST=yes" >> $config_host_mak
> -	ld_symbols=qemu-plugins-ld.symbols
> -	cp "$source_path/plugins/qemu-plugins.symbols" $ld_symbols
> -    elif test "$ld_exported_symbols_list" = "yes" ; then
> -	echo "CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST=yes" >> $config_host_mak
> -	ld64_symbols=qemu-plugins-ld64.symbols
> -	echo "# Automatically generated by configure - do not modify" > $ld64_symbols
> -	grep 'qemu_' "$source_path/plugins/qemu-plugins.symbols" | sed 's/;//g' | \
> -	    sed -E 's/^[[:space:]]*(.*)/_\1/' >> $ld64_symbols
> -    else
> -	error_exit \
> -	    "If \$plugins=yes, either \$ld_dynamic_list or " \
> -	    "\$ld_exported_symbols_list should have been set to 'yes'."
> -    fi
>  fi
>  
>  if test -n "$gdb_bin" ; then
> diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
> index 37435a3fc7..39e77d2980 100644
> --- a/contrib/plugins/hotblocks.c
> +++ b/contrib/plugins/hotblocks.c
> @@ -13,6 +13,7 @@
>  #include <stdio.h>
>  #include <glib.h>
>  
> +#define QEMU_PLUGIN_IMPLEMENTATION
>  #include <qemu-plugin.h>

As mentioned in earlier patch we should be able to just have the tweak
in api.c and avoid touching all the plugins themselves.
>  
> -#define QEMU_PLUGIN_VERSION 0
> +#define QEMU_PLUGIN_VERSION 1
> +
> +typedef void *(*qemu_plugin_global_dlsym_t)(void* context, const char *name);
>  
>  typedef struct {
>      /* string describing architecture */
> @@ -73,8 +71,23 @@ typedef struct {
>              int max_vcpus;
>          } system;
>      };
> +    void *context;
> +    qemu_plugin_global_dlsym_t dlsym;
>  } qemu_info_t;
>  
> +/**
> + * qemu_plugin_initialize() - Initialize a plugin before install
> + * @info: a block describing some details about the guest
> + *
> + * All plugins must export this symbol, and in most case using qemu-plugin.h
> + * provided implementation directly.
> + * For plugin provide this function, the QEMU_PLUGIN_VERSION should >= 1
> + *
> + * Note: This function only used to loading qemu's exported functions, nothing
> + * else should doding in this function.
> + */
> +QEMU_PLUGIN_EXPORT int qemu_plugin_initialize(const qemu_info_t *info);
> +

So this is essentially working around the linker/dlopen stage and
manually linking in all the API functions? Does this affect the
efficiency of the API calls?
> -void qemu_plugin_outs(const char *string);
> +typedef void (*qemu_plugin_outs_t)(const char *string);
> +
> +#if !defined(QEMU_PLUGIN_API_IMPLEMENTATION)
> +#if defined(QEMU_PLUGIN_IMPLEMENTATION)
> +#define QEMU_PLUGIN_EXTERN
> +#else
> +#define QEMU_PLUGIN_EXTERN extern
> +#endif

As mentioned in the earlier patch I want to understand why the extern is
required. Could we avoid it with a parameter to the compiler when
building plugins?

<snip>
>  
>  static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
>  {
> +    qemu_plugin_initialize_func_t initialize = NULL;
>      qemu_plugin_install_func_t install;
>      struct qemu_plugin_ctx *ctx;
>      gpointer sym;
>      int rc;
> +    int version = -1;
>  
>      ctx = qemu_memalign(qemu_dcache_linesize, sizeof(*ctx));
>      memset(ctx, 0, sizeof(*ctx));
> @@ -184,7 +187,7 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
>                       desc->path, g_module_error());
>          goto err_symbol;
>      } else {
> -        int version = *(int *)sym;
> +        version = *(int *)sym;
>          if (version < QEMU_PLUGIN_MIN_VERSION) {
>              error_report("TCG plugin %s requires API version %d, but "
>                           "this QEMU supports only a minimum version of %d",
> @@ -198,6 +201,21 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
>          }
>      }
>  
> +    if (version >= QEMU_PLUGIN_VERSION_1) {
> +        /* This version should call to qemu_plugin_initialize first */
> +        if (!g_module_symbol(ctx->handle, "qemu_plugin_initialize", &sym)) {
> +            error_report("%s: %s", __func__, g_module_error());
> +            goto err_symbol;
> +        }
> +        initialize = (qemu_plugin_initialize_func_t) sym;
> +        /* symbol was found; it could be NULL though */
> +        if (initialize == NULL) {
> +            error_report("%s: %s: qemu_plugin_initialize is NULL",
> +                        __func__, desc->path);
> +            goto err_symbol;
> +        }
> +    }
> +
>      qemu_rec_mutex_lock(&plugin.lock);
>  
>      /* find an unused random id with &ctx as the seed */
> @@ -216,6 +234,16 @@ static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
>          }
>      }
>      QTAILQ_INSERT_TAIL(&plugin.ctxs, ctx, entry);
> +    if (initialize != NULL) {
> +        rc = initialize(info);
> +        if (rc) {
> +            error_report("%s: qemu_plugin_initialize returned error code %d",
> +                        __func__, rc);
> +            /* qemu_plugin_initialize only loading function symbols */
> +            goto err_symbol;
> +        }
> +    }
> +
>      ctx->installing = true;
>      rc = install(ctx->id, info, desc->argc, desc->argv);
>      ctx->installing = false;
> @@ -254,6 +282,17 @@ static void plugin_desc_free(struct qemu_plugin_desc *desc)
>      g_free(desc);
>  }
>  
> +static void *qemu_plugin_global_dlsym(void* context, const char *name)
> +{
> +    GModule *global_handle = context;
> +    gpointer sym = NULL;
> +    if (!g_module_symbol(global_handle, name, &sym)) {
> +        error_report("%s: %s", __func__, g_module_error());
> +        return NULL;
> +    }
> +    return sym;
> +}
> +
>  /**
>   * qemu_plugin_load_list - load a list of plugins
>   * @head: head of the list of descriptors of the plugins to be loaded
> @@ -267,6 +306,7 @@ int qemu_plugin_load_list(QemuPluginList *head)
>  {
>      struct qemu_plugin_desc *desc, *next;
>      g_autofree qemu_info_t *info = g_new0(qemu_info_t, 1);
> +    GModule *global_handle = NULL;
>  
>      info->target_name = TARGET_NAME;
>      info->version.min = QEMU_PLUGIN_MIN_VERSION;
> @@ -276,6 +316,12 @@ int qemu_plugin_load_list(QemuPluginList *head)
>      info->system_emulation = true;
>      info->system.smp_vcpus = ms->smp.cpus;
>      info->system.max_vcpus = ms->smp.max_cpus;
> +    global_handle = g_module_open(NULL, G_MODULE_BIND_LOCAL);
> +    if (global_handle == NULL) {
> +        goto err_dlopen;
> +    }
> +    info->dlsym = qemu_plugin_global_dlsym;
> +    info->context = (void*)global_handle;
>  #else
>      info->system_emulation = false;
>  #endif
> @@ -289,6 +335,8 @@ int qemu_plugin_load_list(QemuPluginList *head)
>          }
>          QTAILQ_REMOVE(head, desc, entry);
>      }
> +
> +err_dlopen:
>      return 0;

This doesn't compile cleanly for both linux-user and softmmu:

  Compiling C object libqemu-aarch64-linux-user.fa.p/tcg_tcg-common.c.o
  ../../plugins/loader.c: In function ‘qemu_plugin_load_list’:
  ../../plugins/loader.c:339:1: error: label ‘err_dlopen’ defined but not used [-Werror=unused-label]
   err_dlopen:
   ^~~~~~~~~~
  ../../plugins/loader.c:309:14: error: unused variable ‘global_handle’ [-Werror=unused-variable]
       GModule *global_handle = NULL;
                ^~~~~~~~~~~~~
  At top level:
  ../../plugins/loader.c:285:14: error: ‘qemu_plugin_global_dlsym’ defined but not used [-Werror=unused-function]
   static void *qemu_plugin_global_dlsym(void* context, const char *name)
                ^~~~~~~~~~~~~~~~~~~~~~~~
  cc1: all warnings being treated as errors
  make: *** [Makefile.ninja:6703: libqemu-aarch64-linux-user.fa.p/plugins_loader.c.o] Error 1
  make: *** Waiting for unfinished jobs....

-- 
Alex Bennée