Re: [Qemu-devel] [PATCH v4 07/11] trace: [tracetool] Rewrite event argument parsing

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

On 02/10/2012 05:25 PM, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova<vilanova@ac.upc.edu>
> ---
>   scripts/tracetool.py |  190 ++++++++++++++++++++++++--------------------------
>   1 files changed, 91 insertions(+), 99 deletions(-)
>
> diff --git a/scripts/tracetool.py b/scripts/tracetool.py
> index f2bcb65..cd1c29d 100755
> --- a/scripts/tracetool.py
> +++ b/scripts/tracetool.py
> @@ -38,49 +38,6 @@ Options:
>   '''
>       sys.exit(1)
>
> -def get_argnames(args):
> -    nfields = 0
> -    str = []
> -    for field in args.split():
> -      nfields = nfields + 1
> -      # Drop pointer star
> -      type, ptr, tail = field.partition('*')
> -      if type != field:
> -        field = tail
> -
> -      name, sep, tail = field.partition(',')
> -
> -      if name == field:
> -        continue
> -      str.append(name)
> -      str.append(", ")
> -
> -    if nfields>  1:
> -      str.append(name)
> -      return ''.join(str)
> -    else:
> -      return ''
> -
> -def calc_sizeofargs(args, argc):
> -    strtype = ('const char*', 'char*', 'const char *', 'char *')
> -    str = []
> -    newstr = ""
> -    if argc>  0:
> -      str = args.split(',')
> -      for elem in str:
> -        if elem.lstrip().startswith(strtype): #strings
> -          type, sep, var = elem.rpartition('*')
> -          newstr = newstr+"4 + strlen("+var.lstrip()+") + "
> -        #elif '*' in elem:
> -        #  newstr = newstr + "4 + " # pointer vars
> -        else:
> -          #type, sep, var = elem.rpartition(' ')
> -          #newstr = newstr+"sizeof("+type.lstrip()+") + "
> -          newstr = newstr + '8 + '
> -    newstr = newstr + '0' # for last +
> -    return newstr
> -
> -
>   def trace_h_begin():
>       print '''#ifndef TRACE_H
>   #define TRACE_H
> @@ -133,13 +90,6 @@ def simple_h(events):
>
>       return
>
> -def is_string(arg):
> -    strtype = ('const char*', 'char*', 'const char *', 'char *')
> -    if arg.lstrip().startswith(strtype):
> -        return True
> -    else:
> -        return False
> -
>   def simple_c(events):
>       rec_off = 0
>       print '#include "trace.h"'
> @@ -154,8 +104,16 @@ def simple_c(events):
>           print
>       print '};'
>       print
> +
>       for num, event in enumerate(events):
> -        argc = event.argc
> +        sizes = []
> +        for type_, name in event.args:
> +            if type_is_string(type_):
> +                sizes.append("4 + strlen(%s)" % name)



> +            else:
> +                sizes.append("8 + sizeof(%s)" % type_)
> +        sizestr = " + ".join(sizes)
> +

Applied with a small change as reqd:

+            else:
+                sizes.append("8")
+        sizestr = " + ".join(sizes)
+        if len(event.args) == 0:
+            sizestr = '0'
+

BTW, I am manually applying your changes on top of my patches as there 
were significant changes in my patches also. I will include your patches 
in my next series.

- Harsh


>           print '''void trace_%(name)s(%(args)s)
>   {
>       unsigned int tbuf_idx, rec_off __attribute__((unused));
> @@ -166,52 +124,52 @@ def simple_c(events):
>       if (!trace_list[%(event_id)s].state) {
>           return;
>       }
> +
> +    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 */
>   ''' % {
>       'name': event.name,
>       'args': event.args,
>       'event_id': num,
> +    'sizestr' : sizestr,
>   }
> -        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 */
> -''' % {'event_id': num, 'sizestr': event.sizestr,}
>
> -        if argc>  0:
> -            str = event.arglist
> -            for elem in str:
> -                if is_string(elem): # if string
> -                    type, sep, var = elem.rpartition('*')
> +        if len(event.args)>  0:
> +            for type_, name in event.args:
> +                # string
> +                if type_is_string(type_):
>                       print '''
> -    slen = strlen(%(var)s);
> +    slen = strlen(%(name)s);
>       write_to_buffer(rec_off, (uint8_t*)&slen, sizeof(slen));
>       rec_off += sizeof(slen);''' % {
> -    'var': var.lstrip()
> +    'name': name
>   }
>                       print '''
> -    write_to_buffer(rec_off, (uint8_t*)%(var)s, slen);
> +    write_to_buffer(rec_off, (uint8_t*)%(name)s, slen);
>       rec_off += slen;''' % {
> -    'var': var.lstrip()
> +    'name': name
>   }
> -                elif '*' in elem: # pointer var (not string)
> -                    type, sep, var = elem.rpartition('*')
> +                # pointer var (not string)
> +                elif type_.endswith('*'):
>                       print '''
> -    pvar64 = (uint64_t)(uint64_t*)%(var)s;
> +    pvar64 = (uint64_t)(uint64_t*)%(name)s;
>       write_to_buffer(rec_off, (uint8_t*)&pvar64, sizeof(uint64_t));
>       rec_off += sizeof(uint64_t);''' % {
> -    'var': var.lstrip()
> +    'name': name
>   }
> -                else: # primitive data type
> -                    type, sep, var = elem.rpartition(' ')
> +                # primitive data type
> +                else:
>                       print '''
> -    var64 = (uint64_t)%(var)s;
> +    var64 = (uint64_t)%(name)s;
>       write_to_buffer(rec_off, (uint8_t*)&var64, sizeof(uint64_t));
>       rec_off += sizeof(uint64_t);''' % {
> -    'var': var.lstrip()
> +    'name': name
>   }
>           print '''
> -    trace_mark_record_complete(tbuf_idx);'''
> -        print '}'
> -        print
> +    trace_mark_record_complete(tbuf_idx);
> +}
> +
> +'''
>
>       return
>
> @@ -220,12 +178,11 @@ def stderr_h(events):
>   #include "trace/stderr.h"
>
>   extern TraceEvent trace_list[];'''
> +
>       for num, event in enumerate(events):
> -        argnames = event.argnames
> -        if event.argc>  0:
> -            argnames = ', ' + event.argnames
> -        else:
> -            argnames = ''
> +        argnames = ", ".join(event.args.names())
> +        if len(event.args)>  0:
> +            argnames = ", "+argnames
>           print '''
>   static inline void trace_%(name)s(%(args)s)
>   {
> @@ -262,13 +219,13 @@ def ust_h(events):
>   #undef wmb'''
>
>       for event in events:
> -        if event.argc>  0:
> +        if len(event.args)>  0:
>               print '''
>   DECLARE_TRACE(ust_%(name)s, TP_PROTO(%(args)s), TP_ARGS(%(argnames)s));
>   #define trace_%(name)s trace_ust_%(name)s''' % {
>       'name': event.name,
>       'args': event.args,
> -    'argnames': event.argnames
> +    'argnames': ", ".join(event.args.names())
>   }
>           else:
>               print '''
> @@ -287,9 +244,9 @@ def ust_c(events):
>   #undef wmb
>   #include "trace.h"'''
>       for event in events:
> -        argnames = event.argnames
> -        if event.argc>  0:
> -            argnames = ', ' + event.argnames
> +        argnames = ", ".join(event.args.names())
> +        if len(event.args)>  0:
> +            argnames = ', ' + argnames
>               print '''
>   DEFINE_TRACE(ust_%(name)s);
>
> @@ -339,7 +296,7 @@ def dtrace_h(events):
>       'name': event.name,
>       'args': event.args,
>       'uppername': event.name.upper(),
> -    'argnames': event.argnames,
> +    'argnames': ", ".join(event.args.names()),
>   }
>
>   def dtrace_c(events):
> @@ -379,12 +336,12 @@ probe %(probeprefix)s.%(name)s = process("%(binary)s").mark("%(name)s")
>       'binary': binary
>   }
>           i = 1
> -        if event.argc>  0:
> -            for arg in event.argnames.split(','):
> +        if len(event.args)>  0:
> +            for name in event.args.names():
>                   # 'limit' is a reserved keyword
> -                if arg == 'limit':
> -                    arg = '_limit'
> -                print '  %s = $arg%d;' % (arg.lstrip(), i)
> +                if name == 'limit':
> +                    name = '_limit'
> +                print '  %s = $arg%d;' % (name.lstrip(), i)
>                   i += 1
>           print '}'
>       print
> @@ -478,6 +435,47 @@ trace_gen = {
>       },
>   }
>
> +# Event arguments
> +def type_is_string(type_):
> +    strtype = ('const char*', 'char*', 'const char *', 'char *')
> +    return type_.startswith(strtype)
> +
> +class Arguments:
> +    def __init__ (self, arg_str):
> +        self._args = []
> +        for arg in arg_str.split(","):
> +            arg = arg.strip()
> +            parts = arg.split()
> +            head, sep, tail = parts[-1].rpartition("*")
> +            parts = parts[:-1]
> +            if tail == "void":
> +                assert len(parts) == 0 and sep == ""
> +                continue
> +            arg_type = " ".join(parts + [ " ".join([head, sep]).strip() ]).strip()
> +            self._args.append((arg_type, tail))
> +
> +    def __iter__(self):
> +        return iter(self._args)
> +
> +    def __len__(self):
> +        return len(self._args)
> +
> +    def __str__(self):
> +        if len(self._args) == 0:
> +            return "void"
> +        else:
> +            return ", ".join([ " ".join([t, n]) for t,n in self._args ])
> +
> +    def names(self):
> +        return [ name for _, name in self._args ]
> +
> +    def types(self):
> +        return [ type_ for type_, _ in self._args ]
> +
> +    def size_str(self):
> +        res = ""
> +        return res
> +
>   # A trace event
>   import re
>   cre = re.compile("((?P<props>.*)\s+)?(?P<name>[^(\s]+)\((?P<args>[^)]*)\)\s*(?P<fmt>\".*)?")
> @@ -489,17 +487,11 @@ class Event(object):
>           m = cre.match(line)
>           assert m is not None
>           groups = m.groupdict('')
> -        self.args = groups["args"]
> -        self.arglist = self.args.split(',')
>           self.name = groups["name"]
> -        if len(self.arglist) == 1 and self.arglist[0] == "void":
> -            self.argc = 0
> -        else:
> -            self.argc = len(self.arglist)
> -        self.argnames = get_argnames(self.args)
> -        self.sizestr = calc_sizeofargs(self.args, self.argc)
>           self.fmt = groups["fmt"]
>           self.properties = groups["props"].split()
> +        self.args = Arguments(groups["args"])
> +
>           unknown_props = set(self.properties) - VALID_PROPS
>           if len(unknown_props)>  0:
>               raise ValueError("Unknown properties: %s" % ", ".join(unknown_props))
>