std::logic_error - Initialization Order Fiasco

[sehe]

(reported by Exagon http://stackoverflow.com/a/41785268/85371)

Due to Static Initialization Order Fiasco sometimes X3 parsing aborts with a std::logic_error. The cause is unitialized rule::name fields.

It is clear that the design of Spirit X3 has accounted for static initialization order in that the parse_rule specializations contain function-local instances that defeat the issue.

However, the rule name initialization lives inside the namespace-scope static variable and its initialization is not guarded. This is probably an oversight.

Unfortunately I don't think there is a clean/elegant solution that will not introduce more function-local statics (with their syntax and runtime overhead).

This PR adds diagnostics to detect when a rule was copied before its initialization. The commit comments and code detail the approach.

[djowel]

Hmmm... Tricky. Let me analyze this some more.

[djowel]

OK, there should be a better way to solve this, but for now, I think this is a good intermediate step.

[wanghan02]

I'm wondering if we should add this assert_initialized_rule in the second commit 16320a5. If the coding style is bad and with 50% chance it will trigger the Static Initialization Order Fiasco (SIOF) in debug build. But even if it does not trigger SIOF in debug build (also with 50% chance), it could still trigger SIOF in release build and we know nothing about that.

Most x3 parsers could have constexpr constructor (I'm trying to do this now). But this assert_initialized_rule cannot be constexpr and it makes all derived classes of unary_parser/binary_parser cannot have constexpr constructor.

Can we reconsider the solution here?

[Kojoley]

@wanghan02 I am sorry I do not understand your issue. Hopefully, rule placeholders SIOF will no cause a real problem unless you are enabling BOOST_SPIRIT_DEBUG.

Most x3 parsers could have constexpr constructor (I'm trying to do this now). But this assert_initialized_rule cannot be constexpr and it makes all derived classes of unary_parser/binary_parser cannot have constexpr constructor.

AFAIK you can use BOOST_ASSERT in constexpr function, at least https://godbolt.org/z/sVV_ku compiles fine on every major compiler, and if you are talking about assert firing at compile time after making constructor constexpr -- it is an issue in the user code.

[wanghan02]

Thanks for the information. I still have some questions.

Hopefully, rule placeholders SIOF will no cause a real problem unless you are enabling BOOST_SPIRIT_DEBUG.

Then why don't we wrap it with BOOST_SPIRIT_DEBUG instead of NDEBUG?

2. From my understanding, this assert over SIOF will only fire at run time. It cannot be a static assert at compile time in a constexpr funtion. Is it correct?

3. I found constructing a big parser takes time. For example,
   x3::parse(str.begin(), str.end(), some_big_parser_construct_in_place)
   in this parse function, the construction of the parser can take about 40% of total run time in release build and 90% in debug build (on windows). So if most of the parsers could have a constexpr constructor, most of the parsers we write could be a constexpr variable and save us valuable running time. Therefore if we could at least move that assert_initialized_rule(cannot be in constexpr function) into some better macro instead of NDEBUG, we could make the constructors conditional constexpr (controlled by a macro).

[djowel]

I am not sure if I have time to look into this now, but as I said above, I'm hoping to have a better solution. One thing I really want to do is remove the need for the rule::name use of a std::string. Most of the time, you use literals with no lifetime issues. It will be a breaking change, but the risk is low, and the payoff is great. Rules should be able to be constexpr too.

[djowel]

I am not sure if I have time to look into this now, but as I said above, I'm hoping to have a better solution. One thing I really want to do is remove the need for the rule::name use of a std::string. Most of the time, you use literals with no lifetime issues. It will be a breaking change, but the risk is low, and the payoff is great. Rules should be able to be constexpr too.

OK, I'll have another look today.

[Kojoley]

1. Then why don't we wrap it with BOOST_SPIRIT_DEBUG instead of NDEBUG?

Because get_info/what could be used directly,

2. From my understanding, this assert over SIOF will only fire at run time. It cannot be a static assert at compile time in a constexpr funtion. Is it correct?

Short answer is: the check is needed to cover cases that cannot be resolved at compile time.

3. I found constructing a big parser takes time. For example, x3::parse(str.begin(), str.end(), some_big_parser_construct_in_place)

Why you are initializing a 'big parser' in a function? If it has to be constructed dynamically (because of something is known at run-time) constexpr constructors will not solve your problem.

One thing I really want to do is remove the need for the rule::name use of a std::string. Most of the time, you use literals with no lifetime issues. It will be a breaking change, but the risk is low, and the payoff is great. Rules should be able to be constexpr too.

I would vote for it back then, but since C++20 will have constexpr std::string I am not so keen now.

[wanghan02]

I agree that constexpr cannot solve all the problems. But in my opinion, it does bring many benefits. If most of the parser/rules can be constexpr, then why not?

What I propose is, we use another macro other than NDEBUG for this run time check. Then it's the user's decision to choose between this run time check and constexpr.

[Kojoley]

The check, as is, should not interfere with your constexpr things. Please open an issue with a code sample that show the actual problem.

[wanghan02]

The check, as is, should not interfere with your constexpr things. Please open an issue with a code sample that show the actual problem.

@Kojoley Have you read the code? The check calls what(p) under #ifndef NDEBUG trying to build a std::string which cannot be in a constexpr function.

        template <typename Parser>
        static void assert_initialized_rule(Parser const& p) {
            boost::ignore_unused(p);

            // Assert that we are not copying an unitialized static rule. If
            // the static is in another TU, it may be initialized after we copy
            // it. If so, its name member will be nullptr.
            //
            // Rather than hardcoding behaviour for rule-type subject parsers,
            // we simply allow get_info<> to do the check in debug builds.
#ifndef NDEBUG
            what(p); // note: allows get_info<> to diagnose the issue
#endif
        }

[djowel]

One thing I really want to do is remove the need for the rule::name use of a std::string. Most of the time, you use literals with no lifetime issues. It will be a breaking change, but the risk is low, and the payoff is great. Rules should be able to be constexpr too.

I would vote for it back then, but since C++20 will have constexpr std::string I am not so keen now.

Sure, but we'll still have to deal with c++17 for a long time.