Anthony, Any input on this one. Seems like too complicated a fix for 1.34.1 Thomas Johan Nilsson wrote:
http://svn.boost.org/trac/boost/ticket/906
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
-- Thomas Witt witt@acm.org
Thomas Witt <witt@acm.org> writes:
Any input on this one. Seems like too complicated a fix for 1.34.1
I believe the patch below will fix Johan's immediate problems; I would be grateful if he could try it. Anthony -- Anthony Williams Just Software Solutions Ltd - http://www.justsoftwaresolutions.co.uk Registered in England, Company Number 5478976. Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL Index: tss.cpp =================================================================== RCS file: /cvsroot/boost/boost/libs/thread/src/tss.cpp,v retrieving revision 1.16.10.4 diff -u -r1.16.10.4 tss.cpp --- tss.cpp 1 Oct 2006 12:57:18 -0000 1.16.10.4 +++ tss.cpp 22 May 2007 07:18:57 -0000 @@ -60,6 +60,7 @@ tss_data_mutex = 0; #if defined(BOOST_HAS_WINTHREADS) TlsFree(tss_data_native_key); + tss_data_native_key=0xFFFFFFFF; #elif defined(BOOST_HAS_PTHREADS) pthread_key_delete(tss_data_native_key); #elif defined(BOOST_HAS_MPTASKS) @@ -78,6 +79,9 @@ (*(*tss_data_cleanup_handlers)[i])((*slots)[i]); (*slots)[i] = 0; } +#if define(BOOST_HAS_WINTHREADS) + TlsSetValue(tss_data_native_key,0); +#endif tss_data_dec_use(lock); delete slots; }
Anthony Williams wrote:
Thomas Witt <witt@acm.org> writes:
Any input on this one. Seems like too complicated a fix for 1.34.1
I believe the patch below will fix Johan's immediate problems; I would be grateful if he could try it.
There was a typo at (patched) line 82: "#if define(BOOST_HAS_WINTHREADS)" instead of "#if defined(BOOST_HAS_WINTHREADS)". IIRC we earlier discussed checking for a valid TSS index before calling TlsSetValue when in the cleanup_slots context. Further, I'd suggest statically initializing tss_data_native_key to 0xFFFFFFFF instead of having it zero initialized. The attached patch file (against RC_1_34_0) has these things fixed/added. Please review. I've applied the patch and successfully run all tests under libs/thread/test for current RC_1_34_0 under WinXP SP2 (SMP + HT). My own tests also keep running successfully with the patch applied. Even though the patch "couldn't possibly" have any effects under non-windows platforms I also applied the patch and ran the tests under OpenSUSE 10.2 using gcc 4.1.2. Adding a named constant instead of hardcoding 0xFFFFFFFF all over the place would also be great. However, as I believe this is the last Boost release containing the "original" thread code I guess the point is moot? [A bit off-topic] Is it intentional to have the 1.34.1 updates go under the RC_1_34_0 branch? Thanks / Johan
"Johan Nilsson" <r.johan.nilsson@gmail.com> writes:
Anthony Williams wrote:
Thomas Witt <witt@acm.org> writes:
Any input on this one. Seems like too complicated a fix for 1.34.1
I believe the patch below will fix Johan's immediate problems; I would be grateful if he could try it.
There was a typo at (patched) line 82: "#if define(BOOST_HAS_WINTHREADS)" instead of "#if defined(BOOST_HAS_WINTHREADS)".
Oops. Thanks for spotting it.
IIRC we earlier discussed checking for a valid TSS index before calling TlsSetValue when in the cleanup_slots context.
That was when you suggested calling TlsSetValue *after* cleanup slots --- at which point the TSS index may not be valid. In cleanup_slots (where it is now) it is always valid, so we don't need the check.
Further, I'd suggest statically initializing tss_data_native_key to 0xFFFFFFFF instead of having it zero initialized.
Good idea.
The attached patch file (against RC_1_34_0) has these things fixed/added. Please review. I've applied the patch and successfully run all tests under libs/thread/test for current RC_1_34_0 under WinXP SP2 (SMP + HT). My own tests also keep running successfully with the patch applied.
Even though the patch "couldn't possibly" have any effects under non-windows platforms I also applied the patch and ran the tests under OpenSUSE 10.2 using gcc 4.1.2.
Thanks for doing this.
Adding a named constant instead of hardcoding 0xFFFFFFFF all over the place would also be great. However, as I believe this is the last Boost release containing the "original" thread code I guess the point is moot?
Actually, this is TLS_OUT_OF_INDEXES (the return value from TlsAlloc if there's not a free slot), so I've replaced it with that constant.
[A bit off-topic] Is it intentional to have the 1.34.1 updates go under the RC_1_34_0 branch?
Yes. Attached is a revised patch. Anthony -- Anthony Williams Just Software Solutions Ltd - http://www.justsoftwaresolutions.co.uk Registered in England, Company Number 5478976. Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL Index: tss.cpp =================================================================== RCS file: /cvsroot/boost/boost/libs/thread/src/tss.cpp,v retrieving revision 1.16.10.4 diff -u -r1.16.10.4 tss.cpp --- tss.cpp 1 Oct 2006 12:57:18 -0000 1.16.10.4 +++ tss.cpp 22 May 2007 10:55:06 -0000 @@ -31,7 +31,7 @@ boost::mutex* tss_data_mutex = 0; tss_data_cleanup_handlers_type* tss_data_cleanup_handlers = 0; #if defined(BOOST_HAS_WINTHREADS) - DWORD tss_data_native_key; + DWORD tss_data_native_key=TLS_OUT_OF_INDEXES; #elif defined(BOOST_HAS_PTHREADS) pthread_key_t tss_data_native_key; #elif defined(BOOST_HAS_MPTASKS) @@ -60,6 +60,7 @@ tss_data_mutex = 0; #if defined(BOOST_HAS_WINTHREADS) TlsFree(tss_data_native_key); + tss_data_native_key=TLS_OUT_OF_INDEXES; #elif defined(BOOST_HAS_PTHREADS) pthread_key_delete(tss_data_native_key); #elif defined(BOOST_HAS_MPTASKS) @@ -78,6 +79,9 @@ (*(*tss_data_cleanup_handlers)[i])((*slots)[i]); (*slots)[i] = 0; } +#if defined(BOOST_HAS_WINTHREADS) + TlsSetValue(tss_data_native_key,0); +#endif tss_data_dec_use(lock); delete slots; } @@ -97,7 +101,7 @@ //Allocate tls slot tss_data_native_key = TlsAlloc(); - if (tss_data_native_key == 0xFFFFFFFF) + if (tss_data_native_key == TLS_OUT_OF_INDEXES) return; #elif defined(BOOST_HAS_PTHREADS) int res = pthread_key_create(&tss_data_native_key, &cleanup_slots);
Anthony Williams wrote:
"Johan Nilsson" <r.johan.nilsson@gmail.com> writes:
[snip]
IIRC we earlier discussed checking for a valid TSS index before calling TlsSetValue when in the cleanup_slots context.
That was when you suggested calling TlsSetValue *after* cleanup slots --- at which point the TSS index may not be valid. In cleanup_slots (where it is now) it is always valid, so we don't need the check.
True. [snip]
[A bit off-topic] Is it intentional to have the 1.34.1 updates go under the RC_1_34_0 branch?
Yes.
Isn't it a bit illogical to have 1.34.1 go under RC_1_34_0 (with emphasis on the trailing zero)?
Attached is a revised patch.
Ok, applied and retested. Looks good. FWIW, I vote to accept it. Thanks / Johan
Johan Nilsson wrote:
[A bit off-topic] Is it intentional to have the 1.34.1 updates go under the RC_1_34_0 branch? Yes.
Isn't it a bit illogical to have 1.34.1 go under RC_1_34_0 (with emphasis on the trailing zero)?
Yes, it is. Nevertheless, there's a tradition to do it that way for Boost. I hope this will get fixed by the new procedures, too. Regards, m
Martin Wille wrote:
Johan Nilsson wrote:
[A bit off-topic] Is it intentional to have the 1.34.1 updates go under the RC_1_34_0 branch? Yes. Isn't it a bit illogical to have 1.34.1 go under RC_1_34_0 (with emphasis on the trailing zero)?
Yes, it is. Nevertheless, there's a tradition to do it that way for Boost. I hope this will get fixed by the new procedures, too.
Yeah. I think the branch should be called '1_34', since all minor (bug-fix) releases are spun off of that. Speaking of numbering, we also discussed removing the leading '1', since it doesn't appear to bear any meaning. And, since there is no attempt being made to be backward-compatible (neither API nor ABI), a simple linear numbering scheme appears just natural... Regards, Stefan -- ...ich hab' noch einen Koffer in Berlin...
Thomas Witt <witt@acm.org> writes:
Any input on this one. Seems like too complicated a fix for 1.34.1
I've added a patch to the ticket. Everything is as before on the boost regression tests, and Johan reports that it fixes his problems. Anthony -- Anthony Williams Just Software Solutions Ltd - http://www.justsoftwaresolutions.co.uk Registered in England, Company Number 5478976. Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL
Anthony Williams wrote:
Thomas Witt <witt@acm.org> writes:
Any input on this one. Seems like too complicated a fix for 1.34.1
I've added a patch to the ticket. Everything is as before on the boost regression tests, and Johan reports that it fixes his problems.
Thanks! I've set the milestone. Could you please apply the patch on the branch. Thomas -- Thomas Witt witt@acm.org
Thomas Witt <witt@acm.org> writes:
Anthony Williams wrote:
Thomas Witt <witt@acm.org> writes:
Any input on this one. Seems like too complicated a fix for 1.34.1
I've added a patch to the ticket. Everything is as before on the boost regression tests, and Johan reports that it fixes his problems.
Thanks! I've set the milestone. Could you please apply the patch on the branch.
Done. Anthony -- Anthony Williams Just Software Solutions Ltd - http://www.justsoftwaresolutions.co.uk Registered in England, Company Number 5478976. Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL
participants (5)
-
Anthony Williams -
Johan Nilsson -
Martin Wille -
Stefan Seefeld -
Thomas Witt