[ Sdk ] Safe_delete_array

[xienwolf]

I've had a "phantom crash" going on with my code for a while now. It only happens when you try to load a game after having already played a game for a few turns (loading from ingame or from main menu. Any attempt to initialize a game without first going back to the desktop).


I finally got the source of the break to reveal itself, and it reveals to me a lack in my understanding of when/where to delete arrays.


So, the function SAFE_DELETE_ARRAY is run either in CvPlayerAI::~CvPlayerAI() or void CvPlayerAI::AI_uninit(). My error occurs right after AI_uninit() should be run during void CvPlayerAI::AI_reset(bool bConstructor).

Now, I've added a few arrays to CvPlayerAI, and I delete all of them during ~CvPlayerAI right now. They haven't caused me any problems before, but now that I have added additional Barbarian Civilizations, I am getting an issue with these sections of the code. I am curious if it is something to do with deleting my arrays in the wrong location, so hope someone can shed some light on what the difference is between erasing the array in ~CvPlayerAI and erasing it in AI_uninit() (other than the fact that uninit is run from a lot more places in the code, thus happens considerably more often)

 

[EmperorFool]

Looking at CvPlayerAI, I see that it deletes a few of its arrays in uninit() and recreates them in reset(). I have no idea why it was coded to be different from the normal constructor/destructor pattern. Where do you create the arrays that you delete in uninit()?

If you can't find the problem, I recommend moving the array creation to the constructor and deletion to the destructor like most all of the other arrays. That shouldn't be necessary, but at least you know your arrays will never get deleted out from under you.

 

[xienwolf]

Right now with my arrays I create and delete them all in CvPlayer::CvPlayer and CvPlayer::~CvPlayer like most arrays are handled. Since my problem seems to come while loading data I am not quite sure yet if it IS the arrays causing me issue, I tried moving my arrays to this function, nothing much changed, so maybe I'll try moving the Firaxis arrays out of the function to see if something tosses up a complaint elsewhere to reveal why the setup is how it is.

 

[Jeckel]

Any update on this xienwolf? I can't offer any light on the topic, but I can say I've occationally run into this weird crash, you can't load a game from inside a game and have to exit to the desktop and then load a game.

It has been awhile since this happen to me, but I will think about it and see if I can remember doing anything to fix it.

 

[xienwolf]

I tried moving a few things around between the two functions, but never noticed any overall effect from it. I'm thinking of removing all of the work I have added to CvPlayer to see if it is one of my functions which just happens to be going bad NOW, or if it is indeed the multiple Barbarian Civs causing the issue.

 

[Maniac]

Did you find out anything more about this?

I'm currently having the problem that when I save a game, and then try to load another game or exit to the main meny, the game sometimes crashes. Some turns the crash will always occur; other turns I have to try like five times to get a crash. This makes it more or less impossible to find a specific turn where the problem starts, if there is any. The process of saving screws up the game somehow. I'm not getting crashes during normal gameplay though.

I had a look at the initialization of new stuff in Planetfall, and I noticed some weird stuff from my predecessors.

In CvCity.cpp, this

m_iFreshWaterCount = 0;
m_iFeatureBadHealthModifier = 100;

was initialized both in ::CvCity() and ::reset. I commented out the ones in ::CvCity().

In CvGame.cpp, this

m_iFloweringCounter = 0;
m_iFloweringCounter_NativeHabitable = 0;
m_iFloweringCounter_NativeHabitablePlots100 = 0;

this was initialized in ::init. I moved it to ::reset.

Could this somehow cause troubles?? As the crashes don't always happen, it's hard to know without major testing.

That Flowering Counter stuff is ancient code though, and I've only been noticing these phantom crashes recently. And I haven't added any complex arrays lately. Just a couple straightforward m_iX, m_bX, etc.

 

[xienwolf]

Pretty sure this one was the crash we uncovered where native Firaxis code didn't account for the fact that you might have units without a unitcombat involved in certain actions. There was a CvPlayer array of getNumUnitCombatInfos size to track some data about them (threat index I think?), anyway, in FfH/FF we use a lot of NO_UNITCOMBAT units in battle, and this meant it was storing data in [-1]. Oddly wasn't ever hitting a non-crash error with the retrieval of this data, just caused problems when trying to delete things later on.

As for your potentials, I think you have to keep looking. Integers tend to be pretty docile, and initialization can't cause a crash as long as the initialized values are legal values. Worst case it would cause you to lose some information and revert to default conditions.

 

[Maniac]

Thanks, the getNumUnitCombatInfos is a good possibility. I also have a NO_UNITCOMBAT unit, and I already stumbled on an assert because of it. Didn't think to check all other uses of getNumUnitCombatInfos. :idiot:

 

[Maniac]

Did you use any debugging method to get to getNumUnitCombatInfos as the problem, or was it a lucky Eureka moment?

 

[xienwolf]

It was a talented mod user who posted it. Seemed to have some debugging tools available, but never found out precisely how he came across it.

 

[EmperorFool]

This meant it was storing data in [-1]. Oddly wasn't ever hitting a non-crash error with the retrieval of this data, just caused problems when trying to delete things later on.

This is due to how C and C++ compilers typically implement malloc(), the function that allocates memory for arrays and other objects. Instead of keeping a table of the size of every little allocation itself--which it later needs when you free() it with safe_detele_array()--it stores the size at the beginning of the memory block it returns to the caller.

For simplicity, let's say you allocate an int array of size 20. The first four bytes of the block hold the size, and it returns the address offset by these four bytes so you don't overwrite it with the array. Below, it would return 0x0004 as the address of the new block for the array.

Addr    Data  Array Offset
0x0000  0     [-2]  |- these two combine to hold the 4-byte <size> of the block
0x0002  20    [-1]  |
0x0004  ?     [0]
0x0006  ?     [1]
0x0008  ?     [2]
...

So by assigning a value to the [-1] slot of the array, you are overwriting part of the size of the block. This won't have any ill effects until you try to delete the array and free the memory.

BTW, how did you solve the problem with NO_UNITCOMBAT? Did you create a new UnitCombat for them?

 

[xienwolf]

We just encased the statement in a check to ignore if lacking a unitcombat. It was decided to be fairly unimportant overall as a function (hence me not even remembering precisely what it was). Ideally a unique unitcombat would be designed for units which simply doesn't offer any promotions though, as some other functions may do checks by unitcombat and ignore those who lack it appropriately already, and THEY might be important enough to warrant the rather minor XML work.