Re: [Minios-devel] [UNIKRAFT PATCH 6/8] support/scripts: implement trace buffer parcing

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Yuri,

In the subject: s/parcing/parsing .

Please see my comments inline.

On 5/10/19 9:29 PM, Yuri Volchkov wrote:
> With this patch trace.py can print the data collected in the
> tracefile. And two more use cases are covered for convince:

s/for convince/for convenience/

> 
>   - If --list specified trace.py will parse freshly fetched tracefile
>     and print out what is collected
> 
>   - Gdb command 'uk trace' parses and prints data without intermediate
>     trace files
> 
> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
> ---
>  support/scripts/trace.py          |  23 ++++-
>  support/scripts/uk-gdb.py         |   9 +-
>  support/scripts/unikraft/trace.py | 151 ++++++++++++++++++++++++++++++
>  3 files changed, 180 insertions(+), 3 deletions(-)
> 
> diff --git a/support/scripts/trace.py b/support/scripts/trace.py
> index 49b469a5..5632d39f 100755
> --- a/support/scripts/trace.py
> +++ b/support/scripts/trace.py
> @@ -36,6 +36,8 @@ import click
>  import os, sys
>  import pickle
>  import subprocess
> +from tabulate import tabulate
> +
>  scripts_dir = os.path.dirname(os.path.realpath(__file__))
>  sys.path.append(scripts_dir)
>  
> @@ -64,6 +66,19 @@ def parse_tf(trace_file):
>  
>      return trace.sample_parser(keyvals, tp_defs, trace_buff, ptr_size)
>  
> +@cli.command()
> +@click.argument('trace_file', type=click.Path(exists=True), 
> default='tracefile')
> +@click.option('--no-tabulate', is_flag=True,
> +              help='No pretty printing')
> +def list(trace_file, no_tabulate):
> +    """Parse binary trace file fetched from Unikraft"""
> +    if not no_tabulate:
> +        print_data = [x.tabulate_fmt() for x in parse_tf(trace_file)]
> +        print(tabulate(print_data, headers=['time', 'tp_name', 'msg']))
> +    else:
> +        for i in parse_tf(trace_file):
> +            print(i)
> +
>  @cli.command()
>  @click.argument('uk_img', type=click.Path(exists=True))
>  @click.option('--out', '-o', type=click.Path(),
> @@ -73,8 +88,11 @@ def parse_tf(trace_file):
>                default=':1234', show_default=True,
>                help='How to connect to the gdb session '+
>                '(parameters for "target remote" command)')
> +@click.option('--list', 'do_list', is_flag=True,
> +              default=False,
> +              help='Parse the fetched tracefile and list events')
>  @click.option('--verbose', is_flag=True, default=False)
> -def fetch(uk_img, out, remote, verbose):
> +def fetch(uk_img, out, remote, do_list, verbose):
>      """Fetch binary trace file from Unikraft (using gdb)"""
>  
>      if os.path.exists(out):
> @@ -99,6 +117,9 @@ def fetch(uk_img, out, remote, verbose):
>      if verbose:
>          print(_stdout)
>  
> +    if do_list:
> +        for i in parse_tf(out):
> +            print(i)
>  
>  if __name__ == '__main__':
>      cli()
> diff --git a/support/scripts/uk-gdb.py b/support/scripts/uk-gdb.py
> index 6e9b3930..9ae8ee66 100644
> --- a/support/scripts/uk-gdb.py
> +++ b/support/scripts/uk-gdb.py
> @@ -98,8 +98,13 @@ class uk_trace(gdb.Command):
>          gdb.Command.__init__(self, 'uk trace', gdb.COMMAND_USER,
>                               gdb.COMPLETE_COMMAND, True)
>      def invoke(self, arg, from_tty):
> -        # TODO
> -        pass
> +        elf = gdb.current_progspace().filename
> +        samples = trace.sample_parser(trace.get_keyvals(elf),
> +                                      trace.get_tp_sections(elf),
> +                                      get_trace_buffer(), PTR_SIZE)
> +        for sample in samples:
> +            print(sample)
> +
>  
>  class uk_trace_save(gdb.Command):
>      def __init__(self):
> diff --git a/support/scripts/unikraft/trace.py 
> b/support/scripts/unikraft/trace.py
> index 407f47e3..f74bcf41 100644
> --- a/support/scripts/unikraft/trace.py
> +++ b/support/scripts/unikraft/trace.py
> @@ -31,10 +31,161 @@
>  #
>  # THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
>  
> +import struct
> +import sys
>  import subprocess
>  import re
>  import tempfile
>  
> +TP_HEADER_MAGIC = 'TRhd'
> +TP_DEF_MAGIC = 'TPde'
> +UK_TRACE_ARG_INT = 0
> +UK_TRACE_ARG_STRING = 1
> +# Not sure why gcc aligns data on 32 bytes
> +__STRUCT_ALIGNMENT = 32
> +
> +FORMAT_VERSION = 1
> +
> +def align_down(v, alignment):
> +    return v & ~(alignment - 1)
> +
> +def align_up(v, alignment):
> +    return align_down(v + alignment - 1, alignment)
> +
> +class tp_sample:

It can get confusing on why there are 2 classes for tracepoints,
tracepoint and tp_sample. I think their names should reflect that the
former are the tracepoint definitions and latter are the tracepoint values.

> +    def __init__(self, tp, time, args):
> +        self.tp = tp
> +        self.args = args
> +        self.time = time
> +    def __str__(self):
> +        return (("%016d %s: " % (self.time, self.tp.name)) +
> +                 (self.tp.fmt % self.args))
> +    def tabulate_fmt(self):
> +        return [self.time, self.tp.name, (self.tp.fmt % self.args)]
> +
> +class EndOfBuffer(Exception):
> +    pass
> +
> +# Parsing of trace buffer is designed to be used without gdb. If ever
> +# Unikraft will have a bare metal port, it would be complicated to use
> +# gdb for fetching and parsing runtime data.
> +#
> +# However, it is possible anyways to get an address of the statically
> +# allocated trace buffer using gdb/nm/objdump. And we can use some
> +# different mechanism to copy memory from the running instance of
> +# Unikraft (which is yet to be designed).
> +#
> +# Similar considerations are applied to parsing trace point
> +# definitions. We can do that disregarding if it is possible to attach
> +# gdb to a running instance or not
> +class sample_parser:
> +    def __init__(self, keyvals, tp_defs_data, trace_buff, ptr_size):
> +        if (int(keyvals['format_version']) > FORMAT_VERSION):
> +            print("Warning: Version of trace format is more recent",
> +                  file=sys.stderr)
> +        self.data = unpacker(trace_buff)
> +        self.tps = get_tp_definitions(tp_defs_data, ptr_size)
> +    def __iter__(self):
> +        self.data.pos = 0
> +        return self
> +    def __next__(self):
> +        try:
> +            # TODO: generate format. Cookie can be 4 bytes long on other
> +            # platforms
> +            magic,size,time,cookie = self.data.unpack('4sLQQ')
> +        except EndOfBuffer:
> +            raise StopIteration
> +
> +        magic = magic.decode()
> +        if (magic != TP_HEADER_MAGIC):
> +            raise StopIteration
> +
> +        tp = self.tps[cookie]
> +        args = []
> +        for i in range(tp.args_nr):
> +            if tp.types[i] == UK_TRACE_ARG_STRING:
> +                args += [self.data.unpack_string()]
> +            else:
> +                args += [self.data.unpack_int(tp.sizes[i])]
> +
> +        return tp_sample(tp, time, tuple(args))
> +

I think it would have been clearer if the parsing alone would have
stayed in its own module. Or at least delimit the sections in the module
with (block) comments.

> +class unpacker:
> +    def __init__(self, data):
> +        self.data = data
> +        self.pos = 0
> +        self.endian = '<'
> +    def unpack(self, fmt):
> +        fmt = self.endian + fmt
> +        size = struct.calcsize(fmt)
> +        if size > len(self.data) - self.pos:
> +            raise EndOfBuffer("No data in buffer for unpacking %s bytes" % 
> size)
> +        cur = self.data[self.pos:self.pos + size]
> +        self.pos += size
> +        return struct.unpack(fmt, cur)
> +    def unpack_string(self):
> +        strlen, = self.unpack('B')
> +        fmt = '%ds' % strlen
> +        ret, = self.unpack(fmt)
> +        return ret.decode()
> +    def unpack_int(self, size):
> +        if size == 1:
> +            fmt = 'B'
> +        elif size == 2:
> +            fmt = 'H'
> +        elif size == 4:
> +            fmt = 'I'
> +        elif size == 8:
> +            fmt = 'Q'
> +        ret, = self.unpack(fmt)
> +        return ret
> +    def align_pos(self, alignment):
> +        self.pos = align_up(self.pos, alignment)
> +
> +class tracepoint:
> +    def __init__(self, name, args_nr, fmt, sizes, types):
> +        self.name = name
> +        self.args_nr = args_nr
> +        self.fmt = fmt
> +        self.sizes = sizes
> +        self.types = types
> +
> +    def __str__(self):
> +        return '%s %s' % (self.name,  self.fmt)
> +
> +def get_tp_definitions(tp_data, ptr_size):
> +    ptr_fmt = '0x%0' + '%dx' % (ptr_size * 2)
> +    data = unpacker(tp_data)
> +
> +    ret = dict()
> +
> +    while True:
> +        data.align_pos(__STRUCT_ALIGNMENT)
> +        try:
> +            magic, size, cookie, args_nr, name_len, fmt_len = \
> +                data.unpack("4sIQBBB")
> +        except EndOfBuffer:
> +            break
> +
> +        magic = magic.decode()
> +        if (magic != TP_DEF_MAGIC):
> +            raise Exception("Wrong tracepoint definition magic")
> +
> +        sizes = data.unpack('B' * args_nr)
> +        types = data.unpack('B' * args_nr)
> +        name,fmt = data.unpack('%ds%ds' % (name_len, fmt_len))
> +        # Strange, but terminating '\0' makes a problem if the script
> +        # is running in the gdb
> +        name = name[:-1].decode()
> +        fmt = fmt[:-1].decode()
> +
> +        # Convert from c-printf format into python one
> +        fmt = fmt.replace('%p', ptr_fmt)
> +
> +        ret[cookie] = tracepoint(name, args_nr, fmt, sizes, types)
> +
> +    return ret
> +
>  def get_tp_sections(elf):
>      f = tempfile.NamedTemporaryFile()
>      objcopy_cmd  = 'objcopy -O binary %s ' % elf
> 

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel