| Message ID | 1314646068-31438-1-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 >
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 >
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
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.
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
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
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(-)