From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53986) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rkrdj-0005Xl-R3 for qemu-devel@nongnu.org; Wed, 11 Jan 2012 01:26:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rkrdi-0005x5-5t for qemu-devel@nongnu.org; Wed, 11 Jan 2012 01:26:27 -0500 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:51283) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rkrdh-0005ww-9U for qemu-devel@nongnu.org; Wed, 11 Jan 2012 01:26:26 -0500 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 11 Jan 2012 06:11:52 +1000 Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q0B6LIUE3371066 for ; Wed, 11 Jan 2012 17:21:21 +1100 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q0B6PeEv019542 for ; Wed, 11 Jan 2012 17:25:40 +1100 Message-ID: <4F0D2B61.9060008@linux.vnet.ibm.com> Date: Wed, 11 Jan 2012 11:55:37 +0530 From: Harsh Bora MIME-Version: 1.0 References: <1326193179-19677-1-git-send-email-harsh@linux.vnet.ibm.com> <1326193179-19677-2-git-send-email-harsh@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC PATCH v3 1/3] Converting tracetool.sh to tracetool.py List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , stefanha@linux.vnet.ibm.com Cc: vilanova@ac.upc.edu, mathieu.desnoyers@efficios.com, qemu-devel@nongnu.org, aneesh.kumar@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 > 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 >> +#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. >