News:

Simutrans Chat Room
Where cool people of Simutrans can meet up.

And another crash bug....

Started by neroden, March 27, 2010, 12:09:30 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

neroden

Loading most save files, I get

FATAL ERROR: loadsave_t::rdwr_str()
string longer (20994) than allowed size (256)

(The string length varies; it's also been 1024 and 512.)

Typical backtrace:

#4  0x0809eeb9 in loadsave_t::rdwr_str (this=0xbfffcc8c, s=0xbfffc890 "",
   size=256) at dataobj/loadsave.cc:739
#5  0x081cd06c in convoi_t::rdwr (this=0xecd4f78, file=0xbfffcc8c)
   at simconvoi.cc:2808
#6  0x081cde24 in convoi_t (this=0xecd4f78, wl=0xcf030c0, file=0xbfffcc8c)
   at simconvoi.cc:197
#7  0x08251b65 in karte_t::laden (this=0xcf030c0, file=0xbfffcc8c)
   at simworld.cc:4592
#8  0x08253f48 in karte_t::laden (this=0xcf030c0,
   filename=0xbfffcd40 "save/lincoln-1800a.sve") at simworld.cc:4242


I'm also getting segmentation faults at random during the game, but I haven't managed to get a backtrace for one of them yet.

PS.  Hope you can get a working version released soon, since this version is clearly badly broken.  This is as much testing as I can do this week most likely.

Bernd Gabriel

The new Simutrans Experimental 7.2 should read all savegames save with the official Experimental 7.1 and older versions.
Unfortunatelly it is not compatible with nightly 7.2 builds dated before 15th of March due to a version clash with standard simutrans.

If your savegames are Exp 7.1 or earlier, please upload/post one or two.
The journey is the reward!

neroden

The sa
Quote from: Bernd Gabriel on March 28, 2010, 03:11:37 PM
The new Simutrans Experimental 7.2 should read all savegames save with the official Experimental 7.1 and older versions.
Unfortunatelly it is not compatible with nightly 7.2 builds dated before 15th of March due to a version clash with standard simutrans.

If your savegames are Exp 7.1 or earlier, please upload/post one or two.

The savegame is in fact *with the same version*, indicating an inconsistency between loading and storing code.  I first detected this trying to load an autosave.

However, it was a hand-built copy with patches, so I'm going to retry with the new automatic build to see if I can reproduce with it.

gyom

A clean install of 7.2 over the standard 102.2 gave me the same result.
Crashes on loading saved game with an error similar as the one reported by neroden.
I used pakgerman as I can't even start a game with pak128.britain-ex, it crashes during world creation.

I didn't have any crashes during gameplay though.

I am using linux (i386 version, the amd64 crashes at startup, was same thing with 7.1).

Thanks for your efforts ! I also can't wait for a playable experimental as I now can't enjoy playing the standard and the (in my opinion) broken way it handles passengers.

neroden

Yep this is still happening.   This was an autosave file (renamed).

http://files.[ simutrans [dot] us (site down, do not visit) ]/files/get/m1klBvnns_/dead.sve

If you can't reproduce, I suppose it could conceivably have to do with the base files, though it seems very unlikely.  I had to get the directories font/, music/, and skin/ from 102.2. 


neroden

#5
OK, so this may help....

Starting a very small game, it autosaves, go to load from the autosave, and I get a segmentation fault.  I'm running under GDB, so I backtrace, and I get this:

#0  0x080a14c6 in replace_data_t::decrement_convoys() ()
#1  0x0bfca320 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Oh boy.

Looking at the relevant code:

void replace_data_t::decrement_convoys()
{
 if(--number_of_convoys <= 0)
 {
   // See http://www.parashift.com/c++-faq-lite/freestore-mgmt.html#faq-16.15
   // When maintaining this code, ensure that the above criteria remain satisfied.
   delete this;
 }
}

Oh boy.  Please rewrite your code so it doesn't 'delete this'.  This isn't per se the bug, but it's causing the bug to appear in multiple hard-to-trace forms.

I suspect you haven't managed to fully disable the convoy replacement code and bits of it are what is killing save games.  :-(  In particular, please see line 2824 of simconvoi.cc:
                       replace = new replace_data_t();
                       replace->set_autostart(old_autostart);
                       replace->set_replacing_vehicles(replacing_vehicles);

I'm pretty sure, looking at the logic, that this actually gets triggered.  Once this gets triggered, all kinds of bad things can happen because everything else is guarded only with "if (replace)".  And the replace code is nonfunctional.

Can you please arrange to simply throw away all 'replacing' information from old save games?   (Especially since the "replace" code is getting rewritten.)

EDIT: Note that attempting to load the save game "fresh" (rather than mid-game) results in the "wrong length" error message described before.  So this is all part of the same bug, most likely.

POSTSCRIPT: The attached patch appears to fix the bug for me.   :D  This simply disables the previously identified troublemaking code.  With this patch, if the experimental version is less than 8, there is no save or restore done for "replacing".

drs01

I have the same problem, can you release an update with the patch applied.

Cheers.

Bernd Gabriel

#7
@neroden
I revised you patch. It is a bit too drastic. If you load an older savegame having stored replacements, the stored values must be read, but ignored.

I will provide corrected simconvoi.cc in my branch soon.

The corrected simconvoi.cc can be found in the branch "7.2" at http://github.com/BerndGabriel/simutrans-experimental
The journey is the reward!

neroden

Quote from: Bernd Gabriel on March 31, 2010, 10:26:31 PM
@neroden
I revised you patch. It is a bit too drastic. If you load an older savegame having stored replacements, the stored values must be read, but ignored.
Excellent.  Thank you.   I didn't understand the code, hence the chainsaw-like approach.  :-)

jamespetts

Neroden and Bernd,

thank you very much indeed for your report and patches. Apologies for not replying sooner: I have been otherwise engaged for the past few days. I am currently staying away without access to the code, but I may have time to look into this on Tuesday.

What I think that I'll have to do is hold development of 8.0 until the bugs in 7.2 are fixed, given that the bugs seem quite serious, and produce a version 7.3, being 7.2 with the critical bugs fixed and nothing more.

As to the replacing - the only disabling of replacing was in the GUI to stop people from instigating replacing in the first place. The code that is present is the partly rewritten code (previously, there was no "replace_data_t" class, all the replacing information being contained within convoi_t - the idea is to save memory in the convoy_t class, since, for most of the time, there is no replacing going on). If the replace code is invoked otherwise than by the disabled GUI, there is indeed a bug that needs fixing in any event.

As to "delete this", the C++ FAQ (to which I linked) was clear that this was acceptable coding in certain conditions, which I believe were fulfilled. The system is that, whenever a particular convoy has finished with the replace data object that it is using, it calls "decrement_convoys()" on the replace data object, rather than delete it, as there may be other convoys sharing that object. If it is the last convoy to use it, the replace data object deletes itself. How else would this function be accomplished as efficiently without delete this;?

Thanks again to everyone for all the work on this issue, and apologies that there are so many problems with 7.2.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

neroden

Quote from: jamespetts on April 02, 2010, 01:55:06 PMIf the replace code is invoked otherwise than by the disabled GUI, there is indeed a bug that needs fixing in any event.

Right -- it was invoked by loading of saved games, which is what Bernd fixed.  :-)  When *functioning* it *should* in fact be invoked by loading of saved games, of course.

Quote
As to "delete this", the C++ FAQ (to which I linked) was clear that this was acceptable coding in certain conditions, which I believe were fulfilled. The system is that, whenever a particular convoy has finished with the replace data object that it is using, it calls "decrement_convoys()" on the replace data object, rather than delete it, as there may be other convoys sharing that object. If it is the last convoy to use it, the replace data object deletes itself. How else would this function be accomplished as efficiently without delete this;?
Oh, it is the cleanest implementation -- it just makes for very difficult debugging when something fails to set the data object up correctly and causes a segfault at delete time, because it breaks the backtrace.  If you make sure all the code calling it is bug-free all is well :-) then there's no problem. 

The alternative implementation is to realize that object-oriented programming isn't the be-all and end-all: you implement decrement_convoys(OBJECT) as a function external to the class, but "friend" to it, which calls the internal decrement_convoys on the object, and then checks for deletion and deletes it if necessary.

jamespetts

The loading would only cause a convoy replacer instance to be created if the convoy was in the process of being replaced in the pre-7.2 version, I think.

As to "delete this" - does the backtrace destroying happen in MSVC++? I haven't had that problem, I don't think; and, even if it is a problem, one can just set a conditional breakpoint on the "delete this" line to find it...
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

neroden

Quote from: jamespetts on April 02, 2010, 04:07:17 PM
The loading would only cause a convoy replacer instance to be created if the convoy was in the process of being replaced in the pre-7.2 version, I think.
Unfortunately, while it did not actually cause an instance to be created, it seems to have half-set-up some stuff.  As a result a bunch of IF statements triggered clauses which started calling routines on nonexistent objects, which resulted in the eventual crashes.

QuoteAs to "delete this" - does the backtrace destroying happen in MSVC++? I haven't had that problem, I don't think; and, even if it is a problem, one can just set a conditional breakpoint on the "delete this" line to find it...

I will do that (set the breakpoint right before that) as it's a perfectly feasible workaround.

Bernd Gabriel

Both loading and saving a game created a new replacer.
The journey is the reward!

jamespetts

Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Bernd Gabriel

Yes. is_replacing was only checked in the exp version >= 8 section :(
The exp version < 8 section could not work at all: writing was excluded completely, while reading was completely active.
Thus the codes were incompatible. At least replacing_vehicles_count = 0 had to be written to keep it compatible.
The journey is the reward!

jamespetts

Ahh, thank you for your analysis of this. This may explain a lot of why loading/saving is not working properly. I'll have to fix that!
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Bernd Gabriel

A fix can be found in my "7.2" branch  8)
The journey is the reward!

jamespetts

Ahh, thank you - very kind! Will apply when I get home...
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.