BOOST_HAS_PTHREADS, BOOST_HAS_SCHED_YIELD tests in config_test not strict enough
I'm looking at the results of some new tests I added to smart_ptr: spinlock_try_test, yield_k_test, and I'm seeing failures on platforms that (in single threaded mode, i.e. without <threading>multi) advertise BOOST_HAS_PTHREADS (and BOOST_HAS_SCHED_YIELD) but in reality do one of: 1. provide stub implementations of pthread_mutex_*, failing a pthread_mutex_trylock test; 2. provide stubs and segfault on pthread_mutex_trylock; 3. fail to link because of missing pthread_mutex_trylock; 4. fail to link because of missing sched_yield. This isn't particularly odd or surprising. But I think that config_test ought to fail on these platforms, for the same reason my tests fail. The reason the test passes is because boost_has_pthreads.ipp and boost_has_sched_yield.ipp are too permissive; the first one only tests pthread_mutex_* functions that are stubbed and appear to work, the second doesn't attempt to actually call sched_yield and - presumably - the linker discards the reference. Does this make any sense? :-)
I wrote:
This isn't particularly odd or surprising. But I think that config_test ought to fail on these platforms, for the same reason my tests fail. The reason the test passes is because boost_has_pthreads.ipp and boost_has_sched_yield.ipp are too permissive;
The tests are indeed too permissive... but the real reason is that config_test is always run with <threading>multi, so the default configuration is never tested. Is this correct? Most of our other tests do not use <threading>multi, so it seems reasonable to run config_test in this mode as well (in addition to the existing <threading>multi run).
Peter Dimov wrote:
I wrote:
This isn't particularly odd or surprising. But I think that config_test ought to fail on these platforms, for the same reason my tests fail. The reason the test passes is because boost_has_pthreads.ipp and boost_has_sched_yield.ipp are too permissive;
The tests are indeed too permissive... but the real reason is that config_test is always run with <threading>multi, so the default configuration is never tested. Is this correct? Most of our other tests do not use <threading>multi, so it seems reasonable to run config_test in this mode as well (in addition to the existing <threading>multi run).
Ah, I'll add another build variant for that test. John.
John Maddock:
Peter Dimov wrote:
The tests are indeed too permissive... but the real reason is that config_test is always run with <threading>multi, so the default configuration is never tested. Is this correct? Most of our other tests do not use <threading>multi, so it seems reasonable to run config_test in this mode as well (in addition to the existing <threading>multi run).
Ah, I'll add another build variant for that test.
Can you please add it for config_info as well? Oddly, it seems that currently it's being run without <threading>multi.
Peter Dimov wrote:
John Maddock:
Peter Dimov wrote:
The tests are indeed too permissive... but the real reason is that config_test is always run with <threading>multi, so the default configuration is never tested. Is this correct? Most of our other tests do not use <threading>multi, so it seems reasonable to run config_test in this mode as well (in addition to the existing <threading>multi run).
Ah, I'll add another build variant for that test.
Can you please add it for config_info as well? Oddly, it seems that currently it's being run without <threading>multi.
Done, John.
John Maddock:
Done, John.
Thanks! The tests for sched_yield and nanosleep now fail as expected on g++/Sun. :-) clock_gettime also fails to link across the board. I think that we ought to fix this by adding -lrt even in <threading>single instead of #undef-ing the feature macros, since these calls aren't technically tied to threads. The pthread_mutex_setattr test also fails; this is, I believe, a symptom of missing pthread support that should be indicated by the absence of BOOST_HAS_PTHREADS. The has_pthreads test doesn't fail because it only checks for pthread_mutex_lock/unlock. It should probably attempt at least a pthread_create. (I also need pthread_mutex_trylock availability for the new shared_ptr tests, but it might be covered by the pthread_create test.)
Another observation: the difference between g++/Sun in ST and MT mode: http://www.boost.org/development/tests/trunk/developer/output/Sandia-sun-boo... http://www.boost.org/development/tests/trunk/developer/output/Sandia-sun-boo... is in the _PTHREADS macro (_REENTRANT is defined in both cases, which causes BOOST_HAS_THREADS and BOOST_HAS_PTHREADS to be defined as well even in ST mode.) Taking a Linux g++ at random: http://www.boost.org/development/tests/trunk/developer/output/Sandia-gcc-boo... http://www.boost.org/development/tests/trunk/developer/output/Sandia-gcc-boo... _REENTRANT is correctly only defined in MT mode, but BOOST_HAS_THREADS (and BOOST_HAS_PTHREADS) incorrectly remains defined in both. g++ 4.1 also seems to work the same way, whereas g++ 4.0 and g++ 3.4 do not, they always define _REENTRANT and there doesn't seem to be a way to detect MT mode (at least judging by the output of config_info).
Peter Dimov wrote:
Another observation: the difference between g++/Sun in ST and MT mode:
http://www.boost.org/development/tests/trunk/developer/output/Sandia-sun-boo...
http://www.boost.org/development/tests/trunk/developer/output/Sandia-sun-boo...
is in the _PTHREADS macro (_REENTRANT is defined in both cases, which causes BOOST_HAS_THREADS and BOOST_HAS_PTHREADS to be defined as well even in ST mode.)
OK, I've taken a shot at fixing this, but I don't have Solaris to test on, so <shrug> ! Basically <unistd.h> always advertises the presence of pthreads on Solaris so BOOST_HAS_PTHREADS gets set regardless. I've now added an additional check to solaris.hpp to turn it off for gcc when _PTHREADS is not set. I've also beefed up the pthread test cases: they now create a thread, join and do a try lock as requested.
Taking a Linux g++ at random:
http://www.boost.org/development/tests/trunk/developer/output/Sandia-gcc-boo...
http://www.boost.org/development/tests/trunk/developer/output/Sandia-gcc-boo...
_REENTRANT is correctly only defined in MT mode, but BOOST_HAS_THREADS (and BOOST_HAS_PTHREADS) incorrectly remains defined in both.
g++ 4.1 also seems to work the same way, whereas g++ 4.0 and g++ 3.4 do not, they always define _REENTRANT and there doesn't seem to be a way to detect MT mode (at least judging by the output of config_info).
Yeh, g++ has always been lax about advertising whether it's in thread safe mode or not. Lets see where these changes get us, and then we need to decide what to do on Win32 in strict mode when <windows.h> is unavailable :-) John.
John Maddock:
OK, I've taken a shot at fixing this, but I don't have Solaris to test on, so <shrug> !
Neither do I. :-)
Basically <unistd.h> always advertises the presence of pthreads on Solaris so BOOST_HAS_PTHREADS gets set regardless. I've now added an additional check to solaris.hpp to turn it off for gcc when _PTHREADS is not set.
Yep. But won't this code in suffix.hpp: #if (defined(__MT__) || defined(_MT) || defined(_REENTRANT) \ || defined(_PTHREADS)) && !defined(BOOST_HAS_THREADS) # define BOOST_HAS_THREADS #endif turn it back on? I admit that the task of setting BOOST_HAS_THREADS is pretty hard. But the current practice of distributing the setting into compiler/platform/suffix headers seems too fragile to me. I'm not sure I understand all the subtle details yet though.
Yeh, g++ has always been lax about advertising whether it's in thread safe mode or not.
Right. My point however was that this is no longer true, starting from g++ 4.1. _REENTRANT is now being set properly (and ignored by Boost.Config). (Unfortunately intel-linux still emulates the older g++ practice of always setting _REENTRANT.)
Lets see where these changes get us, and then we need to decide what to do on Win32 in strict mode when <windows.h> is unavailable :-)
I believe that the fact that <windows.h> is unavailable (this can happen in non-strict mode as well under VC++ Express, if the Platform SDK hasn't been installed) should be indicated by a new macro that is documented as such. There's also the general problem of using BOOST_DISABLE_THREADS inside Boost.Config to communicate lack of threading support from one header to another. I don't necessarily view this as unacceptable... as long as Boost.Config is unquestionably right to do so; that is, as long as there is no situation in which the user would want to not define it. Still, the easiest way out is just to use an internal macro for communication and leave BOOST_DISABLE_* as pure user macros.
Peter Dimov wrote:
Yep. But won't this code in suffix.hpp:
#if (defined(__MT__) || defined(_MT) || defined(_REENTRANT) \ || defined(_PTHREADS)) && !defined(BOOST_HAS_THREADS) # define BOOST_HAS_THREADS #endif
turn it back on?
No, well actually yes, but since there is no available threading API, (BOOST_HAS_PTHREADS not set), it'll get turned off again...
I admit that the task of setting BOOST_HAS_THREADS is pretty hard. But the current practice of distributing the setting into compiler/platform/suffix headers seems too fragile to me. I'm not sure I understand all the subtle details yet though.
The trouble is - there ain't no easy way - strictly IMO of course! I accept it's often hard to work out what's setting what macro where - but the alternative is a big bunch of spaggetti code to handle all the platforms/compilers in one go, which isn't necessarily better.. or worse :-(
Yeh, g++ has always been lax about advertising whether it's in thread safe mode or not.
Right.
My point however was that this is no longer true, starting from g++ 4.1. _REENTRANT is now being set properly (and ignored by Boost.Config).
Is it? Threading support on Linux should only be turned on when _REENTRANT is set, there might be issues on other platforms though... I'll have to look into that.
(Unfortunately intel-linux still emulates the older g++ practice of always setting _REENTRANT.)
Lets see where these changes get us, and then we need to decide what to do on Win32 in strict mode when <windows.h> is unavailable :-)
I believe that the fact that <windows.h> is unavailable (this can happen in non-strict mode as well under VC++ Express, if the Platform SDK hasn't been installed) should be indicated by a new macro that is documented as such.
Well we have BOOST_DISABLE_WIN32 which is set when <windows.h> is unavailable - or alternatively optionally by the user.
There's also the general problem of using BOOST_DISABLE_THREADS inside Boost.Config to communicate lack of threading support from one header to another. I don't necessarily view this as unacceptable... as long as Boost.Config is unquestionably right to do so; that is, as long as there is no situation in which the user would want to not define it.
Still, the easiest way out is just to use an internal macro for communication and leave BOOST_DISABLE_* as pure user macros.
If we stop setting BOOST_DISABLE_THREADS when BOOST_DISABLE_WIN32 is set this issue become moot doesn't it? John.
John Maddock
My point however was that this is no longer true, starting from g++ 4.1. _REENTRANT is now being set properly (and ignored by Boost.Config).
Is it?
Yes. See the link I posted previously. http://www.boost.org/development/tests/trunk/developer/output/Sandia-gcc-boo... This is Linux, _REENTRANT is not set, BOOST_HAS_THREADS is set - as far as I can see.
Peter Dimov wrote:
John Maddock
My point however was that this is no longer true, starting from g++ 4.1. _REENTRANT is now being set properly (and ignored by Boost.Config).
Is it?
Yes. See the link I posted previously.
http://www.boost.org/development/tests/trunk/developer/output/Sandia-gcc-boo...
This is Linux, _REENTRANT is not set, BOOST_HAS_THREADS is set - as far as I can see.
Blast, OK I'll look into that. John.
Peter Dimov wrote:
John Maddock
My point however was that this is no longer true, starting from g++ 4.1. _REENTRANT is now being set properly (and ignored by Boost.Config).
Is it?
Yes. See the link I posted previously.
http://www.boost.org/development/tests/trunk/developer/output/Sandia-gcc-boo...
This is Linux, _REENTRANT is not set, BOOST_HAS_THREADS is set - as far as I can see.
Ah, I think I see the issue here, and I'm not sure what the right thing to do is - if libstdc++ is built thread safe and it has threading support, then BOOST_HAS_THREADS gets turned on. We did this for two reasons: 1) Prior to gcc-4.1.2 there's nothing in the compiler defines that let you determine whether it's in multithread mode or not - we opened a gcc bug report on this ages ago, and it claims to be fixed for 4.1.2: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=11953 2) In a sense gcc is either "always thread safe" or "never thread safe" depending on how it and libstdc++ were built. For example on Linux (and most other platforms I believe) -pthread does nothing except pass -lpthread to the linker. So... assuming (1) really has been fixed in gcc-4.1, what's the right thing to do about (2)? There's a school of thought which says that if gcc was built multithreaded then maybe all installed libraries should be as well "just in case". The basic issue I guess is that it may be common to build without -pthread and just add -lpthread to the linker instead? Or to try and mix code built with and without -pthread? Any thoughts...? John.
John Maddock:
Ah, I think I see the issue here, and I'm not sure what the right thing to do is - if libstdc++ is built thread safe and it has threading support, then BOOST_HAS_THREADS gets turned on.
We did this for two reasons:
1) Prior to gcc-4.1.2 there's nothing in the compiler defines that let you determine whether it's in multithread mode or not - we opened a gcc bug report on this ages ago, and it claims to be fixed for 4.1.2: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=11953
2) In a sense gcc is either "always thread safe" or "never thread safe" depending on how it and libstdc++ were built. For example on Linux (and most other platforms I believe) -pthread does nothing except pass -lpthread to the linker.
So... assuming (1) really has been fixed in gcc-4.1, what's the right thing to do about (2)?
Good question. :-/ We have four options for the <threading>single case: 1. !HAS_THREADS, !HAS_PTHREADS 2. HAS_THREADS, !HAS_PTHREADS 3. HAS_THREADS, HAS_PTHREADS 4. HAS_THREADS, HAS_PTHREADS, add -lpthread I really have no idea which of these is best. I can see arguments for each. If .o files compiled with and without -pthread are indeed link compatible, our current choice of (3) seems pretty reasonable. A practical problem I have with (3) is that pthread_mutex_trylock links and crashes. But I'm not sure whether this warrants any changes. Maybe the correct way to deal with it is to mark it up and move on. If we do choose to stay with (3) as our choice, we probably need to revert the has_pthreads test back to the stubs and move the real has_pthreads (and has_pthread_mutexattr_settype) to separate tests outside of config_test.
Peter Dimov wrote:
2) In a sense gcc is either "always thread safe" or "never thread safe" depending on how it and libstdc++ were built. For example on Linux (and most other platforms I believe) -pthread does nothing except pass -lpthread to the linker.
So... assuming (1) really has been fixed in gcc-4.1, what's the right thing to do about (2)?
Good question. :-/
We have four options for the <threading>single case:
1. !HAS_THREADS, !HAS_PTHREADS 2. HAS_THREADS, !HAS_PTHREADS 3. HAS_THREADS, HAS_PTHREADS 4. HAS_THREADS, HAS_PTHREADS, add -lpthread
I really have no idea which of these is best. I can see arguments for each. If .o files compiled with and without -pthread are indeed link compatible, our current choice of (3) seems pretty reasonable.
A quick servey of the GCC manual found these entries for -pthread: -pthread Add support for multithreading using the POSIX threads library. This option sets flags for both the preprocessor and linker. It does not affect the thread safety of object code produced by the compiler or that of libraries supplied with it. These are HP-UX specific flags. -pthread Adds support for multithreading with the pthreads library. This option sets flags for both the preprocessor and linker. -pthreads Add support for multithreading using the POSIX threads library. This option sets flags for both the preprocessor and linker. This option does not affect the thread safety of object code produced by the compiler or that of libraries supplied with it. -pthread This is a synonym for '-pthreads'. So on all platforms that support this, it changes the linker flags and the preprocessor defines but that's it.
A practical problem I have with (3) is that pthread_mutex_trylock links and crashes. But I'm not sure whether this warrants any changes. Maybe the correct way to deal with it is to mark it up and move on.
Do you have a test case?
If we do choose to stay with (3) as our choice, we probably need to revert the has_pthreads test back to the stubs and move the real has_pthreads (and has_pthread_mutexattr_settype) to separate tests outside of config_test.
Sigh :-( Yep. Still not sure what the right answer is yours, John.
John Maddock:
Peter Dimov wrote:
A practical problem I have with (3) is that pthread_mutex_trylock links and crashes. But I'm not sure whether this warrants any changes. Maybe the correct way to deal with it is to mark it up and move on.
Do you have a test case?
Yes, it's the spinlock_try_test on http://www.boost.org/development/tests/trunk/developer/smart_ptr.html Some of the tests results have disappeared though, and I no longer see crashes (exit code 139, which is I believe a SIGSEGV). (All the intel-linux results are gone, for example.) I only see one test failure due to a stubbed pthread_mutex_trylock and one link failure due to a missing trylock. The Sun failures will hopefully clear as soon as you commit the -lrt fix. (I only have access to Windows at the moment, and can't test it, sorry. I suggest you go ahead with it; I'll watch the table for breakages :-)
John Maddock:
Lets see where these changes get us, and then we need to decide what to do on Win32 in strict mode when <windows.h> is unavailable :-)
I believe that the fact that <windows.h> is unavailable (this can happen in non-strict mode as well under VC++ Express, if the Platform SDK hasn't been installed) should be indicated by a new macro that is documented as such.
Well we have BOOST_DISABLE_WIN32 which is set when <windows.h> is unavailable - or alternatively optionally by the user.
I don't like the fact that the macro names do not reflect their actual meaning. This inevitably leads to their being set in situations where they shouldn't be, and vice versa. A macro that indicates the absence of windows.h should be called BOOST_NO_WINDOWS_H, and the reverse should be BOOST_HAS_WINDOWS_H. A user macro that overrides the detection should be called BOOST_DISABLE_WINDOWS_H. In general, we should follow a consistent naming scheme. It's obvious when this macro should and should not be set, and it's obvious when it needs to be tested - when the code decides whether to #include <windows.h> or not. The problem with making the macro clear and unambiguous in this manner is that it exposes that libraries - in reality - have no use for it. :-)
Still, the easiest way out is just to use an internal macro for communication and leave BOOST_DISABLE_* as pure user macros.
If we stop setting BOOST_DISABLE_THREADS when BOOST_DISABLE_WIN32 is set this issue become moot doesn't it?
There are other places where BOOST_DISABLE_THREADS is being set by Boost.Config. The problem is that I can't really know whether they are correct or not - from user point of view. The good thing about testing a user macro is that one is 100% certain that the user means what he says; the library better do what it's being told.
Peter Dimov wrote:
John Maddock:
Lets see where these changes get us, and then we need to decide what to do on Win32 in strict mode when <windows.h> is unavailable :-)
I believe that the fact that <windows.h> is unavailable (this can happen in non-strict mode as well under VC++ Express, if the Platform SDK hasn't been installed) should be indicated by a new macro that is documented as such.
Well we have BOOST_DISABLE_WIN32 which is set when <windows.h> is unavailable - or alternatively optionally by the user.
I don't like the fact that the macro names do not reflect their actual meaning. This inevitably leads to their being set in situations where they shouldn't be, and vice versa. A macro that indicates the absence of windows.h should be called BOOST_NO_WINDOWS_H, and the reverse should be BOOST_HAS_WINDOWS_H. A user macro that overrides the detection should be called BOOST_DISABLE_WINDOWS_H. In general, we should follow a consistent naming scheme.
You right after I posted that I thought: "Doh! Should have suggested BOOST_HAS_WINDOWS_H".
It's obvious when this macro should and should not be set, and it's obvious when it needs to be tested - when the code decides whether to #include <windows.h> or not.
The problem with making the macro clear and unambiguous in this manner is that it exposes that libraries - in reality - have no use for it. :-)
Still, the easiest way out is just to use an internal macro for communication and leave BOOST_DISABLE_* as pure user macros.
If we stop setting BOOST_DISABLE_THREADS when BOOST_DISABLE_WIN32 is set this issue become moot doesn't it?
There are other places where BOOST_DISABLE_THREADS is being set by Boost.Config. The problem is that I can't really know whether they are correct or not - from user point of view.
The good thing about testing a user macro is that one is 100% certain that the user means what he says; the library better do what it's being told.
Understood, looking through its set in the following cases: 1) In suffix.hpp when BOOST_DISABLE_WIN32 is set.. 2) In amigaos.hpp unconditionally: I assume there is just no (working) threading support here? 3) In hpux.hpp for versions of gcc that can't be built multithreaded there. 4) In irix.hpp for versions of gcc that can't be built multithreaded there. 5) In libstdcpp.hpp if the std lib was built single threaded. My suspision is that all except (5) can be removed - indeed 2,3 and 4 have probably been obsoleted by (5). IMO it doesn't seem unreasonable to set this macro in case (5) regardless of what _REENTRANT and other macros may be saying? John.
John Maddock:
Understood, looking through its set in the following cases:
1) In suffix.hpp when BOOST_DISABLE_WIN32 is set.. 2) In amigaos.hpp unconditionally: I assume there is just no (working) threading support here? 3) In hpux.hpp for versions of gcc that can't be built multithreaded there. 4) In irix.hpp for versions of gcc that can't be built multithreaded there. 5) In libstdcpp.hpp if the std lib was built single threaded.
My suspision is that all except (5) can be removed - indeed 2,3 and 4 have probably been obsoleted by (5).
IMO it doesn't seem unreasonable to set this macro in case (5) regardless of what _REENTRANT and other macros may be saying?
I agree that setting it in case (5) is reasonable. For now I've updated sp_counted_base.hpp as follows: #elif defined( BOOST_DISABLE_THREADS ) && !defined( BOOST_SP_ENABLE_THREADS ) && !defined( BOOST_DISABLE_WIN32 ) # include <boost/detail/sp_counted_base_nt.hpp> When we drop (1) I'll remove the !defined( BOOST_DISABLE_WIN32 ) test. Looking through the uses of BOOST_DISABLE_WIN32 still makes me slightly nervous though. It seems that it's sometimes (ab)used to mean "the compiler is in strict mode". I believe that strict/non-strict modes are link compatible, and indeed, it's common for strict mode users to compile everything in strict mode except the few platform-specific translation units where <windows.h> is included. Testing for BOOST_DISABLE_WIN32 in Boost may well lead to ODR violations in these cases. The same is true for the other typical use where a CRITICAL_SECTION is optimized out when BOOST_DISABLE_WIN32 is detected. (A "safe" CRITICAL_SECTION that I believe works in strict mode is provided by detail/lightweight_mutex.hpp, by the way.)
The tests for sched_yield and nanosleep now fail as expected on g++/Sun. :-) clock_gettime also fails to link across the board.
I think that we ought to fix this by adding -lrt even in <threading>single instead of #undef-ing the feature macros, since these calls aren't technically tied to threads.
How about this one?
Peter Dimov wrote:
The tests for sched_yield and nanosleep now fail as expected on g++/Sun. :-) clock_gettime also fails to link across the board.
I think that we ought to fix this by adding -lrt even in <threading>single instead of #undef-ing the feature macros, since these calls aren't technically tied to threads.
How about this one?
These aren't really threading API's, so I guess they're either present or not right? In which case adding -lrt on platforms that require seems to be the right thing to do, the following patch should do the job, but I don't have a machine handy to test it on, can you check this on Linux at least? Cheers, John. Index: build/v2/tools/gcc.jam =================================================================== --- build/v2/tools/gcc.jam (revision 44056) +++ build/v2/tools/gcc.jam (working copy) @@ -712,7 +712,7 @@ case SunOS* : { flags gcc OPTIONS <threading>multi : -pthreads ; - flags gcc FINDLIBS-SA <threading>multi : rt ; + flags gcc FINDLIBS-SA : rt ; } case BeOS : { @@ -740,7 +740,7 @@ case * : { flags gcc OPTIONS <threading>multi : -pthread ; - flags gcc FINDLIBS-SA <threading>multi : rt ; + flags gcc FINDLIBS-SA : rt ; } } }
Peter Dimov wrote:
I'm looking at the results of some new tests I added to smart_ptr: spinlock_try_test, yield_k_test, and I'm seeing failures on platforms that (in single threaded mode, i.e. without <threading>multi) advertise BOOST_HAS_PTHREADS (and BOOST_HAS_SCHED_YIELD) but in reality do one of:
1. provide stub implementations of pthread_mutex_*, failing a pthread_mutex_trylock test;
2. provide stubs and segfault on pthread_mutex_trylock;
3. fail to link because of missing pthread_mutex_trylock;
4. fail to link because of missing sched_yield.
This isn't particularly odd or surprising. But I think that config_test ought to fail on these platforms, for the same reason my tests fail. The reason the test passes is because boost_has_pthreads.ipp and boost_has_sched_yield.ipp are too permissive; the first one only tests pthread_mutex_* functions that are stubbed and appear to work, the second doesn't attempt to actually call sched_yield and - presumably - the linker discards the reference.
Does this make any sense? :-)
Yes, but the intention was that you check for BOOST_HAS_THREADS before the other macros. John.
participants (2)
-
John Maddock -
Peter Dimov