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

[Lluís Vilanova]

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/"?


>> ######################################################################
>> # 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").

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.

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


>> +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.


>> +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 ".".


>> +trace/generated.c-timestamp: $(SRC_PATH)/trace-events 
>> $(BUILD_DIR)/config-host.mak
>> +    $(call quiet-command,$(TRACETOOL) \
>> +            --format=c \
>> +            --backend=$(TRACE_BACKEND) \
>> +            < $< > $@,"  GEN   trace/generated.c")
>> +    @cmp -s $@ trace/generated.c || cp $@ trace/generated.c
>> +
>> +trace/generated.o: trace/generated.c trace.h
>> +
>> +
>> +ifneq ($(TRACE_BACKEND),dtrace)
>> +trace-obj-y += generated.o
>> +endif
>> +
>> +
>> +######################################################################
>> +# Auto-generated DTrace code
>> +
>> +# 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/generated-dtrace.dtrace: trace/generated-dtrace.dtrace-timestamp
>> +trace/generated-dtrace.dtrace-timestamp: $(SRC_PATH)/trace-events 
>> $(BUILD_DIR)/config-host.mak
>> +    $(call quiet-command,$(TRACETOOL) \
>> +            --format=d \
>> +            --backend=$(TRACE_BACKEND) \
>> +            < $< > $@,"  GEN   trace/generated-dtrace.dtrace")
>> +    @cmp -s $@ trace/generated-dtrace.dtrace || cp $@ 
>> trace/generated-dtrace.dtrace
>> +
>> +
>> +trace/generated-dtrace.h: trace/generated-dtrace.dtrace
>> +    $(call quiet-command,dtrace -o $@ -h -s $<, "  GEN   
>> trace/generated-dtrace.h")
>> +
>> +trace/generated-dtrace.o: trace/generated-dtrace.dtrace $(GENERATED_HEADERS)
>> +    $(call quiet-command,dtrace -o $@ -G -s $<, "  GEN   
>> trace/generated-dtrace.o")
>> +
>> +
>> +ifeq ($(LIBTOOL),)
>> +trace/generated-dtrace.lo: trace/generated-dtrace.dtrace
>> +    @echo "missing libtool. please install and rerun configure."; exit 1
>> +else
>> +trace/generated-dtrace.lo: trace/generated-dtrace.dtrace
>> +    $(call quiet-command,$(LIBTOOL) --mode=compile --tag=CC dtrace -o $@ -G 
>> -s $<, "  lt GEN trace/generated-dtrace.lo")
>> +endif
>> +
>> +
>> +trace-obj-$(CONFIG_TRACE_DTRACE) += generated-dtrace.o
>> +
>> +
>> +######################################################################
>> +# Keep at bottom
>> +
>> +$(trace-obj-y): $(GENERATED_HEADERS)

> This does not do what you think, because trace-obj-y includes a relative
> path.  It can be simply

> trace/%.o: $(GENERATED_HEADERS)

> but it is actually unnecessary because of this in the toplevel:

> # Add a dependency on the generated files, so that they are always
> # rebuilt before other object files
> ifneq ($(filter-out %clean,$(MAKECMDGOALS)),$(if $(MAKECMDGOALS),,fail))
> Makefile: $(GENERATED_HEADERS)
> endif

Nice, I wasn't aware of this addition. Then I suppose none of the rules need to
depend on GENERATED_HEADERS.


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?


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


Thanks,
  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