From d7e1cf3eb0a5616634391eeb0d721fd6444e59dc Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Mon, 11 Feb 2019 05:23:29 -0500 Subject: [PATCH 1/6] Problem: std::condition_variable can only be used with std::unique_lock, leading to two mutexes used in condition_variable_t Solution: use std::condition_variable_any instead --- src/condition_variable.hpp | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/condition_variable.hpp b/src/condition_variable.hpp index e5c72243..16502b94 100644 --- a/src/condition_variable.hpp +++ b/src/condition_variable.hpp @@ -83,7 +83,6 @@ class condition_variable_t #if _SUPPORT_CONDITION_VARIABLE || defined(ZMQ_HAVE_WINDOWS_TARGET_XP) #include -#include #endif namespace zmq @@ -132,32 +131,28 @@ class condition_variable_t inline int wait (mutex_t *mutex_, int timeout_) { - std::unique_lock lck (_mtx); // lock mtx - mutex_->unlock (); // unlock mutex_ + // this assumes that the mutex mutex_ has been locked by the caller int res = 0; if (timeout_ == -1) { _cv.wait ( - lck); // unlock mtx and wait cv.notify_all(), lock mtx after cv.notify_all() - } else if (_cv.wait_for (lck, std::chrono::milliseconds (timeout_)) + *mutex_); // unlock mtx and wait cv.notify_all(), lock mtx after cv.notify_all() + } else if (_cv.wait_for (*mutex_, std::chrono::milliseconds (timeout_)) == std::cv_status::timeout) { // time expired errno = EAGAIN; res = -1; } - lck.unlock (); // unlock mtx - mutex_->lock (); // lock mutex_ return res; } inline void broadcast () { - std::unique_lock lck (_mtx); // lock mtx + // this assumes that the mutex associated with _cv has been locked by the caller _cv.notify_all (); } private: - std::condition_variable _cv; - std::mutex _mtx; + std::condition_variable_any _cv; // Disable copy construction and assignment. condition_variable_t (const condition_variable_t &); From bfb092c3ecee53ecd4dd6a0b509564dfa997254e Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Mon, 11 Feb 2019 05:35:24 -0500 Subject: [PATCH 2/6] Problem: value for _WIN32_WINNT not always known in CMakeLists.txt Solution: calculate from CMAKE_SYSTEM_VERSION Problem: CMAKE_SYSTEM_VERSION might be newer than Windows SDK Version Solution: limit _WIN32_WINNT value to Visual Studio default Windows SDK version --- CMakeLists.txt | 44 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6f305c75..a15697c0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -264,9 +264,51 @@ if(NOT CYGWIN) check_include_files(windows.h ZMQ_HAVE_WINDOWS) endif() +if (WIN32) + # from https://stackoverflow.com/a/40217291/2019765 + macro(get_WIN32_WINNT version) + if (CMAKE_SYSTEM_VERSION) + set(ver ${CMAKE_SYSTEM_VERSION}) + string(REGEX MATCH "^([0-9]+).([0-9])" ver ${ver}) + string(REGEX MATCH "^([0-9]+)" verMajor ${ver}) + # Check for Windows 10, b/c we'll need to convert to hex 'A'. + if ("${verMajor}" MATCHES "10") + set(verMajor "A") + string(REGEX REPLACE "^([0-9]+)" ${verMajor} ver ${ver}) + endif ("${verMajor}" MATCHES "10") + # Remove all remaining '.' characters. + string(REPLACE "." "" ver ${ver}) + # Prepend each digit with a zero. + string(REGEX REPLACE "([0-9A-Z])" "0\\1" ver ${ver}) + set(${version} "0x${ver}") + endif(CMAKE_SYSTEM_VERSION) + endmacro(get_WIN32_WINNT) + + get_WIN32_WINNT(ZMQ_WIN32_WINNT_DEFAULT) + message(STATUS "Detected _WIN32_WINNT from CMAKE_SYSTEM_VERSION: ${ZMQ_WIN32_WINNT_DEFAULT}") + + # TODO limit _WIN32_WINNT to the actual Windows SDK version, which might be different from the default version installed with Visual Studio + if(MSVC_VERSION STREQUAL "1500" AND CMAKE_SYSTEM_VERSION VERSION_GREATER "6.0") + set(ZMQ_WIN32_WINNT_LIMIT "0x0600") + elseif(MSVC_VERSION STREQUAL "1600" AND CMAKE_SYSTEM_VERSION VERSION_GREATER "6.1") + set(ZMQ_WIN32_WINNT_LIMIT "0x0601") + elseif(MSVC_VERSION STREQUAL "1700" AND CMAKE_SYSTEM_VERSION VERSION_GREATER "6.1") + set(ZMQ_WIN32_WINNT_LIMIT "0x0601") + elseif(MSVC_VERSION STREQUAL "1800" AND CMAKE_SYSTEM_VERSION VERSION_GREATER "6.2") + set(ZMQ_WIN32_WINNT_LIMIT "0x0602") + endif() + if(ZMQ_WIN32_WINNT_LIMIT) + message(STATUS "Mismatch of Visual Studio Version (${MSVC_VERSION}) and CMAKE_SYSTEM_VERSION (${CMAKE_SYSTEM_VERSION}), limiting _WIN32_WINNT to ${ZMQ_WIN32_WINNT_LIMIT}, you may override this by setting ZMQ_WIN32_WINNT") + set(ZMQ_WIN32_WINNT_DEFAULT "${ZMQ_WIN32_WINNT_LIMIT}") + endif() + + set(ZMQ_WIN32_WINNT "${ZMQ_WIN32_WINNT_DEFAULT}" CACHE STRING "Value to set _WIN32_WINNT to for building [default=autodetect from build environment]") + + add_definitions(-D_WIN32_WINNT=${ZMQ_WIN32_WINNT}) +endif(WIN32) + if(CMAKE_SYSTEM_NAME STREQUAL "WindowsStore" AND CMAKE_SYSTEM_VERSION STREQUAL "10.0") set(ZMQ_HAVE_WINDOWS_UWP ON) - add_definitions(-D_WIN32_WINNT=_WIN32_WINNT_WIN10) endif() if(NOT MSVC) check_include_files(ifaddrs.h ZMQ_HAVE_IFADDRS) From 120edd980969dce4624c72476d33f867efdee97f Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Mon, 11 Feb 2019 07:05:04 -0500 Subject: [PATCH 3/6] Problem: selection of condition_variable_t implementation is confusing and not configurable Solution: move configuration to build definition --- CMakeLists.txt | 38 +++++++++++++++++++++++++++++ acinclude.m4 | 41 +++++++++++++++++++++++++++++++ builds/cmake/platform.hpp.in | 5 ++++ configure.ac | 3 +++ src/condition_variable.hpp | 47 +++++++++++------------------------- 5 files changed, 101 insertions(+), 33 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a15697c0..a631d50f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -250,6 +250,7 @@ include(CheckCSourceRuns) include(CMakeDependentOption) include(CheckCXXSymbolExists) include(CheckSymbolExists) +include(FindThreads) execute_process(COMMAND getconf LEVEL1_DCACHE_LINESIZE OUTPUT_VARIABLE CACHELINE_SIZE ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE) if(CACHELINE_SIZE STREQUAL "" OR CACHELINE_SIZE EQUAL 0 OR CACHELINE_SIZE EQUAL -1) @@ -307,6 +308,43 @@ if (WIN32) add_definitions(-D_WIN32_WINNT=${ZMQ_WIN32_WINNT}) endif(WIN32) +###################### BEGIN condition_variable_t selection +if(NOT ZMQ_CV_IMPL) + # prefer C++11 STL std::condition_variable implementation, if available + check_include_files(condition_variable ZMQ_HAVE_STL_CONDITION_VARIABLE LANGUAGE CXX) + + if (ZMQ_HAVE_STL_CONDITION_VARIABLE) + set(ZMQ_CV_IMPL_DEFAULT "stl11") + else() + if(WIN32 AND NOT CMAKE_SYSTEM_VERSION VERSION_LESS "6.0") + # Win32API CONDITION_VARIABLE is supported from Windows Vista only + set(ZMQ_CV_IMPL_DEFAULT "win32api") + elseif(CMAKE_USE_PTHREADS_INIT) + set(ZMQ_CV_IMPL_DEFAULT "pthreads") + else() + set(ZMQ_CV_IMPL_DEFAULT "none") + endif() + endif() + + # TODO a vxworks implementation also exists, but vxworks is not currently supported with cmake at all + set(ZMQ_CV_IMPL "${ZMQ_CV_IMPL_DEFAULT}" CACHE STRING "Choose condition_variable_t implementation. Valid values are + stl11, win32api, pthreads, none [default=autodetect]") +endif() + +message(STATUS "Using condition_variable_t implementation: ${ZMQ_CV_IMPL}") +if(ZMQ_CV_IMPL STREQUAL "stl11") + set(ZMQ_USE_CV_IMPL_STL11 1) +elseif(ZMQ_CV_IMPL STREQUAL "win32api") + set(ZMQ_USE_CV_IMPL_WIN32API 1) +elseif(ZMQ_CV_IMPL STREQUAL "pthreads") + set(ZMQ_USE_CV_IMPL_PTHREADS 1) +elseif(ZMQ_CV_IMPL STREQUAL "none") + set(ZMQ_USE_CV_IMPL_NONE 1) +else() + message(ERROR "Unknown value for ZMQ_CV_IMPL: ${ZMQ_CV_IMPL}") +endif() +###################### END condition_variable_t selection + if(CMAKE_SYSTEM_NAME STREQUAL "WindowsStore" AND CMAKE_SYSTEM_VERSION STREQUAL "10.0") set(ZMQ_HAVE_WINDOWS_UWP ON) endif() diff --git a/acinclude.m4 b/acinclude.m4 index 8e3f4d2a..733b289b 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -1189,3 +1189,44 @@ AC_DEFUN([LIBZMQ_CHECK_CACHELINE], [{ AC_MSG_NOTICE([Using "$zmq_cacheline_size" bytes alignment for lock-free data structures]) AC_DEFINE_UNQUOTED(ZMQ_CACHELINE_SIZE, $zmq_cacheline_size, [Using "$zmq_cacheline_size" bytes alignment for lock-free data structures]) }]) + +dnl ################################################################################ +dnl # LIBZMQ_CHECK_CV_IMPL([action-if-found], [action-if-not-found]) # +dnl # Choose condition variable implementation # +dnl ################################################################################ + +AC_DEFUN([LIBZMQ_CHECK_CV_IMPL], [{ + # Allow user to override condition variable autodetection + AC_ARG_WITH([cv-impl], + [AS_HELP_STRING([--with-cv-impl], + [choose condition variable implementation manually. Valid values are 'stl11', 'pthread', 'none', or 'auto'. [default=auto]])]) + + if test "x$with_cv_impl" == "x"; then + cv_impl=auto + else + cv_impl=$with_cv_impl + fi + case $host_os in + vxworks*) + cv_impl="vxworks" + AC_DEFINE(ZMQ_USE_CV_IMPL_VXWORKS, 1, [Use vxworks condition variable implementation.]) + ;; + esac + if test "$cv_impl" == "auto" || test "$cv_impl" == "stl11"; then + AC_LANG_PUSH([C++]) + AC_CHECK_HEADERS(condition_variable, [stl11="yes" + AC_DEFINE(ZMQ_USE_CV_IMPL_STL11, 1, [Use stl11 condition variable implementation.])], + [stl11="no"]) + AC_LANG_POP([C++]) + if test "$cv_impl" == "stl11" && test "x$stl11" == "xno"; then + AC_MSG_ERROR([--with-cv-impl set to stl11 but cannot find condition_variable]) + fi + fi + if test "$cv_impl" == "pthread" || test "x$stl11" == "xno"; then + AC_DEFINE(ZMQ_USE_CV_IMPL_PTHREADS, 1, [Use pthread condition variable implementation.]) + fi + if test "$cv_impl" == "none"; then + AC_DEFINE(ZMQ_USE_CV_IMPL_NONE, 1, [Use no condition variable implementation.]) + fi + AC_MSG_NOTICE([Using "$cv_impl" condition variable implementation.]) +}]) diff --git a/builds/cmake/platform.hpp.in b/builds/cmake/platform.hpp.in index 21a848fd..0a4740f0 100644 --- a/builds/cmake/platform.hpp.in +++ b/builds/cmake/platform.hpp.in @@ -1,6 +1,11 @@ #ifndef __ZMQ_PLATFORM_HPP_INCLUDED__ #define __ZMQ_PLATFORM_HPP_INCLUDED__ +#cmakedefine ZMQ_USE_CV_IMPL_STL11 +#cmakedefine ZMQ_USE_CV_IMPL_WIN32API +#cmakedefine ZMQ_USE_CV_IMPL_PTHREADS +#cmakedefine ZMQ_USE_CV_IMPL_NONE + #cmakedefine ZMQ_IOTHREAD_POLLER_USE_KQUEUE #cmakedefine ZMQ_IOTHREAD_POLLER_USE_EPOLL #cmakedefine ZMQ_IOTHREAD_POLLER_USE_EPOLL_CLOEXEC diff --git a/configure.ac b/configure.ac index 90c042ec..6bb98202 100644 --- a/configure.ac +++ b/configure.ac @@ -381,6 +381,9 @@ LIBZMQ_CHECK_POLLER # Check cacheline size, set appropriate macro in src/platform.hpp LIBZMQ_CHECK_CACHELINE +# Check condition variable implementation, set appropriate macro in src/platform.hpp +LIBZMQ_CHECK_CV_IMPL + # Checks for header files. AC_HEADER_STDC AC_CHECK_HEADERS(\ diff --git a/src/condition_variable.hpp b/src/condition_variable.hpp index 16502b94..6c0a5d86 100644 --- a/src/condition_variable.hpp +++ b/src/condition_variable.hpp @@ -35,25 +35,7 @@ // Condition variable class encapsulates OS mutex in a platform-independent way. -#ifdef ZMQ_HAVE_WINDOWS - -#include "windows.hpp" -#if defined(_MSC_VER) -#if _MSC_VER >= 1800 -#define _SUPPORT_CONDITION_VARIABLE 1 -#else -#define _SUPPORT_CONDITION_VARIABLE 0 -#endif -#else -#if _cplusplus >= 201103L -#define _SUPPORT_CONDITION_VARIABLE 1 -#else -#define _SUPPORT_CONDITION_VARIABLE 0 -#endif -#endif - -// Condition variable is supported from Windows Vista only, to use condition variable define _WIN32_WINNT to 0x0600 -#if _WIN32_WINNT < 0x0600 && !_SUPPORT_CONDITION_VARIABLE +#if defined(ZMQ_USE_CV_IMPL_NONE) namespace zmq { @@ -79,16 +61,12 @@ class condition_variable_t }; } -#else +#elif defined(ZMQ_USE_CV_IMPL_WIN32API) -#if _SUPPORT_CONDITION_VARIABLE || defined(ZMQ_HAVE_WINDOWS_TARGET_XP) -#include -#endif +#include "windows.hpp" namespace zmq { - -#if !defined(ZMQ_HAVE_WINDOWS_TARGET_XP) && _WIN32_WINNT >= 0x0600 class condition_variable_t { public: @@ -121,7 +99,14 @@ class condition_variable_t condition_variable_t (const condition_variable_t &); void operator= (const condition_variable_t &); }; -#else +} + +#elif defined(ZMQ_USE_CV_IMPL_STL11) + +#include + +namespace zmq +{ class condition_variable_t { public: @@ -158,13 +143,9 @@ class condition_variable_t condition_variable_t (const condition_variable_t &); void operator= (const condition_variable_t &); }; - -#endif } -#endif - -#elif defined ZMQ_HAVE_VXWORKS +#elif defined(ZMQ_USE_CV_IMPL_VXWORKS) #include @@ -248,7 +229,8 @@ class condition_variable_t const condition_variable_t &operator= (const condition_variable_t &); }; } -#else + +#elif defined(ZMQ_USE_CV_IMPL_PTHREADS) #include @@ -344,5 +326,4 @@ class condition_variable_t #endif - #endif From 21a389ca787933c8bf0223e8480bf27910b1d2d4 Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Mon, 11 Feb 2019 07:24:56 -0500 Subject: [PATCH 4/6] Problem: test_security_curve build fails with Windows targeting 8 or newer due to duplicate definition of htonll Solution: use custom implementation only on older Windows versions --- tests/test_security_curve.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/test_security_curve.cpp b/tests/test_security_curve.cpp index a5d6b4f4..5373462c 100644 --- a/tests/test_security_curve.cpp +++ b/tests/test_security_curve.cpp @@ -326,8 +326,8 @@ zmq::curve_client_tools_t make_curve_client_tools () valid_server_public_decoded); } -#ifndef htonll -uint64_t htonll (uint64_t value_) +// same as htonll, which is only available on few platforms (recent Windows, but not on Linux, e.g.( +static uint64_t host_to_network (uint64_t value_) { // The answer is 42 static const int num = 42; @@ -343,7 +343,6 @@ uint64_t htonll (uint64_t value_) return value_; } } -#endif template void send_command (fd_t s_, char (&command_)[N]) { @@ -353,7 +352,7 @@ template void send_command (fd_t s_, char (&command_)[N]) send_all (s_, &len, 1); } else { send (s_, "\x06"); - uint64_t len = htonll (N); + uint64_t len = host_to_network (N); send_all (s_, (char *) &len, 8); } send_all (s_, command_, N); From 2759f459dfb7eca9f7b7ead296fe0da2d3e97ca6 Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Mon, 11 Feb 2019 09:30:37 -0500 Subject: [PATCH 5/6] Problem: C4267 warnings due to implicit conversion from size_t to int Solution: change variable type to size_t --- src/tcp_address.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tcp_address.cpp b/src/tcp_address.cpp index 01b09bf4..b1e23d87 100644 --- a/src/tcp_address.cpp +++ b/src/tcp_address.cpp @@ -124,7 +124,7 @@ static std::string make_address_string (const char *hbuf, char *pos = buf; memcpy (pos, ipv6_prefix, sizeof ipv6_prefix - 1); pos += sizeof ipv6_prefix - 1; - const int hbuf_len = strlen (hbuf); + const size_t hbuf_len = strlen (hbuf); memcpy (pos, hbuf, hbuf_len); pos += hbuf_len; memcpy (pos, ipv6_suffix, sizeof ipv6_suffix - 1); @@ -296,7 +296,7 @@ int zmq::tcp_address_mask_t::to_string (std::string &addr_) const memcpy (pos, ipv6_prefix, sizeof ipv6_prefix - 1); pos += sizeof ipv6_prefix - 1; } - const int hbuf_len = strlen (hbuf); + const size_t hbuf_len = strlen (hbuf); memcpy (pos, hbuf, hbuf_len); pos += hbuf_len; if (_network_address.family () == AF_INET6) { From 7fbd97718457adeaabe904dfea38a2b18d445165 Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Mon, 11 Feb 2019 11:23:48 -0500 Subject: [PATCH 6/6] Problem: assertion triggered in stream_connecter_base::close Solution: change into regular control flow condition --- src/stream_connecter_base.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/stream_connecter_base.cpp b/src/stream_connecter_base.cpp index bf7db348..f54ad195 100644 --- a/src/stream_connecter_base.cpp +++ b/src/stream_connecter_base.cpp @@ -130,17 +130,19 @@ void zmq::stream_connecter_base_t::rm_handle () void zmq::stream_connecter_base_t::close () { - zmq_assert (_s != retired_fd); + // TODO before, this was an assertion for _s != retired_fd, but this does not match usage of close + if (_s != retired_fd) { #ifdef ZMQ_HAVE_WINDOWS - const int rc = closesocket (_s); - wsa_assert (rc != SOCKET_ERROR); + const int rc = closesocket (_s); + wsa_assert (rc != SOCKET_ERROR); #else - const int rc = ::close (_s); - errno_assert (rc == 0); + const int rc = ::close (_s); + errno_assert (rc == 0); #endif - _socket->event_closed (make_unconnected_connect_endpoint_pair (_endpoint), - _s); - _s = retired_fd; + _socket->event_closed ( + make_unconnected_connect_endpoint_pair (_endpoint), _s); + _s = retired_fd; + } } void zmq::stream_connecter_base_t::in_event ()