boostorg / thread

Boost.org thread module
http://boost.org/libs/thread
199 stars 161 forks source link

Errors under g++/Cygwin #228

Closed pdimov closed 5 years ago

pdimov commented 6 years ago

I'm running the tests for Boost.Pool under g++/Cygwin, and this is what I get:

In file included from ..\../boost/thread/win32/recursive_mutex.hpp:13:0,
                 from ..\../boost/thread/recursive_mutex.hpp:14,
                 from ..\../boost/thread.hpp:18,
                 from test\test_threading.cpp:10:
..\../boost/thread/win32/basic_recursive_mutex.hpp: In member function 'bool boo
st::detail::basic_recursive_mutex_impl<underlying_mutex_type>::timed_lock(const
Duration&)':
..\../boost/thread/win32/basic_recursive_mutex.hpp:70:61: error: 'boost::detail:
:winapi' has not been declared
                 long const current_thread_id=boost::detail::winapi::GetCurrentT
hreadId();
                                                             ^~~~~~
..\../boost/thread/win32/basic_recursive_mutex.hpp: In member function 'bool boo
st::detail::basic_recursive_mutex_impl<underlying_mutex_type>::try_lock_for(cons
t boost::chrono::duration<Rep2, Period2>&)':
..\../boost/thread/win32/basic_recursive_mutex.hpp:79:61: error: 'boost::detail:
:winapi' has not been declared
                 long const current_thread_id=boost::detail::winapi::GetCurrentT
hreadId();
                                                             ^~~~~~
..\../boost/thread/win32/basic_recursive_mutex.hpp: In member function 'bool boo
st::detail::basic_recursive_mutex_impl<underlying_mutex_type>::try_lock_until(co
nst boost::chrono::time_point<Clock, Duration>&)':
..\../boost/thread/win32/basic_recursive_mutex.hpp:85:61: error: 'boost::detail:
:winapi' has not been declared
                 long const current_thread_id=boost::detail::winapi::GetCurrentT
hreadId();
                                                             ^~~~~~

This should presumably be boost::winapi?

Also, there's this:

..\..\libs\thread\src\win32\thread.cpp: In function 'void boost::{anonymous}::cr
eate_current_thread_tls_key()':
..\..\libs\thread\src\win32\thread.cpp:86:13: error: 'tss_cleanup_implemented' w
as not declared in this scope
             tss_cleanup_implemented(); // if anyone uses TSS, we need the clean
up linked in
             ^~~~~~~~~~~~~~~~~~~~~~~

Additionally, with cxxstd=03 I get these:

In file included from ..\../boost/thread/detail/config.hpp:14:0,
                 from ..\../boost/thread/win32/thread_data.hpp:9,
                 from ..\../boost/thread/thread_only.hpp:15,
                 from ..\..\libs\thread\src\win32\thread.cpp:11:
..\../boost/thread/detail/thread_safety.hpp:26:38: warning: anonymous variadic m
acros were introduced in C++11 [-Wvariadic-macros]
 #define BOOST_THREAD_ACQUIRED_BEFORE(...) \
                                      ^~~
..\../boost/thread/detail/thread_safety.hpp:29:37: warning: anonymous variadic m
acros were introduced in C++11 [-Wvariadic-macros]
 #define BOOST_THREAD_ACQUIRED_AFTER(...) \
                                     ^~~

...

In file included from ..\../boost/thread/win32/condition_variable.hpp:19:0,
                 from ..\../boost/thread/condition_variable.hpp:14,
                 from ..\../boost/thread/thread_only.hpp:26,
                 from ..\..\libs\thread\src\win32\thread.cpp:11:
..\../boost/thread/lock_guard.hpp:65:40: warning: invoking macro BOOST_THREAD_RE
LEASE argument 1: empty macro arguments are undefined in ISO C++98 [-Wpedantic]
     ~lock_guard() BOOST_THREAD_RELEASE()
                                        ^
viboes commented 6 years ago

Hi Peter,

The variadic and empty macros come from a PR I accepted long time ago. Do you know how to silent those warnings at the source level. Patch welcome.

For winapi, Andrei did a move to boost::winapi. Maybe this is still missing. Andrei, maybe you could help.

For tss_cleanup_implementedI never understood that.

I don't have time to look at this just now.

Individual patches to fix any of this are welcome.

viboes commented 5 years ago

The warning should be fixed now (using system header).

For the tss_cleanup_implemented I have no idea.

Any PR will be more than welcome

pdimov commented 5 years ago

The tss_cleanup_implemented error seems to have two causes. First, under Cygwin, threadapi=win32 is chosen, whereas it should be pthread (target-os is cygwin, not windows.) I haven't been able to track down why it's chosen, will open an issue in Build.

Second, tss_hooks.hpp only declares tss_cleanup_implemented when BOOST_HAS_WINTHREADS is defined, but the win32 sources are included on the basis of threadapi, which in this case does not correspond to whether BOOST_HAS_WINTHREADS is defined. The Jamfile defines BOOST_THREAD_WIN32 on threadapi=win32, which is what presumably is the correct macro here.

viboes commented 5 years ago

I don't know why, but Boost.Thread decided that under Cygwin the thread api is windows and not posix :( Maybe Anthony remember why. I believe it was because it can and more services and more efficient are available.

Note that for Boost.Chrono, this is not the case, and I was asking to change this when Anthony maintained Boost.Thread.

viboes commented 5 years ago

Peter, do you plan to provide a patch for this issue after #352 patch?

pdimov commented 5 years ago

After https://github.com/boostorg/build/issues/352 is resolved, Thread should build on Cygwin as threadapi=pthread will be default.

Making it build with threadapi=win32 on Cygwin might also be possible, but I'm not sure if it'll be worth the effort.

pdimov commented 5 years ago

After https://github.com/boostorg/build/commit/a5704a926bc7bc69311d472bb97f7a80c53d017a, Thread's tests now seem to work for me with toolset=gcc and Cygwin.

viboes commented 5 years ago

Great. Glad to see that it works again, after so many years.

Peter, is there something to fix for this ticket yet? If no, maybe you can close it.

pdimov commented 5 years ago

We can also fix the threadapi=win32 case under Cygwin, see #244. With that patch, there's only one failure, test_tss_lib says there's no TSS cleanup in the static lib case, which there indeed isn't.

If you want to support Cygwin going forward, it might be worth adding it to Appveyor.

Apart from that, it looks like we're done here.

viboes commented 5 years ago

Do you have an example of setup for Appveyor? or even a PR would be better :)

pdimov commented 5 years ago

Boost.System has Cygwin and Cygwin 64 configurations: https://github.com/boostorg/system/blob/develop/appveyor.yml#L26

viboes commented 5 years ago

Thanks