Subzero: Second attempt at fixing MacOS 10.6 build.

Subzero: Second attempt at fixing MacOS 10.6 build.

Manages thread_local pointer fields through a set of macros.  If ICE_THREAD_LOCAL_HACK is defined, the thread_local definitions and accesses are defined in terms of pthread operations.  This assumes that the underlying std::thread library is based on pthread.

BUG= none
R=jfb@chromium.org, jvoung@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=a5fe17a11eae0bbbbed481ea31806d45606198c9

[Jim Stichnoth, 2015-01-24 03:41:47]

stichnot@chromium.org changed reviewers:
+ jfb@chromium.org, jvoung@chromium.org

[Jim Stichnoth, 2015-01-24 03:42:53]

To complete this, I need to define ICE_THREAD_LOCAL_HACK if the MacOS 10.6
environment is detected.  Any ideas?

[JF, 2015-01-24 05:42:47]

On cmake you have CMAKE_SYSTEM_NAME and CMAKE_SYSTEM_VERSION, and then you can
define something in the preprocessor. I'm not sure what the configure build has,
but I assume it's similar.

https://codereview.chromium.org/872933002/diff/20001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/872933002/diff/20001/src/IceDefs.h#newcode48
src/IceDefs.h:48: #endif // !_MSC_VER
ICE_ATTRIBUTE_TLS is sad about being left out of IceTLS.h.

https://codereview.chromium.org/872933002/diff/20001/src/IceTLS.h
File src/IceTLS.h (right):

https://codereview.chromium.org/872933002/diff/20001/src/IceTLS.h#newcode34
src/IceTLS.h:34: // thread_local field.  Must be done within the context of its
class.
I have a mild preference for argument order being the same as in code: Type,
Class, Name, Value.

https://codereview.chromium.org/872933002/diff/20001/src/IceTLS.h#newcode56
src/IceTLS.h:56: pthread_key_create(&ClassName::FieldName##__key, nullptr)
It's not clear from the pthread docs that you can't legally call
pthread_key_create as a global static initializer. This paragraph especially
makes me think it may not be guaranteed to work:

Thus, this proposal was rejected. The pthread_key_create() function performs no
implicit synchronization. It is the responsibility of the programmer to ensure
that it is called exactly once per key before use of the key. Several
straightforward mechanisms can already be used to accomplish this, including
calling explicit module initialization functions, using mutexes, and using
pthread_once(). This places no significant burden on the programmer, introduces
no possibly confusing ad hoc implicit synchronization mechanism, and potentially
allows commonly used thread-specific data operations to be more efficient.


Maybe a global init function would be better (if a bit more annoying)?

[Jim Stichnoth, 2015-01-24 16:55:34]

https://codereview.chromium.org/872933002/diff/20001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/872933002/diff/20001/src/IceDefs.h#newcode48
src/IceDefs.h:48: #endif // !_MSC_VER
On 2015/01/24 05:42:47, JF wrote:
> ICE_ATTRIBUTE_TLS is sad about being left out of IceTLS.h.

Whoops.  I meant to remove ICE_ATTRIBUTE_TLS entirely, but I guess it may still
be good to keep around for Visual Studio.

https://codereview.chromium.org/872933002/diff/20001/src/IceTLS.h
File src/IceTLS.h (right):

https://codereview.chromium.org/872933002/diff/20001/src/IceTLS.h#newcode34
src/IceTLS.h:34: // thread_local field.  Must be done within the context of its
class.
On 2015/01/24 05:42:47, JF wrote:
> I have a mild preference for argument order being the same as in code: Type,
> Class, Name, Value.

Done.

https://codereview.chromium.org/872933002/diff/20001/src/IceTLS.h#newcode56
src/IceTLS.h:56: pthread_key_create(&ClassName::FieldName##__key, nullptr)
On 2015/01/24 05:42:47, JF wrote:
> It's not clear from the pthread docs that you can't legally call
> pthread_key_create as a global static initializer. This paragraph especially
> makes me think it may not be guaranteed to work:
> 
> Thus, this proposal was rejected. The pthread_key_create() function performs
no
> implicit synchronization. It is the responsibility of the programmer to ensure
> that it is called exactly once per key before use of the key. Several
> straightforward mechanisms can already be used to accomplish this, including
> calling explicit module initialization functions, using mutexes, and using
> pthread_once(). This places no significant burden on the programmer,
introduces
> no possibly confusing ad hoc implicit synchronization mechanism, and
potentially
> allows commonly used thread-specific data operations to be more efficient.
> 
> 
> Maybe a global init function would be better (if a bit more annoying)?

I don't see how that could be a problem here, unless the C++ runtime is allowed
to run dynamic initializers in parallel?  Even if the dynamic initializers
somehow get run more than once, it wouldn't be a problem because the keys are
never used before dynamic initialization completes.

[jvoung (off chromium), 2015-01-24 18:47:40]

Small nit, but otherwise looks okay =(

Is the difference libc++ on OSX vs an old libstdc++ on OSX? How did support for
__thread or thread_local appear? Otherwise, might be able to check for __APPLE__
&& lack of libc++ or something?

https://codereview.chromium.org/872933002/diff/40001/src/IceTLS.h
File src/IceTLS.h (right):

https://codereview.chromium.org/872933002/diff/40001/src/IceTLS.h#newcode45
src/IceTLS.h:45: // declaring and defining F::C, we create two static fields:
"instead of declaring and defining C::F" ?

[jvoung (off chromium), 2015-01-24 18:59:39]

On 2015/01/24 18:47:40, jvoung wrote:
> Small nit, but otherwise looks okay =(
> 
> Is the difference libc++ on OSX vs an old libstdc++ on OSX? How did support
for
> __thread or thread_local appear? Otherwise, might be able to check for
__APPLE__
> && lack of libc++ or something?

Err, n/m looks like the toolchain bots already use libc++, so it is the OS
version.

> 
> https://codereview.chromium.org/872933002/diff/40001/src/IceTLS.h
> File src/IceTLS.h (right):
> 
> https://codereview.chromium.org/872933002/diff/40001/src/IceTLS.h#newcode45
> src/IceTLS.h:45: // declaring and defining F::C, we create two static fields:
> "instead of declaring and defining C::F" ?

[Jim Stichnoth, 2015-01-24 21:47:30]

So is it __APPLE__ that is defined for those builds?  I would add that check to
IceTLS.h if so.

https://codereview.chromium.org/872933002/diff/40001/src/IceTLS.h
File src/IceTLS.h (right):

https://codereview.chromium.org/872933002/diff/40001/src/IceTLS.h#newcode45
src/IceTLS.h:45: // declaring and defining F::C, we create two static fields:
On 2015/01/24 18:47:40, jvoung wrote:
> "instead of declaring and defining C::F" ?

Done.

[JF, 2015-01-25 04:37:02]

> I don't see how that could be a problem here, unless the C++ runtime is
allowed
> to run dynamic initializers in parallel?  Even if the dynamic initializers
> somehow get run more than once, it wouldn't be a problem because the keys are
> never used before dynamic initialization completes.

C++11 static initialization was fixed to be thread-safe. My worry is that the
pthread doesn't say you can initialize TLS during static initialization, so
pthread may be allowed to have its own static initialization which runs after
yours (and yours depends on pthread's initialization), causing Odd
Initialization Order Issues That Would Be Hard To Debug. Especially if they
happen on developer's machines and we can't repro on the bots. There likely
isn't an issue, but if there is we'll be sorry (I have scars in exactly this
domain). Your call :-)


On 2015/01/24 21:47:30, stichnot wrote:
> So is it __APPLE__ that is defined for those builds?  I would add that check
to
> IceTLS.h if so.

I guess that would work if we don't do cross-builds?

Would you have the hack only for 10.6, or for all OSX variants? The cmake thing
I suggest is easy, the makefile one could be:
  http://stackoverflow.com/questions/714100/os-detecting-makefile

https://codereview.chromium.org/872933002/diff/60001/src/IceTLS.h
File src/IceTLS.h (right):

https://codereview.chromium.org/872933002/diff/60001/src/IceTLS.h#newcode64
src/IceTLS.h:64: static_cast<Type>(pthread_getspecific(FieldName##__key))
Small thing I just thought about to make the diff in other files smaller:
DECLARE_FIELD can do:
  typedef FieldName##__type Type;

And then GET_FIELD doesn't need a Type parameter. DEFINE_FIELD should probably
still have it since the TLS-hack-less version of the code does have the type.

[Jim Stichnoth, 2015-01-25 08:05:52]

On 2015/01/25 04:37:02, JF wrote:
> > I don't see how that could be a problem here, unless the C++ runtime is
> allowed
> > to run dynamic initializers in parallel?  Even if the dynamic initializers
> > somehow get run more than once, it wouldn't be a problem because the keys
are
> > never used before dynamic initialization completes.
> 
> C++11 static initialization was fixed to be thread-safe. My worry is that the
> pthread doesn't say you can initialize TLS during static initialization, so
> pthread may be allowed to have its own static initialization which runs after
> yours (and yours depends on pthread's initialization), causing Odd
> Initialization Order Issues That Would Be Hard To Debug. Especially if they
> happen on developer's machines and we can't repro on the bots. There likely
> isn't an issue, but if there is we'll be sorry (I have scars in exactly this
> domain). Your call :-)

I see, that makes perfect sense now, thanks for the explanation.

But it makes me even sadder now. :(
(Good, good, let the hate flow through you!)

I thought about changing the dynamic initializer to append to some private data
structure and then explicitly convert it to pthread calls after main() starts. 
But then I remembered that the Chromium coding style bans dynamic initializers,
so I should probably stay away from that even if it's a hack for a single
outdated system.  So I guess I'll make main() explicitly call an initializer
method in every class with a static thread_local field.  I can probably also add
asserts to the GET and SET macros to ensure the initializer method has been
called.

[Jim Stichnoth, 2015-01-25 08:05:53]

On 2015/01/25 04:37:02, JF wrote:
> > I don't see how that could be a problem here, unless the C++ runtime is
> allowed
> > to run dynamic initializers in parallel?  Even if the dynamic initializers
> > somehow get run more than once, it wouldn't be a problem because the keys
are
> > never used before dynamic initialization completes.
> 
> C++11 static initialization was fixed to be thread-safe. My worry is that the
> pthread doesn't say you can initialize TLS during static initialization, so
> pthread may be allowed to have its own static initialization which runs after
> yours (and yours depends on pthread's initialization), causing Odd
> Initialization Order Issues That Would Be Hard To Debug. Especially if they
> happen on developer's machines and we can't repro on the bots. There likely
> isn't an issue, but if there is we'll be sorry (I have scars in exactly this
> domain). Your call :-)

I see, that makes perfect sense now, thanks for the explanation.

But it makes me even sadder now. :(
(Good, good, let the hate flow through you!)

I thought about changing the dynamic initializer to append to some private data
structure and then explicitly convert it to pthread calls after main() starts. 
But then I remembered that the Chromium coding style bans dynamic initializers,
so I should probably stay away from that even if it's a hack for a single
outdated system.  So I guess I'll make main() explicitly call an initializer
method in every class with a static thread_local field.  I can probably also add
asserts to the GET and SET macros to ensure the initializer method has been
called.

[JF, 2015-01-25 18:06:19]

On 2015/01/25 08:05:53, stichnot wrote:
> On 2015/01/25 04:37:02, JF wrote:
> > > I don't see how that could be a problem here, unless the C++ runtime is
> > allowed
> > > to run dynamic initializers in parallel?  Even if the dynamic initializers
> > > somehow get run more than once, it wouldn't be a problem because the keys
> are
> > > never used before dynamic initialization completes.
> > 
> > C++11 static initialization was fixed to be thread-safe. My worry is that
the
> > pthread doesn't say you can initialize TLS during static initialization, so
> > pthread may be allowed to have its own static initialization which runs
after
> > yours (and yours depends on pthread's initialization), causing Odd
> > Initialization Order Issues That Would Be Hard To Debug. Especially if they
> > happen on developer's machines and we can't repro on the bots. There likely
> > isn't an issue, but if there is we'll be sorry (I have scars in exactly this
> > domain). Your call :-)
> 
> I see, that makes perfect sense now, thanks for the explanation.
> 
> But it makes me even sadder now. :(
> (Good, good, let the hate flow through you!)
> 
> I thought about changing the dynamic initializer to append to some private
data
> structure and then explicitly convert it to pthread calls after main() starts.

> But then I remembered that the Chromium coding style bans dynamic
initializers,
> so I should probably stay away from that even if it's a hack for a single
> outdated system.  So I guess I'll make main() explicitly call an initializer
> method in every class with a static thread_local field.  I can probably also
add
> asserts to the GET and SET macros to ensure the initializer method has been
> called.

Here's another idea that may not be any better: you know exactly hoe many TLS
slots you need, so on OSX 10.6 roll your own TLS without using pthread but
instead using std::map<std::thread::id> (or some other container) inside the
global context. Initialize the slots when you create the threads (with
std::thread::id), and change the macro to do the lookup (using
std::this_thread::get_id). It'll be slow but only on OSX 10.6, it won't use
pthread directly, but it may be less invasive.

You might also end up needing such a map to each thread anyways?

[jvoung (off chromium), 2015-01-25 18:43:00]

On 2015/01/25 04:37:02, JF wrote:
> > I don't see how that could be a problem here, unless the C++ runtime is
> allowed
> > to run dynamic initializers in parallel?  Even if the dynamic initializers
> > somehow get run more than once, it wouldn't be a problem because the keys
are
> > never used before dynamic initialization completes.
> 
> C++11 static initialization was fixed to be thread-safe. My worry is that the
> pthread doesn't say you can initialize TLS during static initialization, so
> pthread may be allowed to have its own static initialization which runs after
> yours (and yours depends on pthread's initialization), causing Odd
> Initialization Order Issues That Would Be Hard To Debug. Especially if they
> happen on developer's machines and we can't repro on the bots. There likely
> isn't an issue, but if there is we'll be sorry (I have scars in exactly this
> domain). Your call :-)
> 
> 
> On 2015/01/24 21:47:30, stichnot wrote:
> > So is it __APPLE__ that is defined for those builds?  I would add that check
> to
> > IceTLS.h if so.
> 
> I guess that would work if we don't do cross-builds?
> 
> Would you have the hack only for 10.6, or for all OSX variants? The cmake
thing
> I suggest is easy, the makefile one could be:
>   http://stackoverflow.com/questions/714100/os-detecting-makefile
> 

Just to clarify, __APPLE__ comes from the compiler and should also show up for
cross builds.

E.g., on linux:

clang++ -x c++ /dev/null -O2 --target=x86_64-apple-darwin11 -std=c++11 -dM -E |
grep "__APPLE__"
#define __APPLE__ 1

But yes, that would mean all OSX variants and I don't know how to get the actual
version other than through the make/configure/cmake/toolchain_build system.



> https://codereview.chromium.org/872933002/diff/60001/src/IceTLS.h
> File src/IceTLS.h (right):
> 
> https://codereview.chromium.org/872933002/diff/60001/src/IceTLS.h#newcode64
> src/IceTLS.h:64: static_cast<Type>(pthread_getspecific(FieldName##__key))
> Small thing I just thought about to make the diff in other files smaller:
> DECLARE_FIELD can do:
>   typedef FieldName##__type Type;
> 
> And then GET_FIELD doesn't need a Type parameter. DEFINE_FIELD should probably
> still have it since the TLS-hack-less version of the code does have the type.

[Jim Stichnoth, 2015-01-25 18:58:40]

In the latest version, an initialization function must be explicitly called
after main() starts.

Also in this version, I'm being bold and using the hack for all platforms, to
avoid further delays in pushing the PNaCl update.  We can try to dial it down to
MacOS 10.6 in a later CL.  For the single-threaded version, I couldn't see a
performance difference on my laptop.

https://codereview.chromium.org/872933002/diff/60001/src/IceTLS.h
File src/IceTLS.h (right):

https://codereview.chromium.org/872933002/diff/60001/src/IceTLS.h#newcode64
src/IceTLS.h:64: static_cast<Type>(pthread_getspecific(FieldName##__key))
On 2015/01/25 04:37:01, JF wrote:
> Small thing I just thought about to make the diff in other files smaller:
> DECLARE_FIELD can do:
>   typedef FieldName##__type Type;
> 
> And then GET_FIELD doesn't need a Type parameter. DEFINE_FIELD should probably
> still have it since the TLS-hack-less version of the code does have the type.

Very nice, done.

[Jim Stichnoth, 2015-01-25 19:14:34]

On 2015/01/25 18:06:19, JF wrote:
> Here's another idea that may not be any better: you know exactly hoe many TLS
> slots you need, so on OSX 10.6 roll your own TLS without using pthread but
> instead using std::map<std::thread::id> (or some other container) inside the
> global context. Initialize the slots when you create the threads (with
> std::thread::id), and change the macro to do the lookup (using
> std::this_thread::get_id). It'll be slow but only on OSX 10.6, it won't use
> pthread directly, but it may be less invasive.
> 
> You might also end up needing such a map to each thread anyways?

In general, I think this is something I should try to wherever possible.  In
most of the current uses of TLS, we have access to the Cfg which is a great
place to store the thread index, except for the parser thread and the emitter
thread, which would have their own way to get the thread index.  This would be
better because the performance of accessing this data would vary much less
across platforms.

The one place where this can't be done (or at least I can't see a way, or at
least an easy/efficient way) is the definition/use of CfgLocalAllocator as the
arena allocator for STL containers.  Even though the Cfg is available at all
uses, the allocator is stateless so it has to access TLS.

[JF, 2015-01-25 23:05:05]

lgtm after one last comment on pthread_key_create.


> Also in this version, I'm being bold and using the hack for all platforms, to
> avoid further delays in pushing the PNaCl update.  We can try to dial it down
to
> MacOS 10.6 in a later CL.  For the single-threaded version, I couldn't see a
> performance difference on my laptop.

Sounds good, let's see if it shows up in profiles, and by the time it matters we
may have dropped 10.6.


> In general, I think this is something I should try to wherever possible.  In
> most of the current uses of TLS, we have access to the Cfg which is a great
> place to store the thread index, except for the parser thread and the emitter
> thread, which would have their own way to get the thread index.  This would be
> better because the performance of accessing this data would vary much less
> across platforms.
> 
> The one place where this can't be done (or at least I can't see a way, or at
> least an easy/efficient way) is the definition/use of CfgLocalAllocator as the
> arena allocator for STL containers.  Even though the Cfg is available at all
> uses, the allocator is stateless so it has to access TLS.

Ah you're right, I hadn't thought of that.

https://codereview.chromium.org/872933002/diff/80001/src/IceTLS.h
File src/IceTLS.h (right):

https://codereview.chromium.org/872933002/diff/80001/src/IceTLS.h#newcode75
src/IceTLS.h:75: FieldName##__dummy = pthread_key_create(&FieldName##__key,
nullptr)
You should probably handle non-zero returns properly, especially EGAIN and
ENOMEM. I'd also rename "dummy" to "needs_initialization" or something.

Why the weird initialization and not just true?

[Jim Stichnoth, 2015-01-26 05:43:37]

https://codereview.chromium.org/872933002/diff/80001/src/IceTLS.h
File src/IceTLS.h (right):

https://codereview.chromium.org/872933002/diff/80001/src/IceTLS.h#newcode75
src/IceTLS.h:75: FieldName##__dummy = pthread_key_create(&FieldName##__key,
nullptr)
On 2015/01/25 23:05:05, JF wrote:
> You should probably handle non-zero returns properly, especially EGAIN and
> ENOMEM. I'd also rename "dummy" to "needs_initialization" or something.

Done, though I didn't feel like there's much reason to distinguish different
error codes.

> 
> Why the weird initialization and not just true?

Well, I started out initializing it to 1, then I thought maybe something more
nonzero-like like 12345, then maybe something more flamboyant like
0x1BadCafeBabe, then I finally decided to express my "true" feelings.  OK, I
came to my senses and went back to 1.

[jvoung (off chromium), 2015-01-26 18:35:23]

lgtm

[JF, 2015-01-26 18:48:33]

https://codereview.chromium.org/872933002/diff/120001/src/IceTLS.h
File src/IceTLS.h (right):

https://codereview.chromium.org/872933002/diff/120001/src/IceTLS.h#newcode79
src/IceTLS.h:79: llvm_unreachable("Failed to create pthread key");              
         \
This should be a fatal error instead, unreachable gets optimized out.

[Jim Stichnoth, 2015-01-26 19:06:19]

https://codereview.chromium.org/872933002/diff/120001/src/IceTLS.h
File src/IceTLS.h (right):

https://codereview.chromium.org/872933002/diff/120001/src/IceTLS.h#newcode79
src/IceTLS.h:79: llvm_unreachable("Failed to create pthread key");              
         \
On 2015/01/26 18:48:33, JF wrote:
> This should be a fatal error instead, unreachable gets optimized out.

Oops, that's what I meant...  Done

[Jim Stichnoth, 2015-01-26 19:10:07]

Committed patchset #9 (id:160001) manually as
a5fe17a11eae0bbbbed481ea31806d45606198c9 (presubmit successful).