HomeWildfire Games

Fix replay menu loading time by using a cache file
AuditedrP19674

Description

Fix replay menu loading time by using a cache file

Reviewed by: elexis
Fixes #3433
Differential Revision: https://code.wildfiregames.com/D39

Details

Event Timeline

weberkai raised a concern with this commit.EditedApr 26 2019, 12:52 PM
weberkai added a subscriber: weberkai.

Hi!

After this commit, during variable creation, tempCacheFileName and also cacheFileName are created at VisualReplay.cpp (https://code.wildfiregames.com/rP19674#change-sw16fvIpoPBs):

static const OsPath tempCacheFileName = VisualReplay::GetDirectoryName() / L"replayCache_temp.json";
static const OsPath cacheFileName = VisualReplay::GetDirectoryName() / L"replayCache.json";

which calls GetDirectoryName which uses global variable g_args:

const Paths paths(g_args);

But in main.cpp g_args is filled at function RunGameOrAtlas during runtime, after variable creation:

g_args = args;

Using global g_args early at variable creation while it is not filled yet is causing segmentation fault in Slackware. Don't know why in other distros this won't happen (maybe newer stdlib version... Or different default compiler options... I've tried with g++ 5.3.0).

In previous Slackware working version (0.0.21), VisualReplay.cpp used GetDirectoryName only in runtime. That's why there was no error in Slackware at that version.

Just moved these at VisualReplay.cpp again back to inside VisualReplay::ReadCacheFile and VisualReplay::StoreCacheFile and now its working:

static OsPath tempCacheFileName = VisualReplay::GetDirectoryName() / L"replayCache_temp.json";
static OsPath cacheFileName = VisualReplay::GetDirectoryName() / L"replayCache.json";

This solves Bug Tracker: https://trac.wildfiregames.com/ticket/4789
Forum thread: https://wildfiregames.com/forum/index.php?/topic/24735-unable-to-start-on-slackware64-142/

My solution is more like "Proof of Concept", not a definitive optimized solution, as I am not a very experienced coder. I believe the coder staff can provide a much better one.

Please, may you provide a better solution for us? (Maybe using some kind of g_args reference... Don't know how yet...)
Or, if you think this solution is ok, then please tell me and I will post this as a diff file.
Thank you very much!

And thanks for your awesome work!

This commit now has outstanding concerns.Apr 26 2019, 12:52 PM

First of all thanks for the clear information, I agree that

static const OsPath tempCacheFileName = VisualReplay::GetDirectoryName() / L"replayCache_temp.json";

is wrong because it was intended to use the value after g_args was set in RunGameOrAtlas of main.cpp.

Using global g_args early at variable creation while it is not filled yet is causing segmentation fault in Slackware

However, CmdLineArgs g_args; is constructed with the default constructor already, its member OsPath m_Arg0;. The OsPath has a std::wstring path; and a wchar_t separator;`.
The crash occurs in:

#0  0xb6672805 in std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::basic_string(std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&) () from /usr/lib/libstdc++.so.6

which sounds like the std::wstring path; default constructor fails (before the separator is set to slash or backslash).

This call occurs in /usr/lib/libstdc++.so.6, so slackware must have some different libstdc++ version or configuration, probably related to __static_initialization_and_destruction_0:

#5  0x0851e645 in __static_initialization_and_destruction_0 (__initialize_p=1, __priority=65535) at ../../../source/ps/VisualReplay.cpp:43

So to me it sounds like both the VisualReplay.cpp static init call should be fixed, and perhaps one can fix the path class(es) somewhere before removing the one way to reproduce this segfault.
(Quick search says all other static consts are primitves that don't call functions that are assigned constant values, so VisualReplay.cpp is the only place with this mistake).

Trying to make sense from the C++ specs, the member std::wstring path of the class Path is not default-initialized to indeterminate value, but zero-initialized to empty string, right?
So does it segfaults when the Path constructor tries to zero-initialize path to empty string, or does it happen in the constructor when it tries to read from path in path.find('\\') in DetectSeparator()?
Either the Path class or Slackware settings don't abide by the C++ specs if I understand correctly.

@vladislavbelov do you see undefined behavior for the std::wstring path member in the path.h constructor that could explain why Slackware segfaults but not the other distributions, see stacktrace at #4789?

This comment was removed by weberkai.

So it sounds like it segfaults due to an attempt to dereference an uninitialized pointer p.

But why is Path(const char* p) called and not Path()? Is the stacktrace otherwise identical to the one in the ticket?

weberkai added a comment.EditedApr 26 2019, 2:47 PM

The crash happens here:

At ps/GameSetup/CmdLineArgs.cpp:

OsPath CmdLineArgs::GetArg0() const
{
	return m_Arg0; <-- Here
}
weberkai added a comment.EditedApr 26 2019, 2:56 PM

I've started;

gdb ../pyrogenesis

Then

  break CmdLineArgs.cpp:109
(this line -> OsPath CmdLineArgs::GetArg0() const)

Then issue 5 'step', at 6th 'step':

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff3fe4bdb in std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::basic_string(std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> > const&) () from /usr/lib64/libstdc++.so.6

then issue:

thread apply all bt full

Output is:
(gdb) #0 Path::Path (this=0x7fffffffd6d0) at ../../../source/lib/path.h:77
(gdb) No locals.
(gdb) #1 CmdLineArgs::GetArg0 (this=this@entry=0xe312c0 <g_args>) at ../../../source/ps/GameSetup/CmdLineArgs.cpp:111
(gdb) No locals.
(gdb) #2 0x00000000006750db in Paths::Paths (this=0x7fffffffdae0, args=...) at ../../../source/ps/GameSetup/Paths.cpp:35
(gdb) subdirectoryName = <optimized out>
Undefined command: "subdirectoryName". Try "help".
(gdb) func = "Paths"
Undefined command: "func". Try "help".
(gdb) #3 0x00000000006f7bc7 in VisualReplay::GetDirectoryName () at ../../../source/ps/VisualReplay.cpp:47
(gdb) paths = {m_root = {path = {static npos = 18446744073709551615,
Undefined command: "paths". Try "help".
(gdb) _M_dataplus = {<std::allocator<wchar_t>> = {<gnu_cxx::new_allocator<wchar_t>> = {<No data fields>}, <No data fields>},
Undefined command: "_M_dataplus". Try "help".
(gdb) _M_p = 0x7ffff4290158 <std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::_Rep::_S_empty_rep_storage+24> L""}},
Undefined command: "_M_p". Try "help".
(gdb) separator = 47 L'/'}, m_rdata = {path = {static npos = 18446744073709551615,
Undefined command: "separator". Try "help".
(gdb) _M_dataplus = {<std::allocator<wchar_t>> = {<
gnu_cxx::new_allocator<wchar_t>> = {<No data fields>}, <No data fields>},
Undefined command: "_M_dataplus". Try "help".
(gdb) _M_p = 0x7ffff4290158 <std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::_Rep::_S_empty_rep_storage+24> L""}},
Undefined command: "_M_p". Try "help".
(gdb) separator = 47 L'/'}, m_gameData = {path = {static npos = 18446744073709551615,
Undefined command: "separator". Try "help".
(gdb) _M_dataplus = {<std::allocator<wchar_t>> = {<gnu_cxx::new_allocator<wchar_t>> = {<No data fields>}, <No data fields>},
Undefined command: "_M_dataplus". Try "help".
(gdb) _M_p = 0x7ffff4290158 <std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::_Rep::_S_empty_rep_storage+24> L""}},
Undefined command: "_M_p". Try "help".
(gdb) separator = 47 L'/'}, m_userData = {path = {static npos = 18446744073709551615,
Undefined command: "separator". Try "help".
(gdb) _M_dataplus = {<std::allocator<wchar_t>> = {<
gnu_cxx::new_allocator<wchar_t>> = {<No data fields>}, <No data fields>},
Undefined command: "_M_dataplus". Try "help".
(gdb) _M_p = 0x7ffff4290158 <std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::_Rep::_S_empty_rep_storage+24> L""}},
Undefined command: "_M_p". Try "help".
(gdb) separator = 47 L'/'}, m_config = {path = {static npos = 18446744073709551615,
Undefined command: "separator". Try "help".
(gdb) _M_dataplus = {<std::allocator<wchar_t>> = {<gnu_cxx::new_allocator<wchar_t>> = {<No data fields>}, <No data fields>},
Undefined command: "_M_dataplus". Try "help".
(gdb) _M_p = 0x7ffff4290158 <std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::_Rep::_S_empty_rep_storage+24> L""}},
Undefined command: "_M_p". Try "help".
(gdb) separator = 47 L'/'}, m_cache = {path = {static npos = 18446744073709551615,
Undefined command: "separator". Try "help".
(gdb) _M_dataplus = {<std::allocator<wchar_t>> = {<
gnu_cxx::new_allocator<wchar_t>> = {<No data fields>}, <No data fields>},
Undefined command: "_M_dataplus". Try "help".
(gdb) _M_p = 0x7ffff4290158 <std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::_Rep::_S_empty_rep_storage+24> L""}},
Undefined command: "_M_p". Try "help".
(gdb) separator = 47 L'/'}, m_logs = {path = {static npos = 18446744073709551615,
Undefined command: "separator". Try "help".
(gdb) _M_dataplus = {<std::allocator<wchar_t>> = {<gnu_cxx::new_allocator<wchar_t>> = {<No data fields>}, <No data fields>},
Undefined command: "_M_dataplus". Try "help".
(gdb) _M_p = 0x7ffff4290158 <std::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::_Rep::_S_empty_rep_storage+24> L""}},
Undefined command: "_M_p". Try "help".
(gdb) separator = 47 L'/'}}
Undefined command: "separator". Try "help".
(gdb) #4 0x0000000000433501 in
static_initialization_and_destruction_0 (initialize_p=1, priority=65535) at ../../../source/ps/VisualReplay.cpp:42
(gdb) No locals.
(gdb) #5 _GLOBALsub_I_VisualReplay.cpp(void) () at ../../../source/ps/VisualReplay.cpp:514
(gdb) No locals.
(gdb) #6 0x0000000000afd356 in
do_global_ctors_aux ()
(gdb) No symbol table info available.
(gdb) #7 0x0000000000000000 in ?? ()
(gdb) No symbol table info available.

weberkai added a comment.EditedApr 26 2019, 3:17 PM

This is the content of g_args

break Paths.cpp:35

( which is 'm_root = Root(args.GetArg0());')

(gdb) p args
$1 = (const CmdLineArgs &) @0xe312c0: {m_Args = {<std::_Vector_base<std::pair<CStr8, CStr8>, std::allocator<std::pair<CStr8, CStr8> > >> = {

    _M_impl = {<std::allocator<std::pair<CStr8, CStr8> >> = {<__gnu_cxx::new_allocator<std::pair<CStr8, CStr8> >> = {<No data fields>}, <No data fields>}, 
      _M_start = 0x0, _M_finish = 0x0, _M_end_of_storage = 0x0}}, <No data fields>}, m_Arg0 = {path = {static npos = 18446744073709551615, 
    _M_dataplus = {<std::allocator<wchar_t>> = {<__gnu_cxx::new_allocator<wchar_t>> = {<No data fields>}, <No data fields>}, _M_p = 0x0}}, separator = 0 L'\000'}, 
m_ArgsWithoutName = {<std::_Vector_base<CStr8, std::allocator<CStr8> >> = {
    _M_impl = {<std::allocator<CStr8>> = {<__gnu_cxx::new_allocator<CStr8>> = {<No data fields>}, <No data fields>}, _M_start = 0x0, _M_finish = 0x0, 
      _M_end_of_storage = 0x0}}, <No data fields>}}

After change, g_args at runtime:
$2 = {m_Args = {<std::_Vector_base<std::pair<CStr8, CStr8>, std::allocator<std::pair<CStr8, CStr8> > >> = {

    _M_impl = {<std::allocator<std::pair<CStr8, CStr8> >> = {<__gnu_cxx::new_allocator<std::pair<CStr8, CStr8> >> = {<No data fields>}, <No data fields>}, 
      _M_start = 0x0, _M_finish = 0x0, _M_end_of_storage = 0x0}}, <No data fields>}, m_Arg0 = {path = {static npos = 18446744073709551615, 
    _M_dataplus = {<std::allocator<wchar_t>> = {<__gnu_cxx::new_allocator<wchar_t>> = {<No data fields>}, <No data fields>}, 
      _M_p = 0xe700f8 L"/home/weberkai/0ad-0.0.23b-alpha/binaries/system/pyrogenesis"}}, separator = 47 L'/'}, 
m_ArgsWithoutName = {<std::_Vector_base<CStr8, std::allocator<CStr8> >> = {
    _M_impl = {<std::allocator<CStr8>> = {<__gnu_cxx::new_allocator<CStr8>> = {<No data fields>}, <No data fields>}, _M_start = 0x0, _M_finish = 0x0, 
      _M_end_of_storage = 0x0}}, <No data fields>}}
weberkai added a comment.EditedApr 26 2019, 3:36 PM

Sorry, if stupid question, but... The error can be in the initialization, if empty?

: path(p, p+wcslen(p)) <- Here?

At source/lib/os_path.h:

    Path(const wchar_t* p)
		: path(p, p+wcslen(p))
    {
		DetectSeparator();
    }

Sorry, if stupid question, but... The error can be in the initialization, if empty?

: path(p, p+wcslen(p)) <- Here?

At source/lib/os_path.h:

    Path(const wchar_t* p)
		: path(p, p+wcslen(p))
    {
		DetectSeparator();
    }

If it's this function http://www.cplusplus.com/reference/cwchar/wcslen/

This is the number of wide characters between wcs and the first null wide character (without including it).

Then uhm, the Slackware empty strings might not be null-terminated while the ones on other platforms might be. Maybe.
One can move the init list to the code and see with gdb what's in the given string p.

Also wondering whether the constructor with 0 arguments wasn't supposed to be called.

weberkai added a comment.EditedApr 26 2019, 5:36 PM

I've tried this test:

break path.h:82
break path.h:95
run

Then the program breaked at

Breakpoint 2, Path::Path (this=0x7fffffffdbb0, p=0xb24ec0 L"replayCache_temp.json") at ../../../source/lib/path.h:95
95		Path(const wchar_t* p)

And then segfault. I think the variable creation is fine, even GetDirectoryName() is working with empty string.

I think creating "OsPath tempCacheFileName" after evaluating the right side with this value (L"replayCache_temp.json") is causing Path to segfault.

I think creating "OsPath tempCacheFileName" after evaluating the right side with this value (L"replayCache_temp.json") is causing Path to segfault.

Then this would also segfault:

static const OsPath cacheFileName = OsPath() / L"replayCache.json";
static const OsPath cacheFileName = L"replayCache.json";

?

Also why is that empty string, when I run it, it's not empty, but the correct replay path (GameDataPaths).

g_args was created with the default constructor, so the the UserData() function still returns the correct replay folder - except for Slackware.

vladislavbelov added a comment.EditedApr 28 2019, 3:44 AM

@vladislavbelov do you see undefined behavior for the std::wstring path member in the path.h constructor that could explain why Slackware segfaults but not the other distributions, see stacktrace at #4789?

There is no undefined behaviour in the path.h. The problem is in the commit. g_args isn't constructed yet.

By C++11 standard:

Variables with ordered initialization defined within a single translation unit shall be initialized in the order of their definitions in the translation units. Otherwise, the initialization of a variable is indeterminately sequenced with respect to the initialization of a variable defined in a different translation unit.

So you shouldn't add dependencies between different non-trivial constructable objects that are in different translation unit. Why it doesn't work only for the Slackware platform? Probably because of some specific linkage order.

The current problem can be solved by a static local variable:

const OsPath& GetTempCacheFileName()
{
     static const OsPath tempCacheFileName = VisualReplay::GetDirectoryName() / L"replayCache_temp.json";
     return tempCacheFileName;
}

const OsPath& GetCacheFileName()
{
    static const OsPath cacheFileName = VisualReplay::GetDirectoryName() / L"replayCache.json";
    return cacheFileName;
}

But still you should be sure that it won't be called on a static global variable constructor stack.

weberkai added a comment.EditedApr 28 2019, 6:29 AM

Maybe a solution like this could be possible. Fill an object once at runtime and then call this object when needed.
https://stackoverflow.com/questions/14131855/c-assign-a-const-value-at-run-time
But I think declaring them as static const will not be possible.

This comment was removed by weberkai.

But I think declaring them as static const will not be possible.

I don't see a problem for a local static const except wrong dependencies.

g_args isn't constructed yet.

Are you sure that g_args_ isn't constructed yet on Slackware or that it is only not guaranteed to be constructed yet?

Because I agree with the latter, but the stacktrace looked suspicious, I was wondering whether there is a second bug on top of the first bug.
Why is the path constructor with one argument called, I thought the path constructor with zero arguments was to be called for the path in the CmdLineArgs constructor.

@weberkai can you confirm that the CmdLineArgs constructor had not been called before the VisualReplay lines are called?

Don't worry about VisualReplay, Imarok and me should be able to fix this, and it doesn't have to be static to begin with I think.

Are you sure that g_args_ isn't constructed yet on Slackware or that it is only not guaranteed to be constructed yet?

Yes, it's not guaranteed, and it's not constructed. I just tested it on Slackware 14.2.

Why is the path constructor with one argument called, I thought the path constructor with zero arguments was to be called for the path in the CmdLineArgs constructor.

If you have UB then the following actions are not guaranteed to be correct. First of all you need to fix the usage of uninitialised memory and only then check which constructor is called.

it's not constructed. I just tested it on Slackware 14.2.

Well then this doesn't indicate an issue with path.h (there might still be one despite the lack of indication though :P)

The current problem can be solved by a static local variable:

const OsPath& GetTempCacheFileName()
{
     static const OsPath tempCacheFileName = VisualReplay::GetDirectoryName() / L"replayCache_temp.json";
     return tempCacheFileName;
}
const OsPath& GetCacheFileName()
{
    static const OsPath cacheFileName = VisualReplay::GetDirectoryName() / L"replayCache.json";
    return cacheFileName;
}

I would propose to put it into the namespace VisualReplay::GetTempCacheFileName().

Do you want to upload a patch for that that the author of this patch can review or should the author create the patch?

But still you should be sure that it won't be called on a static global variable constructor stack.

I checked all static globals in source/, no others have this problem unless I missed some.

Well then this doesn't indicate an issue with path.h (there might still be one despite the lack of indication though :P)

Yep.

I would propose to put it into the namespace VisualReplay::GetTempCacheFileName().
Do you want to upload a patch for that that the author of this patch can review or should the author create the patch?

I don't mind, but I'm a bit busy with other patches. So if it's needed ASAP then it should be done by someone else.

I checked all static globals in source/, no others have this problem unless I missed some.

Thank you!

@Imarok wanna fix this, have it tested by @weberkai and approved by vladislav and me?

@Imarok wanna fix this, have it tested by @weberkai and approved by vladislav and me?

Sure, I can. Though tbh until now I don't fully understand the cause of the issue. I'll reread the whole thread...

@elexis should GetDirectoryName really called that name? Isn't GetDirectoryPath more appropriate?

@elexis should GetDirectoryName really called that name? Isn't GetDirectoryPath more appropriate?

Yes, name relates to the topmost folder in the chain, so name is inaccurate.

(Was wondering whether it makes sense to pass the filename GetAboslutePath(relativePath), but that would have the disadvantage that one can't get the root directory without passing an empty string.)

Hi!

Good news!

The Differential D1852 fix the issue!

Awesome work guys! Congratulations!

Thank you very much!

weberkai accepted this commit.Apr 29 2019, 6:01 AM
All concerns with this commit have now been addressed.Apr 29 2019, 6:01 AM

Hi!
Good news!
The Differential D1852 fix the issue!
Awesome work guys! Congratulations!
Thank you very much!

Thank your for your effort in looking into the issue and notifying us 👍