Patchwork Add CONFIG_QEMU_TIMER to handle qemu-timer-common.o dep

login
register
mail settings
Submitter Michael Roth
Date Aug. 29, 2011, 7:27 p.m.
Message ID <1314646068-31438-1-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/112123/
State New
Headers show

Comments

Michael Roth - Aug. 29, 2011, 7:27 p.m.
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(-)
Stefan Hajnoczi - Aug. 30, 2011, 9:22 a.m.
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
=?utf-8?Q?Llu=C3=ADs?= - Aug. 30, 2011, 12:02 p.m.
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
Anthony Liguori - Aug. 30, 2011, 2:17 p.m.
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
>
Michael Roth - Aug. 30, 2011, 3:20 p.m.
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
>
=?utf-8?Q?Llu=C3=ADs?= - Aug. 30, 2011, 6:32 p.m.
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
Blue Swirl - Aug. 30, 2011, 7:37 p.m.
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.
=?utf-8?Q?Llu=C3=ADs?= - Aug. 30, 2011, 7:40 p.m.
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

Patch

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