contrib/plugins: ensure build does not pick up a system copy of plugin header

[PATCH] contrib/plugins: ensure build does not pick up a system copy of plugin header

Posted by Brad Smith 1 month ago

contrib/plugins: ensure build does not pick up a system copy of plugin header

With the ordering of the header path if a copy of QEMU is installed it
will pickup the system copy of the header before the build paths copy
and the build will fail.

Signed-off-by: Brad Smith <brad@comstyle.com>
---
 contrib/plugins/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
index 05a2a45c5c..52fc390376 100644
--- a/contrib/plugins/Makefile
+++ b/contrib/plugins/Makefile
@@ -41,9 +41,10 @@ SONAMES := $(addsuffix $(SO_SUFFIX),$(addprefix lib,$(NAMES)))
 
 # The main QEMU uses Glib extensively so it is perfectly fine to use it
 # in plugins (which many example do).
-PLUGIN_CFLAGS := $(shell $(PKG_CONFIG) --cflags glib-2.0)
-PLUGIN_CFLAGS += -fPIC -Wall
+GLIB_CFLAGS := $(shell $(PKG_CONFIG) --cflags glib-2.0)
 PLUGIN_CFLAGS += -I$(TOP_SRC_PATH)/include/qemu
+PLUGIN_CFLAGS += $(GLIB_CFLAGS)
+PLUGIN_CFLAGS += -fPIC -Wall
 
 # Helper that honours V=1 so we get some output when compiling
 quiet-@ = $(if $(V),,@$(if $1,printf "  %-7s %s\n" "$(strip $1)" "$(strip $2)" && ))
-- 
2.46.1

Re: [PATCH] contrib/plugins: ensure build does not pick up a system copy of plugin header

Posted by Alex Bennée 1 month ago

Brad Smith <brad@comstyle.com> writes:

> contrib/plugins: ensure build does not pick up a system copy of plugin
> header

I'm confused because this changes the ordering of the GLIB inclusion. We
shouldn't be including the whole QEMU include path.

How does this fail?

> With the ordering of the header path if a copy of QEMU is installed it
> will pickup the system copy of the header before the build paths copy
> and the build will fail.
>
> Signed-off-by: Brad Smith <brad@comstyle.com>
> ---
>  contrib/plugins/Makefile | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
> index 05a2a45c5c..52fc390376 100644
> --- a/contrib/plugins/Makefile
> +++ b/contrib/plugins/Makefile
> @@ -41,9 +41,10 @@ SONAMES := $(addsuffix $(SO_SUFFIX),$(addprefix lib,$(NAMES)))
>  
>  # The main QEMU uses Glib extensively so it is perfectly fine to use it
>  # in plugins (which many example do).
> -PLUGIN_CFLAGS := $(shell $(PKG_CONFIG) --cflags glib-2.0)
> -PLUGIN_CFLAGS += -fPIC -Wall
> +GLIB_CFLAGS := $(shell $(PKG_CONFIG) --cflags glib-2.0)
>  PLUGIN_CFLAGS += -I$(TOP_SRC_PATH)/include/qemu

Not withstanding the fact I've just borrowed bswap.h for a test plugin
maybe we should actually copy qemu-plugin.h to an entirely new location
during the build and then include from there to avoid any other
potential pollutions?

> +PLUGIN_CFLAGS += $(GLIB_CFLAGS)
> +PLUGIN_CFLAGS += -fPIC -Wall
>  
>  # Helper that honours V=1 so we get some output when compiling
>  quiet-@ = $(if $(V),,@$(if $1,printf "  %-7s %s\n" "$(strip $1)" "$(strip $2)" && ))

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] contrib/plugins: ensure build does not pick up a system copy of plugin header

Posted by Brad Smith 1 month ago

On 2024-09-21 8:55 a.m., Alex Bennée wrote:
> Brad Smith <brad@comstyle.com> writes:
>
>> contrib/plugins: ensure build does not pick up a system copy of plugin
>> header
> I'm confused because this changes the ordering of the GLIB inclusion. We
> shouldn't be including the whole QEMU include path.

That's intentional. The GLIB header paths cannot come before the header path
for the plugin header otherwise it pulls in the older plugin header from the
installed copy of QEMU and breaks. The QEMU include path is necessary
for the plugin header.


cc  -O2 -g  -I/usr/local/include/glib-2.0 
-I/usr/local/lib/glib-2.0/include -I/usr/local/include -fPIC -Wall 
-I/home/brad/tmp/qemu/contrib/plugins/../../include/qemu -c -o execlog.o 
/home/brad/tmp/qemu/contrib/plugins/execlog.c
/home/brad/tmp/qemu/contrib/plugins/execlog.c:262:41: error: too many 
arguments to function call, expected single argument 'insn', have 3 
arguments
             qemu_plugin_insn_data(insn, &insn_opcode, sizeof(insn_opcode));
             ~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/include/qemu-plugin.h:407:13: note: 'qemu_plugin_insn_data' 
declared here
const void *qemu_plugin_insn_data(const struct qemu_plugin_insn *insn);
             ^
1 error generated.

> How does this fail?
>
>> With the ordering of the header path if a copy of QEMU is installed it
>> will pickup the system copy of the header before the build paths copy
>> and the build will fail.
>>
>> Signed-off-by: Brad Smith <brad@comstyle.com>
>> ---
>>   contrib/plugins/Makefile | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
>> index 05a2a45c5c..52fc390376 100644
>> --- a/contrib/plugins/Makefile
>> +++ b/contrib/plugins/Makefile
>> @@ -41,9 +41,10 @@ SONAMES := $(addsuffix $(SO_SUFFIX),$(addprefix lib,$(NAMES)))
>>   
>>   # The main QEMU uses Glib extensively so it is perfectly fine to use it
>>   # in plugins (which many example do).
>> -PLUGIN_CFLAGS := $(shell $(PKG_CONFIG) --cflags glib-2.0)
>> -PLUGIN_CFLAGS += -fPIC -Wall
>> +GLIB_CFLAGS := $(shell $(PKG_CONFIG) --cflags glib-2.0)
>>   PLUGIN_CFLAGS += -I$(TOP_SRC_PATH)/include/qemu
> Not withstanding the fact I've just borrowed bswap.h for a test plugin
> maybe we should actually copy qemu-plugin.h to an entirely new location
> during the build and then include from there to avoid any other
> potential pollutions?

I don't see how that would make any difference, but either way as long 
as the header
path ordering is corrected so this new path is not passed last on the 
command line
getting the ordering wrong.

>
>> +PLUGIN_CFLAGS += $(GLIB_CFLAGS)
>> +PLUGIN_CFLAGS += -fPIC -Wall
>>   
>>   # Helper that honours V=1 so we get some output when compiling
>>   quiet-@ = $(if $(V),,@$(if $1,printf "  %-7s %s\n" "$(strip $1)" "$(strip $2)" && ))

Re: [PATCH] contrib/plugins: ensure build does not pick up a system copy of plugin header

Posted by Alex Bennée 1 month ago

Brad Smith <brad@comstyle.com> writes:

> On 2024-09-21 8:55 a.m., Alex Bennée wrote:
>> Brad Smith <brad@comstyle.com> writes:
>>
>>> contrib/plugins: ensure build does not pick up a system copy of plugin
>>> header
>> I'm confused because this changes the ordering of the GLIB inclusion. We
>> shouldn't be including the whole QEMU include path.
>
> That's intentional. The GLIB header paths cannot come before the header path
> for the plugin header otherwise it pulls in the older plugin header from the
> installed copy of QEMU and breaks. The QEMU include path is necessary
> for the plugin header.
>
>
> cc  -O2 -g  -I/usr/local/include/glib-2.0
> -I/usr/local/lib/glib-2.0/include -I/usr/local/include -fPIC -Wall
> -I/home/brad/tmp/qemu/contrib/plugins/../../include/qemu -c -o
> execlog.o /home/brad/tmp/qemu/contrib/plugins/execlog.c
> /home/brad/tmp/qemu/contrib/plugins/execlog.c:262:41: error: too many
> arguments to function call, expected single argument 'insn', have 3
> arguments
>             qemu_plugin_insn_data(insn, &insn_opcode, sizeof(insn_opcode));
>             ~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /usr/local/include/qemu-plugin.h:407:13: note: 'qemu_plugin_insn_data'
> declared here
> const void *qemu_plugin_insn_data(const struct qemu_plugin_insn *insn);
>             ^
> 1 error generated.

Ahh gotcha, I confused myself thinking this was QEMU's internal API header.

Queued to plugins/next, thanks.

>
>> How does this fail?
>>
>>> With the ordering of the header path if a copy of QEMU is installed it
>>> will pickup the system copy of the header before the build paths copy
>>> and the build will fail.
>>>
>>> Signed-off-by: Brad Smith <brad@comstyle.com>
>>> ---
>>>   contrib/plugins/Makefile | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
>>> index 05a2a45c5c..52fc390376 100644
>>> --- a/contrib/plugins/Makefile
>>> +++ b/contrib/plugins/Makefile
>>> @@ -41,9 +41,10 @@ SONAMES := $(addsuffix $(SO_SUFFIX),$(addprefix lib,$(NAMES)))
>>>     # The main QEMU uses Glib extensively so it is perfectly fine
>>> to use it
>>>   # in plugins (which many example do).
>>> -PLUGIN_CFLAGS := $(shell $(PKG_CONFIG) --cflags glib-2.0)
>>> -PLUGIN_CFLAGS += -fPIC -Wall
>>> +GLIB_CFLAGS := $(shell $(PKG_CONFIG) --cflags glib-2.0)
>>>   PLUGIN_CFLAGS += -I$(TOP_SRC_PATH)/include/qemu
>> Not withstanding the fact I've just borrowed bswap.h for a test plugin
>> maybe we should actually copy qemu-plugin.h to an entirely new location
>> during the build and then include from there to avoid any other
>> potential pollutions?
>
> I don't see how that would make any difference, but either way as long
> as the header
> path ordering is corrected so this new path is not passed last on the
> command line
> getting the ordering wrong.
>
>>
>>> +PLUGIN_CFLAGS += $(GLIB_CFLAGS)
>>> +PLUGIN_CFLAGS += -fPIC -Wall
>>>     # Helper that honours V=1 so we get some output when compiling
>>>   quiet-@ = $(if $(V),,@$(if $1,printf "  %-7s %s\n" "$(strip $1)" "$(strip $2)" && ))

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro