And another crash bug.... March 27, 2010, 12:09:30 pm 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:4242I'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. Quote Selected Last Edit: March 27, 2010, 12:12:55 pm by neroden
Re: And another crash bug.... Reply #1 – 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. Quote Selected
Re: And another crash bug.... Reply #2 – March 29, 2010, 11:29:09 pm The saQuote from: Bernd Gabriel – on March 28, 2010, 03:11:37 pmThe 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. Quote Selected
Re: And another crash bug.... Reply #3 – March 30, 2010, 12:30:51 am 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 p****engers. Quote Selected
Re: And another crash bug.... Reply #4 – March 30, 2010, 05:00:12 am 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.sveIf 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. Quote Selected
Re: And another crash bug.... Reply #5 – March 30, 2010, 05:39:59 am 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. 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". Quote Selected Last Edit: March 30, 2010, 01:03:20 pm by neroden
Re: And another crash bug.... Reply #6 – March 31, 2010, 05:39:10 pm I have the same problem, can you release an update with the patch applied.Cheers. Quote Selected
Re: And another crash bug.... Reply #7 – March 31, 2010, 10:26:31 pm @nerodenI 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 Quote Selected Last Edit: April 01, 2010, 12:30:09 am by Bernd Gabriel
Re: And another crash bug.... Reply #8 – April 01, 2010, 02:39:51 pm Quote from: Bernd Gabriel – on March 31, 2010, 10:26:31 pm@nerodenI 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. :-) Quote Selected
Re: And another crash bug.... Reply #9 – April 02, 2010, 01:55:06 pm 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" cl****, all the replacing information being contained within convoi_t - the idea is to save memory in the convoy_t cl****, 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. Quote Selected
Re: And another crash bug.... Reply #10 – April 02, 2010, 03:53:02 pm Quote from: jamespetts – on April 02, 2010, 01:55:06 pm If 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.QuoteAs 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 cl****, but "friend" to it, which calls the internal decrement_convoys on the object, and then checks for deletion and deletes it if necessary. Quote Selected
Re: And another crash bug.... Reply #11 – 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.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... Quote Selected
Re: And another crash bug.... Reply #12 – April 02, 2010, 06:18:31 pm Quote from: jamespetts – on April 02, 2010, 04:07:17 pmThe 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. Quote Selected
Re: And another crash bug.... Reply #13 – April 02, 2010, 09:55:44 pm Both loading and saving a game created a new replacer. Quote Selected
Re: And another crash bug.... Reply #14 – April 02, 2010, 10:02:20 pm For every convoy? Quote Selected
Re: And another crash bug.... Reply #15 – April 02, 2010, 10:14:38 pm 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. Quote Selected
Re: And another crash bug.... Reply #16 – April 02, 2010, 10:17:10 pm 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! Quote Selected
Re: And another crash bug.... Reply #17 – April 02, 2010, 10:19:05 pm A fix can be found in my "7.2" branch Quote Selected
Re: And another crash bug.... Reply #18 – April 02, 2010, 10:22:16 pm Ahh, thank you - very kind! Will apply when I get home... Quote Selected
Topic: And another crash bug.... (Read 12392 times)
previous topic - next topic
