[SofaKernel] FIX segfault created by static initialisers on OSX/clang compiler

[jnbrunet]

To reproduce the bug, simply add a msg_error() at the first line of static std::string computeSofaPathPrefix() (sofa/helper/Utils.cpp) on a mac OSX system.

I believe this bug was due to an order mixup of global static initializer since this bug wasn't affecting Linux (not sure about Windows). I would need more time to prove this though. I moved the faulty global static intializer to local static initializer and that fixed the crashes.

It may be a good idea in the future to minimize the uses of static objects (singletons, static global variables, etc) since they are pretty hard to debug and can cause behaviors dependent of the compiler used.

@guparan could you test this in your PR #635 (by reinserting the msg_error) and make sure that the seg fault is gone?

Fixes #636

---

This PR:

• [x] builds with SUCCESS for all platforms on the CI.
• [x] does not generate new warnings.
• [x] does not generate new unit test failures.
• [x] does not generate new scene test failures.
• [x] does not break API compatibility.
• [x] is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

[guparan]

Thanks @jnbrunet ! Will change #635 accordingly :wink:

[damienmarchal]

Haha, this is funny... The initial implementation was using static in function like in this pr but this was causing crash & segfault with multihtreading on windows 2013 or 2015 i don't remember.

[jnbrunet]

Well, I guess this isn't thread safe.

One solution could be to simply remove these static variables and create one loopup table per object created (using a good old private class member). It will duplicate the lookup table for each class instance, but seriously, how many DefaultStyleMessageFormatter objects can be created in your typical simulation :-P

[guparan]

[ci-build][with-scene-tests]

[epernod]

@fspadoni maybe you could add your input regarding the thread safe question.

[fspadoni]

I just add a remark (perhaps trivial) about the thread safe question. From C++ 11 a static variable initialization is guaranteed to be thread safe only if it's instantiated inside a block scope (locally). The DefaultStyleMessageFormatter::getInstance() method @jnbrunet implemented is guaranteed to be thread safe. That's called Meyers Singleton. ( http://www.modernescpp.com/index.php/thread-safe-initialization-of-a-singleton )

Another remark. The same function DefaultStyleMessageFormatter::getInstance() returns a reference to a base class and the base class MessageFormatter is polymorphic, Isn't it better to return a pointer to the base class ? static MessageFormatter *getInstance () { static DefaultStyleMessageFormatter instance; return &instance; }

[damienmarchal]

Since we have dropped vs2013 support (which was not a c++11compiler) there is no doubt that this PR is welcome.

[jnbrunet]

Thanks for your inputs @fspadoni and @damienmarchal , and sorry for the late response.

There are two commits in this PR (two versions of the fix); the first one fixed completely the problem on mac OS X, but may introduce thread race condition as only the constructor of a static variable is thread safe. Since the messageTypePrefixes and messageTypeColors static arrays are initialized in the formatMessage method, two threads can collide there.

The second one tries to fix that, but isn't working yet (hence the commit name "temp"). In this one, the DefaultStyleMessageFormatter singleton instantiation doesn't cause a seg fault on mac os x anymore, but the messageTypeColors static array fails to instantiate correctly, causing all console messages to be blue. This may be because Console::BRIGHT_GREEN, Console::BRIGHT_YELLOW, etc. are all, of course, static objects. Since they are initialized in another .cpp, the order of their constructor calls with respect to the DefaultStyleMessageFormatter singleton instantiation cannot be predicted (compiler bound).

I'll try to find a solution in two weeks (I'll be away for the next 10 days working hard on my sun tan ☀️)

[damienmarchal]

Thanks a lot of the effort you put in fixing this.

[jnbrunet]

[ci-build][with-scene-tests]

[jnbrunet]

This PR is ready to review. It tries to solve the often called "static initialization order fiasco" by removing the use of static variables where it can be easily replaced by try catch/enum function or function's local context static variables only.

While I was in this part of Sofa, I also tried to uniformize the different message formatter classes as mush as I could.

[jnbrunet]

Thanks for the review @damienmarchal . However I'm not sure what you are expecting for the tests based on BaseTest? My modification is needed for the compilation of these cpp files, I didn't go further than this.

[damienmarchal]

When inheriting from BaseTest this automatically add to the test the adequate message handler so there is no need to add them manually. But refactoring these tests is out of the scope of the PR :) So it was more a comment for future work ;)

EDIT: Do you know why the windows build is failing ?

[jnbrunet]

Oops, forgot about Windows

Back to WIP

[jnbrunet]

[ci-build][with-scene-tests]

[jnbrunet]

Now ready to review.

A complete re-factoring of the console helper has been done to improves the efficiency of the console output formatting. It also adds style features to supported OS terminals. On Windows, both ainsi and native terminals are now supported and automatically detected.

[guparan]

[ci-build][with-scene-tests]

[guparan]

[ci-build][with-scene-tests]