Fix and test singleton implementation

[Flamefire]

As described in #104 the singleton implementation was broken in b0a794d which led to crashes especially in the context of (multiple) shared libraries.

The "fix" allocating the instance on the heap basically just never freed the instances and hence leaked memory. This was due to not understanding the root cause of the problem.

To summarize:
There are 2 use cases for the singleton: On where the type T is derived from singleton<T> and one where singleton<T> is used without deriving T from it. The above commit put the is_destroyed handling into the ctor/dtor of singleton<T>. However if T is not derived from it, then singleton<T> is never instantiated and hence is_destroyed is never set. See also some discussion in #104 and #79.

The crash/memory corruption now happens (or happened before the leaky implementation) if the tkmap used from extended_type_info_typeid is destroyed first. As it used from the ctor of extended_type_info_typeid this should normally not happen. However it does happen when used from multiple shared libraries under (at least) GCC 5.4 and 7.1. The reason is currently unknown (see https://stackoverflow.com/questions/50064617/order-of-destruction-for-static-function-members-in-shared-libraries) but we can just assume, the order of destruction is undefined. Hence the need for the is_destroyed flag.

On this PR:

• It is built on top of Various Appveyor & Travis improvements #94 to enable more testing
• It also tests in release mode to ensure that e.g. stripping of symbols by the linker does not cause problems
• Add a test case to ensure, that singletons are properly constructed, destroyed and the is_destroyed flag is correctly set
• Add a test case to test the same for shared libraries
• Add a test case based on https://gist.github.com/Flamefire/286e9e0e501731a04f10786450d3e711 to test the issue with multiple shared libraries. Before this PR this caused a double-free which is detected by the debug-glibc. It would be even better to run this in valgrind, but I'm not familiar enough with the Jam-Files and windows doesn't have it.
• Fix the failing testcases by partially reverting b0a794d :
• Reintroduce detail:singleton_wrapper with the correct is_destroyed setting in the dtor. It uses a method-local variable to enforce initialization on first use (preferred way to avoid static init disaster https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2)
• Replace heap-allocated variable by static instance (see isocpp FAQ, avoids static init disaster and is thread safe)
• Assert !is_destroyed on enter of get_instance. Note that the comment was wrong. It did not refer to the instance and is even removed in non-debug builds
• Make the ctor of singleton protected and default to avoid accidental misuse (it might never be called if T is not derived from singleton<T>)
• Excessively comment everything to avoid breaking changes by misunderstandings

Other notes:

• The comment on the heap allocated variable was wrong: as singleton_wrapper is the most derived type of the instance it will always be destructed before T and singleton<T> (if T is derived from it) There is nothing one can do about it!
• Valgrind tests would be nice
• static instance is superior to static pointer (see isocpp FAQ), it is not about visibility, double-destruction or anything else. It simply makes sure that the instance is actually destructed at program exit
• Again: It is safe to assume the destruction order is undefined. Hence all asserts assuming otherwise have been removed (they had a runtime check afterwards anyway which is the correct way)
• Not deriving from singleton<T> but using it directly is potentially dangerous as you can have multiple T instances and you have only 1 singleton<T> where you might not expect it. Example: extended_type_info uses a singleton<detail::tkmap> to work around this issue because extended_type_info_typeid already uses a singleton<tkmap>. It might be wise to enforce deriving from singleton<T> and using T::get_instance() or at least do that in the library.

Finally: Please try to break this! I'm pretty sure I covered everything by using TDD but I might have missed something.

[Flamefire]

@robertramey Note how this also fixes the current test failure in test_dll_export. The failure is a segmentation fault as described here and in #104

[tobias-loew]

I'm absolutely in favor for this PR. It essentially removes the changes from #79. Since the destruction sequence is well defined in the standard [basic.start.term] (and nobody else before had this problem), it seems to be a non-standard-conformance of gcc.

[Flamefire]

@tobias-loew The description in #79 is simply wrong. The singleton_wrapper will always be cleaned up first as it is the most derived type. The inheritance is: singleton_wrapper->Foo(->singleton<Foo>) (last is optional, see my comments in the src here)

On standard destruction order: Normally it is all good. Just in case of multiple shared libraries the problem occurs. See https://stackoverflow.com/questions/50064617/order-of-destruction-for-static-function-members-in-shared-libraries There I show how the same types are destructed together although the defined destruction order should not allow this. Although this should be blamed on GCC (or whatever) one cannot ignore it. Hence the need for is_destroyed and tests for it.

[tobias-loew]

@Flamefire I agree that #79 is screwed up. But I'd really like to see a gcc-workaround-macro here and a comment explaining what's going on.
The standard explicitly specifies that the dtor are sequenced in opposite order to the completion of the ctors:

"If the completion of the constructor or dynamic initialization of an object with static storage duration
strongly happens before that of another, the completion of the destructor of the second is sequenced before the initiation of the destructor of the first."

And the stackoverflow-example shows that gcc seems to have trouble obeying that.
This is a bug in gcc and not in serialization, so let's not put a burden on standard conforming compilers.

[Flamefire]

The is_destroyed is actually the work-around and it works well. However it is also a good general method to avoid problems at all: If you have a class, that is used as a singleton and that accesses other singletons in its dtor, first check the is_destroyed of the accessed singleton and if set, don't use it.

I agree that it is a bug in GCC, but we cannot ignore it. I won't call the is_destroyed method a burden and IMO it is well-suited to avoid problems (maybe other compilers or specific versions suffer from the same problem? How to be sure it is only a GCC problem?)

From my tests GCC 6.4 and 7.1 are affected. Triggering the bug with a simple test case seems to be quite hard. It requires static boost, but dynamic libraries. But besides that I'm not quite sure.

[Flamefire]

I found that Clang is also affected. It seems to be a general problem or dependent on e.g. GLIBC (using 2.12 and 2.23 here)

For the test_multi_shared_libs I'd actually want to reproduce what I reported on StackOverflow For that I need to link the shared libraries against the currently build boost or at least against static Boost.Serialization. One can add /<link>static to https://github.com/boostorg/serialization/pull/105/files#diff-e97299ad55a4013dc19889043aa464a5R58 (and same for the 2nd library) But this requires testing with cxxflags=-fPIC. Not sure if this is acceptable.
With that change and without the patch the testcase crashes everytime reproducing the behavior I mentioned and maybe at least similar to the current segmentation fault of test_dll_exported although I wasn't able to confirm the behavior for shared Boost.Serialization so test_dll_exported might be something else. However as this PR also fixes test_dll_exported I'm quite sure it is pretty much the same.

@robertramey Could you tell me what you need to get some progress here? Or is there another maintainer who is able to verify this PR?

[MikeGitb]

Just to make sure I understand this correctly: The leaky version only leaks when a shared library is unloaded - correct? Not saying this is ideal, but is this really something you have to worry about?

[Flamefire]

This is correct. The heap-allocated instance is never freed and hence leaks on terminate/unload. It is a problem once you have a long running app that loads/unloads the library multiple times. There are people doing that and for serialization related stuff it makes sense. Some user complaint here: #104 (comment)

[davydden]

This PR also fixes the segmentation fault with 1.68.0 (which was not the case with 1.67.0) we have in a simple test in dealii/dealii#7074 . @masterleinad can probably point to the configure test which I can't find at the moment.

[masterleinad]

@masterleinad can probably point to the configure test which I can't find at the moment.

It's here.

[Flamefire]

Thanks for the additional input and the confirmation, that this PR works for you too.
Note: See #111 which is a test for the crash. Also note that 1.65 introduced the crash, changed to a memory leak in 1.66 to 1.67

[masterleinad]

Yes, 1.67 worked. I wonder what changed since then such that it is crashing again.

[Flamefire]

The commit from 1.66 was reverted, changing the memory leak back to a crash: 8ca532a

[davydden]

it looks like this PR works for everyone, perhaps @robertramey can review and merge it?

[tobias-loew]

Sorry to crash the party, but I'm really surprised how this PR can fix the crash with gcc-builds.
Essentially, this PR changes the singletons ctor from public to protected there is no other semantic change in singleton.hpp. (Moving the singleton_wrapper and is_destoyed-flag to another class doesn't change the semantics at all as "detail" namespaces don't belong to the public interface.)
There is just one tiny syntatic change for release builds: is_destroyed won't get initialized eagerly during construction of the singleton but only when explicitly accessed or during destruction of the singleton.
And finally a comment to the reverted changes from 1.66: most programmers are concerned about memory leaks (e.g. with tons of "unimportant" memory leaks from singletons in the output you may miss the important one, that lets your program crash after a month)

[davydden]

but I'm really surprised how this PR can fix the crash with gcc-builds.

it's also Intel 18.03 (paired with gcc 4.8.5) on Linux HPC.

[Flamefire]

Moving the singleton_wrapper and is_destoyed-flag to another class doesn't change the semantics at all as "detail" namespaces don't belong to the public interface

Wrong: See #110 which shows how is_destroyed is broken

is_destroyed won't get initialized eagerly during construction of the singleton but only when explicitly accessed or during destruction of the singleton.

You forgot: On first access (e.g. https://github.com/boostorg/serialization/pull/105/files#diff-c23e78320ecd29888bb77891a5943f41L54)

most programmers are concerned about memory leaks (e.g. with tons of "unimportant" memory leaks from singletons in the output you may miss the important one, that lets your program crash after a month)

I'm more concerned about a crash than a memory leak.

[Flamefire]

Rebased on current develop, #110 and #111, added the remaining test and applied the fixes last.

Please note the failure of aac07cf and how this is fixed

[tobias-loew]

Moving the singleton_wrapper and is_destoyed-flag to another class doesn't change the semantics at all as "detail" namespaces don't belong to the public interface

Wrong: See #110 which shows how is_destroyed is broken

I checked #110 with Boost 1.63 and 1.67 with VS 2015 and it runs fine with BOTH versions.

is_destroyed won't get initialized eagerly during construction of the singleton but only when explicitly accessed or during destruction of the singleton.

You forgot: On first access (e.g. https://github.com/boostorg/serialization/pull/105/files#diff-c23e78320ecd29888bb77891a5943f41L54)

this seems to be a case of "explicitly accessed"

most programmers are concerned about memory leaks (e.g. with tons of "unimportant" memory leaks from singletons in the output you may miss the important one, that lets your program crash after a month)

I'm more concerned about a crash than a memory leak.

This may be true for the single case where it's either memory-leak or crash. But here we have crashes of some against memory-leaks of the whole Boost-community, and that's a different story. You shouldn't make your code run on the expense of others.
This is still a non-conforming-compiler problem and should be solved there. Boost allows workarounds for broken compilers, but they should be guarded by respective macros.

[Flamefire]

I checked #110 with Boost 1.63 and 1.67 with VS 2015 and it runs fine with BOTH versions.

1.63 is expected to work, 1.65+ is not. If 1.67 worked for you, something is wrong on your side: See appveyor. Did you actually run all the tests? Reconfigured? Started from a clean checkout?

this seems to be a case of "explicitly accessed"

Yes, missed this in your text. So what was your complaint about this?

But here we have crashes of some against memory-leaks of the whole Boost-community, and that's a different story. You shouldn't make your code run on the expense of others.

Dito. You have a memory leak on shutdown or lib unload, we have crashes. So at whose expense should it be?
BTW: The bugfix is here, the reasons are explained.

This is still a non-conforming-compiler problem and should be solved there. Boost allows workarounds for broken compilers, but they should be guarded by respective macros.

Nope, its the runtime for ALL (AFAIK) linux runtimes. And the crash is a use-after-free/destroy which can easily be caught by fixing is_destroyed which I did here.

[Flamefire]

I seriously doubt that is true. But no matter. Your saying that main will return 0 regardless of the values of f() and g(). So how would this test fail?

You may check the C++ standard: https://stackoverflow.com/a/19293663/1930508. However I added new commits to #111 to make this explicit and not confuse people not knowing the speciality of main.

This test does not simply fail. As explained in that PR it crashes which is what I'm talking about: *** Error in bin.v2/libs/serialization/test/test_multi_shared_lib.test/gcc-7/debug/link-static/test_multi_shared_lib': double free or corruption (fasttop): 0x000000000088fc20 ***`

Thanks for your explanation on BOOST_CLASS_EXPORT_KEY (seems you confused the names earlier?) but that is irrelevant for #111. No classes are exportet, there are simply 2 functions implemented in 2 shared libraries which are called from the main executable. All classes (2 singleton) are from boost.Serialization which are instantiated directly to show the bug (same would occur if you serialize/create serialization code for those 2 types (float, double))

I should say I believe that my version of your patch is equivalent to your original one - without which I couldn't have created it. That is, I believe that your patch included some stuff that wasn't strictly necessary and I believe that testing will support that believe.

This is wrong. Your patch doesn't include the fix for is_destroyed, keeps inconsistent and plain wrong comments and additionally removes the feature that singleton classes with protected constructors are allowed (now they would fail to compile)
Testing: You didn't review the tests I made and did not merge them. Those tests show that those bugs are still there, see #110 and #111 (CI is still running after I rebased them)

I will rebase this PR on current dev but will overwrite your changes. What you did is dangerous again: Your get_singleton_module function may be eliminated by optimization in shared libs. This could have been pointed out by review...

This will be my last effort on this unless

• You properly review the test PRs More tests on CI #127 (factored out from Add singleton tests (Failure is expected) #110), Add singleton tests (Failure is expected) #110 and Add test showing the crash in the context of shared libraries #111 and merge them once done. Failure in develop is acceptable so further PRs (e.g. this one) can fix them
  • Those PRs are intentionally made as small as possible.
• You properly review this PR

What is happening here is not how Open Source projects should work. Thanks @davydden for summarizing it. Exactly my point!

[tobias-loew]

@davydden, @Flamefire

I'd also like to be frank:
The boost-libraries is not another github open source project: it has been there long before github or even git was created.
Boost-libraries stand for highest-quality, peer-reviewed code. They are used world-wide in many different projects, ranging from little toy-projects up to crucial production code in companies and public institutions, where a lot of money or even lives depend on its correctness.
The boost-community decided that once a library is part of boost, the library authors (resp. owners) are responsible for its maintenance: they are the instance you have to convince that your PR should be merged into the code. So, they have a lot of power, but on the other side, it's their heads that got chopped off if something gets broken. So in a way, they're the last line of defense between order and PR-chaos.
Coming now to this specific problem: I learned that there are certain problems with the destruction order on linux-systems with dynamically loaded modules. You tried to fix it and came up with a solution that worked for you and the maintainer merged the changes into the public release. Later on it showed up that the fix broke other people's code (just look into the boost-dev archive), and that definitely shouldn't happen.
The current state of the PR looks ok for me, but I still got two issues with it:

• Even though construction/destruction of static variables is all what this PR is about, the debug and release versions differ here a lot. So the debug version is only of limited value for analyses.
• Since all the changes result from faults only present on linux-system (IMHO from a C++ perspective the original pre 1.63 code is absolutely correct), I really would like to see a comment on that or the usage of target-specific macros. Otherwise, I expect, some years from now, other coders will "clean up" code and reintroduce the linux problem.

The only functional difference I can see, is that the PR-code forces is_destroyed to true when the singleton_wrapper gets destructed. If this is the crucial change then why isn't there any comment in the code? Please, additionally, try to explain the code with code-comments.

An a final please: let's not draw red lines - we're all better than that.

[Flamefire]

@tobias-loew Thank you. Your points are valuable as they ask real questions about the code at hand.

So the debug version is only of limited value for analyses.

For this there is #127 which tests release versions on CI. Is this what you meant?

I really would like to see a comment on that or the usage of target-specific macros.

There are none due to the following reason: I acknowledge that there ARE situations where a singleton is destroyed although it shouldn't be. The reasons for this are unclear (although they happen only on linux, and they may be solved, or not... they are hard at least). So I added tests for is_destroyed and destructors accessing singletons check this before accessing the singleton. This solves the problem in all cases without resorting to target-dependent macros (What if another platform behaves like Linux? Is this solution incorrect for other platforms? I believe not)

original pre 1.63 code is absolutely correct

Did you mean pre 1.65? If yes, I mostly agree. There the is_destroyed function was working and it was checked (it still is) as it should. Only "Bug" is the assert above the runtime check which might make people want to remove this.

The only functional difference I can see, is that the PR-code forces is_destroyed to true when the singleton_wrapper gets destructed. If this is the crucial change then why isn't there any comment in the code?

Is there really a need to explain by comments what is_destroyed is supposed to do? See https://github.com/boostorg/serialization/pull/105/files#diff-3f1e861be15fd4926f5e5b00388f2309R134
What would you explain there? (Real question, IMO its self-explanatory but your question suggests this to be not true)
Edit: Added more comments: 89aec9a

Please, additionally, try to explain the code with code-comments.

I Did. See the most important parts here:

• https://github.com/boostorg/serialization/pull/105/files#diff-3f1e861be15fd4926f5e5b00388f2309R84
• https://github.com/boostorg/serialization/pull/105/files#diff-3f1e861be15fd4926f5e5b00388f2309R120
• https://github.com/boostorg/serialization/pull/105/files#diff-3f1e861be15fd4926f5e5b00388f2309R153
• https://github.com/boostorg/serialization/pull/105/files#diff-3f1e861be15fd4926f5e5b00388f2309R169

What needs more explanation? Could you comment this on patch lines?

Otherwise, I expect, some years from now, other coders will "clean up" code and reintroduce the linux problem.

This is why everything (here) is accompanied by tests showing RED before and GREEN after this PR.

Finally:

Boost-libraries stand for highest-quality, peer-reviewed code.

If the maintainer pushes own changes without even giving anyone the chance to review/comment it, the value of this is reduced. Yes it ultimately comes down to the maintainer what gets in, but if it is only the maintainer and his opinion that matters, then it is no longer "peer-reviewed".

[vinniefalco]

"The dream" for a library author is for enthusiastic community contributors to step forward and first assist, then take over maintenance for the library in a fashion that is as consistent, good, or better than the level of quality sustained by the original author. This can never happen without cultivating their interest through engagement, mentoring, and being generous with credit.

[robertramey]

@robertramey to be frank, for my experience with open source projects on github I expected that the following would happen for this pull request (PR)

I don't doubt your good intentions here. But I think your expectations are unrealistic. you seem to picture the "open source development" process as some sort of cooperative venture among equals. It is not and can never be. The main issue is who takes responsibility for delivering a quality product. Responsibility cannot be assigned or delegated to an amorphous group. When something goes wrong - someone has to be tasked with getting it fixed. If this "someone" is a group of people, there is no guarantee that this "assignment/delegation" can be accomplished. This not a comment on open source development, GitHub or boost, It's a long held and agreed upon feature of effective human collaboration. This is why we have presidents, ceos, team captains and ... software product managers.

In the context of Github/Boost, this function is fulfilled by the library author or maintainer. His job is to act as a "gate keeper" to accept only the best improvements. Along with this responsibility he as the authority to decide what to accept, delegate tasks to other people, reverse those decisions when necessary. When something goes wrong he, and only he, takes the blame. Responsibility cannot be delegated. This is unlike authority (to fix or test or whatever) which can be delegated. If I accept a less than optimal fix, it's my problem. I couldn't just point the finger at contributor X and assign blame to him. It also means that inevitable trade offs are made by a single person, in this case me. Of course every time such a tradeoff is made, someone will disagree and that's unfortunate. But that is the nature of the word "tradeoff". But by keeping this authority in the hands of one person, it's much more likely that the work (in this case the serialization library) will maintain a logical coherence and singularity of purpose which has been essential for keeping it relevant and useful for these many years.

Note that in the course of discussion of this issue and in the past, I've suggested that anyone with interest could peel off the singleton and make it a new boost library and submit it to boost. This makes a lot of sense as the issues related to a singleton are a lot more subtle than meets the eye and the chances of a casual user getting it right are very small. This is especially true in presence of issues related to order of static initialization, DLLS, visibility No one has shown any interest in undertaking such a task and accepting responsibility for it. about 15 years ago such a library was submitted to boost by a very highly regarded author of other boost libraries. The submission was reject for some reason that I can't remember. Faced with this, I made my own - and here we are.

The PR system is very helpful. But the way it's setup it just permits me to merge the PR into the branch before I can test it. And many times the PR has some useful parts, but has some other parts besides. I choose to handle this in the way I have. This is based solely on expediency - nothing more. Note that I make these changes in the develop branch so that I can watch the results on the Boost Test matrix - which is much more comprehensive than the CI. Also not that all changes to the develop branch are public and considered provisional. So the door is still open to those who might feel that I got something wrong.

I try to give credit to everyone who makes a useful contribution. I don't think I've been remiss in that in this instance. Some PRs are misguided, incomplete, go beyond appropriate scope, not well tested, cannot be verified, break something else, narrow the applicability of the library, or suffer from some other problem so they can't be merged without change. Only the most trivial can be merged without change. In this particular case, my motivation for the changes I did rather than just accepting the PR should be pretty obvious by just comparing the actual changes the proposed ones.

I think your comment greatly underestimates the time, effort and commitment I or any author of a boost library invests to keep improving this library. You don't see the work involved in reviewing PRs which suffer from the problems above. The approach you suggest would greatly increase that requirements of that commitment while diminishing the quality of the library (such as it is).

[davydden]

@robertramey

some sort of cooperative venture among equals

Perhaps my wording was not precise, but i never implied that. I also never implied that the responsibility is shared among an amorphous group. All what you say is perfectly fine and I don't disagree with that.

My main concern was the workflow of PRs, i.e. discussion on specific issues/requests @tobias-loew and @Flamefire have above. This does not imply that you and @Flamefire are equal in decision taking/responsibility/etc.

I think your comment greatly underestimates the time, effort and commitment I or any author of a boost library invests to keep improving this library.

I apologies if this sounded so, that was never my intention. I was commenting on the workflow only.
At the end of the day, it's your decision, nobody argues about this. I think you, as an author and maintainer of the library, can get more in return by following the other approach. But again, different strokes for different folks.

But let's not dive into this topic. I will refrain from making further comments on this PR, as those are not related to the suggested changes. I hope that you, @Flamefire and @tobias-loew will continue discussing technical aspects of this PR.

p.s. disclaimer: I never met @Flamefire on-line or off-line, so my comments here don't have any personal bias towards either sides (author or maintainer). The only reason I watch this PR is because this issue affects other projects.

[pdimov]

Why is this line here?

[Flamefire]

This is from #110 where I reverted changes made to a test I also changed. As explained in #110 the changed test currently in develop does rather test the compiler than the library which is why I reverted/overwrote it.

If this is objectionable I'll add my own version of test_singleton.cpp as test_singleton2.cpp or below into this file but that would mean redundant code.

[pdimov]

What I don't understand is what's Gennadiy Rozental's e-mail doing here, on a line of its own, it makes no sense to me.

Re test_singleton2.cpp, yes, I was going to suggest the same thing. I'd also drop the Travis changes, or just keep the essential changes to a minimum.

I also wonder whether the test will be cleaner if it's split into two, one testing a plain singleton, the other - inherited. There will be duplication, sure, but the array of states and the base class will vanish, so it could be more readable that way.

[Flamefire]

As mentioned I reverted the original commit: 8ca532a#diff-b2f7572da5c9081e523924931012ab08

Re test_singleton2.cpp, yes, I was going to suggest the same thing. I'd also drop the Travis changes, or just keep the essential changes to a minimum.

Some are required (-fPIC), I can drop the rest, as they are in #105

I also wonder whether the test will be cleaner if it's split into two, one testing a plain singleton, the other - inherited. There will be duplication, sure, but the array of states and the base class will vanish, so it could be more readable that way.

This would mean 2 separate compilation units. I wanted to have the 2 use cases (plain, inherited) next to each other, to show the similarity and difference. But I can split this in 2 files, you think, this is more readable.

[pdimov]

What I didn't like about this patch is that it's not clear which changes are necessary to make the test pass, and what aren't. The singleton_module changes, for instance, and a few other things.

But I now see that you're simply undoing e1893dd#diff-3f1e861be15fd4926f5e5b00388f2309.

[Flamefire]

Yes got messy due to the failed attempt to fix this which is why I added an explicit revert commit. My Patch is a single commit here: e0691b0 (see the name)

The changes there are much less and mostly comments. Also compare the pre-1.65 version which that patch is based on.

[Flamefire]

As requested I split the is_destroyed test into 2: #129

[pdimov]

use(m_instance) - as before this patch - is undefined behavior, but I think that this one is undefined too. Non-undefined would have been to use a pointer as m_instance. Of course getting this past Robert may prove problematic. :-)

[Flamefire]

Can you explain why it is UB?

I do think this line is not required because of https://github.com/boostorg/serialization/pull/105/files/53392794c49ba129c4f3eb70debb7426746b325b#diff-3f1e861be15fd4926f5e5b00388f2309R198 but I'm to afraid to remove it ;)

[pdimov]

In either case, I don't see how this is going to work - use will just get inlined here and disappear. But who knows.

[pdimov]

At the point the initializer for m_instance is called, m_instance is not yet initialized, and using a reference that does not refer to a valid object is undefined behavior.

A pointer would not have this issue because globals are zero-initialized first, before the dynamic initializers are run, so the value of m_instance when accessed in its own initializer would be nullptr.

Even better would probably be to use a pointer for m_instance and use(&m_instance), which avoids all these subtleties, as the address of m_instance is always valid no matter whether it's initialized or not.

[Flamefire]

Wait. My patch uses &m_instance, hence it takes the address of the object. This is never dereferenced. With your statement

as the address of m_instance is always valid no matter whether it's initialized or not.

This is ok, isn't it?

So at no point a reference is used, we only get the address of a static member.

[pdimov]

m_instance is an uninitialized reference. &m_instance is undefined in that case.

[Flamefire]

Actually I don't understand WHY it is required. Just judging from the comment this is to "construct the instance at pre-execution time". But the m_instance with its initialization is there for exactly the same reason (I added this as a comment with a link to https://groups.google.com/forum/#!topic/microsoft.public.vc.language/kDVNLnIsfZk)

But @robertramey wrote in an email:

LOL - it's only in there because the tests fail without it.

I verified this locally (Linux, GCC), but don't understand it...

I'm currently testing static T * m_instance; and it seems to work. But this would be a separate PR.

[pdimov]

My guess would be that when there are no references to m_instance, and it's in a static library, it's not being linked at all.

[Flamefire]

Closed in favor of the simpler #131