[RFC][PATCH 00/15] tools: Unify perf and trace-cmd trace event format parsing v2

On Thu, Apr 5, 2012 at 3:47 PM, Frederic Weisbecker wrote:

Hi,

So this is a new iteration of the libtracevent library, basically a
rebase of https://lkml.org/lkml/2011/8/5/299 against latest progresses
(latest tip/perf/core + tip/perf/urgent).

This library unifies the trace events parsing code between perf and
trace-cmd. I initially took this parsing code from trace-cmd to make
perf able to display trace-events and play with their contents.

Hi Frederic. Thanks for taking this on.

Could you consider adding these patches to this set?
https://lkml.org/lkml/2011/3/21/335

On Fri, Apr 06, 2012 at 12:47:52AM +0200, Frederic Weisbecker wrote:

From: Steven Rostedt

Move the trace-event-parse.c code that originally came from trace-cmd into
their own files. The new file will be called trace-parse-events.c, as
the name of trace-cmd's file was parse-events.c too, but it conflicted
with the parse-events.c file in perf that parses the command line.

This tries to update the code with mimimal changes.

Perf specific code stays in the trace-event-parse.[ch] files and
the common parsing code is now in trace-parse-events.c and
trace-parse-events.h.

Signed-off-by: Steven Rostedt
Cc: Ingo Molnar
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: Arnaldo Carvalho de Melo
Cc: Steven Rostedt
Cc: Borislav Petkov
Cc: Jiri Olsa
Cc: Arun Sharma
Cc: Namhyung Kim
Signed-off-by: Frederic Weisbecker

 tools/perf/Makefile                  |    2 +
 tools/perf/util/trace-event-parse.c  | 3135 -
 tools/perf/util/trace-event.h        |  275 +
 tools/perf/util/trace-parse-events.c | 3125 +++++++++++++++++++++++++++++++++
 tools/perf/util/trace-parse-events.h |  273 +++
 5 files changed, 3405 insertions(+), 3405 deletions(-)
 create mode 100644 tools/perf/util/trace-parse-events.c
 create mode 100644 tools/perf/util/trace-parse-events.h

Err, maybe I'm missing something but why this additional step? I mean,
you could do:

patch 1: Add libtraceevent.a from trace-cmd
patch 2: Fixup perf so that it is ready to be switched to the new lib (specific
               wrappers, etc)
patch 3: switch it to libtraceevent.a

Only three patches, right?

On Fri, Apr 06, 2012 at 12:47:54AM +0200, Frederic Weisbecker wrote:

From: Steven Rostedt

Have building perf also build libtraceevent.a. Currently, perf does
not use the code within libtraceevent.a, but it soon will.

Signed-off-by: Steven Rostedt
Cc: Ingo Molnar
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: Arnaldo Carvalho de Melo
Cc: Steven Rostedt
Cc: Borislav Petkov
Cc: Jiri Olsa
Cc: Arun Sharma
Cc: Namhyung Kim
Signed-off-by: Frederic Weisbecker

 tools/perf/Makefile |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 6f93a99..8e60e51 100644
a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -1,5 +1,6 @@
 ifeq ("$(origin O)", "command line")
      OUTPUT := $(O)/
+     COMMAND_O := O=$(O)
 endif

 # The default target of this Makefile is...
@@ -211,6 +212,9 @@ $(OUTPUT)python/perf.so: $(PYRF_OBJS) $(PYTHON_EXT_SRCS) $(PYTHON_EXT_DEPS)

 SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH))

+EVENT_PARSE_DIR = ../lib/traceevent/
+LIBTRACEEVENT = $(OUTPUT)$(EVENT_PARSE_DIR)libtraceevent.a
+
 #
 # Single 'perf' binary right now:
 #
@@ -335,6 +339,7 @@ LIB_H += util/cpumap.h
 LIB_H += util/top.h
 LIB_H += $(ARCH_INCLUDE)
 LIB_H += util/cgroup.h
+LIB_H += $(EVENT_PARSE_DIR)event-parse.h

 LIB_OBJS += $(OUTPUT)util/abspath.o
 LIB_OBJS += $(OUTPUT)util/alias.o
@@ -741,7 +746,7 @@ $(OUTPUT)perf.o: perf.c $(OUTPUT)common-cmds.h $(OUTPUT)PERF-CFLAGS
              '-DPERF_HTML_PATH="$(htmldir_SQ)"' \
              $(ALL_CFLAGS) -c $(filter %.c,$^) -o $@

-$(OUTPUT)perf: $(OUTPUT)perf.o $(BUILTIN_OBJS) $(PERFLIBS)
+$(OUTPUT)perf: $(OUTPUT)perf.o $(BUILTIN_OBJS) $(PERFLIBS) $(LIBTRACEEVENT)
      $(QUIET_LINK)$(CC) $(ALL_CFLAGS) $(ALL_LDFLAGS) $(OUTPUT)perf.o \
                $(BUILTIN_OBJS) $(LIBS) -o $@

@@ -847,6 +852,10 @@ $(sort $(dir $(DIRECTORY_DEPS))):
 $(LIB_FILE): $(LIB_OBJS)
      $(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(LIB_OBJS)

+# libparsevent.a
+$(LIBTRACEEVENT):
+     make -C $(EVENT_PARSE_DIR) $(COMMAND_O) libtraceevent.a
+

With a toplevel Makefile in tools/, this should be made into its own
target residing in tools/lib/traceevent/Makefile and a perf build should
be dependent on it instead of adding a perf-specific libtraceevent
target here.

2012/4/6 Borislav Petkov :
> On Fri, Apr 06, 2012 at 12:47:54AM +0200, Frederic Weisbecker wrote:
>> From: Steven Rostedt
>>
>> Have building perf also build libtraceevent.a. Currently, perf does
>> not use the code within libtraceevent.a, but it soon will.
>>
>> Signed-off-by: Steven Rostedt
>> Cc: Ingo Molnar
>> Cc: Thomas Gleixner
>> Cc: Peter Zijlstra
>> Cc: Arnaldo Carvalho de Melo
>> Cc: Steven Rostedt
>> Cc: Borislav Petkov
>> Cc: Jiri Olsa
>> Cc: Arun Sharma
>> Cc: Namhyung Kim
>> Signed-off-by: Frederic Weisbecker
>>
>>  tools/perf/Makefile |   11 ++++++++++-
>>  1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
>> index 6f93a99..8e60e51 100644
>> a/tools/perf/Makefile
>> +++ b/tools/perf/Makefile
>> @@ -1,5 +1,6 @@
>>  ifeq ("$(origin O)", "command line")
>>       OUTPUT := $(O)/
>> +     COMMAND_O := O=$(O)
>>  endif
>>
>>  # The default target of this Makefile is...
>> @@ -211,6 +212,9 @@ $(OUTPUT)python/perf.so: $(PYRF_OBJS) $(PYTHON_EXT_SRCS) $(PYTHON_EXT_DEPS)
>>
>>  SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH))
>>
>> +EVENT_PARSE_DIR = ../lib/traceevent/
>> +LIBTRACEEVENT = $(OUTPUT)$(EVENT_PARSE_DIR)libtraceevent.a
>> +
>>  #
>>  # Single 'perf' binary right now:
>>  #
>> @@ -335,6 +339,7 @@ LIB_H += util/cpumap.h
>>  LIB_H += util/top.h
>>  LIB_H += $(ARCH_INCLUDE)
>>  LIB_H += util/cgroup.h
>> +LIB_H += $(EVENT_PARSE_DIR)event-parse.h
>>
>>  LIB_OBJS += $(OUTPUT)util/abspath.o
>>  LIB_OBJS += $(OUTPUT)util/alias.o
>> @@ -741,7 +746,7 @@ $(OUTPUT)perf.o: perf.c $(OUTPUT)common-cmds.h $(OUTPUT)PERF-CFLAGS
>>               '-DPERF_HTML_PATH="$(htmldir_SQ)"' \
>>               $(ALL_CFLAGS) -c $(filter %.c,$^) -o $@
>>
>> -$(OUTPUT)perf: $(OUTPUT)perf.o $(BUILTIN_OBJS) $(PERFLIBS)
>> +$(OUTPUT)perf: $(OUTPUT)perf.o $(BUILTIN_OBJS) $(PERFLIBS) $(LIBTRACEEVENT)
>>       $(QUIET_LINK)$(CC) $(ALL_CFLAGS) $(ALL_LDFLAGS) $(OUTPUT)perf.o \
>>                 $(BUILTIN_OBJS) $(LIBS) -o $@
>>
>> @@ -847,6 +852,10 @@ $(sort $(dir $(DIRECTORY_DEPS))):
>>  $(LIB_FILE): $(LIB_OBJS)
>>       $(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(LIB_OBJS)
>>
>> +# libparsevent.a
>> +$(LIBTRACEEVENT):
>> +     make -C $(EVENT_PARSE_DIR) $(COMMAND_O) libtraceevent.a
>> +
>
> With a toplevel Makefile in tools/, this should be made into its own
> target residing in tools/lib/traceevent/Makefile and a perf build should
> be dependent on it instead of adding a perf-specific libtraceevent
> target here.

Not sure what you mean. The libtraceevent makefile is standalone and
perf is dependant on it.
What am I missing?

> Yeah, the name "do_warning" is awkward. Can we call it emit_warning or
> _warning or whatever?

As this is just a macro defined as a helper in this file (not global),
I'm not sure it needs to be changed. "emit" has two more characters to
type over "do" ;-)

> I see you're using kernel-doc style comments. Is this aimed at providing
> documentation for this library? If so, good idea!

That was the plan. It also forced me to document the API.

> > + * pevent_buffer_init - init buffer for parsing
> > + * @buf: buffer to parse
> > + * @size: the size of the buffer
> > + *
> > + * For use with pevent_read_token(), this initializes the internal
> > + * buffer that pevent_read_token() will parse.
> > + */
> > +void pevent_buffer_init(const char *buf, unsigned long long size)
> > +{
> > + init_input_buf(buf, size);
> > +}
>
> Can't we unify pevent_buffer_init and init_input_buf()? I mean, it is
> useless to have that kind of indirection.

Yeah I agree. This was more of a historical artifact. It use to be just
init_input_buf() then I needed to export it for other users. Instead of
doing a full rename, I cheesed out and did this.

> In general, is "pevent" the prefix for this library's functions which
> are getting exported to users?

Yep. It's the "namespace" used. But if people do not like it, we can
come up with another name. I just want it to be a short prefix.

> > + * pevent_print_funcs - print out the stored functions
> > + * @pevent: handle for the pevent
> > + *
> > + * This prints out the stored functions.
> > + */
> > +void pevent_print_funcs(struct pevent *pevent)
> > +{
> > + int i;
> > +
> > + if (!pevent->func_map)
> > + func_map_init(pevent);
> > +
> > + for (i = 0; i < (int)pevent->func_count; i++) {
>
> simply declare i as "unsigned int" and you don't need the cast here.

I think I did that once but it caused problems elsewhere. But we can
look at it again.

>
> Ok, enough for today. I'll review the rest only if you really want me to
> :-).
>

Sure thanks for the comments now. The checkpatch clean ups should
probably be fixed before this goes in, and removing the breakpoint
function. But the other changes probably should wait. I would like what
goes in to be as close to what is in trace-cmd as possible. Perhaps, we
should add the cleanups as a separate patch. Just to have a nice
transition.