Dynamically initialized globals are problematic

[dmah42]

we have a few global non-PODs in tests. these should be removed.

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

[skaidon-tech]

I am not 100% sure this is the same issue as what is reported here but it sounds very similar. In my statically linked application static initialization of a test fixture (constructor) in one of the translation units was executed before libstl was ready, as a result a bunch of normal, well tested code (for example boost path normalization and std altorithms were crashing. For example here is one valgrind output:

==13359== Invalid read of size 4
==13359== at 0x4C8E9D0: std::locale::locale(std::locale const&) (atomicity.h:53)
==13359== by 0x479DAB: boost::filesystem::path::codecvt() (path.cpp:918)
==13359== by 0x47A257: boost::filesystem::path::begin() const (path.hpp:202)
==13359== by 0x47BFBC: boost::filesystem::path::m_normalize() (path.cpp:393)
...

I fixed the code by moving my fixture constructor code out and make fixture very plain and then use boolean latch to execute initialization code from the body of the test itself.

One way to fix this issue is to delay fixture instantiation until main is entered. At static initialization time static registrar can register a templated factory and fixture can be instantiated with this factory after main is entered.

[dmah42]

it's not exactly the same, but it is related. your solution sounds reasonable.

[EricWF]

we have a few global non-PODs in tests. these should be removed.

I believe your referencing the class-type globals in benchmark.cc like timer_manager but that's the least of our worries. There is a far worse static initialization order fiasco (SIOF) in our public API.

The real issue here is"dynamic initialization" as opposed to "static initialization", which is determined more by the initializer than the type it's initializing. Static initialization is ironically ordered and entirely safe, the opposite of dynamic initialization.

For example:

struct PODType {
  int value;
};
int foo();
PODType p1 = 42; // OK. Has a constant initializer.
PODType p2 = foo(); // Unsafe. The initializer is dynamic
int bar() {
  extern int i;
  static int local = i;
  return local;
}
int y = bar(); // Unsafe. 'local' is initialized but its initializer, 'i', may not have been..

Unfortunately the entire library API is built around dynamic initialization. The BENCHMARK registration macros work by executing arbitrary code in the initializers of trivially constructible global objects. Actually addressing this issue means deprecating the BENCHMARK interface in favor of a new runtime interface like the RegisterBenchmark function.

However I'm not convinced this is worth while in our case. The global registration facilities are super convenient and easy to use. Although such techniques would be ill-advised in large production codebases that's not what we provide.. SIOF issues between the user and the library can be avoided with a couple of cleverly placed initializers Unfortunately the API can still cause the SIOF outside of the library because things like fixture constructors execute arbitrary user code during program startup (as @rtzcoder mentioned).

In conclusion I think we should begin to implement and promote the new safe API which is not designed for use during program startup. The intent is not to replace the old registration macros but to provide an alternative interface without builtin SIOF bugs.