diff mbox series

[RFC,41/48] configure: add --enable-plugins

Message ID 20181025172057.20414-42-cota@braap.org
State New
Headers show
Series Plugin support | expand

Commit Message

Emilio Cota Oct. 25, 2018, 5:20 p.m. UTC
For now only add it for ELF platforms, since we rely on the linker's
--dynamic-list flag to pass a list of symbols to be exported to the
executable. An alternative would be to use -rdynamic, but that would
expose all of QEMU's objects to plugins.

I have no experience with non-ELF systems but I suspect adding support
for those should be pretty easy.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 configure | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

Comments

Alex Bennée Nov. 27, 2018, 12:11 p.m. UTC | #1
Emilio G. Cota <cota@braap.org> writes:

> For now only add it for ELF platforms, since we rely on the linker's
> --dynamic-list flag to pass a list of symbols to be exported to the
> executable. An alternative would be to use -rdynamic, but that would
> expose all of QEMU's objects to plugins.
>
> I have no experience with non-ELF systems but I suspect adding support
> for those should be pretty easy.
>
> Signed-off-by: Emilio G. Cota <cota@braap.org>

As far as the configure logic is concerned:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

I'm not quite following what is so special about the dynamic-list
symbols compared to the rest of the symbols in the binary. when I do
readelf -s they are all specified as GLOBAL DEFAULT.

Perhaps this will become clearer to me once I get to the implementation
of the plugins later in the series?


> ---
>  configure | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/configure b/configure
> index 03bf719ca7..78e86098e5 100755
> --- a/configure
> +++ b/configure
> @@ -30,6 +30,7 @@ TMPO="${TMPDIR1}/${TMPB}.o"
>  TMPCXX="${TMPDIR1}/${TMPB}.cxx"
>  TMPE="${TMPDIR1}/${TMPB}.exe"
>  TMPMO="${TMPDIR1}/${TMPB}.mo"
> +TMPTXT="${TMPDIR1}/${TMPB}.txt"
>
>  rm -f config.log
>
> @@ -477,6 +478,7 @@ libxml2=""
>  docker="no"
>  debug_mutex="no"
>  libpmem=""
> +plugins="no"
>
>  # cross compilers defaults, can be overridden with --cross-cc-ARCH
>  cross_cc_aarch64="aarch64-linux-gnu-gcc"
> @@ -1443,6 +1445,10 @@ for opt do
>    ;;
>    --disable-libpmem) libpmem=no
>    ;;
> +  --enable-plugins) plugins="yes"
> +  ;;
> +  --disable-plugins) plugins="no"
> +  ;;
>    *)
>        echo "ERROR: unknown option $opt"
>        echo "Try '$0 --help' for more information"
> @@ -1633,6 +1639,8 @@ Advanced options (experts only):
>                             xen pv domain builder
>    --enable-debug-stack-usage
>                             track the maximum stack usage of stacks created by qemu_alloc_stack
> +  --enable-plugins
> +                           enable plugins via shared library loading
>
>  Optional features, enabled with --enable-FEATURE and
>  disabled with --disable-FEATURE, default is enabled if available:
> @@ -5204,6 +5212,42 @@ if compile_prog "" "" ; then
>    atomic64=yes
>  fi
>
> +#########################################
> +# See if --dynamic-list is supported by the linker
> +
> +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"
> +else
> +  if test "$plugins" = "yes" ; then
> +    error_exit \
> +        "Plugin support requires specifying a set of symbols that " \
> +        "are exported to plugins. Unfortunately your linker doesn't " \
> +        "support the flag (--dynamic-list) used for this purpose."
> +  fi
> +fi
> +
>  ########################################
>  # See if 16-byte vector operations are supported.
>  # Even without a vector unit the compiler may expand these.
> @@ -6091,6 +6135,7 @@ echo "VxHS block device $vxhs"
>  echo "capstone          $capstone"
>  echo "docker            $docker"
>  echo "libpmem support   $libpmem"
> +echo "plugin support    $plugins"
>
>  if test "$sdl_too_old" = "yes"; then
>  echo "-> Your SDL version is too old - please upgrade to have SDL support"
> @@ -6849,6 +6894,12 @@ if test "$libpmem" = "yes" ; then
>    echo "CONFIG_LIBPMEM=y" >> $config_host_mak
>  fi
>
> +if test "$plugins" = "yes" ; then
> +    echo "CONFIG_PLUGINS=y" >> $config_host_mak
> +    LIBS="-ldl $LIBS"
> +    LDFLAGS="-Wl,--dynamic-list=\$(SRC_PATH)/qemu-plugins.symbols $LDFLAGS"
> +fi
> +
>  if test "$tcg_interpreter" = "yes"; then
>    QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
>  elif test "$ARCH" = "sparc64" ; then


--
Alex Bennée
Roman Bolshakov Nov. 27, 2018, 12:43 p.m. UTC | #2
On Thu, Oct 25, 2018 at 01:20:50PM -0400, Emilio G. Cota wrote:
> For now only add it for ELF platforms, since we rely on the linker's
> --dynamic-list flag to pass a list of symbols to be exported to the
> executable. An alternative would be to use -rdynamic, but that would
> expose all of QEMU's objects to plugins.
> 
> I have no experience with non-ELF systems but I suspect adding support
> for those should be pretty easy.
> 
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  configure | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/configure b/configure
> index 03bf719ca7..78e86098e5 100755
> --- a/configure
> +++ b/configure
> @@ -30,6 +30,7 @@ TMPO="${TMPDIR1}/${TMPB}.o"
>  TMPCXX="${TMPDIR1}/${TMPB}.cxx"
>  TMPE="${TMPDIR1}/${TMPB}.exe"
>  TMPMO="${TMPDIR1}/${TMPB}.mo"
> +TMPTXT="${TMPDIR1}/${TMPB}.txt"
>  
>  rm -f config.log
>  
> @@ -477,6 +478,7 @@ libxml2=""
>  docker="no"
>  debug_mutex="no"
>  libpmem=""
> +plugins="no"
>  
>  # cross compilers defaults, can be overridden with --cross-cc-ARCH
>  cross_cc_aarch64="aarch64-linux-gnu-gcc"
> @@ -1443,6 +1445,10 @@ for opt do
>    ;;
>    --disable-libpmem) libpmem=no
>    ;;
> +  --enable-plugins) plugins="yes"
> +  ;;
> +  --disable-plugins) plugins="no"
> +  ;;
>    *)
>        echo "ERROR: unknown option $opt"
>        echo "Try '$0 --help' for more information"
> @@ -1633,6 +1639,8 @@ Advanced options (experts only):
>                             xen pv domain builder
>    --enable-debug-stack-usage
>                             track the maximum stack usage of stacks created by qemu_alloc_stack
> +  --enable-plugins
> +                           enable plugins via shared library loading
>  
>  Optional features, enabled with --enable-FEATURE and
>  disabled with --disable-FEATURE, default is enabled if available:
> @@ -5204,6 +5212,42 @@ if compile_prog "" "" ; then
>    atomic64=yes
>  fi
>  
> +#########################################
> +# See if --dynamic-list is supported by the linker
> +
> +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"
> +else
> +  if test "$plugins" = "yes" ; then
> +    error_exit \
> +        "Plugin support requires specifying a set of symbols that " \
> +        "are exported to plugins. Unfortunately your linker doesn't " \
> +        "support the flag (--dynamic-list) used for this purpose."
> +  fi
> +fi
> +
>  ########################################
>  # See if 16-byte vector operations are supported.
>  # Even without a vector unit the compiler may expand these.
> @@ -6091,6 +6135,7 @@ echo "VxHS block device $vxhs"
>  echo "capstone          $capstone"
>  echo "docker            $docker"
>  echo "libpmem support   $libpmem"
> +echo "plugin support    $plugins"
>  
>  if test "$sdl_too_old" = "yes"; then
>  echo "-> Your SDL version is too old - please upgrade to have SDL support"
> @@ -6849,6 +6894,12 @@ if test "$libpmem" = "yes" ; then
>    echo "CONFIG_LIBPMEM=y" >> $config_host_mak
>  fi
>  
> +if test "$plugins" = "yes" ; then
> +    echo "CONFIG_PLUGINS=y" >> $config_host_mak
> +    LIBS="-ldl $LIBS"
> +    LDFLAGS="-Wl,--dynamic-list=\$(SRC_PATH)/qemu-plugins.symbols $LDFLAGS"
> +fi
> +
>  if test "$tcg_interpreter" = "yes"; then
>    QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
>  elif test "$ARCH" = "sparc64" ; then
> -- 
> 2.17.1
> 
> 

ld64 on macOS has similar -exported_symbols_list option. Here's the reference:

     -exported_symbols_list filename
	The specified filename contains a list of global symbol names
	that will remain as global symbols in the output file.  All other
	global symbols will be treated as if they were marked as
	__private_extern__ (aka visibility=hidden) and will not be global in
	the output file. The symbol names listed in filename must be one per
	line.  Leading and trailing white space are not part of the symbol
	name.  Lines starting with # are ignored, as are lines with only white
	space.  Some wildcards (similar to shell file matching) are supported.
	The * matches zero or more characters.  The ?  matches one character.
	[abc] matches one character which must be an 'a', 'b', or 'c'.
	[a-z] matches any single lower case letter from 'a' to 'z'.


I can try your branch if you add support of the linker flag or send required
changes via GitHub.

Best regards,
Roman
Emilio Cota Nov. 27, 2018, 5:08 p.m. UTC | #3
On Tue, Nov 27, 2018 at 12:11:32 +0000, Alex Bennée wrote:
> 
> Emilio G. Cota <cota@braap.org> writes:
> 
> > For now only add it for ELF platforms, since we rely on the linker's
> > --dynamic-list flag to pass a list of symbols to be exported to the
> > executable. An alternative would be to use -rdynamic, but that would
> > expose all of QEMU's objects to plugins.
> >
> > I have no experience with non-ELF systems but I suspect adding support
> > for those should be pretty easy.
> >
> > Signed-off-by: Emilio G. Cota <cota@braap.org>
> 
> As far as the configure logic is concerned:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 
> I'm not quite following what is so special about the dynamic-list
> symbols compared to the rest of the symbols in the binary. when I do
> readelf -s they are all specified as GLOBAL DEFAULT.
> 
> Perhaps this will become clearer to me once I get to the implementation
> of the plugins later in the series?

You should look at `readelf --dyn-syms's output. There you'll see
that the only defined function symbols are qemu_plugin_* ones (plus
_init and _fini).

The goal is to prevent plugins from accessing symbols outside of
the plugin API. For instance, if from plugin 'foo' I try to call
qht_init (after having added the necessary declarations in foo.c),
at dlopen time I get:
  plugin_load: libfoo.so: undefined symbol: qht_init

The alternative is to export all symbols; we can do this by
passing --export-dynamic to the linker instead of an explicit
--dynamic-list. We can then access *any* global symbol in the
QEMU executable. The previous example now works past dlopen, i.e.
I can call qht_init from my plugin:
  ERROR:/data/src/qemu/util/qht.c:401:qht_init: assertion failed: (cmp)

Thanks,

		Emilio
Emilio Cota Nov. 27, 2018, 11:13 p.m. UTC | #4
On Tue, Nov 27, 2018 at 15:43:52 +0300, Roman Bolshakov wrote:
> ld64 on macOS has similar -exported_symbols_list option. Here's the reference:
> 
>      -exported_symbols_list filename
> 	The specified filename contains a list of global symbol names
> 	that will remain as global symbols in the output file.  All other
> 	global symbols will be treated as if they were marked as
> 	__private_extern__ (aka visibility=hidden) and will not be global in
> 	the output file. The symbol names listed in filename must be one per
> 	line.  Leading and trailing white space are not part of the symbol
> 	name.  Lines starting with # are ignored, as are lines with only white
> 	space.  Some wildcards (similar to shell file matching) are supported.
> 	The * matches zero or more characters.  The ?  matches one character.
> 	[abc] matches one character which must be an 'a', 'b', or 'c'.
> 	[a-z] matches any single lower case letter from 'a' to 'z'.
> 
> 
> I can try your branch if you add support of the linker flag or send required
> changes via GitHub.

Can you please try this branch? I added a commit with the appended.
  https://github.com/cota/qemu/tree/plugin-v2

You can inspect the symbols in the final binary with
`readelf --dyn-syms' or `nm -D'.

Thanks,

		Emilio

---
diff --git a/configure b/configure
index fe9707d951..3dc9c9697b 100755
--- a/configure
+++ b/configure
@@ -5176,15 +5176,31 @@ int main(void)
 }
 EOF
 
+ld_dynamic_list="no"
 if compile_prog "" "-Wl,--dynamic-list=$TMPTXT" ; then
   ld_dynamic_list="yes"
-else
-  if test "$plugins" = "yes" ; then
-    error_exit \
-        "Plugin support requires specifying a set of symbols that " \
-        "are exported to plugins. Unfortunately your linker doesn't " \
-        "support the flag (--dynamic-list) used for this purpose."
-  fi
+fi
+
+#########################################
+# See if -exported_symbols_list is supported by the linker
+
+cat > $TMPTXT <<EOF
+  foo
+EOF
+
+ld_exported_symbols_list="no"
+if compile_prog "" "-Wl,-exported_symbols_list,$TMPTXT" ; then
+  ld_exported_symbols_list="yes"
+fi
+
+if  test "$plugins" = "yes" &&
+    test "$ld_dynamic_list" = "no" &&
+    test "$ld_exported_symbols_list" = "no" ; then
+  error_exit \
+      "Plugin support requires 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."
 fi
 
 ########################################
@@ -6827,7 +6843,18 @@ fi
 if test "$plugins" = "yes" ; then
     echo "CONFIG_PLUGINS=y" >> $config_host_mak
     LIBS="-ldl $LIBS"
-    LDFLAGS="-Wl,--dynamic-list=\$(SRC_PATH)/qemu-plugins.symbols $LDFLAGS"
+    if test "$ld_dynamic_list" = "yes" ; then
+	LDFLAGS="-Wl,--dynamic-list=\$(SRC_PATH)/qemu-plugins.symbols $LDFLAGS"
+    elif test "$ld_exported_symbols_list" = "yes" ; then
+	ld64_symbols=qemu-plugins-ld64.symbols
+	echo "# Automatically generated by configure - do not modify" > $ld64_symbols
+	cat "$source_path/qemu-plugins.symbols" | grep qemu_ | sed 's/;//g' >> $ld64_symbols
+	LDFLAGS="-Wl,-exported_symbols_list,\$(BUILD_DIR)/$ld64_symbols $LDFLAGS"
+    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 "$tcg_interpreter" = "yes"; then
Roman Bolshakov Nov. 28, 2018, 10:43 a.m. UTC | #5
On Tue, Nov 27, 2018 at 06:13:57PM -0500, Emilio G. Cota wrote:
> On Tue, Nov 27, 2018 at 15:43:52 +0300, Roman Bolshakov wrote:
> > ld64 on macOS has similar -exported_symbols_list option. Here's the reference:
> > 
> >      -exported_symbols_list filename
> > 	The specified filename contains a list of global symbol names
> > 	that will remain as global symbols in the output file.  All other
> > 	global symbols will be treated as if they were marked as
> > 	__private_extern__ (aka visibility=hidden) and will not be global in
> > 	the output file. The symbol names listed in filename must be one per
> > 	line.  Leading and trailing white space are not part of the symbol
> > 	name.  Lines starting with # are ignored, as are lines with only white
> > 	space.  Some wildcards (similar to shell file matching) are supported.
> > 	The * matches zero or more characters.  The ?  matches one character.
> > 	[abc] matches one character which must be an 'a', 'b', or 'c'.
> > 	[a-z] matches any single lower case letter from 'a' to 'z'.
> > 
> > 
> > I can try your branch if you add support of the linker flag or send required
> > changes via GitHub.
> 
> Can you please try this branch? I added a commit with the appended.
>   https://github.com/cota/qemu/tree/plugin-v2
> 
> You can inspect the symbols in the final binary with
> `readelf --dyn-syms' or `nm -D'.
> 
> Thanks,
> 
> 		Emilio
> 
> ---
> diff --git a/configure b/configure
> index fe9707d951..3dc9c9697b 100755
> --- a/configure
> +++ b/configure
> @@ -5176,15 +5176,31 @@ int main(void)
>  }
>  EOF
>  
> +ld_dynamic_list="no"
>  if compile_prog "" "-Wl,--dynamic-list=$TMPTXT" ; then
>    ld_dynamic_list="yes"
> -else
> -  if test "$plugins" = "yes" ; then
> -    error_exit \
> -        "Plugin support requires specifying a set of symbols that " \
> -        "are exported to plugins. Unfortunately your linker doesn't " \
> -        "support the flag (--dynamic-list) used for this purpose."
> -  fi
> +fi
> +
> +#########################################
> +# See if -exported_symbols_list is supported by the linker
> +
> +cat > $TMPTXT <<EOF
> +  foo
> +EOF
> +
> +ld_exported_symbols_list="no"
> +if compile_prog "" "-Wl,-exported_symbols_list,$TMPTXT" ; then
> +  ld_exported_symbols_list="yes"
> +fi
> +
> +if  test "$plugins" = "yes" &&
> +    test "$ld_dynamic_list" = "no" &&
> +    test "$ld_exported_symbols_list" = "no" ; then
> +  error_exit \
> +      "Plugin support requires 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."
>  fi
>  
>  ########################################
> @@ -6827,7 +6843,18 @@ fi
>  if test "$plugins" = "yes" ; then
>      echo "CONFIG_PLUGINS=y" >> $config_host_mak
>      LIBS="-ldl $LIBS"
> -    LDFLAGS="-Wl,--dynamic-list=\$(SRC_PATH)/qemu-plugins.symbols $LDFLAGS"
> +    if test "$ld_dynamic_list" = "yes" ; then
> +	LDFLAGS="-Wl,--dynamic-list=\$(SRC_PATH)/qemu-plugins.symbols $LDFLAGS"
> +    elif test "$ld_exported_symbols_list" = "yes" ; then
> +	ld64_symbols=qemu-plugins-ld64.symbols
> +	echo "# Automatically generated by configure - do not modify" > $ld64_symbols
> +	cat "$source_path/qemu-plugins.symbols" | grep qemu_ | sed 's/;//g' >> $ld64_symbols
> +	LDFLAGS="-Wl,-exported_symbols_list,\$(BUILD_DIR)/$ld64_symbols $LDFLAGS"
> +    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 "$tcg_interpreter" = "yes"; then
> 

Hi Emilio,

The test of -exported_symbols_list fails:
Undefined symbols for architecture x86_64:
  "foo", referenced from:
       -exported_symbol[s_list] command line option
            (maybe you meant: _foo)

All functions on macOS are prefixed with underscore [1]:
"The name of a symbol representing a function that conforms to standard C
calling conventions is the name of the function with an underscore prefix.
Thus, the name of the symbol representing the function main would be _main."

So it can be fixed by prepending foo and qemu-plugins-ld64.symbols with
underscore:

diff --git a/configure b/configure
index 3dc9c9697b..85dd15732c 100755
--- a/configure
+++ b/configure
@@ -5185,7 +5185,7 @@ fi
 # See if -exported_symbols_list is supported by the linker

 cat > $TMPTXT <<EOF
-  foo
+  _foo
 EOF

 ld_exported_symbols_list="no"
@@ -6848,7 +6848,7 @@ if test "$plugins" = "yes" ; then
     elif test "$ld_exported_symbols_list" = "yes" ; then
        ld64_symbols=qemu-plugins-ld64.symbols
        echo "# Automatically generated by configure - do not modify" > $ld64_symbols
-       cat "$source_path/qemu-plugins.symbols" | grep qemu_ | sed 's/;//g' >> $ld64_symbols
+        cat "$source_path/qemu-plugins.symbols" | sed -nE 's/^([[:space:]]+)(.+);/\1_\2/p' >> $ld64_symbols
        LDFLAGS="-Wl,-exported_symbols_list,\$(BUILD_DIR)/$ld64_symbols $LDFLAGS"
     else
        error_exit \

--

Then if I print globally defined functions I can see it in
config-temp/qemu-conf.exe:

nm -g -U config-temp/qemu-conf.exe
0000000100000f50 T _foo

qemu-ga fails to link because it doesn't have symbols declared in
qemu-plugins-ld64.symbols. Perhaps "-Wl,-exported_symbols_list" should
be applied only to qemu-system?

[1] https://developer.apple.com/library/archive/documentation/DeveloperTools/Conceptual/MachOTopics/1-Articles/executing_files.html#//apple_ref/doc/uid/TP40001829-97182-TPXREF112

Best regards,
Roman
Emilio Cota Nov. 28, 2018, 5:23 p.m. UTC | #6
On Wed, Nov 28, 2018 at 13:43:30 +0300, Roman Bolshakov wrote:
> The test of -exported_symbols_list fails:
> Undefined symbols for architecture x86_64:
>   "foo", referenced from:
>        -exported_symbol[s_list] command line option
>             (maybe you meant: _foo)
> 
> All functions on macOS are prefixed with underscore [1]:
> "The name of a symbol representing a function that conforms to standard C
> calling conventions is the name of the function with an underscore prefix.
> Thus, the name of the symbol representing the function main would be _main."
> 
> So it can be fixed by prepending foo and qemu-plugins-ld64.symbols with
> underscore:

Ah I see, thanks!

(snip)
> qemu-ga fails to link because it doesn't have symbols declared in
> qemu-plugins-ld64.symbols. Perhaps "-Wl,-exported_symbols_list" should
> be applied only to qemu-system?

I just pushed to the same github branch the appended fixup.
The idea is to only add the linker flags when the output
binary links plugin.o.

Can you please test?

Thanks,

		Emilio

---
commit 8f45416b79765b66e5ce0fca7db93b97bbcfcfbb
Author: Emilio G. Cota <cota@braap.org>
Date:   Wed Nov 28 12:11:23 2018 -0500

    configure: ld64 fixup
    
    And copy the file to the build dir also for !ld64.
    
    Signed-off-by: Emilio G. Cota <cota@braap.org>

diff --git a/Makefile.target b/Makefile.target
index 719699696d..7ea17d71cb 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -107,7 +107,23 @@ obj-y += target/$(TARGET_BASE_ARCH)/
 obj-y += disas.o
 obj-$(call notempty,$(TARGET_XML_FILES)) += gdbstub-xml.o
 
-obj-$(CONFIG_PLUGINS) += plugin.o
+ifdef CONFIG_PLUGINS
+obj-y += plugin.o
+# Abuse -libs suffix to only link with --dynamic-list/-exported_symbols_list
+# when the final binary includes the plugin object.
+#
+# Note that simply setting LDFLAGS is not enough: we build binaries that
+# never link plugin.o, and the linker might fail (at least ld64 does)
+# if the symbols in the list are not in the output binary.
+ ifdef CONFIG_HAS_LD_DYNAMIC_LIST
+ plugin.o-libs := -Wl,--dynamic-list=$(BUILD_DIR)/qemu-plugins-ld.symbols
+ else
+  ifdef CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST
+  plugin.o-libs := \
+	-Wl,-exported_symbols_list,$(BUILD_DIR)/qemu-plugins-ld64.symbols
+  endif
+ endif
+endif
 
 #########################################################
 # Linux user emulator target
diff --git a/configure b/configure
index 3dc9c9697b..395acf831e 100755
--- a/configure
+++ b/configure
@@ -5185,7 +5185,7 @@ fi
 # See if -exported_symbols_list is supported by the linker
 
 cat > $TMPTXT <<EOF
-  foo
+  _foo
 EOF
 
 ld_exported_symbols_list="no"
@@ -6843,13 +6843,17 @@ fi
 if test "$plugins" = "yes" ; then
     echo "CONFIG_PLUGINS=y" >> $config_host_mak
     LIBS="-ldl $LIBS"
+    # Copy the export object list to the build dir
     if test "$ld_dynamic_list" = "yes" ; then
-	LDFLAGS="-Wl,--dynamic-list=\$(SRC_PATH)/qemu-plugins.symbols $LDFLAGS"
+	echo "CONFIG_HAS_LD_DYNAMIC_LIST=yes" >> $config_host_mak
+	ld_symbols=qemu-plugins-ld.symbols
+	cp "$source_path/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
-	cat "$source_path/qemu-plugins.symbols" | grep qemu_ | sed 's/;//g' >> $ld64_symbols
-	LDFLAGS="-Wl,-exported_symbols_list,\$(BUILD_DIR)/$ld64_symbols $LDFLAGS"
+	grep 'qemu_' "$source_path/qemu-plugins.symbols" | sed 's/;//g' | \
+	    sed -E 's/^\s*(.*)/_\1/' >> $ld64_symbols
     else
 	error_exit \
 	    "If \$plugins=yes, either \$ld_dynamic_list or " \
Roman Bolshakov Nov. 29, 2018, 9:57 a.m. UTC | #7
On Wed, Nov 28, 2018 at 12:23:32PM -0500, Emilio G. Cota wrote:
> On Wed, Nov 28, 2018 at 13:43:30 +0300, Roman Bolshakov wrote:
> > qemu-ga fails to link because it doesn't have symbols declared in
> > qemu-plugins-ld64.symbols. Perhaps "-Wl,-exported_symbols_list" should
> > be applied only to qemu-system?
> 
> I just pushed to the same github branch the appended fixup.
> The idea is to only add the linker flags when the output
> binary links plugin.o.
> 
> Can you please test?
> 
> Thanks,
> 
> 		Emilio
> 
> ---
> commit 8f45416b79765b66e5ce0fca7db93b97bbcfcfbb
> Author: Emilio G. Cota <cota@braap.org>
> Date:   Wed Nov 28 12:11:23 2018 -0500
> 
>     configure: ld64 fixup
>     
>     And copy the file to the build dir also for !ld64.
>     
>     Signed-off-by: Emilio G. Cota <cota@braap.org>
> 
> diff --git a/Makefile.target b/Makefile.target
> index 719699696d..7ea17d71cb 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -107,7 +107,23 @@ obj-y += target/$(TARGET_BASE_ARCH)/
>  obj-y += disas.o
>  obj-$(call notempty,$(TARGET_XML_FILES)) += gdbstub-xml.o
>  
> -obj-$(CONFIG_PLUGINS) += plugin.o
> +ifdef CONFIG_PLUGINS
> +obj-y += plugin.o
> +# Abuse -libs suffix to only link with --dynamic-list/-exported_symbols_list
> +# when the final binary includes the plugin object.
> +#
> +# Note that simply setting LDFLAGS is not enough: we build binaries that
> +# never link plugin.o, and the linker might fail (at least ld64 does)
> +# if the symbols in the list are not in the output binary.
> + ifdef CONFIG_HAS_LD_DYNAMIC_LIST
> + plugin.o-libs := -Wl,--dynamic-list=$(BUILD_DIR)/qemu-plugins-ld.symbols
> + else
> +  ifdef CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST
> +  plugin.o-libs := \
> +	-Wl,-exported_symbols_list,$(BUILD_DIR)/qemu-plugins-ld64.symbols
> +  endif
> + endif
> +endif
>  
>  #########################################################
>  # Linux user emulator target
> diff --git a/configure b/configure
> index 3dc9c9697b..395acf831e 100755
> --- a/configure
> +++ b/configure
> @@ -5185,7 +5185,7 @@ fi
>  # See if -exported_symbols_list is supported by the linker
>  
>  cat > $TMPTXT <<EOF
> -  foo
> +  _foo
>  EOF
>  
>  ld_exported_symbols_list="no"
> @@ -6843,13 +6843,17 @@ fi
>  if test "$plugins" = "yes" ; then
>      echo "CONFIG_PLUGINS=y" >> $config_host_mak
>      LIBS="-ldl $LIBS"
> +    # Copy the export object list to the build dir
>      if test "$ld_dynamic_list" = "yes" ; then
> -	LDFLAGS="-Wl,--dynamic-list=\$(SRC_PATH)/qemu-plugins.symbols $LDFLAGS"
> +	echo "CONFIG_HAS_LD_DYNAMIC_LIST=yes" >> $config_host_mak
> +	ld_symbols=qemu-plugins-ld.symbols
> +	cp "$source_path/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
> -	cat "$source_path/qemu-plugins.symbols" | grep qemu_ | sed 's/;//g' >> $ld64_symbols
> -	LDFLAGS="-Wl,-exported_symbols_list,\$(BUILD_DIR)/$ld64_symbols $LDFLAGS"
> +	grep 'qemu_' "$source_path/qemu-plugins.symbols" | sed 's/;//g' | \
> +	    sed -E 's/^\s*(.*)/_\1/' >> $ld64_symbols
>      else
>  	error_exit \
>  	    "If \$plugins=yes, either \$ld_dynamic_list or " \
> 
Hi Emilio,

I think there's an issue with "\s" character class, it's not recognized
by macOS sed  and I'm getting incorrect lines in
qemu-plugins-ld64.symbols:
_  qemu_xxx
_  qemu_xyz

After I replaced "\s" with "[[:space:]]", linking proceeds further, but
doesn't succeed because of an unresolved reference for qemu-system cris,
lm32, m68k, microblaze, microblazeel, moxie, nios2, or1k, riscv32,
riscv64, sparc, unicore32, tricore, xtensa, xtensaeb:

Undefined symbols for architecture x86_64:
  "_pci_register_bar", referenced from:
      _plugin_chan_realize in plugin-chan.o

It probably has nothing to do with macOS per-se and shouldn't link on
Linux as well. If I disable the aforementioned targets the build
succeeds and I can see the symbols from qemu-plugins-ld64.symbols in
compiled qemu-system binaries.

Best regards,
Roman
Emilio Cota Nov. 29, 2018, 5 p.m. UTC | #8
On Thu, Nov 29, 2018 at 12:57:16 +0300, Roman Bolshakov wrote:
> Hi Emilio,
> 
> I think there's an issue with "\s" character class, it's not recognized
> by macOS sed  and I'm getting incorrect lines in
> qemu-plugins-ld64.symbols:
> _  qemu_xxx
> _  qemu_xyz
> 
> After I replaced "\s" with "[[:space:]]", linking proceeds further

Nice, thanks. Will update.

> , but doesn't succeed because of an unresolved reference for qemu-system cris,
> lm32, m68k, microblaze, microblazeel, moxie, nios2, or1k, riscv32,
> riscv64, sparc, unicore32, tricore, xtensa, xtensaeb:
> 
> Undefined symbols for architecture x86_64:
>   "_pci_register_bar", referenced from:
>       _plugin_chan_realize in plugin-chan.o
> 
> It probably has nothing to do with macOS per-se and shouldn't link on
> Linux as well. If I disable the aforementioned targets the build
> succeeds and I can see the symbols from qemu-plugins-ld64.symbols in
> compiled qemu-system binaries.

Yes, that's because plugin-chan should only be built if the guest has PCI
support. Will fix.

Thanks,

		Emilio
Emilio Cota Nov. 29, 2018, 5:49 p.m. UTC | #9
On Thu, Nov 29, 2018 at 12:00:55 -0500, Emilio G. Cota wrote:
> On Thu, Nov 29, 2018 at 12:57:16 +0300, Roman Bolshakov wrote:
> > Hi Emilio,
> > 
> > I think there's an issue with "\s" character class, it's not recognized
> > by macOS sed  and I'm getting incorrect lines in
> > qemu-plugins-ld64.symbols:
> > _  qemu_xxx
> > _  qemu_xyz
> > 
> > After I replaced "\s" with "[[:space:]]", linking proceeds further
> 
> Nice, thanks. Will update.
> 
> > , but doesn't succeed because of an unresolved reference for qemu-system cris,
> > lm32, m68k, microblaze, microblazeel, moxie, nios2, or1k, riscv32,
> > riscv64, sparc, unicore32, tricore, xtensa, xtensaeb:
> > 
> > Undefined symbols for architecture x86_64:
> >   "_pci_register_bar", referenced from:
> >       _plugin_chan_realize in plugin-chan.o
> > 
> > It probably has nothing to do with macOS per-se and shouldn't link on
> > Linux as well. If I disable the aforementioned targets the build
> > succeeds and I can see the symbols from qemu-plugins-ld64.symbols in
> > compiled qemu-system binaries.
> 
> Yes, that's because plugin-chan should only be built if the guest has PCI
> support. Will fix.

Pushed the fixes to the github branch. Hope it works for you now!

Thanks,

		Emilio
Roman Bolshakov Nov. 29, 2018, 7:34 p.m. UTC | #10
On Thu, Nov 29, 2018 at 12:49:27PM -0500, Emilio G. Cota wrote:
> On Thu, Nov 29, 2018 at 12:00:55 -0500, Emilio G. Cota wrote:
> > On Thu, Nov 29, 2018 at 12:57:16 +0300, Roman Bolshakov wrote:
> > > Hi Emilio,
> > > 
> > > I think there's an issue with "\s" character class, it's not recognized
> > > by macOS sed  and I'm getting incorrect lines in
> > > qemu-plugins-ld64.symbols:
> > > _  qemu_xxx
> > > _  qemu_xyz
> > > 
> > > After I replaced "\s" with "[[:space:]]", linking proceeds further
> > 
> > Nice, thanks. Will update.
> > 
> > > , but doesn't succeed because of an unresolved reference for qemu-system cris,
> > > lm32, m68k, microblaze, microblazeel, moxie, nios2, or1k, riscv32,
> > > riscv64, sparc, unicore32, tricore, xtensa, xtensaeb:
> > > 
> > > Undefined symbols for architecture x86_64:
> > >   "_pci_register_bar", referenced from:
> > >       _plugin_chan_realize in plugin-chan.o
> > > 
> > > It probably has nothing to do with macOS per-se and shouldn't link on
> > > Linux as well. If I disable the aforementioned targets the build
> > > succeeds and I can see the symbols from qemu-plugins-ld64.symbols in
> > > compiled qemu-system binaries.
> > 
> > Yes, that's because plugin-chan should only be built if the guest has PCI
> > support. Will fix.
> 
> Pushed the fixes to the github branch. Hope it works for you now!
> 

Thank you Emilio,
the build succeded with the set of declared symbols exposed in
qemu-system.

I've just noticed qemu-plugins-ld64.symbols should be added to
.gitignore.

Best regards,
Roman
diff mbox series

Patch

diff --git a/configure b/configure
index 03bf719ca7..78e86098e5 100755
--- a/configure
+++ b/configure
@@ -30,6 +30,7 @@  TMPO="${TMPDIR1}/${TMPB}.o"
 TMPCXX="${TMPDIR1}/${TMPB}.cxx"
 TMPE="${TMPDIR1}/${TMPB}.exe"
 TMPMO="${TMPDIR1}/${TMPB}.mo"
+TMPTXT="${TMPDIR1}/${TMPB}.txt"
 
 rm -f config.log
 
@@ -477,6 +478,7 @@  libxml2=""
 docker="no"
 debug_mutex="no"
 libpmem=""
+plugins="no"
 
 # cross compilers defaults, can be overridden with --cross-cc-ARCH
 cross_cc_aarch64="aarch64-linux-gnu-gcc"
@@ -1443,6 +1445,10 @@  for opt do
   ;;
   --disable-libpmem) libpmem=no
   ;;
+  --enable-plugins) plugins="yes"
+  ;;
+  --disable-plugins) plugins="no"
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -1633,6 +1639,8 @@  Advanced options (experts only):
                            xen pv domain builder
   --enable-debug-stack-usage
                            track the maximum stack usage of stacks created by qemu_alloc_stack
+  --enable-plugins
+                           enable plugins via shared library loading
 
 Optional features, enabled with --enable-FEATURE and
 disabled with --disable-FEATURE, default is enabled if available:
@@ -5204,6 +5212,42 @@  if compile_prog "" "" ; then
   atomic64=yes
 fi
 
+#########################################
+# See if --dynamic-list is supported by the linker
+
+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"
+else
+  if test "$plugins" = "yes" ; then
+    error_exit \
+        "Plugin support requires specifying a set of symbols that " \
+        "are exported to plugins. Unfortunately your linker doesn't " \
+        "support the flag (--dynamic-list) used for this purpose."
+  fi
+fi
+
 ########################################
 # See if 16-byte vector operations are supported.
 # Even without a vector unit the compiler may expand these.
@@ -6091,6 +6135,7 @@  echo "VxHS block device $vxhs"
 echo "capstone          $capstone"
 echo "docker            $docker"
 echo "libpmem support   $libpmem"
+echo "plugin support    $plugins"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -6849,6 +6894,12 @@  if test "$libpmem" = "yes" ; then
   echo "CONFIG_LIBPMEM=y" >> $config_host_mak
 fi
 
+if test "$plugins" = "yes" ; then
+    echo "CONFIG_PLUGINS=y" >> $config_host_mak
+    LIBS="-ldl $LIBS"
+    LDFLAGS="-Wl,--dynamic-list=\$(SRC_PATH)/qemu-plugins.symbols $LDFLAGS"
+fi
+
 if test "$tcg_interpreter" = "yes"; then
   QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
 elif test "$ARCH" = "sparc64" ; then