Re: [Qemu-devel] [RFC PATCH v3 1/3] Converting tracetool.sh to tracetool.py

[Harsh Bora <harsh@linux.vnet.ibm.com>]

Hi Stefan,

Thanks for an early review, I shall address your comments in next version.

Also, please confirm, whether I should work on top for qemu.git or your 
tracing branch on repo.or.cz/stefanha.git ?

regards,
Harsh

On 01/10/2012 08:20 PM, Stefan Hajnoczi wrote:
> On Tue, Jan 10, 2012 at 10:59 AM, Harsh Prateek Bora
> <harsh@linux.vnet.ibm.com>  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.
>