Use thread_local instead of static in EASY_LOCAL_STATIC_PTR.

[madevgeny]

Hi, please look at my changes, without them it's impossible to profile functions called from multiple threads.

[cas4ey]

Hi
Which compiler do you use?
The branch you have changed should fire only for compilers with support of c++11 thread-safe statics.

[madevgeny]

Hi
Which compiler do you use?
The branch you have changed should fire only for compilers with support of c++11 thread-safe statics.

I use visual studio 2015.

I have the following case:
there is method

void ed::PoolTask::run()
{
EASY_BLOCK(id.c_str()); // it's unique for each object.
func();
EASY_END_BLOCK;
}

Which is called from many threads. So if you use static variable it'll be changed in the middle of profiling. It appears under relatively heavy load.

[madevgeny]

I think this issue isn't compiler specific.

[cas4ey]

C++11 standard guarantees that local static initialization is thread-safe. And this static in EASY_BLOCK doesn't change after it is initialized (and additionally it's initialization is performed with a thread-safe method).

I think your problem is related to the usage of id.c_str() - you must ensure that it will exist and will not change during EASY_BLOCK execution. Try to replace id.c_str() with a compie-time string and if it is OK, then check again that id does not change during block execution.

[cas4ey]

For example:

void ed::PoolTask::run()
{
    const std::string local_id = id; // try to use temporary buffer
    EASY_BLOCK(local_id.c_str());
    func();
    // EASY_END_BLOCK; // this is not necessary because EASY_BLOCK using RAII
}

[cas4ey]

Hi @madevgeny
Any news?

[madevgeny]

Hi, it appears that we use /Zc:threadSafeInit- flag which disables thread safe statics. We need it for our protection system and unfortunately we will not be able to change it in near future.

I attached simple example to reproduce crash. But with my patch it works without crash.

Another thought. EASY_BLOCK unrolls to something like this:

static const ::profiler::BaseBlockDescriptor* desc = ::profiler::registerDescription("unique arguments");
... other code

if we call the same function from multiple threads but with different "unique arguments" we will write to desc pointers to different objects. So there is no guarantee that after writing pointer to desc we will have read the same pointer from it. As static variable desc can be changed from other threads. tread_local solves this problem too.

easy_crash.zip

[madevgeny]

For example:

void ed::PoolTask::run()
{
    const std::string local_id = id; // try to use temporary buffer
    EASY_BLOCK(local_id.c_str());
    func();
    // EASY_END_BLOCK; // this is not necessary because EASY_BLOCK using RAII
}

No, id is member of PoolTask class.

[cas4ey]

Hi, it appears that we use /Zc:threadSafeInit- flag which disables thread safe statics. We need it for our protection system and unfortunately we will not be able to change it in near future.

Well, this explains the problem :-)

If this flag could be checked at compile-time in source code then the fix will be:
(I don't know is there _MSC_ZC_THREAD_SAFE_INIT or something like this or not)

#ifndef EASY_LOCAL_STATIC_PTR
# if defined(_MSC_VER) && defined(_MSC_ZC_THREAD_SAFE_INIT) // Something like this
#  define EASY_LOCAL_STATIC_PTR(VarType, VarName, VarInitializer) thread_local static VarType ...
# else // This is existing branch:
#  define EASY_LOCAL_STATIC_PTR(VarType, VarName, VarInitializer) static VarType ...
#  define EASY_MAGIC_STATIC_AVAILABLE
#endif

If it's impossible to check the flag in source code then you will have to add new CMake option into easy_profiler_core/CMakeLists.txt and check it in the same way (the code would be exactly the same like in example above).

[cas4ey]

It would be great if you could fix it :-)

[madevgeny]

There is no macros to detect /Zc:threadSafeInit- at compile time.

I'll add option to easy_profiler_core/CMakeLists.txt.

[madevgeny]

Hi, I added EASY_OPTION_THREAD_SAFE_INIT option and generation of config header.

[cas4ey]

EASY_MAGIC_STATIC_AVAILABLE should not be defined if thread-safe static init is disabled

[cas4ey]

Could you, please, use existing easy_define_target_option cmake command instead of generating new file?

[dmikushin]

Is it true that this PR addresses a critical multuthreading usability issue?