Memory corruption on the passenger-generation branch

[jamespetts]

I have spent the last two days' worth of Simutrans time, give or take, trying to track down some very, very odd memory corruption on the passenger-generation branch, so far without much success. The ultimate problem appears to be corruption of ding_t objects representing the gabaeude_t objects of factories. The problem that I was initially getting was that, after a while (it seems that there has to be a month boundary, although disabling various neuer_monat() methods has made no difference), saving the game (when testing with the saved game from the Bridgewater-Brunel server from July 1968) would almost inevitably produce an assertion failure followed by an access violation caused by a gabaeude_t object being an industry when rdwr() is called on it. I later discovered that this was the data in it had become corrupted: the "typ" value had for some reason in each case ended up as a type other than a factory, with the result that the code designed to check whether it is a factory failed and the gabaeude_t object itself tried to save, which fails, as the code is not designed to do this.  The failing only occurred with industries (I checked the position of each one). There would also be numerous vague warnings given in MSVC++ about heap corruption, although nothing specific enough to track down. Even when the saving did not produce this error, loading a saved game failed: it seemed that games saved after at least a month had passed were corrupt.

Since incorporating Neroden's latest changes to the passenger_generation branch on his "passenger_generation" branch, the problems have modified slightly. I no longer get the issues saving the game as above, but instead get extraordinarily odd symptoms after the game has been running for a while: the game will sometimes freeze, but not in an infinite loop: pausing and restarting in the debugger will often (but not always) bring it back to life for a time. Also, there was one occasion when mouse scrolling stopped working entirely, but the game otherwise seemed to function as normal until I tried to load another saved game. However, inevitably, failure will occur when closing the existing game (either quitting or loading another game): there will be access violations in memory management modules when deleting gebaeude_t objects, which suggests to me a double free error. Running Dr. Memory finds nothing of relevance (nothing but leaks in obj_reader and other similar methods), but that might be because freelist hides these errors from Dr. Memory.

I did try one test in which I started a new map with one town and one industry, and put a breakpoint at the beginning of the destructor for the gebaeude_t method. This was called many times when destroying a map (the demo map to start the new game) and also when re-loading the test game from a save point (the deleting happening during the destruction phase), but after these occurred, there seemed to be no trouble.

I ran it for a while, and noticed that the gabeaude_t destructor was also called by the city renovation code (presumably when destroying an old city building to make way for a new one). The game seemed to run as normal thereafter, but on quitting, I got heap corruption warnings in MSVC++, although no crashes this time.

This is all rather odd, as I have not altered the building renovation code in the passenger-generation branch, and I am a little confused. Any insight would be much appreciated.

[jamespetts]

I have run a test in relation to the renovation of city buildings, by reverting in this commit a change by Neroden that had the effect of destroying and re-building rather than merely upgrading city buildings. This has no effect, however: various errors still occur because gebaede_t objects that belong to factories are being incorrectly marked as not belonging to a factory, or the factory object to which they are connected is being deleted. I am still rather perplexed by this, especially since it is hard to see how the code that I have written on this branch has any significant memory management implications, and should appreciate any insights.

[jamespetts]

I have just tested, and these bugs do not appear on the devel-new branch, the only significant change on which is Neroden's fix-city-limits branch. This would appear to confirm that the issue is in the passenger-generation branch itself, and not in the fix-city-limits code.

[jamespetts]

I have tried disabling multi-threading and putting the new karte_t::step_passengers_and_mail() method before rather than after the stepping code for individual cities, but neither has met with any success in avoiding this issue.

Edit: Disabling city growth by commenting out:

Code Select Expand


while(stadt_t::city_growth_step < next_growth_step) {
        calc_growth();
        step_grow_city();
        next_growth_step -= stadt_t::city_growth_step;
    }


does not affect the problem (although it had a slightly different manifestation, but it is unlikely that this is significant).

[neroden]

First thing we should try: change the freelist to allocate memory in minimum hunks of 64 bits.  This requires more work than it appears, but it makes it possible for me to run valgrind, which really dislikes the unaligned allocations.

[jamespetts]

Thank you very much for the suggestion - that is helpful.

However, I have just fixed this: it was caused by incorrect use of a union leading to data corruption in the things to which the union was pointing, the union use error in turn being caused by failing to update enums when porting the passenger generation code from simcity.cc to simworld.cc: I had incorrectly retained the raw value of "1" when I ought to have changed it to the enum "town" (whose raw value equivalent is now 0), meaning that some factory pointers were incorrectly treated as city pointers, corrupting the value of certain pointers and other values within their structure. I have pushed the fix to my passenger-generation branch.