On Tue, Jan 10, 2012 at 10:59 AM, Harsh Prateek Bora
<address@hidden> wrote:
diff --git a/scripts/tracetool.py b/scripts/tracetool.py
new file mode 100755
index 0000000..6874f66
--- /dev/null
+++ b/scripts/tracetool.py
@@ -0,0 +1,585 @@
+#!/usr/bin/env python
+# Python based tracetool script (Code generator for trace events)
Nitpick: please don't use comments or names that reflect the fact that
you're changing something. Once this is merged readers might not even
know there was a non-Python tracetool script. "Code generator for
trace events" explains what this script does. "Python based tracetool
script" doesn't add any information because the content is clearly in
Python and the filename is tracetool.
A related point is please don't put simpletrace "v2" into the code
because if we ever do a v3 all those references would need to be
renamed, even if the value stays the same. (For example
ST_V2_REC_HDR_LEN.) When changing code make it look like it was
written like this from day 1 rather than leaving visible marks where
things were modified - git log already provides the code change
history. It's distracting and sometimes misleading when code contains
references to outdated things which have been replaced.
+#
+# Copyright IBM, Corp. 2011
+#
+# This work is licensed under the terms of the GNU GPL, version 2. See
+# the COPYING file in the top-level directory.
+#
+#
+
+import sys
+import getopt
+
+def usage():
+ print "Tracetool: Generate tracing code for trace events file on stdin"
+ print "Usage:"
+ print sys.argv[0], " --backend=[nop|simple|stderr|dtrace|ust]
[-h|-c|-d|--stap]"
print 'a', 'b' puts a space between "a" and "b". "--backend=..."
would work fine, space is not needed.
+def calc_sizeofargs(line):
+ args = get_args(line)
+ argc = get_argc(line)
+ strtype = ('const char*', 'char*', 'const char *', 'char *')
This is repeated elsewhere, please share is_string().
+def trace_h_begin():
+ print '''#ifndef TRACE_H
+#define TRACE_H
+
+/* This file is autogenerated by tracetool, do not edit. */
+
+#include "qemu-common.h"'''
+ return
A function that has no explicit return statement implicitly returns
None. Please remove returns to keep the code concise.
+
+def trace_h_end():
+ print '#endif /* TRACE_H */'
+ return
+
+def trace_c_begin():
+ print '/* This file is autogenerated by tracetool, do not edit. */'
+ return
+
+def trace_c_end():
+ # nop, required for trace_gen
+ return
def trace_c_end():
pass # nop, required for trace_gen
+def simple_c(events):
+ rec_off = 0
+ print '#include "trace.h"'
+ print '#include "trace/simple.h"'
+ print
+ print 'TraceEvent trace_list[] = {'
+ print
+ eventlist = list(events)
+ for event in eventlist:
+ print '{.tp_name = "%(name)s", .state=0},' % {
+ 'name': event.name
+}
+ print
+ print '};'
+ print
+ for event in eventlist:
+ argc = event.argc
+ print '''void trace_%(name)s(%(args)s)
+{
+ unsigned int tbuf_idx, rec_off;
CC trace.o
trace.c: In function ‘trace_slavio_misc_update_irq_raise’:
trace.c:3411:28: warning: variable ‘rec_off’ set but not used
[-Wunused-but-set-variable]
+ uint64_t var64 __attribute__ ((unused));
+ uint64_t pvar64 __attribute__ ((unused));
+ uint32_t slen __attribute__ ((unused));
+
+ if (!trace_list[%(event_id)s].state) {
+ return;
+ }
+''' % {
+ 'name': event.name,
+ 'args': event.args,
+ 'event_id': event.num,
+}
+ print '''
+ tbuf_idx = trace_alloc_record(%(event_id)s, %(sizestr)s);
+ rec_off = (tbuf_idx + ST_V2_REC_HDR_LEN) %% TRACE_BUF_LEN; /* seek record
header */
This can be cleaned up with the following interface:
/**
* Initialize a trace record and claim space for it in the buffer
*
* @arglen number of bytes required for arguments
*/
void trace_record_start(TraceBufferRecord *rec, TraceEventID id, size_t arglen);
/**
* Append a 64-bit argument to a trace record
*/
void trace_record_write_u64(TraceBufferRecord *rec, uint64_t val);
/**
* Append a string argument to a trace record
*/
void trace_record_write_str(TraceBufferRecord *rec, const char *s);
/**
* Mark a trace record completed
*
* Don't append any more arguments to the trace record after calling this.
*/
void trace_record_finish(TraceBufferRecord *rec);
Then tracetool.py and trace.c don't need to manage offsets or worry
about temporary variables. We also don't need to expose
ST_V2_REC_HDR_LEN here or wrap by TRACE_BUF_LEN.
The TraceBufferRecord encapsulates rec_off so that we don't need to
emit code that increments it. Instead trace_record_write_*() handles
that.
+def stderr_h(events):
+ print '''#include<stdio.h>
+#include "trace/stderr.h"
+
+extern TraceEvent trace_list[];'''
+ for event in events:
+ argnames = event.argnames
+ if event.argc> 0:
+ argnames = ', ' + event.argnames
+ else:
+ argnames = ''
+ print '''
+static inline void trace_%(name)s(%(args)s)
+{
+ if (trace_list[%(event_num)s].state != 0) {
+ fprintf(stderr, "%(name)s " %(fmt)s "\\n" %(argnames)s);
+ }
+}''' % {
+ 'name': event.name,
+ 'args': event.args,
+ 'event_num': event.num,
+ 'fmt': event.fmt.rstrip('\n'),
We shouldn't be parsing the input here, please move the .rstrip() to
where the trace-events input gets parsed.
+def trace_stap_begin():
+ global probeprefix
+ if backend != "dtrace":
+ print 'SystemTAP tapset generator not applicable to %s backend' %
backend
+ sys.exit(1)
+ if binary == "":
+ print '--binary is required for SystemTAP tapset generator'
+ sys.exit(1)
+ if ((probeprefix == "") and (targettype == "")):
The parentheses are unnecessary. Here are two other ways of writing this:
if not probeprefix and not targettype:
or
if probeprefix == '' and targettype == '':
+# Registry of backends and their converter functions
+converters = {
+ 'simple': {
+ 'h': simple_h,
+ 'c': simple_c,
Missing space.
+# Generator that yields Event objects given a trace-events file object
+def read_events(fobj):
+ event_num = 0
+ for line in fobj:
+ if not line.strip():
+ continue
+ if line.lstrip().startswith('#'):
+ continue
+ yield Event(event_num, line)
+ event_num += 1
Indentation.
+
+backend = ""
+output = ""
+binary = ""
+targettype = ""
+targetarch = ""
+probeprefix = ""
+
+def main():
+ global backend, output, binary, targettype, targetarch, probeprefix
Please avoid the globals, it tempts people to hack in bad things in
the future. The only globals that are hard to avoid are binary and
probeprefix.
If you move all the checks to main() before we start emitting output,
then there's no need to use most of these globals. In a way it makes
sense to do checks that abort before entering the main program.