The International Simutrans Forum

 

Author Topic: And another crash bug....  (Read 6273 times)

0 Members and 1 Guest are viewing this topic.

Offline neroden

  • Devotees (Inactive)
  • *
  • Posts: 831
  • Nathanael Nerode
And another crash bug....
« on: 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: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.
« Last Edit: March 27, 2010, 12:12:55 PM by neroden »

Offline Bernd Gabriel

  • *
  • Posts: 230
  • Addicted to Simutrans: since 2003
    • Fast Function Factory
  • Languages: DE, EN, C++
Re: And another crash bug....
« Reply #1 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.

Offline neroden

  • Devotees (Inactive)
  • *
  • Posts: 831
  • Nathanael Nerode
Re: And another crash bug....
« Reply #2 on: March 29, 2010, 11:29:09 PM »
The sa
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

  • Guest
Re: And another crash bug....
« Reply #3 on: 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 passengers.

Offline neroden

  • Devotees (Inactive)
  • *
  • Posts: 831
  • Nathanael Nerode
Re: And another crash bug....
« Reply #4 on: March 30, 2010, 05:00:12 AM »
Yep this is still happening.   This was an autosave file (renamed).

http://files.simutrans.us/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. 


Offline neroden

  • Devotees (Inactive)
  • *
  • Posts: 831
  • Nathanael Nerode
Re: And another crash bug....
« Reply #5 on: 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.   :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".
« Last Edit: March 30, 2010, 01:03:20 PM by neroden »

drs01

  • Guest
Re: And another crash bug....
« Reply #6 on: March 31, 2010, 05:39:10 PM »
I have the same problem, can you release an update with the patch applied.

Cheers.

Offline Bernd Gabriel

  • *
  • Posts: 230
  • Addicted to Simutrans: since 2003
    • Fast Function Factory
  • Languages: DE, EN, C++
Re: And another crash bug....
« Reply #7 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.

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
« Last Edit: April 01, 2010, 12:30:09 AM by Bernd Gabriel »

Offline neroden

  • Devotees (Inactive)
  • *
  • Posts: 831
  • Nathanael Nerode
Re: And another crash bug....
« Reply #8 on: April 01, 2010, 02:39:51 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.  :-)

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18745
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: And another crash bug....
« Reply #9 on: 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" 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.

Offline neroden

  • Devotees (Inactive)
  • *
  • Posts: 831
  • Nathanael Nerode
Re: And another crash bug....
« Reply #10 on: April 02, 2010, 03:53:02 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.

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.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18745
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: And another crash bug....
« Reply #11 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.

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...

Offline neroden

  • Devotees (Inactive)
  • *
  • Posts: 831
  • Nathanael Nerode
Re: And another crash bug....
« Reply #12 on: April 02, 2010, 06:18:31 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.

Quote
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...

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

Offline Bernd Gabriel

  • *
  • Posts: 230
  • Addicted to Simutrans: since 2003
    • Fast Function Factory
  • Languages: DE, EN, C++
Re: And another crash bug....
« Reply #13 on: April 02, 2010, 09:55:44 PM »
Both loading and saving a game created a new replacer.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18745
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: And another crash bug....
« Reply #14 on: April 02, 2010, 10:02:20 PM »
For every convoy?

Offline Bernd Gabriel

  • *
  • Posts: 230
  • Addicted to Simutrans: since 2003
    • Fast Function Factory
  • Languages: DE, EN, C++
Re: And another crash bug....
« Reply #15 on: 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.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18745
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: And another crash bug....
« Reply #16 on: 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!

Offline Bernd Gabriel

  • *
  • Posts: 230
  • Addicted to Simutrans: since 2003
    • Fast Function Factory
  • Languages: DE, EN, C++
Re: And another crash bug....
« Reply #17 on: April 02, 2010, 10:19:05 PM »
A fix can be found in my "7.2" branch  8)

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 18745
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: And another crash bug....
« Reply #18 on: April 02, 2010, 10:22:16 PM »
Ahh, thank you - very kind! Will apply when I get home...