Re: [hatari-devel] memory snapshot format/issues (was: Release time for 1.8.1 ?)

[Steven Noonan <steven@xxxxxxxxxxxxxx>]

On Fri, May 8, 2015 at 2:40 PM, Eero Tamminen <oak@xxxxxxxxxxxxxx> wrote:
> Hi,
>
> On perjantai 08 toukokuu 2015, Steven Noonan wrote:
>> On Fri, May 8, 2015 at 1:31 PM, Nicolas Pomarède <npomarede@xxxxxxxxxxxx>
> wrote:
>>> I thought about using 1.9.0, 1.10.0, 1.11.0, .. too, but as Thomas
>>> wrote the version if of the form "x.y.z\0" and it's kinda hardcoded in
>>> memorysnapshot.c to take 6 bytes when saving/restoring, so using 1.10.0
>>> for example would take 7 bytes in the memory snapshot header, which
>>> might not be correctly handled (we need to handle the case where an
>>> older Hatari version will load a more recent snapshot and should tell
>>> it's not compatible).
>>>
>>> If we can update the use of VersionString in MemorySnapShot_OpenFile to
>>> work with more than 6 bytes of ident string, then I think we can use
>>> 1.9.0 for next version, then 1.10.0, 1.11.0, ...
>
> Older version would do strcmp() for strings with lengths of 6 & 7 bytes.
> First 6 chars will obviously be different so it doesn't try to load
> wrong version.
>
> I would also assume that strcmp() stops at first differing
> character, so loading 6 bytes of 7 byte string shouldn't be
> a problem in practice.
>
> However, I checked memorysnapShot.c history, and earlier versions
> try to print out the on-disk snapshot version, which would now
> be missing the terminating NULL, which could cause issues
> (at least mess up terminal with non-printable characters).
>
> So, the new format snapshot identification file format should
> have '\0' within first 6 characters.
>
>
>> Hmm, I'm not sure I agree with the "6 vs 7 bytes is a problem"
>> argument. Yes, it's a true statement if you are only considering
>> snapshot portability between different Hatari versions. I'll grant
>> that.
>>
>> But I think snapshots really need an overhaul for correctness reasons
>> -- for example if you save the memory state in a 64-bit build of
>> Hatari and try to load it in a 32-bit build of Hatari, it won't work
>> (usually by crashing) because it serialized a bunch of 64-bit pointers
>> to disk and the structure offsets of the state on disk are all wrong.
>
> I haven't really looked at this in detail/ages.  Where it's
> serializing pointers?

Look at pretty much any of the MemorySnapShot_Store callsites. They're
serializing whole structures, many of which contain pointers. e.g.
take a look at the serialization of the array of ACIA_STRUCTs -- there
are 7 function pointers in that structure. Or look at floppy_stx.c
where the STX_State structure is serialized.

On restore, these pointer values are ignored, and re-linked up to
other memory locations (so we are not going to dereference any of the
serialized pointers). But when written to disk they have a variable
width, depending on your Hatari build.

>> In my opinion, the whole "fwrite() all your in-memory structures to a
>> file" is a poor design.
>
> AFAIK it's primarily meant for snapshots that are used by
> the same binary, e.g. when you're actively debugging things.
> It's not meant as something portable.

I understand. It's a fair first pass at snapshotting, but it is fragile.

>> There are even some issues with saving/restoring snapshots going from
>> Hatari 32-bit x86 to Hatari using the Linux x32 ABI (basically full
>> x86_64 but with 32-bit pointers). This *should* be totally compatible
>> because the pointer sizes are the same, and thus all the structure
>> sizes should be identical. However, some of the structure packing is
>> different between the two builds which causes memory state
>> save/restore to break. I fixed a few of these issues in my local repo,
>> but haven't submitted changes to the list.
>
> Please do.

I'm not quite pleased with my work yet. You can see the WIP version here:

http://git.uplinklabs.net/snoonan/projects/hatari.git/commit/?id=bdc22dd209ddf3a3efdda07efc0302a26aae7b32

(There are a bunch of other commits on the master branch there, too,
but I'm not sure many of them are really candidates for mainline,
except maybe the 'REGPARAM' changes which make x86 use a fastcall-like
ABI in the hot paths.)

>> There are also problems trying to move snapshots between different
>> architectures. e.g. 32-bit x86 snapshot will have different endianness
>> than a 32-bit PowerPC snapshot. So endianness needs normalization too.
>
> Or Hatari could just reject snapshot if it has wrong endianess.
>
> Having some magic 32-bit value (one with '\0' for older
> Hatari versions) as first thing in the file, could
> be that check.

This would indeed be better than what we have. One thing I did locally
when debugging save portability was write a 32-bit integer indicating
the size of the structure being serialized before writing the
structure itself, and then verifying that those matched the expected
values on restore:

http://git.uplinklabs.net/snoonan/projects/hatari.git/commit/?id=ecded6f3b1b60a462376c47bb8cf02dd5be699b4

This identified the differences breaking x86_64/x86 and x86/x32 ABI
incompatibility.

>> I've been playing with some ideas to use something like protobuf-c or
>> msgpack-c for portable and compact memory state save/restore, but I
>> haven't touched it in a while. Of the two options, msgpack felt like
>> the easiest and simplest approach. The serialization API for msgpack-c
>> is quite pleasant and is code size friendly, but the deserialization
>> side wasn't nearly as simple just because of the msgpack-c API design.
>> Perhaps there's an alternative I haven't looked at yet.
>
> From user's point of view having completely portable memory snapshots
> could be interesting, but I think people mostly just want their memory
> snapshots of games to work with newer Hatari versions, on the same
> HW & OS.  However, that's unlikely to happen as there are changes
> between Hatari versions on the data that needs to be stored.

Perhaps it is an odd use case that nobody would really take advantage
of. It does feel more "right" to me, though. And I consider it to
generally be a good litmus test of emulation to save the state in one
place and restore it in another, and see that the emulator doesn't
crash.