Assertion failed in saveload.cpp:1527

[mikro]

Hi,

I hope this is the right section (not sure whether it's a dev thing or a general problem).

I've tried to port OpenTTD to a platform with big endian, no threads, m68k (non-intel) target. Since there's a POSIX environment available, all needed prerequisites didn't pose any trouble. I didn't expect a flawless run but there's one thing I'm really not sure about: I see an assert on line 1527 in saveload.cpp. Short investigation has revealed that it fails in this part of the code:

case SLE_VAR_I8:
case SLE_VAR_U8:
return sld->size == sizeof(int8);

Now it looks like a compiler issue, right? Not true. The offending part is "sld->size", it's two while sizeof(int8) is (as expected) one. I'm not sure how this could happen? I tested it on trunk, open data from the latest nightly builds, video/music/sound driver = null.

Is it possible it's a little endian/big endian issue? If so, how to debug it? I want to emphasize this didn't happen on the first occasion -- about 10 checks had passed correctly, incl. another int8 ones.

[Eddi]

this is probably a configuration issue, where types are not mapped according to expectations, but unlikely to be endian specific.

anyway, this is a helper function to check preconditions before anything serious goes wrong, to prevent errors further down the line. but we need a bit more context about where these values came from to judge why it is wrong.

the values in sld->size probably come out of table/settings.ini somewhere, but there is a lot of magic involved

[frosch]

Within the saveload code you will find tables, that describe the savegame data like:

Code: Select all

SLE_VAR(OrderBackup, cur_real_order_index,     SLE_UINT8),

This specifies that OrderBackup::cur_real_order_index should be saved as uint8 in the savegame.
In this case the assertion you encounter checks whether sizeof(OrderBackup::cur_real_order_index) matches sizeof(uint8).

However, "cur_real_order_index" is only an example. You need to use your debugger, follow the backtrace, and try to figure out which member in which struct triggers it. That member has some dubious type.

[mikro]

Frosch, you're right, actually it came from a saving operation. It's happening after "Saving chunk DATE", on the first occasion where sld->size is compared against sizeof(int8) in IsVariableSizeRight, called from SlObjectMember().

Do you have any idea what could be wrong here? In what circumstances the 'size' member can be set to a wrong value?

[Alberth]

Usually when the data you're trying to save at that moment has the wrong type (for saving).

In other words, dig deeper until you find the actual field you're saving at that point, and check its type.

[mikro]

Guys, you've been most helpful. The culprit is _pause_mode, its data type to be precise. This funny enum template takes two bytes on my gcc configuration despite the fact the storage_type is byte.

EDIT: hmm, now as I think about it, isn't this quite a dangerous way how to store variables? sizeof(struct) is very dependent on the specific ABI, for instance, my platform doesn't allow for some CPU models access to odd address so every struct with odd number of bytes is aligned on 16-bit boundary...

[Eddi]

i don't think sizeof(struct) is used anywhere. only sizeof(struct.member)

[mikro]

Eddi, maybe I'm wrong but to me it seems that all enums are packaged in SimpleTinyEnumT / TinyEnumT, i.e. a struct and yes, sizeof() is evaluated on this structs, not on its (single) member.

[Eddi]

ok, i don't know the details of how these things work.

you might need to find a way to force your platforms to put these in one byte then. or adapt the saveload code to map these 2-byte structs onto an uint8. i don't know what is easier.

anyway, i don't see how it is "dangerous", because all these things are known on compile-time.

[mikro]

It's dangerous for network communication or loading state on another machine.

Anyway, a simple #pragma pack (1) for both enum templates has solved it and the port seems to be working.

Would you be interested to add support for a new (not very wide spread) platform? If so, where can I send patches?

[Eddi]

to submit patches for review, post them on https://bugs.openttd.org/

[frosch]

isn't this quite a dangerous way how to store variables?

Yes, and that's exactly why the assertion is there: To check whether the compiler does what the code expects it to do. But well, somewhen we will switch to C++11 with definitely sized enums

If you need to add platform specific stuff, please add it to stdafx.h
Possibly also add an assert_compile / static_assert to TinyEnumT.