Re: [PATCH v7 14/14] tests/acceptance: add reverse debugging test

[Cleber Rosa]

On Tue, Oct 06, 2020 at 06:09:55PM +0300, Pavel Dovgalyuk wrote:
> On 06.10.2020 16:36, Cleber Rosa wrote:
> > On Sat, Oct 03, 2020 at 08:14:06PM +0300, Pavel Dovgalyuk wrote:
> > > From: Pavel Dovgalyuk <Pavel.Dovgaluk@gmail.com>
> > > 
> > > This is a test for GDB reverse debugging commands: reverse step and 
> > > reverse continue.
> > > Every test in this suite consists of two phases: record and replay.
> > > Recording saves the execution of some instructions and makes an initial
> > > VM snapshot to allow reverse execution.
> > > Replay saves the order of the first instructions and then checks that they
> > > are executed backwards in the correct order.
> > > After that the execution is replayed to the end, and reverse continue
> > > command is checked by setting several breakpoints, and asserting
> > > that the execution is stopped at the last of them.
> > > 
> > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > Reviewed-by: Willian Rampazzo <willianr@redhat.com>
> > > 
> > > --
> > > 
> > > v5:
> > >   - disabled (as some other tests) when running on gitlab
> > >     due to the unidentified timeout problem
> > > ---
> > >   MAINTAINERS                           |    1
> > >   tests/acceptance/reverse_debugging.py |  208 
> > > +++++++++++++++++++++++++++++++++
> > >   2 files changed, 209 insertions(+)
> > >   create mode 100644 tests/acceptance/reverse_debugging.py
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index ea4fa3e481..bd3a7efb75 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -2693,6 +2693,7 @@ F: include/sysemu/replay.h
> > >   F: docs/replay.txt
> > >   F: stubs/replay.c
> > >   F: tests/acceptance/replay_kernel.py
> > > +F: tests/acceptance/reverse_debugging.py
> > >   F: qapi/replay.json
> > >   IOVA Tree
> > > diff --git a/tests/acceptance/reverse_debugging.py 
> > > b/tests/acceptance/reverse_debugging.py
> > > new file mode 100644
> > > index 0000000000..b72fdf6cdc
> > > --- /dev/null
> > > +++ b/tests/acceptance/reverse_debugging.py
> > > @@ -0,0 +1,208 @@
> > > +# Reverse debugging test
> > > +#
> > > +# Copyright (c) 2020 ISP RAS
> > > +#
> > > +# Author:
> > > +#  Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> > > +#
> > > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > > +# later.  See the COPYING file in the top-level directory.
> > > +import os
> > > +import logging
> > > +
> > > +from avocado import skipIf
> > > +from avocado_qemu import BUILD_DIR
> > > +from avocado.utils import gdb
> > > +from avocado.utils import process
> > > +from avocado.utils.path import find_command
> > > +from boot_linux_console import LinuxKernelTest
> > > +
> > > +class ReverseDebugging(LinuxKernelTest):
> > > +    """
> > > +    Test GDB reverse debugging commands: reverse step and reverse 
> > > continue.
> > > +    Recording saves the execution of some instructions and makes an 
> > > initial
> > > +    VM snapshot to allow reverse execution.
> > > +    Replay saves the order of the first instructions and then checks 
> > > that they
> > > +    are executed backwards in the correct order.
> > > +    After that the execution is replayed to the end, and reverse continue
> > > +    command is checked by setting several breakpoints, and asserting
> > > +    that the execution is stopped at the last of them.
> > > +    """
> > > +
> > > +    timeout = 10
> > > +    STEPS = 10
> > > +    endian_is_le = True
> > 
> > Have you attmepted a "be" test too?  I'm curious about why this is
> > defined (and used later) but it's never used as `False`.
> 
> It was intended to be used with PPC, but PPCs record-replay is not reliable
> enough.
>

OK, thanks for the explanation.

> > 
> > > +
> > > +    def run_vm(self, record, shift, args, replay_path, image_path):
> > > +        logger = logging.getLogger('replay')
> > > +        vm = self.get_vm()
> > > +        vm.set_console()
> > > +        if record:
> > > +            logger.info('recording the execution...')
> > > +            mode = 'record'
> > > +        else:
> > > +            logger.info('replaying the execution...')
> > > +            mode = 'replay'
> > > +            vm.add_args('-s', '-S')
> > > +        vm.add_args('-icount', 
> > > 'shift=%s,rr=%s,rrfile=%s,rrsnapshot=init' %
> > > +                    (shift, mode, replay_path),
> > > +                    '-net', 'none')
> > > +        vm.add_args('-drive', 'file=%s,if=none' % image_path)
> > > +        if args:
> > > +            vm.add_args(*args)
> > > +        vm.launch()
> > > +        return vm
> > > +
> > > +    @staticmethod
> > > +    def get_reg_le(g, reg):
> > > +        res = g.cmd(b'p%x' % reg)
> > > +        num = 0
> > > +        for i in range(len(res))[-2::-2]:
> > > +            num = 0x100 * num + int(res[i:i + 2], 16)
> > > +        return num
> > > +
> > > +    @staticmethod
> > > +    def get_reg_be(g, reg):
> > > +        res = g.cmd(b'p%x' % reg)
> > > +        return int(res, 16)
> > > +
> > > +    def get_reg(self, g, reg):
> > > +        # value may be encoded in BE or LE order
> > > +        if self.endian_is_le:
> > > +            return self.get_reg_le(g, reg)
> > > +        else:
> > > +            return self.get_reg_be(g, reg)
> > > +
> > > +    def get_pc(self, g):
> > > +        return self.get_reg(g, self.REG_PC)
> > > +
> > > +    def check_pc(self, g, addr):
> > > +        pc = self.get_pc(g)
> > > +        if pc != addr:
> > > +            self.fail('Invalid PC (read %x instead of %x)' % (pc, addr))
> > > +
> > > +    @staticmethod
> > > +    def gdb_step(g):
> > > +        g.cmd(b's', b'T05thread:01;')
> > > +
> > > +    @staticmethod
> > > +    def gdb_bstep(g):
> > > +        g.cmd(b'bs', b'T05thread:01;')
> > > +
> > > +    @staticmethod
> > > +    def vm_get_icount(vm):
> > > +        return vm.qmp('query-replay')['return']['icount']
> > > +
> > > +    def reverse_debugging(self, shift=7, args=None):
> > > +        logger = logging.getLogger('replay')
> > > +
> > > +        # create qcow2 for snapshots
> > > +        logger.info('creating qcow2 image for VM snapshots')
> > > +        image_path = os.path.join(self.workdir, 'disk.qcow2')
> > > +        qemu_img = os.path.join(BUILD_DIR, 'qemu-img')
> > > +        if not os.path.exists(qemu_img):
> > > +            qemu_img = find_command('qemu-img', False)
> > > +        if qemu_img is False:
> > > +            self.cancel('Could not find "qemu-img", which is required to 
> > > '
> > > +                        'create the temporary qcow2 image')
> > 
> > This snippet is clearly modeled after the snippet in
> > `boot_linux.BootLinuxBase.download_boot()`.  I'm adding an action
> > item to create a generic utility:
> > 
> >    https://gitlab.com/cleber.gnu/qemu/-/issues/1
> > 
> > > +        cmd = '%s create -f qcow2 %s 128M' % (qemu_img, image_path)
> > > +        process.run(cmd)
> > > +
> > > +        replay_path = os.path.join(self.workdir, 'replay.bin')
> > > +
> > > +        # record the log
> > > +        vm = self.run_vm(True, shift, args, replay_path, image_path)
> > > +        while self.vm_get_icount(vm) <= self.STEPS:
> > > +            pass
> > > +        last_icount = self.vm_get_icount(vm)
> > > +        vm.shutdown()
> > > +
> > > +        logger.info("recorded log with %s+ steps" % last_icount)
> > > +
> > > +        # replay and run debug commands
> > > +        vm = self.run_vm(False, shift, args, replay_path, image_path)
> > > +        logger.info('connecting to gdbstub')
> > > +        g = gdb.GDBRemote('127.0.0.1', 1234, False, False)
> > > +        g.connect()
> > > +        r = g.cmd(b'qSupported')
> > > +        if b'qXfer:features:read+' in r:
> > > +            g.cmd(b'qXfer:features:read:target.xml:0,ffb')
> > > +        if b'ReverseStep+' not in r:
> > > +            self.fail('Reverse step is not supported by QEMU')
> > > +        if b'ReverseContinue+' not in r:
> > > +            self.fail('Reverse continue is not supported by QEMU')
> > > +
> > > +        logger.info('stepping forward')
> > > +        steps = []
> > > +        # record first instruction addresses
> > > +        for _ in range(self.STEPS):
> > > +            pc = self.get_pc(g)
> > > +            logger.info('saving position %x' % pc)
> > > +            steps.append(pc)
> > > +            self.gdb_step(g)
> > 
> > Do you think it'd make sense to have more utility methods, such as
> > `step()` and `bstep()` in `avocado.utils.gdb.GDBRemote` itself?
> 
> I thought about it, but it was easier to not have the dependency on newer
> avocado version.

Agreed.

> But now we can move these functions into avocado in two steps.
>

OK.  I think the versions of these functions in
`avocado.utils.gdb.GDBRemote` can benefit from parsing the reply
packets.  With that, in addition to using a strict expected reponse
(like you've done with b'T05thread:01;') the caller may inspect
only the aspects that it deems important.

For instance, one may be interested in asserting that the signal
was a SIGTRAP, but may not care about the thread ID.

Anyway, I'm opening an issue on the Avocado project page:

   https://github.com/avocado-framework/avocado/issues/4258

If you have ideas about the interface, please let me know.

Thanks,
- Cleber.

signature.asc
Description: PGP signature