Thread (win32) and current_thread_tls_key issues
Hi, Currently Threads's Win32 TLS implementation uses a global static variable thread::current_thread_tls_key that is defined in src/win32/thread.cpp. As you may know, static variables have a tendency of not being allocated in a very specific order. Say, that you have a statically allocated class A that ends up calling thread::set_current_thread_data() (eg. uses thread::thread_specific_ptr). If compiler initializes this class before it initializes current_thread_tls_key (this did happen to me on MSVC9) things go boom: a) Compiler initializes class A, thread::set_current_thread_data() is called, thread::create_current_thread_tls_key() is called once (as expected) and ::TlsAlloc() result is stored to thread::current_thread_tls_key. b) Compiler initializes thread::current_thread_tls_key, TLS_OUT_OF_INDEXES is stored and, as you may guess, c) Any following thread::set_current_thread_data calls will fail as the proper result from ::TlsAlloc() was overwritten. Don't know why this haven't happened before, as previous versions use a static variable, too. Maybe some compilers (MSVC in this case) omit initialization code unless initial value is a non-zero (as with TLS_OUT_OF_INDEXES [0xffffffff] in this case) value. I propose that thread::current_thread_tls_key is moved to a static function that sole purpose is to return reference to that variable (and thus ensure that current_thread_tls_key is initialized before it is used for the first time). Also, move thread::current_thread_tls_init_flag to thread::set_current_thread_data() to make sure this won't happen to that, too. Attached a patch that does the above trick (did move TLS_OUT_OF_INDEXES back to a macro as otherwise the code got too cumbersome, does get undef'd unless it was already defined by Winapi). If you like it, I can submit a ticket to Trac that includes the patch. (Haven't looked what happens during destruction, but guess it's not safe to use TLS features thru Thread.) -- Pekka
Le 02/07/12 10:50, Pekka Seppänen a écrit :
Hi,
Currently Threads's Win32 TLS implementation uses a global static variable thread::current_thread_tls_key that is defined in src/win32/thread.cpp. As you may know, static variables have a tendency of not being allocated in a very specific order.
Say, that you have a statically allocated class A that ends up calling thread::set_current_thread_data() (eg. uses thread::thread_specific_ptr). If compiler initializes this class before it initializes current_thread_tls_key (this did happen to me on MSVC9) things go boom:
a) Compiler initializes class A, thread::set_current_thread_data() is called, thread::create_current_thread_tls_key() is called once (as expected) and ::TlsAlloc() result is stored to thread::current_thread_tls_key.
b) Compiler initializes thread::current_thread_tls_key, TLS_OUT_OF_INDEXES is stored and, as you may guess,
c) Any following thread::set_current_thread_data calls will fail as the proper result from ::TlsAlloc() was overwritten.
Don't know why this haven't happened before, as previous versions use a static variable, too. Maybe some compilers (MSVC in this case) omit initialization code unless initial value is a non-zero (as with TLS_OUT_OF_INDEXES [0xffffffff] in this case) value.
I propose that thread::current_thread_tls_key is moved to a static function that sole purpose is to return reference to that variable (and thus ensure that current_thread_tls_key is initialized before it is used for the first time). Also, move thread::current_thread_tls_init_flag to thread::set_current_thread_data() to make sure this won't happen to that, too.
Attached a patch that does the above trick (did move TLS_OUT_OF_INDEXES back to a macro as otherwise the code got too cumbersome, does get undef'd unless it was already defined by Winapi). If you like it, I can submit a ticket to Trac that includes the patch.
(Haven't looked what happens during destruction, but guess it's not safe to use TLS features thru Thread.)
Hi, Thanks for the report and the deep analysis. Please, create a ticket, I will give it a try soon. A test case that shows the problem and that your patch is a solution would be much appreciated. Best, Vicente
On 7/2/2012 12:11 PM, Vicente J. Botet Escriba wrote:
Le 02/07/12 10:50, Pekka Seppänen a écrit :
... I propose that thread::current_thread_tls_key is moved to a static function that sole purpose is to return reference to that variable (and thus ensure that current_thread_tls_key is initialized before it is used for the first time). Also, move thread::current_thread_tls_init_flag to thread::set_current_thread_data() to make sure this won't happen to that, too.
Could this fix the test_tss_lib test failures in <http://www.boost.org/development/tests/release/developer/thread.html>, e.g., <http://www.boost.org/development/tests/release/developer/output/MinGW-w64-4-5%20jc-bell-com-boost-bin-v2-libs-thread-test-test_tss_lib-test-gcc-mingw-4-5-4-debug-threading-multi.html>? That would be nice.
On 2.7.2012 20:11, Vicente J. Botet Escriba wrote:
Le 02/07/12 10:50, Pekka Seppänen a écrit :
Hi,
Currently Threads's Win32 TLS implementation uses a global static variable thread::current_thread_tls_key that is defined in src/win32/thread.cpp. As you may know, static variables have a tendency of not being allocated in a very specific order.
blah blah
Hi,
Thanks for the report and the deep analysis.
Please, create a ticket, I will give it a try soon.
A test case that shows the problem and that your patch is a solution would be much appreciated.
Ok, created a ticked #7066 (https://svn.boost.org/trac/boost/ticket/7066) with the patch attached. Not really sure how to write a test case for this, I guess it becomes a tedious job even if you could predict the initialization order for each compiler version. Anyway, the way I spotted this in the first place was to simply add a memory write breakpoint for current_thread_tls_key; Saw it was initialized after it was used for the first time (didn't check release builds, but at least MSVC9 debug builds have a simple loop before entry point that do this kind of init stuff). -- Pekka
Le 03/07/12 08:41, Pekka Seppänen a écrit :
On 2.7.2012 20:11, Vicente J. Botet Escriba wrote:
Le 02/07/12 10:50, Pekka Seppänen a écrit :
Hi,
Currently Threads's Win32 TLS implementation uses a global static variable thread::current_thread_tls_key that is defined in src/win32/thread.cpp. As you may know, static variables have a tendency of not being allocated in a very specific order.
blah blah
Hi,
Thanks for the report and the deep analysis.
Please, create a ticket, I will give it a try soon.
A test case that shows the problem and that your patch is a solution would be much appreciated.
Ok, created a ticked #7066 (https://svn.boost.org/trac/boost/ticket/7066) with the patch attached.
Not really sure how to write a test case for this, I guess it becomes a tedious job even if you could predict the initialization order for each compiler version.
Anyway, the way I spotted this in the first place was to simply add a memory write breakpoint for current_thread_tls_key; Saw it was initialized after it was used for the first time (didn't check release builds, but at least MSVC9 debug builds have a simple loop before entry point that do this kind of init stuff). Thanks, Vicente
On 02/07/12 09:50, Pekka Seppänen wrote:
Currently Threads's Win32 TLS implementation uses a global static variable thread::current_thread_tls_key that is defined in src/win32/thread.cpp. As you may know, static variables have a tendency of not being allocated in a very specific order.
Oops. This was changed from direct init via a macro (which does lead to static initialization) to init via an intermediate variable (which doesn't) in the recent batch of thread updates and I didn't spot it. I'll commit a fix for it. Anthony -- Author of C++ Concurrency in Action http://www.stdthread.co.uk/book/ just::thread C++11 thread library http://www.stdthread.co.uk Just Software Solutions Ltd http://www.justsoftwaresolutions.co.uk 15 Carrallack Mews, St Just, Cornwall, TR19 7UL, UK. Company No. 5478976
On 3.7.2012 10:39, Anthony Williams wrote:
On 02/07/12 09:50, Pekka Seppänen wrote:
Currently Threads's Win32 TLS implementation uses a global static variable thread::current_thread_tls_key that is defined in src/win32/thread.cpp. As you may know, static variables have a tendency of not being allocated in a very specific order.
Oops. This was changed from direct init via a macro (which does lead to static initialization) to init via an intermediate variable (which doesn't) in the recent batch of thread updates and I didn't spot it.
I'll commit a fix for it.
/o\ Argh, of course that's the real problem why this was occurring in the first place. I wonder how could I miss that one especially when I've been lately patching some internal (static) values of a certain 3rd party DLLs. Should've spotted that. Well, no harm was luckily done even my proposed cure was worse than the disease this time.. -- Pekka
On 03/07/12 08:39, Anthony Williams wrote:
On 02/07/12 09:50, Pekka Seppänen wrote:
Currently Threads's Win32 TLS implementation uses a global static variable thread::current_thread_tls_key that is defined in src/win32/thread.cpp. As you may know, static variables have a tendency of not being allocated in a very specific order.
Oops. This was changed from direct init via a macro (which does lead to static initialization) to init via an intermediate variable (which doesn't) in the recent batch of thread updates and I didn't spot it.
I'll commit a fix for it.
Revision 79239 on trunk Anthony -- Author of C++ Concurrency in Action http://www.stdthread.co.uk/book/ just::thread C++11 thread library http://www.stdthread.co.uk Just Software Solutions Ltd http://www.justsoftwaresolutions.co.uk 15 Carrallack Mews, St Just, Cornwall, TR19 7UL, UK. Company No. 5478976
Le 03/07/12 10:12, Anthony Williams a écrit :
On 03/07/12 08:39, Anthony Williams wrote:
On 02/07/12 09:50, Pekka Seppänen wrote:
Currently Threads's Win32 TLS implementation uses a global static variable thread::current_thread_tls_key that is defined in src/win32/thread.cpp. As you may know, static variables have a tendency of not being allocated in a very specific order. Oops. This was changed from direct init via a macro (which does lead to static initialization) to init via an intermediate variable (which doesn't) in the recent batch of thread updates and I didn't spot it.
I'll commit a fix for it. Revision 79239 on trunk
Anthony Hi,
it seems it was me that committed this change http://svn.boost.org/trac/boost/changeset/76752 while trying to fix http://svn.boost.org/trac/boost/ticket/4885 Sorry, Vicente
participants (4)
-
Anthony Williams -
Jim Bell -
Pekka Seppänen -
Vicente J. Botet Escriba