Re: [Qemu-devel] [PATCH v2 01/47] trace: introduce some Makefile rules for module code gen

[Lluís Vilanova]

Daniel P Berrange writes:

> On Fri, Jan 06, 2017 at 06:20:51PM +0100, Lluís Vilanova wrote:
>> Daniel P Berrange writes:
>> 
>> > Introduce rules that are able to generate trace.[ch] files
>> > in every subdirectory which has a trace-events file.
>> 
>> > Signed-off-by: Daniel P. Berrange <address@hidden>
>> > ---
>> >  .gitignore                               |  6 +++
>> >  Makefile                                 | 65 
>> > +++++++++++++++++++++++++++-----
>> >  Makefile.objs                            |  5 +++
>> >  Makefile.target                          |  7 +++-
>> >  scripts/tracetool/backend/dtrace.py      |  7 +++-
>> >  scripts/tracetool/backend/ust.py         |  7 +++-
>> >  scripts/tracetool/format/ust_events_c.py |  2 +-
>> >  scripts/tracetool/format/ust_events_h.py |  7 +++-
>> >  tests/Makefile.include                   |  2 +-
>> >  trace/Makefile.objs                      | 23 ++++++++---
>> >  10 files changed, 109 insertions(+), 22 deletions(-)
>> 
>> > diff --git a/.gitignore b/.gitignore
>> > index e43c304..fe6add3 100644
>> > --- a/.gitignore
>> > +++ b/.gitignore
>> > @@ -111,3 +111,9 @@ tags
>> >  TAGS
>> >  docker-src.*
>> >  *~
>> > +trace.h
>> > +trace.c
>> > +trace-events-common
>> > +trace-ust.h
>> > +trace-dtrace.h
>> > +trace-dtrace.dtrace
>> > diff --git a/Makefile b/Makefile
>> > index 214cbad..181802b 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -70,11 +70,53 @@ GENERATED_SOURCES += trace/generated-helpers.c
>> 
>> >  ifeq ($(findstring ust,$(TRACE_BACKENDS)),ust)
>> >  GENERATED_HEADERS += trace/generated-ust-provider.h
>> > +GENERATED_HEADERS += trace/generated-ust-provider-all.h
>> >  GENERATED_SOURCES += trace/generated-ust.c
>> >  endif
>> 
>> >  GENERATED_HEADERS += module_block.h
>> 
>> > +GENERATED_HEADERS += $(trace-events-subdirs:%=%/trace.h)
>> > +GENERATED_SOURCES += $(trace-events-subdirs:%=%/trace.c)
>> > +GENERATED_DTRACE =
>> > +ifdef CONFIG_TRACE_DTRACE
>> > +GENERATED_HEADERS += $(trace-events-subdirs:%=%/trace-dtrace.h)
>> > +GENERATED_DTRACE += $(trace-events-subdirs:%=%/trace-dtrace.dtrace)
>> > +endif
>> > +ifdef CONFIG_TRACE_UST
>> > +GENERATED_HEADERS += $(trace-events-subdirs:%=%/trace-ust.h)
>> > +endif
>> > +
>> > +%/trace.h: $(SRC_PATH)/%/trace-events $(tracetool-y)
>> > +  $(call quiet-command,$(TRACETOOL) \
>> > +          --format=h \
>> > +          --backends=$(TRACE_BACKENDS) \
>> > +          $< > $@,"GEN","$@")
>> > +
>> > +%/trace.c: $(SRC_PATH)/%/trace-events $(tracetool-y)
>> > +  $(call quiet-command,$(TRACETOOL) \
>> > +          --format=c \
>> > +          --backends=$(TRACE_BACKENDS) \
>> > +          $< > $@,"GEN","$@")
>> > +
>> > +%/trace-ust.h: $(SRC_PATH)/%/trace-events $(tracetool-y)
>> > +  $(call quiet-command,$(TRACETOOL) \
>> > +          --format=ust-events-h \
>> > +          --backends=$(TRACE_BACKENDS) \
>> > +          $< > $@,"GEN","$@")
>> 
>> There's a discrepancy between generation in trace/Makefile.objs, where 
>> timestamp
>> files are used to filter out internal changes in tracetool, and here.

> I took the view that the timestamp comparisons are an uneccessary
> complexity - we don't do that for other places where we generate
> code from scripts.

AFAIR, the idea was to avoid re-building all QEMU when any of the tracetool
script files change without affecting all the generated files.


>> > diff --git a/scripts/tracetool/backend/dtrace.py 
>> > b/scripts/tracetool/backend/dtrace.py
>> > index 79505c6..1319990 100644
>> > --- a/scripts/tracetool/backend/dtrace.py
>> > +++ b/scripts/tracetool/backend/dtrace.py
>> > @@ -36,7 +36,12 @@ def binary():
>> 
>> 
>> >  def generate_h_begin(events, group):
>> > -    out('#include "trace/generated-tracers-dtrace.h"',
>> > +    if group == "common":
>> > +        include = "trace/generated-tracers-dtrace.h"
>> > +    else:
>> > +        include = "trace-dtrace.h"
>> > +
>> > +    out('#include "%s"' % include,
>> >          '')
>> 
>> Why is the top-level events file treated as a special case?

> This is temporary until the end of the series when the special
> cases go away

Yes, I saw you removed this special-casing (except in one case) in a later
patch. Sorry about that.


Thanks,
  Lluis