Crash on OSX with r7787 build: deadlock in parser lib

[Mitten.O]

I tried the fix that worked last time this occurred and replaced the function with static variables in value_ref_parser in IntValueRefParser.cpp and DoubleValueRefParser.cpp (get_int_parser_rules and get_double_parser_rules, respectively) with unprotected global variables. This resolved the static initialization lock, but caused a segfault when actually attempting the parse. (When parsing techs, to be precise).

[Geoff the Medio]

I tried the fix that worked last time this occurred...

Link?

[Mitten.O]

viewtopic.php?f=9&t=8886

[Vezzra]

Got around to test things on OSX: r7790 works fine. Uncommenting lines 281-286 in ValueRefParserImpl.h leads to exactly the same crash I've had with r7786+. I can build trunk with boost up to version 1.55 (boost 1.56 requires the changes in the SDL/SDK branch, which aren't in trunk), but that too didn't change anything. Just for the records, I did a full rebuild each time.

[Geoff the Medio]

Attached should be an alternative parser implementation for numeric statistics that attempts to do the same as the problematic changes mentioned in this thread. Testing reports (ie. does it crash / deadlock) from non Windows users would be appreciated...

[Mitten.O]

Deadlocks on ubuntu 14.10.

Code: Select all

#0  syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
#1  0x00007ffff32d7c93 in __cxa_guard_acquire ()
   from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#2  0x00007ffff68d1b15 in (anonymous namespace)::get_int_parser_rules ()
    at .../Source/freeorion/private/FreeOrion/parse/IntValueRefParser.cpp:134
#3  0x00007ffff68d5e29 in value_ref_parser<int> ()
    at .../Source/freeorion/private/FreeOrion/parse/IntValueRefParser.cpp:154
#4  initialize_numeric_statistic_parser<int>(statistic_rule<int>::type&, statistic_rule<int>::type&, statistic_rule<int>::type&, statistic_rule<int>::type&, boost::spirit::qi::rule<boost::spirit::lex::lexertl::iterator<boost::spirit::lex::lexertl::functor<boost::spirit::lex::lexertl::position_token<__gnu_cxx::__normal_iterator<char const*, std::string>, boost::mpl::vector<bool, int, double, char const*, std::string, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mpl_::bool_<true>, unsigned long>, boost::spirit::lex::lexertl::detail::data, __gnu_cxx::__normal_iterator<char const*, std::string>, mpl_::bool_<true>, mpl_::bool_<true> > >, char const* (), boost::spirit::qi::in_state_skipper<boost::spirit::lex::detail::lexer_def_<boost::spirit::lex::lexer<boost::spirit::lex::lexertl::actor_lexer<boost::spirit::lex::lexertl::position_token<__gnu_cxx::__normal_iterator<char const*, std::string>, boost::mpl::vector<bool, int, doubl---Type <return> to continue, or q <return> to quit---
e, char const*, std::string, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mpl_::bool_<true>, unsigned long>, __gnu_cxx::__normal_iterator<char const*, std::string>, boost::spirit::lex::lexertl::functor<boost::spirit::lex::lexertl::position_token<__gnu_cxx::__normal_iterator<char const*, std::string>, boost::mpl::vector<bool, int, double, char const*, std::string, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mpl_::bool_<true>, unsigned long>, boost::spirit::lex::lexertl::detail::data, __gnu_cxx::__normal_iterator<char const*, std::string>, mpl_::bool_<true>, mpl_::bool_<true> > > > >, char const*>, boost::spirit::unused_type, boost::spirit::unused_type> const&) (statistic=..., statistic_1=..., statistic_2=..., statistic_3=..., 
    variable_name=...)
    at .../Source/freeorion/private/FreeOrion/parse/ValueRefParserImpl.h:285
#5  0x00007ffff68d10cc in (anonymous namespace)::int_parser_rules::int_parser_rules (
    this=0x7ffff6c9b640 <(anonymous namespace)::get_int_parser_rules()::retval>) at .../Source/freeorion/private/FreeOrion/parse/IntValueRefParser.cpp:66
#6  0x00007ffff68d1b34 in (anonymous namespace)::get_int_parser_rules ()
    at .../Source/freeorion/private/FreeOrion/parse/IntValueRefParser.c

cxa_guard_acquire is a lock meant to stop threading issues in initialization of statics, but which makes static initialization recursion not work. Which I think makes a lot of sense, if you need an object to create that same object, there is a logic error.

[Geoff the Medio]

Does the attached patch build and run for you?

[Mitten.O]

Builds, deadlocks.

[Geoff the Medio]

Builds, deadlocks.

??? That patch has the parser additions that were causing deadlocks commented out.

I'm going to need you to figure out what in that patch is actually causing the deadlock problem then. I didn't add any dependency between types. If anything, I removed them, by replacing direct use of a condition parser object with calls to a function that returns a reference, and changing the include to a shorter alternative header.

[Dilvish]

Builds, deadlocks.

??? That patch has the parser additions that were causing deadlocks commented out.

That patch hangs for me as well. There's quite a bit in that patch, a large number of deletions seeming unrelated to the statistics; deletions I was having hard time making sense of. Besides pointing that out, I'm afraid I won't be of much help tracking down the errors-- seems it's an llvm tool that Mitten is using for that & I haven't gotten up to speed on using llvm (keep seeing more reasons to, though).

[Vezzra]

Same result on OSX with these last two patches as Mitten and Dilvish had on Linux. Just tried it, did a full rebuild each time, building succeeds, but FO crashes with the same deadlock error as before.

[Geoff the Medio]

There's quite a bit in that patch, a large number of deletions seeming unrelated to the statistics; deletions I was having hard time making sense of.

There were also a bunch of changes to replace direct uses of the condition parser with calls to a function that returns a reference to it. The hope was that this would affect the order of static initializations or somehow make them non-interdependent. It also seems cleaner to not have the parsers including the implementation headers of other parsers, but to instead just have the minimal include for the definition and function to access the condition parser. Here's a patch that does just that: switching the way the condition parser is accessed from elsewhere...

Edit: and here's a patch that just tweaks the statistic parsers, almost as I did before, but also additionally simplifying the rule a bit as I realized there was no need to have the condition within the statistic be stored temporarily as it's always the last thing parsed so can be passed directly to the constructor.

[Dilvish]

There were also a bunch of changes to replace direct uses of the condition parser with calls to a function that returns a reference to it. The hope was that this would affect the order of static initializations or somehow make them non-interdependent.... Here's a patch that does just that: switching the way the condition parser is accessed from elsewhere...

This hangs for me as well. To try preserving some of the separation of into the detail namespace, in case that was significant, I tried (starting from after application of your parse_condition_call.patch) renaming condition_parser() to get_condition_parser() and moved it into the detail namespace, and then adjusted all the references to it. Unfortunately, that still hung for me.

Edit: and here's a patch that just tweaks the statistic parsers, almost as I did before, but also additionally simplifying the rule a bit as I realized there was no need to have the condition within the statistic be stored temporarily as it's always the last thing parsed so can be passed directly to the constructor.

I also tried this patch (by istelf, applied to r7856) but again, also , a hang.

[Geoff the Medio]

Edit: and here's a patch that just tweaks the statistic parsers, almost as I did before, but also additionally simplifying the rule a bit as I realized there was no need to have the condition within the statistic be stored temporarily as it's always the last thing parsed so can be passed directly to the constructor.

I also tried this patch (by istelf, applied to r7856) but again, also , a hang.

Looking at the patch, I think I missed reverting a few parse::condition_parser() to parse::detail::condition_parser reverts in the statistic parser definitions. Try replacing those...