[Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Blue Swirl]

957f1f99f263d57612807a9535f75ca4473f05f0 didn't consider
that qemu-timer-common.o is needed by simpletrace.

Fix by adding it to qga object list.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 Makefile.objs |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index d1f3e5d..df11aec 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -404,6 +404,9 @@ qga-obj-y = $(addprefix qga/, $(qga-nested-y))
 qga-obj-y += qemu-ga.o qemu-tool.o qemu-error.o qemu-sockets.o
module.o qemu-option.o cutils.o osdep.o
 qga-obj-$(CONFIG_WIN32) += oslib-win32.o
 qga-obj-$(CONFIG_POSIX) += oslib-posix.o
+ifeq ($(TRACE_BACKEND),simple)
+qga-obj-y += qemu-timer-common.o
+endif

 vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)

-- 
1.6.2.4

Re: [Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Lluís]

Blue Swirl writes:

> 957f1f99f263d57612807a9535f75ca4473f05f0 didn't consider
> that qemu-timer-common.o is needed by simpletrace.

> Fix by adding it to qga object list.

> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  Makefile.objs |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

> diff --git a/Makefile.objs b/Makefile.objs
> index d1f3e5d..df11aec 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -404,6 +404,9 @@ qga-obj-y = $(addprefix qga/, $(qga-nested-y))
>  qga-obj-y += qemu-ga.o qemu-tool.o qemu-error.o qemu-sockets.o
> module.o qemu-option.o cutils.o osdep.o
>  qga-obj-$(CONFIG_WIN32) += oslib-win32.o
>  qga-obj-$(CONFIG_POSIX) += oslib-posix.o
> +ifeq ($(TRACE_BACKEND),simple)
> +qga-obj-y += qemu-timer-common.o
> +endif

>  vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)

I sent a patch that should fix it for everybody linking with the tracing
objects:

http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg03150.html


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Blue Swirl]

On Fri, Aug 26, 2011 at 7:12 PM, Lluís <xscript@gmx.net> wrote:
> Blue Swirl writes:
>
>> 957f1f99f263d57612807a9535f75ca4473f05f0 didn't consider
>> that qemu-timer-common.o is needed by simpletrace.
>
>> Fix by adding it to qga object list.
>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  Makefile.objs |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index d1f3e5d..df11aec 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -404,6 +404,9 @@ qga-obj-y = $(addprefix qga/, $(qga-nested-y))
>>  qga-obj-y += qemu-ga.o qemu-tool.o qemu-error.o qemu-sockets.o
>> module.o qemu-option.o cutils.o osdep.o
>>  qga-obj-$(CONFIG_WIN32) += oslib-win32.o
>>  qga-obj-$(CONFIG_POSIX) += oslib-posix.o
>> +ifeq ($(TRACE_BACKEND),simple)
>> +qga-obj-y += qemu-timer-common.o
>> +endif
>
>>  vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>
> I sent a patch that should fix it for everybody linking with the tracing
> objects:
>
> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg03150.html

With your patch, there are warnings from linker:
../qemu-timer-common.o: warning: multiple common of `use_rt_clock'
../qemu-timer-common.o: warning: previous common is here

Re: [Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Lluís]

Blue Swirl writes:

> On Fri, Aug 26, 2011 at 7:12 PM, Lluís <xscript@gmx.net> wrote:
>> Blue Swirl writes:
>> 
>>> 957f1f99f263d57612807a9535f75ca4473f05f0 didn't consider
>>> that qemu-timer-common.o is needed by simpletrace.
>> 
>>> Fix by adding it to qga object list.
>> 
>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>>> ---
>>>  Makefile.objs |    3 +++
>>>  1 files changed, 3 insertions(+), 0 deletions(-)
>> 
>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index d1f3e5d..df11aec 100644
>>> --- a/Makefile.objs
>>> +++ b/Makefile.objs
>>> @@ -404,6 +404,9 @@ qga-obj-y = $(addprefix qga/, $(qga-nested-y))
>>>  qga-obj-y += qemu-ga.o qemu-tool.o qemu-error.o qemu-sockets.o
>>> module.o qemu-option.o cutils.o osdep.o
>>>  qga-obj-$(CONFIG_WIN32) += oslib-win32.o
>>>  qga-obj-$(CONFIG_POSIX) += oslib-posix.o
>>> +ifeq ($(TRACE_BACKEND),simple)
>>> +qga-obj-y += qemu-timer-common.o
>>> +endif
>> 
>>>  vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>> 
>> I sent a patch that should fix it for everybody linking with the tracing
>> objects:
>> 
>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg03150.html

> With your patch, there are warnings from linker:
> ../qemu-timer-common.o: warning: multiple common of `use_rt_clock'
> ../qemu-timer-common.o: warning: previous common is here

Ah, yes. These extra errors are fixed by the duplicate elimination patch
:)

http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg02987.html

So, you need both to keep it clean.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Blue Swirl]

On Sat, Aug 27, 2011 at 5:56 PM, Lluís <xscript@gmx.net> wrote:
> Blue Swirl writes:
>
>> On Fri, Aug 26, 2011 at 7:12 PM, Lluís <xscript@gmx.net> wrote:
>>> Blue Swirl writes:
>>>
>>>> 957f1f99f263d57612807a9535f75ca4473f05f0 didn't consider
>>>> that qemu-timer-common.o is needed by simpletrace.
>>>
>>>> Fix by adding it to qga object list.
>>>
>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>>>> ---
>>>>  Makefile.objs |    3 +++
>>>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>>> diff --git a/Makefile.objs b/Makefile.objs
>>>> index d1f3e5d..df11aec 100644
>>>> --- a/Makefile.objs
>>>> +++ b/Makefile.objs
>>>> @@ -404,6 +404,9 @@ qga-obj-y = $(addprefix qga/, $(qga-nested-y))
>>>>  qga-obj-y += qemu-ga.o qemu-tool.o qemu-error.o qemu-sockets.o
>>>> module.o qemu-option.o cutils.o osdep.o
>>>>  qga-obj-$(CONFIG_WIN32) += oslib-win32.o
>>>>  qga-obj-$(CONFIG_POSIX) += oslib-posix.o
>>>> +ifeq ($(TRACE_BACKEND),simple)
>>>> +qga-obj-y += qemu-timer-common.o
>>>> +endif
>>>
>>>>  vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>>>
>>> I sent a patch that should fix it for everybody linking with the tracing
>>> objects:
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg03150.html
>
>> With your patch, there are warnings from linker:
>> ../qemu-timer-common.o: warning: multiple common of `use_rt_clock'
>> ../qemu-timer-common.o: warning: previous common is here
>
> Ah, yes. These extra errors are fixed by the duplicate elimination patch
> :)
>
> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg02987.html
>
> So, you need both to keep it clean.

Using the sort function looks hackish to me. Maybe the linkage should
be controlled by configure instead?

Re: [Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Lluís]

Blue Swirl writes:

> On Sat, Aug 27, 2011 at 5:56 PM, Lluís <xscript@gmx.net> wrote:
>>>> I sent a patch that should fix it for everybody linking with the tracing
>>>> objects:
>>>> 
>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg03150.html
>> 
>>> With your patch, there are warnings from linker:
>>> ../qemu-timer-common.o: warning: multiple common of `use_rt_clock'
>>> ../qemu-timer-common.o: warning: previous common is here
>> 
>> Ah, yes. These extra errors are fixed by the duplicate elimination patch
>> :)
>> 
>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg02987.html
>> 
>> So, you need both to keep it clean.

> Using the sort function looks hackish to me. Maybe the linkage should
> be controlled by configure instead?

What do you mean? Moving the logic for selecting the object files to
link with on each top-level target out into the configure?

In any case, I think that adding qemu-timer-common.o into trace-obj-y is
the cleanest way, as otherwise the object needs to be added again and
again depending on conditions that are checked multiple times, which I
think will lead to to makefile maintenance headaches in the long run.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Blue Swirl]

On Sun, Aug 28, 2011 at 6:13 PM, Lluís <xscript@gmx.net> wrote:
> Blue Swirl writes:
>
>> On Sat, Aug 27, 2011 at 5:56 PM, Lluís <xscript@gmx.net> wrote:
>>>>> I sent a patch that should fix it for everybody linking with the tracing
>>>>> objects:
>>>>>
>>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg03150.html
>>>
>>>> With your patch, there are warnings from linker:
>>>> ../qemu-timer-common.o: warning: multiple common of `use_rt_clock'
>>>> ../qemu-timer-common.o: warning: previous common is here
>>>
>>> Ah, yes. These extra errors are fixed by the duplicate elimination patch
>>> :)
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg02987.html
>>>
>>> So, you need both to keep it clean.
>
>> Using the sort function looks hackish to me. Maybe the linkage should
>> be controlled by configure instead?
>
> What do you mean? Moving the logic for selecting the object files to
> link with on each top-level target out into the configure?

Add CONFIG_QEMU_TIMER, configure sets it to 'y' when it is needed by
simpletrace or other cases.

> In any case, I think that adding qemu-timer-common.o into trace-obj-y is
> the cleanest way, as otherwise the object needs to be added again and
> again depending on conditions that are checked multiple times, which I
> think will lead to to makefile maintenance headaches in the long run.

That is not needed if the logic resides in configure:
obj-$(CONFIG_QEMU_TIMER) += qemu-timer-common.o

Re: [Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Stefan Hajnoczi]

On Sun, Aug 28, 2011 at 10:08 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Aug 28, 2011 at 6:13 PM, Lluís <xscript@gmx.net> wrote:
>> Blue Swirl writes:
>>
>>> On Sat, Aug 27, 2011 at 5:56 PM, Lluís <xscript@gmx.net> wrote:
>>>>>> I sent a patch that should fix it for everybody linking with the tracing
>>>>>> objects:
>>>>>>
>>>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg03150.html
>>>>
>>>>> With your patch, there are warnings from linker:
>>>>> ../qemu-timer-common.o: warning: multiple common of `use_rt_clock'
>>>>> ../qemu-timer-common.o: warning: previous common is here
>>>>
>>>> Ah, yes. These extra errors are fixed by the duplicate elimination patch
>>>> :)
>>>>
>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg02987.html
>>>>
>>>> So, you need both to keep it clean.
>>
>>> Using the sort function looks hackish to me. Maybe the linkage should
>>> be controlled by configure instead?
>>
>> What do you mean? Moving the logic for selecting the object files to
>> link with on each top-level target out into the configure?
>
> Add CONFIG_QEMU_TIMER, configure sets it to 'y' when it is needed by
> simpletrace or other cases.

The $(sort) approach is simpler because it is implicit.  I'm not sure
that explicitly managing these dependencies is necessary.  But the
configure approach works for me too.

Blue: Are you going to post the CONFIG_QEMU_TIMER patch?

Stefan

Re: [Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Michael Roth]

On 08/29/2011 07:28 AM, Stefan Hajnoczi wrote:
> On Sun, Aug 28, 2011 at 10:08 PM, Blue Swirl<blauwirbel@gmail.com>  wrote:
>> On Sun, Aug 28, 2011 at 6:13 PM, Lluís<xscript@gmx.net>  wrote:
>>> Blue Swirl writes:
>>>
>>>> On Sat, Aug 27, 2011 at 5:56 PM, Lluís<xscript@gmx.net>  wrote:
>>>>>>> I sent a patch that should fix it for everybody linking with the tracing
>>>>>>> objects:
>>>>>>>
>>>>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg03150.html
>>>>>
>>>>>> With your patch, there are warnings from linker:
>>>>>> ../qemu-timer-common.o: warning: multiple common of `use_rt_clock'
>>>>>> ../qemu-timer-common.o: warning: previous common is here
>>>>>
>>>>> Ah, yes. These extra errors are fixed by the duplicate elimination patch
>>>>> :)
>>>>>
>>>>> http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg02987.html
>>>>>
>>>>> So, you need both to keep it clean.
>>>
>>>> Using the sort function looks hackish to me. Maybe the linkage should
>>>> be controlled by configure instead?
>>>
>>> What do you mean? Moving the logic for selecting the object files to
>>> link with on each top-level target out into the configure?
>>
>> Add CONFIG_QEMU_TIMER, configure sets it to 'y' when it is needed by
>> simpletrace or other cases.
>
> The $(sort) approach is simpler because it is implicit.  I'm not sure
> that explicitly managing these dependencies is necessary.  But the

It also mirrors how most projects handle duplicate header includes using 
a guard.

Plus, it really sucks to not be able to create a self-sufficient group 
of objects just because someone that includes your group happens to also 
include a common object (in this case, common-obj-y). trace-obj-y should 
absolutely be able to note qemu-timer-common.o as an explicit dependency 
regardless of whether or not it happens to get linked in by something 
else in the common case.

Plus with talk of breaking up QEMU into shared objects I think we'd end 
up needing to have self-sufficient object groups anyway.

But, in the meantime, I broke something again so I put together a patch 
with Blue's suggestions that seems to do the trick fairly reasonably. 
I'll send it as a response for reference, though I'd really prefer the 
$(sort) approach.

> configure approach works for me too.
>
> Blue: Are you going to post the CONFIG_QEMU_TIMER patch?
>
> Stefan
>

[Qemu-devel] [PATCH] Add CONFIG_QEMU_TIMER to handle qemu-timer-common.o dep

[Michael Roth]

This conditionally sets CONFIG_QEMU_TIMER in configure when something
like --enable-trace-backend=simple is set which requires
qemu-timer-common.o. Object groups dependent on such code can then
simply do:

x-obj-$(CONFIG_QEMU_TIMER) += qemu-timer-common.o

This also fixes build issue with qemu-ga due due not linking in
qemu-timer-common.o when using --enable-trace-backend=simple

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 Makefile.objs |    3 ++-
 configure     |    5 +++++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index d1f3e5d..0048650 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -177,6 +177,7 @@ user-obj-y =
 user-obj-y += envlist.o path.o
 user-obj-y += tcg-runtime.o host-utils.o
 user-obj-y += cutils.o cache-utils.o
+user-obj-$(CONFIG_QEMU_TIMER) += qemu-timer-common.o
 
 ######################################################################
 # libhw
@@ -380,7 +381,6 @@ else
 trace-obj-y = trace.o
 ifeq ($(TRACE_BACKEND),simple)
 trace-obj-y += simpletrace.o
-user-obj-y += qemu-timer-common.o
 endif
 endif
 
@@ -404,6 +404,7 @@ qga-obj-y = $(addprefix qga/, $(qga-nested-y))
 qga-obj-y += qemu-ga.o qemu-tool.o qemu-error.o qemu-sockets.o module.o qemu-option.o cutils.o osdep.o
 qga-obj-$(CONFIG_WIN32) += oslib-win32.o
 qga-obj-$(CONFIG_POSIX) += oslib-posix.o
+qga-obj-$(CONFIG_QEMU_TIMER) += qemu-timer-common.o
 
 vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
 
diff --git a/configure b/configure
index 1340c33..64da275 100755
--- a/configure
+++ b/configure
@@ -183,6 +183,7 @@ usb_redir=""
 opengl=""
 zlib="yes"
 guest_agent="yes"
+qemu_timer="no"
 
 # parse CC options first
 for opt do
@@ -3071,6 +3072,7 @@ fi
 # Set the appropriate trace file.
 if test "$trace_backend" = "simple"; then
   trace_file="\"$trace_file-\" FMT_pid"
+  qemu_timer="yes"
 fi
 if test "$trace_backend" = "dtrace" -a "$trace_backend_stap" = "yes" ; then
   echo "CONFIG_SYSTEMTAP_TRACE=y" >> $config_host_mak
@@ -3109,6 +3111,9 @@ echo "LIBS+=$LIBS" >> $config_host_mak
 echo "LIBS_TOOLS+=$libs_tools" >> $config_host_mak
 echo "EXESUF=$EXESUF" >> $config_host_mak
 echo "LIBS_QGA+=$libs_qga" >> $config_host_mak
+if test "$qemu_timer" = "yes" ; then
+    echo "CONFIG_QEMU_TIMER=y" >>$config_host_mak
+fi
 
 # generate list of library paths for linker script
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH] Add CONFIG_QEMU_TIMER to handle qemu-timer-common.o dep

[Stefan Hajnoczi]

On Mon, Aug 29, 2011 at 8:27 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> @@ -380,7 +381,6 @@ else
>  trace-obj-y = trace.o
>  ifeq ($(TRACE_BACKEND),simple)
>  trace-obj-y += simpletrace.o
> -user-obj-y += qemu-timer-common.o
>  endif
>  endif

Now that we have a concrete patch to look at I think this approach is
problematic.  There are several subsystems in QEMU which might be
built outside the main qemu binary for qemu-io, qemu-img, qemu-ga,
etc.

Each subsystem should explicitly include its dependencies (e.g.
subsys-obj-y += qemu-timer-common.o or subsys-obj-y +=
$(more-fundamental-subsys)) so that it can be easily used by a target.
 If this is not done then there are two disadvantages:
1. We spray dependency information across the makefiles instead of
keeping them contained with the subsystem that has the dependency
requirement.
2. We duplicate the dependencies across each target in the form of
conditional objects:
x-obj-$(CONFIG_MY_DEPENDENCY) += ...

If QEMU is split up into libraries then having an explicit list of
dependencies for each subsystem will be very useful, whereas the
CONFIG_* approach doesn't collect that information in one place.

So I think explicit subsys-obj-y += qemu-timer-common.o together with
$(sort) during the link stage actually allows for a cleaner build
system.  I prefer that approach.

Stefan

Re: [Qemu-devel] [PATCH] Add CONFIG_QEMU_TIMER to handle qemu-timer-common.o dep

[Lluís]

Stefan Hajnoczi writes:

> On Mon, Aug 29, 2011 at 8:27 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>> @@ -380,7 +381,6 @@ else
>>  trace-obj-y = trace.o
>>  ifeq ($(TRACE_BACKEND),simple)
>>  trace-obj-y += simpletrace.o
>> -user-obj-y += qemu-timer-common.o
>>  endif
>>  endif

> Now that we have a concrete patch to look at I think this approach is
> problematic.  There are several subsystems in QEMU which might be
> built outside the main qemu binary for qemu-io, qemu-img, qemu-ga,
> etc.
[...]
> If QEMU is split up into libraries then having an explicit list of
> dependencies for each subsystem will be very useful, whereas the
> CONFIG_* approach doesn't collect that information in one place.

> So I think explicit subsys-obj-y += qemu-timer-common.o together with
> $(sort) during the link stage actually allows for a cleaner build
> system.  I prefer that approach.

I couldn't agree more. The only problem I see with '$(sort)' is that it
will invariably change the order of object files, which can influence
code placement.

Whether or not the spatial locality among compilation units is
important, I don't know. Although I believe it won't have much of a
performance penalty.

In any case, I tried to find a straightforward way of filtering-out
repeated words in a list with make, but couldn't find any solution other
than '$(sort)' or calling an external command with '$(shell)'.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH] Add CONFIG_QEMU_TIMER to handle qemu-timer-common.o dep

[Michael Roth]

On 08/30/2011 07:02 AM, Lluís wrote:
> Stefan Hajnoczi writes:
>
>> On Mon, Aug 29, 2011 at 8:27 PM, Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>>> @@ -380,7 +381,6 @@ else
>>>   trace-obj-y = trace.o
>>>   ifeq ($(TRACE_BACKEND),simple)
>>>   trace-obj-y += simpletrace.o
>>> -user-obj-y += qemu-timer-common.o
>>>   endif
>>>   endif
>
>> Now that we have a concrete patch to look at I think this approach is
>> problematic.  There are several subsystems in QEMU which might be
>> built outside the main qemu binary for qemu-io, qemu-img, qemu-ga,
>> etc.
> [...]
>> If QEMU is split up into libraries then having an explicit list of
>> dependencies for each subsystem will be very useful, whereas the
>> CONFIG_* approach doesn't collect that information in one place.
>
>> So I think explicit subsys-obj-y += qemu-timer-common.o together with
>> $(sort) during the link stage actually allows for a cleaner build
>> system.  I prefer that approach.
>
> I couldn't agree more. The only problem I see with '$(sort)' is that it
> will invariably change the order of object files, which can influence
> code placement.
>
> Whether or not the spatial locality among compilation units is
> important, I don't know. Although I believe it won't have much of a
> performance penalty.
>
> In any case, I tried to find a straightforward way of filtering-out
> repeated words in a list with make, but couldn't find any solution other
> than '$(sort)' or calling an external command with '$(shell)'.

Hmm, looking again I'm confused why we need to do this in the first 
place...the rule is:

LINK = $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o 
$@ $(1) $(LIBS),"  LINK  $(TARGET_DIR)$@")

%$(EXESUF): %.o
     $(call LINK,$^)

According to the documentation $^ should remove duplicate dependencies, 
so I'm not getting why we need to de-dupe them once they get passed to LINK:

http://www.gnu.org/software/make/manual/make.html#index-g_t_0024_005e-948

Is there some distinction between duplicate dependencies and duplicate 
words in the dependency list?

>
>
> Lluis
>

Re: [Qemu-devel] [PATCH] Add CONFIG_QEMU_TIMER to handle qemu-timer-common.o dep

[Lluís]

Michael Roth writes:
> Hmm, looking again I'm confused why we need to do this in the first place...the
> rule is:

> LINK = $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o 
> $@ $(1) $(LIBS),"  LINK  $(TARGET_DIR)$@")

> %$(EXESUF): %.o
>     $(call LINK,$^)

> According to the documentation $^ should remove duplicate dependencies, so I'm
> not getting why we need to de-dupe them once they get passed to LINK:

> http://www.gnu.org/software/make/manual/make.html#index-g_t_0024_005e-948

> Is there some distinction between duplicate dependencies and duplicate words in
> the dependency list?

It does indeed work X'D

I was blindly searching for words like "duplicate" and "repeat" in
make's info page.

I just now realized that in my case, the duplication error appeared only
with targets defined in line 434 of Makefile.target.

The fix is as trivial as using this link line:

    $(call LINK,$^)


Nice catch Michael, we got lost in the discussion :)


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH] Add CONFIG_QEMU_TIMER to handle qemu-timer-common.o dep

[Lluís]

Lluís  writes:

> Michael Roth writes:
>> Hmm, looking again I'm confused why we need to do this in the first place...the
>> rule is:

>> LINK = $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o 
>> $@ $(1) $(LIBS),"  LINK  $(TARGET_DIR)$@")

>> %$(EXESUF): %.o
>> $(call LINK,$^)

>> According to the documentation $^ should remove duplicate dependencies, so I'm
>> not getting why we need to de-dupe them once they get passed to LINK:

>> http://www.gnu.org/software/make/manual/make.html#index-g_t_0024_005e-948

>> Is there some distinction between duplicate dependencies and duplicate words in
>> the dependency list?

> It does indeed work X'D

> I was blindly searching for words like "duplicate" and "repeat" in
> make's info page.

> I just now realized that in my case, the duplication error appeared only
> with targets defined in line 434 of Makefile.target.

> The fix is as trivial as using this link line:

>     $(call LINK,$^)


> Nice catch Michael, we got lost in the discussion :)


BTW, I think tis settles the issue with [1] being "The Right Thing" (TM) :)

The patch in [2] is exactly the same, but outside the tracing patch set.


[1] http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg03150.html
[2] http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg02915.html


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH] Add CONFIG_QEMU_TIMER to handle qemu-timer-common.o dep

[Anthony Liguori]

On 08/30/2011 04:22 AM, Stefan Hajnoczi wrote:
> On Mon, Aug 29, 2011 at 8:27 PM, Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>> @@ -380,7 +381,6 @@ else
>>   trace-obj-y = trace.o
>>   ifeq ($(TRACE_BACKEND),simple)
>>   trace-obj-y += simpletrace.o
>> -user-obj-y += qemu-timer-common.o
>>   endif
>>   endif
>
> Now that we have a concrete patch to look at I think this approach is
> problematic.  There are several subsystems in QEMU which might be
> built outside the main qemu binary for qemu-io, qemu-img, qemu-ga,
> etc.

Er, but qemu-timer cannot possibly be used by qemu-io/qemu-img.

Is this all dummy magic in order to let qemu-io build even though simple 
tracing won't work?

Perhaps we should look at making the tracing backends dynamic instead of 
static?

Regards,

Anthony Liguori

>
> Each subsystem should explicitly include its dependencies (e.g.
> subsys-obj-y += qemu-timer-common.o or subsys-obj-y +=
> $(more-fundamental-subsys)) so that it can be easily used by a target.
>   If this is not done then there are two disadvantages:
> 1. We spray dependency information across the makefiles instead of
> keeping them contained with the subsystem that has the dependency
> requirement.
> 2. We duplicate the dependencies across each target in the form of
> conditional objects:
> x-obj-$(CONFIG_MY_DEPENDENCY) += ...
>
> If QEMU is split up into libraries then having an explicit list of
> dependencies for each subsystem will be very useful, whereas the
> CONFIG_* approach doesn't collect that information in one place.
>
> So I think explicit subsys-obj-y += qemu-timer-common.o together with
> $(sort) during the link stage actually allows for a cleaner build
> system.  I prefer that approach.
>
> Stefan
>

Re: [Qemu-devel] [PATCH] Add CONFIG_QEMU_TIMER to handle qemu-timer-common.o dep

[Blue Swirl]

On Tue, Aug 30, 2011 at 9:22 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Aug 29, 2011 at 8:27 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>> @@ -380,7 +381,6 @@ else
>>  trace-obj-y = trace.o
>>  ifeq ($(TRACE_BACKEND),simple)
>>  trace-obj-y += simpletrace.o
>> -user-obj-y += qemu-timer-common.o
>>  endif
>>  endif
>
> Now that we have a concrete patch to look at I think this approach is
> problematic.  There are several subsystems in QEMU which might be
> built outside the main qemu binary for qemu-io, qemu-img, qemu-ga,
> etc.
>
> Each subsystem should explicitly include its dependencies (e.g.
> subsys-obj-y += qemu-timer-common.o or subsys-obj-y +=
> $(more-fundamental-subsys)) so that it can be easily used by a target.
>  If this is not done then there are two disadvantages:
> 1. We spray dependency information across the makefiles instead of
> keeping them contained with the subsystem that has the dependency
> requirement.
> 2. We duplicate the dependencies across each target in the form of
> conditional objects:
> x-obj-$(CONFIG_MY_DEPENDENCY) += ...
>
> If QEMU is split up into libraries then having an explicit list of
> dependencies for each subsystem will be very useful, whereas the
> CONFIG_* approach doesn't collect that information in one place.
>
> So I think explicit subsys-obj-y += qemu-timer-common.o together with
> $(sort) during the link stage actually allows for a cleaner build
> system.  I prefer that approach.

I don't have a strong preference here. In theory each object could
depend on multiple other objects, an explicit dependency approach of
course works but it may become heavyweight.

We could also divide QEMU into core libraries and support libraries
(which may or may not be used by core libraries). Core libraries would
be handled with the obj-y method, support libraries with traditional
AR libraries. That would handle multiple linking issues I think.

Re: [Qemu-devel] [PATCH 1/2] Fix guest agent build with simpletrace

[Lluís]

Blue Swirl writes:

>>> Using the sort function looks hackish to me. Maybe the linkage should
>>> be controlled by configure instead?
>> 
>> What do you mean? Moving the logic for selecting the object files to
>> link with on each top-level target out into the configure?

> Add CONFIG_QEMU_TIMER, configure sets it to 'y' when it is needed by
> simpletrace or other cases.

>> In any case, I think that adding qemu-timer-common.o into trace-obj-y is
>> the cleanest way, as otherwise the object needs to be added again and
>> again depending on conditions that are checked multiple times, which I
>> think will lead to to makefile maintenance headaches in the long run.

> That is not needed if the logic resides in configure:
> obj-$(CONFIG_QEMU_TIMER) += qemu-timer-common.o

Well, this looks more hackish than the 'sort' approach to me, but I can
live with that :)


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth