News:

Simutrans Wiki Manual
The official on-line manual for Simutrans. Read and contribute.

Compiled way-improvements branch; crash when building station on new map

Started by Junna, July 14, 2014, 11:32:22 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Junna

Making a new map, and building a new station crashes the game. For some reason it doesn't happen on the map generated at start (loading demo.sve crashes, so it had to be removed).

jamespetts

Hmm - having trouble reproducing this. Can you give more detailed steps to reproduce?
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.


jamespetts

I still can't produce this, I am afraid, even with your saved game and pakset, building several stations on the railway line and several 'bus stops. Can you give any more details as to the steps necessary to reproduce?
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.

Junna

Hmm, very peculiar. Do you think it's possible it's something off with my compilation?

http://simutrans-germany.com/files/upload/Simutrans_Experimental_Comp.rar

Here's the resultant exe.

jamespetts

Hmm, very odd: even with your executable, I cannot reproduce this. I did not use your config files, since these should all be saved with the saved game. Have you tried running this in MSVS in debug mode to get some idea of where the crash occurs?
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.

Junna

I get errors like this constantly during compilation:

1>c:\simuscr2\dataobj\ribi.h : warning C4819: The file contains a character that cannot be represented in the current code page (932). Save the file in Unicode format to prevent data loss
1>c:\simuscr2\gui\../simconvoi.h : warning C4819: The file contains a character that cannot be represented in the current code page (932). Save the file in Unicode format to prevent data loss

EDIT: using a debug build does not offer any help. It crashes without message.

jamespetts

Hmm, that is extremely odd, especially as I do not get that type of compiler warning so far as I am aware. When you say that it crashes without a message with a debug build, are you actually running the executable with a debugger attached or just starting a debug build as you would start any other application?
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.

Junna

The latter. Using the debug function and attaching the process gives:

Unhandled exception at 0x005DBB39 in Simutrans-Experimental.exe: 0xC0000005: Access violation reading location 0x00000034.

Unhandled exception at 0x77AE016E (ntdll.dll) in Simutrans-Experimental.exe: 0x00000000:


I've never used this function, so I don't know what I am supposed to look for.

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.

Junna

'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\clbcatq.dll'. Symbols loaded.
The thread 0x594c has exited with code 0 (0x0).
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\MMDevAPI.dll'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\propsys.dll'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\wdmaud.drv'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\ksuser.dll'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\avrt.dll'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\setupapi.dll'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\cfgmgr32.dll'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\devobj.dll'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\AudioSes.dll'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\msacm32.drv'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\msacm32.dll'. Symbols loaded.
'Simutrans-Experimental.exe' (Win32): Loaded 'C:\Windows\SysWOW64\midimap.dll'. Symbols loaded.
The thread 0x5f9c has exited with code 7731708 (0x75f9fc).
First-chance exception at 0x005DBB39 in Simutrans-Experimental.exe: 0xC0000005: Access violation reading location 0x00000034.
Unhandled exception at 0x005DBB39 in Simutrans-Experimental.exe: 0xC0000005: Access violation reading location 0x00000034.

Is this the right part?

jamespetts

Ahh, no: I need to know where the little yellow arrow showing the next line of code to be executed is.
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.

Junna

I am afraid I can't see any such window, how do I get that to display? It shows the orange line ready at the bottom, but I can't find any display of the immediate code. I am afraid I am not very bright and cannot work this out very easily. I tried fiddling about but I can't find a way to display the code as suggested?

//

Suppose it would be this:

_NtQueueApcThreadEx@24:
77AF15B4  mov         eax,12Eh 
77AF15B9  xor         ecx,ecx 
77AF15BB  lea         edx,[esp+4] 
77AF15BF  call        dword ptr fs:[0C0h] 
77AF15C6  add         esp,4 
77AF15C9  ret         18h 
_NtRaiseException@12:
77AF15CC  mov         eax,12Fh 
77AF15D1  xor         ecx,ecx 
77AF15D3  lea         edx,[esp+4] 
77AF15D7  call        dword ptr fs:[0C0h] 
--ARROW HERE-77AF15DE  add         esp,4 
77AF15E1  ret         0Ch 

jamespetts

What do you mean by the orange line? Perhaps you could post a screenshot?
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.

Junna

well, I think I found the relevant part, unless that is wrong? (the orange line at the bottom just displays the state of the debugger)

jamespetts

The excerpt that you posted above appears to be assembler code or even machine code: it does not tell me which line of the actual program triggered the crash. And what software are you using as your development environment? I am not aware of any orange lines in MSVS Express 2012.
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.

DrSuperGood

The assembly posted appears to be outside the scope of the problem (thread management code, probably because the thread crashed).

He needs to know what line of source is raising the access violation. Usually this is the result of a null pointer dereference and it causes a crash as the OS Virtual Memory system has that process address page not set as readable (not allocated) or writeable (code). Since null points to either 0 or almost always some garbage then de-referencing a struct member from it will access memory that is usually used for code on Windows systems. For security reasons the OS forces a process crash.

You need to use a debug build and it should then allow you, when run with a debugger, to see what line of code caused the fault.

Junna

Oh, thank you, I was using the non-debug build and attaching it to that. I see now.

in simworld.cc:


Quoterands[23] = 0;

    step_passengers_and_mail(delta_t);

    // the inhabitants stuff
    finance_history_year[0][WORLD_CITICENS] = finance_history_month[0][WORLD_CITICENS] = 0;
    finance_history_year[0][WORLD_JOBS] = finance_history_month[0][WORLD_JOBS] = 0;
    finance_history_year[0][WORLD_VISITOR_DEMAND] = finance_history_month[0][WORLD_VISITOR_DEMAND] = 0;

    FOR(weighted_vector_tpl<stadt_t*>, const city, stadt)
    {
        finance_history_year[0][WORLD_CITICENS] += city->get_finance_history_month(0, HIST_CITICENS);
        finance_history_month[0][WORLD_CITICENS] += city->get_finance_history_year(0, HIST_CITICENS);

        finance_history_month[0][WORLD_JOBS] += city->get_finance_history_month(0, HIST_JOBS);
        finance_history_year[0][WORLD_JOBS] += city->get_finance_history_year(0, HIST_JOBS);

        finance_history_month[0][WORLD_VISITOR_DEMAND] += city->get_finance_history_month(0, HIST_VISITOR_DEMAND);
        finance_history_year[0][WORLD_VISITOR_DEMAND] += city->get_finance_history_year(0, HIST_VISITOR_DEMAND);
    }

    DBG_DEBUG4("karte_t::step", "step factories");
    FOR(vector_tpl<fabrik_t*>, const f, fab_list) {
        f->step(delta_t);
    }
rands[16] = get_random_seed();

    finance_history_year[0][WORLD_FACTORIES] = finance_history_month[0][WORLD_FACTORIES] = fab_list.get_count();

    // step powerlines - required order: pumpe, senke, then powernet
    DBG_DEBUG4("karte_t::step", "step poweline stuff");
    pumpe_t::step_all( delta_t );
    senke_t::step_all( delta_t );
    powernet_t::step_all( delta_t );
rands[17] = get_random_seed();

    DBG_DEBUG4("karte_t::step", "step players");
    // then step all players
    for(  int i=0;  i<MAX_PLAYER_COUNT;  i++  ) {
        if(  spieler!= NULL  ) {
            spieler->step();
        }
    }
--arrow--->rands[18] = get_random_seed();

    DBG_DEBUG4("karte_t::step", "step halts");
    haltestelle_t::step_all();
rands[19] = get_random_seed();

    // Re-check paths if the time has come.
    // Long months means that it might be necessary to do
    // this more than once per month to get up to date
    // routings for goods/passengers.
    // Default: 8192 ~ 1h (game time) at 125m/tile.

    if((steps % get_settings().get_reroute_check_interval_steps()) == 0)
    {
        path_explorer_t::refresh_all_categories(true);
    }

    // ok, next step
    INT_CHECK("karte_t::step 6");

    if((steps%8)==0) {
        DBG_DEBUG4("karte_t::step", "checkmidi");
        check_midi();
    }

    // will also call all objects if needed ...
    if(  recalc_snowline()  ) {
        pending_season_change ++;
    }

    // number of playing clients changed
    if(  env_t::server  &&  last_clients!=socket_list_t::get_playing_clients()  ) {
        if(  env_t::server_announce  ) {
            // inform the master server
            announce_server( 1 );
        }

        // check if player has left and send message
        for(uint32 i=0; i < socket_list_t::get_count(); i++) {
            socket_info_t& info = socket_list_t::get_client(i);
            if (info.state == socket_info_t::has_left) {
                nwc_nick_t::server_tools(this, i, nwc_nick_t::FAREWELL, NULL);
                info.state = socket_info_t::inactive;
            }
        }
        last_clients = socket_list_t::get_playing_clients();
        // add message via tool
        cbuffer_t buf;
        buf.printf(translator::translate("Now %u clients connected.", settings.get_name_language_id()), last_clients);
        werkzeug_t *w = create_tool( WKZ_ADD_MESSAGE_TOOL | SIMPLE_TOOL );
        w->set_default_param( buf );
        set_werkzeug( w, NULL );
        // since init always returns false, it is safe to delete immediately
        delete w;

There's also a warning; "file load: some bytes have been replaced with unicode substitution character while loading file simhalt.cc with Japanese (shift-JIS) encoding. Saving the file will not preserve the original file contents."

Does this have something to do with only I being able to reproduce it?

Quote
008362B5 E8 0A 2F BD FF       call        haltestelle_t::step_all (04091C4h) 
008362BA E8 8F B8 BD FF       call        get_random_seed (0411B4Eh) 
008362BF B9 04 00 00 00       mov         ecx,4 

Trying again, I notice that crashes only occur if there is a building or something within the coverage of the structure. For example, building an industrial station in the middle of nowhere, does not cause the crash.

Building one near a city, it crashes:

Quote}
rands[18] = get_random_seed();

    DBG_DEBUG4("karte_t::step", "step halts");
    haltestelle_t::step_all();
(red ring here)--rands[19] = get_random_seed();

    // Re-check paths if the time has come.
    // Long months means that it might be necessary to do
    // this more than once per month to get up to date
    // routings for goods/passengers.
    // Default: 8192 ~ 1h (game time) at 125m/tile.

    if((steps % get_settings().get_reroute_check_interval_steps()) == 0)
    {
        path_explorer_t::refresh_all_categories(true);
    }

    // ok, next step
    INT_CHECK("karte_t::step 6");

    if((steps%8)==0) {
        DBG_DEBUG4("karte_t::step", "checkmidi");
        check_midi();

jamespetts

You refer to a red ring: is it a solid red circle, or an outline red ring with grey or white in the centre?
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.

Junna

Quote from: jamespetts on July 16, 2014, 09:20:10 AM
You refer to a red ring: is it a solid red circle, or an outline red ring with grey or white in the centre?

The latter.

Junna

This crash does not occur when system nationality setting is changed to a country that uses a Latin alphabet. Changing setting to Japan does, however, result in the crash, at least for me.

jamespetts

That is extremely odd. I have not changed anything to do with the alphabet settings.
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.

prissi

That sounds rather like a problem with the creation of non-unique city names. I may be that there a more than one town named simcity (i.e. run out of unique names). If the list of unique names is empty, then there might be also an attempt to delete an element twice. A log may indicate something like that.

jamespetts

Ahh, thank you, that is helpful. Junna - may I ask you to test this by using a city list file for the relevant language and seeing whether the problem recurs?
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.

Philip

I'm not sure this is the same bug, but I've seen a very similar crash, albeit intermittently. The crash happens in haltestelle_t::step_all(); I'm very suspicious about the static iterator there, and think it's causing use-after-free errors. vector_tpl iterators are just pointers into the vector's array, but that array moves when resized, which happens when we append() a new halt and the array size crosses a power-of-two boundary. Then our pointer is pointing to freed data, which will eventually cause crashes.

What I don't understand is why this crash doesn't happen much more often, since iter will be invalid after every array resize, and that should cause a crash almost certainly, so I'm definitely still missing something. Still, I think the change should be harmless at worst and avoid the crash at best.

Patch:

diff --git a/simhalt.cc b/simhalt.cc
index 31dcf53..8fc12e7 100644
--- a/simhalt.cc
+++ b/simhalt.cc
@@ -498,6 +498,7 @@ haltestelle_t::haltestelle_t(loadsave_t* file)
        rdwr(file);

        alle_haltestellen.append(self);
+       restart_halt_iterator = true;

        // Added by : Knightly
        inauguration_time = 0;
@@ -509,6 +510,7 @@ haltestelle_t::haltestelle_t(koord k, spieler_t* sp)
        self = halthandle_t(this);
        assert( !alle_haltestellen.is_contained(self) );
        alle_haltestellen.append(self);
+       restart_halt_iterator = true;

        //markers[ self.get_id() ] = current_marker;


jamespetts

Thank you very much for looking into this. My understanding is that the iterators of Simutrans's standard collections are intended to work even if elements are deleted from them during the iteration; can any Standard developer confirm whether this is indeed the case or not?

May I ask - what exactly does the restart_halt_iterator flag do?

(Welcome back, incidentally - we haven't seen you in a long time).
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.

Philip

Quote from: jamespetts on August 07, 2014, 09:08:42 AM
Thank you very much for looking into this. My understanding is that the iterators of Simutrans's standard collections are intended to work even if elements are deleted from them during the iteration; can any Standard developer confirm whether this is indeed the case or not?

I believe that's true for deletion, but not for insertion, in the vector_tpl case (they grow but never shrink). It's building new stations that triggers the problem, I think, not removing existing stations. The simhalt.cc code in standard is subtly different, which might reduce the chances of seeing the problem but doesn't appear to avoid it entirely.

It just occurred to me that the reason not everyone's seeing the bug may be that demo.sve contains halts. I'm not loading demo.sve, and I don't think Junna was either.

restart_halt_iterator means we start the iterator over again at (the new) alle_haltestellen.begin(), which avoids the issue and should be okay since new stops aren't being built all the time.

It might be more readable to get rid of the restart_halt_iterator flag and reset the iterator manually when inserting or deleting stations, or to use an index into the vector instead, replacing two static variables (one of which is in function scope) by one (which I suppose can be placed in function scope, too, if you prefer it that way):


diff --git a/simhalt.cc b/simhalt.cc
index 31dcf53..97eb116 100644
--- a/simhalt.cc
+++ b/simhalt.cc
@@ -86,7 +86,7 @@ uint8 haltestelle_t::reconnect_counter = 0;
static const uint8 pedestrian_generate_max = 16;

// controls the halt iterator in step_all():
-static bool restart_halt_iterator = true;
+static uint32 step_all_halt_index = 0;

void haltestelle_t::step_all()
{
@@ -94,15 +94,16 @@ void haltestelle_t::step_all()
        if (count)
        {
                const uint32 loops = min(count, 256);
-               static vector_tpl<halthandle_t>::iterator iter;
                for (uint32 i = 0; i < loops; ++i)
                {
-                       if (restart_halt_iterator || iter == alle_haltestellen.end())
+                       if (step_all_halt_index >= count)
                        {
-                               restart_halt_iterator = false;
-                               iter = alle_haltestellen.begin();
+                               step_all_halt_index = 0;
                        }
-                       (*iter++)->step();
+
+                       alle_haltestellen[step_all_halt_index]->step();
+
+                       step_all_halt_index++;
                }
        }
}
@@ -436,9 +437,6 @@ halthandle_t haltestelle_t::create(loadsave_t *file)
  */
void haltestelle_t::destroy(halthandle_t const halt)
{
-       // just play safe: restart iterator at zero ...
-       restart_halt_iterator = true;
-
        delete halt.get_rep();
}



Thanks for welcoming me back, it's wonderful to see you're still at it! I've had health issues but have been able to hack up a few things in the last few weeks, some of which might eventually be interesting for inclusion.

jamespetts

I am sorry that you have not been well - I do hope that you are on the mend now. I have incorporated your original patch into the way-improvements branch: I should be grateful if Junna could test this to see whether the problem is fixed.

I should say, for Philip's benefit, that I have been forced to neglect Simutrans rather these last few months as my time and energies have been consumed by the process of buying a house, which may very well last another two or three months, but I hope to get back into things once I am moved in. I may be able to do small things in the meantime, however. Any worthwhile progress that can be made by anyone else in the meantime would be most welcome.
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.

jamespetts

Junna, can you confirm whether this original issue is fixed on the way-improvements branch?
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.