Re: [Qemu-devel] [RFC PATCH v7 00/21] Deterministic replay core

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/01/2015 12:59, Pavel Dovgalyuk wrote:
> This set of patches is related to the reverse execution and deterministic 
> replay of qemu execution  This implementation of deterministic replay can 
> be used for deterministic debugging of guest code through gdb remote
> interface.
> 
> These patches include only core function of the replay,
> excluding the support for replaying serial, audio, network, and USB devices'
> operations. Reverse debugging and monitor commands were also excluded to
> be submitted later as separate patches.
> 
> Execution recording writes non-deterministic events log, which can be later 
> used for replaying the execution anywhere and for unlimited number of times. 
> It also supports checkpointing for faster rewinding during reverse debugging. 
> Execution replaying reads the log and replays all non-deterministic events 
> including external input, hardware clocks, and interrupts.

So it's good that all the generic migration stuff went in, and also some
of the RR infrastructure.  v8 should be below 20 patches, which is
pretty good.

I made some generic comments:

1) thread-safety unfortunately looks a bit iffy.  The RR QemuMutex
should be the deepest lock in the hierarchy, which basically means that
replay/ should never call anything outside it while taking the lock.

2) your code generally looks like:

   -void foo(void)
   +void foo_impl(void)
    {
        ...
    }

   +void foo(void)
   +{
   +    if (replay_mode) ...
   +    foo_impl();
   +}

Instead, please use

    void foo(void)
    {
   +    replay_foo();
        ...
    }

and possibly put replay_foo() in the replay/ subdirectory.

3) Inline functions are better than macros.  enums are better than #defines.

4) I've applied patch 10.

Paolo