Re: Boost.Threads - Open Questions from the Review

From: William Kempf (williamkempf_at_[hidden])
Date: 2001-09-17 09:59:30

>From: Jens Maurer <Jens.Maurer_at_[hidden]>
>William Kempf wrote:
>>--- In boost_at_y..., Jens Maurer <Jens.Maurer_at_g...> wrote:
>> >  - xtime.hpp uses "boost/stdint.h", which is deprecated
>> >and fails with como on my Linux box.  Use "boost/cstdint.hpp"
>> >instead (need to fix uintXX_t usage in the code as well).
>>
>>I don't understand the part in parens above.  xtime.hpp uses int_fastXX_t
>>types but not uintXX_t types, and I don't know of anything in the usage 
>>that
>>needs fixed.
>
>Currently, there are two options to get to the int_fastXX_t typedefs:
>You can #include "boost/stdint.h" and use std::int_fastXX_t
>(that's what you do now), or you can #include "boost/cstdint.hpp"
>and use boost:int_fastXX_t.   I'm saying that the former option
>is less portable than the latter.  The reason is that "boost/stdint.h"
>tries to add definitions to namespace std, which is reserved to the
>implementation.

This much I understood.

>If you do switch to boost/cstdint.hpp, you need to be aware of
>the std -> boost namespace change for the int_fastXX_t (or uintXX_t
>or whatever) types.  So the places in the source code where you
>use these typedefs need syntactic adaptations to account for
>the namespace change.

The types were used in only one place, and I didn't qualify the namespace at 
all (an error if they are part of std::, but my compiler didn't complain, go 
figure).  Since they are used inside of a boost:: namespace I shouldn't have 
to qualify them, should I?

>If I'm still talking non-intelligibly, holler (and please state
>which of the two paragraphs above need further detailing).

It wasn't you, it was me.  I've got a pretty good grasp of what you were 
trying to tell me now.

>[re-ordered]
>> >  - If you need to add "extern C" to some function definitions, do
>>make
>> >sure to give those implementation-detail functions "static" linkage.
>> >An unnamed namespace won't cut it, because the namespace is not
>> >coded in the function name for "extern C" functions.
>>
>>I need some help with all of this.
>
>Let me start with a bit of explanation:  The pthread functions are
>defined in POSIX, which specifies a C API.  The implementation in the
>pthread library expects the callbacks to be callable using the C
>calling conventions on the machine.  The C++ calling conventions
>could be radically different from those.  Thus, we need to make sure
>that all function pointers passed to pthread functions can be called
>with the C calling convention, by explicitly declaring them as
>"extern C".

Oh, I understood this, and the same is true when using the Win32 API.  I 
missed this only because my compiler didn't care that I used differing 
linkage specifications for these callbacks.

>(This same issue might or might not arise on Windows, and the Windows
>code should be hand-checked for the issue, because Windows compilers
>might not show appropriate errors.)

It matters in that the APIs are still C APIs and thus technically subject to 
the same standard requirements.  However, on Win32 the only difference 
between the two linkage types (AFAIK) is the name mangling that occurs, 
which doesn't make any difference in this case.  I don't know if other 
compilers would issue errors or warnings on this, but VC++ does not.  This 
does not, however, make it right to avoid this issue.

>>--- In boost_at_y..., Jens Maurer <Jens.Maurer_at_g...> wrote:
>> >  - Something needs to be declared "extern C", or a wrapper added:
>> >"once.cpp", line 45: error: argument of type "void (*)()" is
>>incompatible with
>> >           parameter of type "void (*)() C"
>> >         pthread_once(&flag, func);
>
>While developing a work-around, I wondered why call_once() doesn't
>take a function object?  (This is more a request for adding rationale
>than for changing the implementation.)

Because you'd have a race condition.  You have to construct a function 
object, and if you could do that in a thread safe manner there'd be no need 
for call_once() ;).

>I think this patch (nearly) does what you want.  It uses a similar
>trick than the Windows variant, but doesn't spread the mutex >contention.

[snip patch]

An interesting solution.  I can clean this up some.  There's no need to use 
pthread_once to init the mutex since POSIX defines static initialization of 
mutexes.  Thanks for the idea.

>> >"thread.cpp", line 103: error: argument of type "void *(*)(void *)"
>>is
>> >           incompatible with parameter of type "void *(*)(void *) C"
>> >       int res = pthread_create(&m_thread, 0, &thread_proxy, &param);
>> >                                              ^
>
>This looks easy to fix: Just add "extern C { static ... }" at the
>definition of thread_proxy().

It was.  I think I'm misreading the standard here, causing confusion for 
everyone.

>> >"tss.cpp", line 134: error: argument of type "void (*)(void *)" is
>> >           incompatible with parameter of type "void (*)(void *) C"
>> >       int res = pthread_key_create(&m_key, cleanup);
>> >                                            ^
>
>Why doesn't "tss" take a function object?

Because it doesn't need to.  The tss type is just an implementation detail 
used by thread_specific_storage(), so there's no benefit to using a function 
object (unless you see a way to make such an object solve this particular 
problem, but I see such an object as having the exact same problems as 
thread_specific_ptr so if you can solve it there I should be able to solve 
it for thread_specific_ptr instead).

>>Third, boost::thread_specific_ptr<> poses the most difficulty for me.  The
>>cleanup routine must be a templated routine, which AFAICT can't be given C
>>linkage.
>
>I'm looking at threadcorrect1.zip, and the tss class is not a template.

No, but it's also in the detail namespace and is used only by 
thread_specific_ptr<>, which *IS* a template.

>I'm confused.
>
>(The version of tss.cpp I'm looking at can be fixed by an additional
>level of indirection around the "cleanup" pointer, I believe.)

Probably, but I can't see a clean method of doing so.  Probably just 
"writers block" but everything I can come up with uses dynamic memory and 
requires a lot more overhead for all operations.  If I have to go that route 
I guess I have to, but I was hoping there was a solution I wasn't thinking 
of.

>As a general remark, when throwing thread_resource_error(), it would
>be nice to pass an explanatory string *what* system function failed,
>and possibly with what system error code.

Not a bad idea.  Since this won't change the interface I won't make any 
changes in this regard until after I have the rest of the code stable.

Bill Kempf

_________________________________________________________________
Get your FREE download of MSN Explorer at http://explorer.msn.com/intl.asp