New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Jul 17, 2018

Conversation

6 participants
@jnbrunet
Copy link
Contributor

jnbrunet commented Apr 19, 2018

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:

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

Reviewers will merge only if all these checks are true.

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Apr 25, 2018

Thanks @jnbrunet ! Will change #635 accordingly 😉

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Apr 25, 2018

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

This comment has been minimized.

Copy link
Contributor Author

jnbrunet commented Apr 27, 2018

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

This comment has been minimized.

Copy link
Member

guparan commented May 2, 2018

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

@guparan guparan referenced this pull request May 2, 2018

Merged

[CMake] Improve SOFA installation and packaging #635

6 of 6 tasks complete
@epernod

This comment has been minimized.

Copy link
Contributor

epernod commented May 2, 2018

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

@jnbrunet jnbrunet force-pushed the jnbrunet:636_fix_mac_segfault_on_static_initialiser branch from 099c178 to e5b1857 May 2, 2018

@fspadoni

This comment has been minimized.

Copy link
Contributor

fspadoni commented May 2, 2018

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

This comment has been minimized.

Copy link
Contributor

damienmarchal commented May 6, 2018

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

@jnbrunet

This comment has been minimized.

Copy link
Contributor Author

jnbrunet commented May 17, 2018

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

This comment has been minimized.

Copy link
Contributor

damienmarchal commented May 17, 2018

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

@jnbrunet jnbrunet force-pushed the jnbrunet:636_fix_mac_segfault_on_static_initialiser branch 2 times, most recently from ebb6651 to 4d1eccc Jun 9, 2018

@jnbrunet jnbrunet force-pushed the jnbrunet:636_fix_mac_segfault_on_static_initialiser branch from 4d1eccc to ba9e1e1 Jun 9, 2018

@jnbrunet

This comment has been minimized.

Copy link
Contributor Author

jnbrunet commented Jun 9, 2018

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

@jnbrunet

This comment has been minimized.

Copy link
Contributor Author

jnbrunet commented Jun 13, 2018

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.

@@ -79,7 +79,7 @@ void perTestInit()
}

if(defaultHandler==nullptr)
defaultHandler=new ConsoleMessageHandler(new RichConsoleStyleMessageFormatter) ;
defaultHandler=new ConsoleMessageHandler(&RichConsoleStyleMessageFormatter::getInstance()) ;

This comment has been minimized.

@damienmarchal

damienmarchal Jun 13, 2018

Contributor

Not really related with the PR but these codes are not needed anymore if the test is based on BaseTest.

@@ -79,7 +79,7 @@ void perTestInit()
}

if(defaultHandler==nullptr)
defaultHandler=new ConsoleMessageHandler(new RichConsoleStyleMessageFormatter) ;
defaultHandler=new ConsoleMessageHandler(&RichConsoleStyleMessageFormatter::getInstance()) ;

This comment has been minimized.

@damienmarchal

damienmarchal Jun 13, 2018

Contributor

Not really related with the PR but these codes are not needed anymore if the test is based on BaseTest.

@jnbrunet

This comment has been minimized.

Copy link
Contributor Author

jnbrunet commented Jun 15, 2018

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

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Jun 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

jnbrunet commented Jun 15, 2018

Oops, forgot about Windows

Back to WIP

@jnbrunet jnbrunet force-pushed the jnbrunet:636_fix_mac_segfault_on_static_initialiser branch 2 times, most recently from 64cd3c2 to 000f8cb Jun 23, 2018

jnbrunet added some commits Jun 18, 2018

[SofaKernel] FIX #636 complete refactoring of the console helper
In summary, this commit improves considerably the efficiency of the
customization of console output. It also adds style features to supported
OS terminals. On Windows, both ainsi and native terminals are now supported.

@jnbrunet jnbrunet force-pushed the jnbrunet:636_fix_mac_segfault_on_static_initialiser branch from 000f8cb to 0e658c1 Jun 23, 2018

@jnbrunet

This comment has been minimized.

Copy link
Contributor Author

jnbrunet commented Jun 25, 2018

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

@jnbrunet

This comment has been minimized.

Copy link
Contributor Author

jnbrunet commented Jun 25, 2018

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

This comment has been minimized.

Copy link
Member

guparan commented Jun 26, 2018

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

@guparan guparan changed the title [SofaKernel] FIX #636 Static initialisers were creating segfault on OSX/clang compiler [SofaKernel] FIX segfault created by static initialisers on OSX/clang compiler Jun 27, 2018

@guparan

This comment has been minimized.

Copy link
Member

guparan commented Jul 11, 2018

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

@hugtalbot hugtalbot merged commit 005808a into sofa-framework:master Jul 17, 2018

12 of 13 checks passed

Jk2/mac_clang-3.5_options Build failed.
Details
Dashboard Builds triggered.
Details
Jk2/Dashboard Builds triggered.
Details
Jk2/Scene tests Triggered in latest build.
Details
Jk2/centos_clang-3.4_options Build OK. FIXME: 0 unit tests, 0 scene tests
Details
Jk2/ubuntu_clang-3.8_options Build OK. FIXME: 0 unit tests, 0 scene tests
Details
Jk2/ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit tests, 1 scene tests
Details
Jk2/windows7_VS-2015_options_amd64 Build OK. FIXME: 2 unit tests, 0 scene tests
Details
Scene tests Triggered in latest build.
Details
centos_clang-3.4_options OK (tests ignored, see details)
Details
mac_clang-3.4_options OK (tests ignored, see details)
Details
ubuntu_gcc-5.4_options OK (tests ignored, see details)
Details
windows7_VS-2015_options_amd64 OK (tests ignored, see details)
Details

@guparan guparan added this to the v18.12 milestone Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment