Re: [Qemu-devel] [PATCH] build: Use separate makefile for "trace/"

[Paolo Bonzini]

Il 12/12/2012 15:53, Lluís Vilanova ha scritto:
> Paolo Bonzini writes:
> 
>> Il 11/12/2012 22:44, Lluís Vilanova ha scritto:
>>> Signed-off-by: Lluís Vilanova <address@hidden>
>>> ---
>>> .gitignore                          |    6 +--
>>> Makefile                            |    8 ++-
>>> Makefile.objs                       |   64 +---------------------------
>>> scripts/tracetool/backend/dtrace.py |    2 -
>>> trace/Makefile.objs                 |   81 
>>> +++++++++++++++++++++++++++++++++++
>>> 5 files changed, 92 insertions(+), 69 deletions(-)
>>> create mode 100644 trace/Makefile.objs
>>>
>>> diff --git a/.gitignore b/.gitignore
>>> index bd6ba1c..b67a37e 100644
>>> --- a/.gitignore
>>> +++ b/.gitignore
>>> @@ -3,9 +3,9 @@ config-all-devices.*
>>> config-host.*
>>> config-target.*
>>> trace.h
>>> -trace.c
>>> -trace-dtrace.h
>>> -trace-dtrace.dtrace
>>> +trace/generated.c
>>> +trace/generated-dtrace.h
>>> +trace/generated-dtrace.dtrace
>>> *-timestamp
>>> *-softmmu
>>> *-darwin-user
>>> diff --git a/Makefile b/Makefile
>>> index e9d6848..9dbcca3 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -33,10 +33,10 @@ endif
>>>
>>> GENERATED_HEADERS = config-host.h trace.h qemu-options.def
>>> ifeq ($(TRACE_BACKEND),dtrace)
>>> -GENERATED_HEADERS += trace-dtrace.h
>>> +GENERATED_HEADERS += trace/generated-dtrace.h
>>> endif
>>> GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h
>>> -GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c trace.c
>>> +GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c 
>>> trace/generated.c
>>>
>>> # Don't try to regenerate Makefile or configure
>>> # We don't generate any of them
>>> @@ -252,9 +252,9 @@ clean:
>>> rm -f *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
>>> rm -Rf .libs
>>> rm -f qemu-img-cmds.h
>>> -   rm -f trace-dtrace.dtrace trace-dtrace.dtrace-timestamp
>>> @# May not be present in GENERATED_HEADERS
>>> -   rm -f trace-dtrace.h trace-dtrace.h-timestamp
>>> +   rm -f trace/generated-dtrace.dtrace 
>>> trace/generated-dtrace.dtrace-timestamp
>>> +   rm -f trace/generated-dtrace.h trace/generated-dtrace.h-timestamp
>>> rm -f $(foreach f,$(GENERATED_HEADERS),$(f) $(f)-timestamp)
>>> rm -f $(foreach f,$(GENERATED_SOURCES),$(f) $(f)-timestamp)
>>> rm -rf qapi-generated
>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index 3c7abca..24832a2 100644
>>> --- a/Makefile.objs
>>> +++ b/Makefile.objs
>>> @@ -147,66 +147,7 @@ libdis-$(CONFIG_LM32_DIS) += lm32-dis.o
>>> ######################################################################
>>> # trace
>>>
>>> -ifeq ($(TRACE_BACKEND),dtrace)
>>> -TRACE_H_EXTRA_DEPS=trace-dtrace.h
>>> -endif
>>> -trace.h: trace.h-timestamp $(TRACE_H_EXTRA_DEPS)
>>> -trace.h-timestamp: $(SRC_PATH)/trace-events $(BUILD_DIR)/config-host.mak
>>> -   $(call quiet-command,$(TRACETOOL) \
>>> -           --format=h \
>>> -           --backend=$(TRACE_BACKEND) \
>>> -           < $< > $@,"  GEN   trace.h")
>>> -   @cmp -s $@ trace.h || cp $@ trace.h
>>> -
>>> -trace.c: trace.c-timestamp
>>> -trace.c-timestamp: $(SRC_PATH)/trace-events $(BUILD_DIR)/config-host.mak
>>> -   $(call quiet-command,$(TRACETOOL) \
>>> -           --format=c \
>>> -           --backend=$(TRACE_BACKEND) \
>>> -           < $< > $@,"  GEN   trace.c")
>>> -   @cmp -s $@ trace.c || cp $@ trace.c
>>> -
>>> -trace.o: trace.c $(GENERATED_HEADERS)
>>> -
>>> -trace-dtrace.h: trace-dtrace.dtrace
>>> -   $(call quiet-command,dtrace -o $@ -h -s $<, "  GEN   trace-dtrace.h")
>>> -
>>> -# Normal practice is to name DTrace probe file with a '.d' extension
>>> -# but that gets picked up by QEMU's Makefile as an external dependency
>>> -# rule file. So we use '.dtrace' instead
>>> -trace-dtrace.dtrace: trace-dtrace.dtrace-timestamp
>>> -trace-dtrace.dtrace-timestamp: $(SRC_PATH)/trace-events 
>>> $(BUILD_DIR)/config-host.mak
>>> -   $(call quiet-command,$(TRACETOOL) \
>>> -           --format=d \
>>> -           --backend=$(TRACE_BACKEND) \
>>> -           < $< > $@,"  GEN   trace-dtrace.dtrace")
>>> -   @cmp -s $@ trace-dtrace.dtrace || cp $@ trace-dtrace.dtrace
>>> -
>>> -trace-dtrace.o: trace-dtrace.dtrace $(GENERATED_HEADERS)
>>> -   $(call quiet-command,dtrace -o $@ -G -s $<, "  GEN   trace-dtrace.o")
>>> -
>>> -ifeq ($(LIBTOOL),)
>>> -trace-dtrace.lo: trace-dtrace.dtrace
>>> -   @echo "missing libtool. please install and rerun configure."; exit 1
>>> -else
>>> -trace-dtrace.lo: trace-dtrace.dtrace
>>> -   $(call quiet-command,$(LIBTOOL) --mode=compile --tag=CC dtrace -o $@ -G 
>>> -s $<, "  lt GEN trace-dtrace.o")
>>> -endif
>>> -
>>> -trace/simple.o: trace/simple.c $(GENERATED_HEADERS)
>>> -
>>> -trace-obj-$(CONFIG_TRACE_DTRACE) += trace-dtrace.o
>>> -ifneq ($(TRACE_BACKEND),dtrace)
>>> -trace-obj-y = trace.o
>>> -endif
>>> -
>>> -trace-obj-$(CONFIG_TRACE_DEFAULT) += trace/default.o
>>> -trace-obj-$(CONFIG_TRACE_SIMPLE) += trace/simple.o
>>> -trace-obj-$(CONFIG_TRACE_SIMPLE) += qemu-timer-common.o
>>> -trace-obj-$(CONFIG_TRACE_STDERR) += trace/stderr.o
>>> -trace-obj-y += trace/control.o
>>> -
>>> -$(trace-obj-y): $(GENERATED_HEADERS)
>>> +trace-obj-y += trace/
> 
>> Please just put it into oslib-obj-y.
> 
> You mean line "trace-obj-y += trace/"?

Yeah, make it

oslib-obj-y += trace/

and get rid of trace-obj-y.

> 
>>> ######################################################################
>>> # smartcard
>>> @@ -250,5 +191,6 @@ nested-vars += \
>>> block-obj-y \
>>> user-obj-y \
>>> common-obj-y \
>>> -   extra-obj-y
>>> +   extra-obj-y \
>>> +   trace-obj-y
>>> dummy := $(call unnest-vars)
>>> diff --git a/scripts/tracetool/backend/dtrace.py 
>>> b/scripts/tracetool/backend/dtrace.py
>>> index 23c43e2..0cbc68a 100644
>>> --- a/scripts/tracetool/backend/dtrace.py
>>> +++ b/scripts/tracetool/backend/dtrace.py
>>> @@ -37,7 +37,7 @@ def c(events):
>>>
>>>
>>> def h(events):
>>> -    out('#include "trace-dtrace.h"',
>>> +    out('#include "trace/generated-dtrace.h"',
>>> '')
>>>
>>> for e in events:
>>> diff --git a/trace/Makefile.objs b/trace/Makefile.objs
>>> new file mode 100644
>>> index 0000000..d7e8cdf
>>> --- /dev/null
>>> +++ b/trace/Makefile.objs
>>> @@ -0,0 +1,81 @@
>>> +# -*- mode: makefile -*-
>>> +
>>> +trace/simple.o: trace/simple.c $(GENERATED_HEADERS)
>>> +
>>> +trace-obj-$(CONFIG_TRACE_DEFAULT) += default.o
>>> +trace-obj-$(CONFIG_TRACE_SIMPLE) += simple.o
>>> +trace-obj-$(CONFIG_TRACE_SIMPLE) += ../qemu-timer-common.o
> 
>> Is this line needed?  (Actually, it is obviously unneeded if you use
>> oslib-obj-y).  I am also worried that it causes duplicate symbols when
>> you link in both qemu-timer-common.o and trace/../qemu-timer-common.o.
>> The $(sort) invocation in the LINK macro of rules.mak will not treat
>> these two paths as duplicate.
> 
> "trace-obj-y" is required by some binaries other than QEMU itself, as they use
> routines that require the tracing infrastructure (e.g., qemu-img, which only
> includes "qemu-timer-common.o" as a dependency through "tools-obj-y").

Yes, but they all use oslib-obj-y too.  qemu-timer-common.o is part of
oslib-obj-y.

> I'm not sure how the subdir magic treats paths, but mapping all paths in final
> vars into their respective absolute path should simplify things.

Difficult to do in make. :(

> In any case, compiling with backends none, stderr, and stdout turns up no
> linking problems.

And simple too?  (I suppose stdout is a typo for simple).

> 
>>> +trace-obj-$(CONFIG_TRACE_STDERR) += stderr.o
>>> +trace-obj-y += control.o
>>> +
>>> +
>>> +######################################################################
>>> +# Auto-generated tracing routines
>>> +
>>> +# NOTE: "trace.h" is kept in the top-level dir to shorten common include 
>>> path
> 
>> Ok, I'll take care of moving it to trace/ later when I do the same for
>> all other includes.
> 
> I'll send a new version generating "trace/generated.h" and having "trace.h" 
> just
> include it for now.

Ok, thanks.

> 
>>> +ifeq ($(TRACE_BACKEND),dtrace)
>>> +TRACE_H_EXTRA_DEPS=trace/generated-dtrace.h
>>> +endif
>>> +trace.h: trace.h-timestamp $(TRACE_H_EXTRA_DEPS)
>>> +trace.h-timestamp: $(SRC_PATH)/trace-events $(BUILD_DIR)/config-host.mak
>>> +   $(call quiet-command,$(TRACETOOL) \
>>> +           --format=h \
>>> +           --backend=$(TRACE_BACKEND) \
>>> +           < $< > $@,"  GEN   trace.h")
>>> +   @cmp -s $@ trace.h || cp $@ trace.h
>>> +
>>> +
>>> +trace/generated.c: trace/generated.c-timestamp
> 
>> Please use $(obj)/generated.c, and similarly for everything else.
> 
> Tried it, but "obj" is set to ".", although looking at "rules.mak" that should
> not be the case...
> 
> Inserting "$(error obj=$(obj))" in "trace/Makefile.objs" shows "./trace", but
> echoing "obj" in any of the rules shows ".".

In the rules it is, because the rules expand variables later when the
command runs.  But in the targets it should be evaluated correctly,
because the targets expand variables earlier when they are read.  As
long as you use $@ and $< and $^ in the rules, you're safe.  Anyway,
this can be done later.

> BTW, can I extend the GENERATED_HEADERS variable contents right from
> "trace/Makefile.objs" even though it's not explicitly included from the
> top-level makefile?

Hmm, risky. :)  Need to look at the actual patch, let's do one thing at
a time.

> I'm sure this has already been previously discussed to the point of 
> extenuation,
> but what are the reasons for not using autotools?

Autoconf -> no point, but someone needs to do the work.

Automake -> the build system is just too different.

Libtool -> using it already. :)

Paolo