[Mar29+] bitshift issue with User-Variables during save/load

[Lord Misfit]

I had noticed around March 29 2016, ZDoom in general had some updates to allow floats for user variables, among other things, however because I had not updated my version in so long, my previous version of [G]ZDoom was based on a Pre March 29th version, so updating to a newer version very recently, I discovered that after saving a game, then loading it, certain user variables seem to convert wrong. I'm using "GZDoom g2.2pre-1664" at the moment as I report this bug, but this is specfic to ZDoom in general for any version from and after March 29, 2016.

I know you guys generally want samples, so I tried to put one together as well as possible below.

https://drive.google.com/file/d/0Bw8caF ... sp=sharing [I do hope this link will work, yet to get anyone else I know to test it >.>;]

The only reason I found this in the first place is because my mod has a lifebar function that also shows stats in numeric form when you're looking at a monster, and their Maximum HP as displayed on the lifebar is calculated based on several factors due to an RPG nature [so it's put into a user_variable, instead of using its base spawnhealth, which on non-players can't be changed]. Problem is, when this issue happens, it changes how the lifebars and the readout of the Max Health looks because of it, as well as other user variables that aren't seen when looking at the lifebar, and anyone using User Variables like this can have things break on them in weird ways.

Basically, if you save and load once, and the user variable is set to 128 or above [or -129 or less], it'll convert to some other larger number [128 becomes -32768, 256 becomes 1, etc]. If you save and load again after this, the -32768 goes back to 128, though the 1 stays 1 instead of going back to 256. Any number between -128 and 127 [a single byte] never gets borked up like this when saving and then reloading.

Someone I had looking into this is stating there's a bitshift going awry here, and something to do with endian and double word and stuff. I'm not experienced in those terms though, and he didn't want to bother to register at the forum, so I have to be the one to report it myself, thus my knowledge on this subject is limited. :\

Further instructions to replicate the bug are inside a text file inside the folder you can load in game. ["UserVarTest" folder]

Sorry I don't have a more conveinent way to help you in replicating the issue, but I quickly put this together so I could get notice to you at all about this. I'm honestly surprised that no one else has reported this, but maybe user variables aren't used as much by modders like this as I had thought?

[Guest]

I followed the instructions in the test case and successfully replicated the issue. It looks like a byte swapping issue. for example 0x80 becomes 0x8000 before alternating back to 0x80 with another save/reload. By saving to three different saves I noticed the file sizes different - 46912, 46961 and 46910 for the first, second an third game respectively. This is important, because zdoom attempts to use the minimum number of bytes for each value when saving, so if I understand this right then the problem seems to be in the save load code somewhere.

The main reason I think that is because there are two 'special' values in the test case - 0x0100 and 0x0200, each would take two bytes to write out since the 0xFF is the highest number an unsigned byte can express, but upon being flipped they become 0x01 and 0x02 respectively - values that only need a single byte to express. To this end, note the sizes of the first and third saves - there is a two byte difference. If the issue were in the save code, then it would seem to me the first game should be 46910 bytes instead of 46912.

Of course this could be coincidence. I don't really know the code too well to be sure, nor could I get it compiled under winbuilds 1.5.0 to dig any further.

Cheers

[Graf Zahl]

I never understood the need for this 'space saving', considering that the savegame is

a) compressed and
b) contains so much other bloat that a little more wouldn't really hurt.

[Guest]

After talking a bit more, Lord Misfit went and did a bit of winnowing and found the problem surfaced in the pre-521 build. This build came out on April 4, and from there I went and poked around the commit log on github and found this little guy here: https://github.com/rheit/zdoom/commit/8 ... da67b576cc

Note the date, April 3rd.

Chiefly let's check out this addition here, I mentioned it in a post above:
https://github.com/rheit/zdoom/blob/85c ... e.cpp#L791

This is how values are written in a save game. We can see it minimises the space used to write a value, but this isn't so important as how it's doing it: we see it making calls to WriteByte, WriteInt16 and so on...

So let's open up this guy, src/farchive.cpp:
https://github.com/rheit/zdoom/blob/85c ... ve.cpp#L82

Note the compile condition here, well not so much the condition itself but its implications: farchive reads and writes in big endian format.

Now let's go check out those Write functions:
https://github.com/rheit/zdoom/blob/85c ... e.cpp#L729

Cool, we'll just make sure to call LittleShort to make sure we're writing to little endian format... wait. _little endian_? doesn't farchive expect to _big endian_? Let's go check the read functions just to be sure...

https://github.com/rheit/zdoom/blob/85c ... e.cpp#L924

Now far be it from me to judge, but this is sort of gross. isn't convention to use >> when reading from a stream in this language? commentary aside, let's go check out how that operator is working... back to src/farchive.cpp!

https://github.com/rheit/zdoom/blob/85c ... e.cpp#L947

Cool, we see we're byte swapping stuff in as we uh, read... and write. Okay I guess. Let's go check to see how those SWAP macros are working again.

https://github.com/rheit/zdoom/blob/85c ... ve.cpp#L82

Remember this? Intel processors use little endian mode, so we're not using the __BIG_ENDIAN__ macros, but the either of the ones below it. Definitely looks like they're working as intended, which is great! Except that we're writing little endian values and then reading them back thinking they're big endian.

Pretty typical 4AM bug here all in all. Though I'm a bit confused why the WriteInt16 and such functions were there at all, since the << handles both reading and writing. Unless I'm missing something else, then the best way to fix this would be just to remove those Write functions all together and use the << operator instead. The easiest way would be to just change the LittleShort and such to the proper SWAP_ macros.

Cheers.

[Graf Zahl]

Now far be it from me to judge, but this is sort of gross. isn't convention to use >> when reading from a stream in this language? commentary aside, let's go check out how that operator is working... back to src/farchive.cpp!

only if you are dead set on writing the same code twice, once for loading and once for storing. ZDoom uses one operator doing both, based on access mode, so that in most cases there's only one instance which greatly reduces the possibility of an error due to a mismatch.

[Graf Zahl]

Concerning the error - just looking at this needlessly complex code gives me the creeps. This is the kind of 'efficiency' that always backfires.
Why not just write out the value in its native form and be done with it?

Same for PFloat: What's the point of this crap?

Code: Select all

	if (Size == 8)
	{
		// If it can be written as single precision without information
		// loss, then prefer that over writing a full-sized double.
		double doubleprecision = *(double *)addr;
		singleprecision = (float)doubleprecision;
		if (singleprecision != doubleprecision)
		{
			ar.WriteByte(VAL_Float64);
			ar << doubleprecision;
		}
	}

It ultimately doesn't save anything, it only creates larger code.

[Lord Misfit]

Hate to bug the ZDoom specfic coders here [not Graf, since this isn't specific to GZDoom], but has anything been looked at concerning this bug yet? Or do I need to submit something else that makes more clear the issue here? While I might be able to use inventory items as an alternate to SOME of the issues presented by this, I can't do that for negative values [like 'overkill' HP levels for recalculated monster HP levels], since inventory items can't be negative values. :\

EDIT: Talked to the one buddy [who also posted earlier]. According to him, @ https://github.com/rheit/zdoom/blob/85c ... e.cpp#L731 there's the WriteInt16 and WriteInt32 functions. Tells me they use LittleShort in WriteInt16 instead of SWAP_WORD, and LittleLong in WriteInt32 instead of SWAP_DWORD.

Code: Select all

void FArchive::WriteInt16(WORD val)
{
    WORD out = SWAP_WORD(val);
    m_File->Write(&out, 2);
}

void FArchive::WriteInt32(DWORD val)
{
    int out = SWAP_DWORD(val);
    m_File->Write(&out, 4);
}

He believes changing it to that will fix it. I don't have compling tools for it though, and that kind of thing is a bit too harrowing for me to try to fix it myself, but he told me if I post that here that whoever manages the ZDoom-side code shouldn't have an issue fixing it.

[subenji]

I've tested this locally, and been able to reproduce the issue on the latest zdoom build 2.9pre-1100, and the above proposed fix did correct the issue. I've prepared and submitted a pull request with this change.

[Major Cooke]

https://github.com/rheit/zdoom/pull/718 <--His pull request.

Also bear in mind Graf's not available temporarily. I think he'll be back on the 11th or 10th, not sure.

[Graf Zahl]

Fix added. Normally this should have required a version bump, but since this stuff was broken anyway, I don't thing it's necessary here.