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

Add support for thread-local setlocale() and localeconv(). #4736

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@keyurdg
Contributor

keyurdg commented Jan 29, 2015

This also fixes an icompatibility (with Zend) where printf doesn't respect the
locale.

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Jan 29, 2015

This pull request has been imported into Phabricator, and discussion and review of the diff will take place at https://reviews.facebook.net/D32541

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Jan 29, 2015

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Feb 3, 2015

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@keyurdg keyurdg force-pushed the keyurdg:master branch from 442f92f to 500b301 Feb 14, 2015

@hhvm-bot hhvm-bot closed this in 5a8046c Mar 4, 2015

@JoelMarcey JoelMarcey reopened this Mar 5, 2015

@JoelMarcey

This comment has been minimized.

Contributor

JoelMarcey commented Mar 5, 2015

Hi @keyurdg, we are going to have to revert this pull request because of some segfaults happening on Ubuntu 14.04. Not sure what in this diff is causing it, but it is the commit that started the problem.

See #4954

So I am re-opening this.

Out of curiosity, what system did you build and test this PR on?

cc: @jasone, @sgolemon, @rrh

@keyurdg

This comment has been minimized.

Contributor

keyurdg commented Mar 5, 2015

Hi @JoelMarcey I tested it on Cent 6 and 7 and we are running this in Prod on Cent 7.

Do you have a core dump for the backtrace? I'll also try this on an Ubuntu 14.04 VM tomorrow.

@JoelMarcey

This comment has been minimized.

Contributor

JoelMarcey commented Mar 5, 2015

All I have is what is in #4954

@rrh -- have any more debug info that you can provide?

Fix static-initialization-fiasco as shown in issue #4954. Also correct
a comment.

Summary: libp11-kit.so.0 also uses thread specific keys and because it
calls pthread_key_create() very very early, it gets "assigned" a key
with value 0.
Next when the ICU shared library loads, it calls setlocale() during its
init. setlocale uses the stack-allocated object g_thread_safe_locale_handler
which doesn't have any pointer inside, so it calls ThreadLocal::create()
to create a new ThreadSafeLocaleHandler object. But
ThreadLocalManager::s_manager isn't fully initialized yet, so s_manager's
m_key is 0 and when s_manager::setTop() is called, g_thread_safe_locale_handler
is set into the key. This is the same key as is used by libp11-kit.
As the rest of the HHVM binary loads, ThreadLocalManager::s_manager is properly
initialized and a new ThreadSafeLocaleHandler is created and everything works
fine.
When the process starts to shutdown and p11-kit is unloaded,
pthread_getspecific() with key 0 is called here and it now has the pointer to
the stack object g_thread_safe_locale_handler. When free() is called on this,
there's a SEGV.

hhvm-bot added a commit that referenced this pull request Apr 6, 2015

Make ThreadManager initialization explicit, rather than load-time
Summary: Anything which touches a managed ThreadLocal can currently not rely on
anything using static initialization at process start time because
ThreadManager:s_manager may (or may not) have been constructed by then.

Move access to ThreadManager::s_manager to a factory method so that
we can explicitly intialize it on use, rather than hoping the load
order is correct.

This fixes D1813245 / #4736
which ran into the initialization ordering issue due to setlocale()
being called during process startup by third-party libraries.

Reviewed By: @markw65

Differential Revision: D1927502

Pulled By: @sgolemon
@@ -0,0 +1,14 @@
<?php
test(LC_TIME, 'fr_FR');

This comment has been minimized.

@glensc

glensc Apr 6, 2015

Contributor

maybe use utf-8 locales. looks better in github and in terminal :)

fr_FR.UTF-8

@sgolemon

This comment has been minimized.

Contributor

sgolemon commented Apr 7, 2015

FYI Looks like all the side-issues with this diff have been worked out. Should be landing it today after all the test runs have completed. Thanks for all your patience!

sgolemon added a commit that referenced this pull request Apr 8, 2015

Revert "Revert "Add support for thread-local setlocale() and localeco…
…nv().""

Summary: Re-apply #4736 ( D1813245 ) on top of D1927502 which should fix its
initialization ordering issues.

{sync, type="parent", child="external", childrevid="35451", childdiffid="187203", parentdiffid="6949522"}

Reviewed By: @jwatzman

Differential Revision: D1927535
@sgolemon

This comment has been minimized.

Contributor

sgolemon commented Apr 8, 2015

3df8f4c should take care of it.

@sgolemon sgolemon closed this Apr 8, 2015

@keyurdg

This comment has been minimized.

Contributor

keyurdg commented Apr 8, 2015

Thanks much for shepherding this through @sgolemon!

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