Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Accidental introduction of pthread when using std::thread #2951

Open
Andarwinux opened this issue May 11, 2024 · 9 comments
Open

[BUG] Accidental introduction of pthread when using std::thread #2951

Andarwinux opened this issue May 11, 2024 · 9 comments
Assignees
Labels
[build] Area: Changes in build files help wanted Indicates that a maintainer wants help on an issue or pull request Type: Bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@Andarwinux
Copy link

Andarwinux commented May 11, 2024

Describe the bug
A clear and concise description of what the bug is.

To Reproduce
Steps to reproduce the behavior:

  1. Configure with llvm-mingw or gcc14 with win32thread
  2. nm libsrt.a|grep pthread
                 U pthread_getname_np
                 U pthread_self
                 U pthread_getname_np
                 U pthread_self
                 U pthread_setname_np

Expected behavior

libc++ and libstdc++ support the win32thread implementation of std::thread, in which case the pthread dependency should not be introduced.

Additional context

Although both llvm-mingw and gcc 14 implement std::thread via win32thread, they still support pthread.
This happens because CMake detects pthread support and then threadname.h introduces pthread because HAVE_PTHREAD_GETNAME_NP is defined.

-- Checking for pthread_(g/s)etname_np in 'pthread_np.h':
-- Looking for pthread_getname_np
-- Looking for pthread_getname_np - not found
-- Looking for pthread_setname_np
-- Looking for pthread_setname_np - not found
-- Checking for pthread_(g/s)etname_np in 'pthread.h':
-- Looking for pthread_getname_np
-- Looking for pthread_getname_np - found
-- Looking for pthread_setname_np
-- Looking for pthread_setname_np - found
-- Checking for module 'openssl libcrypto'
--   Found openssl libcrypto, version 3.4.0-dev
-- SSL via pkg-config: -L /build/install/x86_64-w64-mingw32/lib;/build/install/x86_64-w64-mingw32/lib -I /build/install/x86_64-w64-mingw32/include -l;ssl;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32
-- ENCRYPTION: ENABLED, using: openssl libcrypto
-- SSL libraries: ssl;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32
-- ENCRYPTION AEAD API: DISABLED
-- MAXREXMITBW API: DISABLED
-- COMPILER: Clang (/build/install/bin/x86_64-w64-mingw32-g++) - GNU compat
-- NOTE: CLANG 19.0.0 detected, unsure if >=C++11 is default, forcing C++11 on applications
-- Looking for __atomic_fetch_add_8 in atomic
-- Looking for __atomic_fetch_add_8 in atomic - not found
-- Performing Test HAVE_LIBATOMIC_COMPILES
-- Performing Test HAVE_LIBATOMIC_COMPILES - Failed
-- Performing Test HAVE_GCCATOMIC_INTRINSICS
-- Performing Test HAVE_GCCATOMIC_INTRINSICS - Success
-- Performing Test HAVE_CXX_ATOMIC
-- Performing Test HAVE_CXX_ATOMIC - Success
-- Performing Test HAVE_CXX_ATOMIC_STATIC
-- Performing Test HAVE_CXX_ATOMIC_STATIC - Success
-- Checking for C++ 'std::put_time()':
-- Performing Test HAVE_CXX_STD_PUT_TIME
-- Performing Test HAVE_CXX_STD_PUT_TIME - Success
-- STDCXX_SYNC: ON
@Andarwinux Andarwinux added the Type: Bug Indicates an unexpected problem or unintended behavior label May 11, 2024
@maxsharabayko maxsharabayko added [build] Area: Changes in build files help wanted Indicates that a maintainer wants help on an issue or pull request labels May 13, 2024
@maxsharabayko maxsharabayko added this to the Backlog milestone May 13, 2024
@yomnes0
Copy link
Collaborator

yomnes0 commented May 13, 2024

This might need a rework :

srt/CMakeLists.txt

Lines 883 to 884 in 38a3a16

elseif (MICROSOFT)
find_package(pthreads QUIET)

Using
find_package( Threads )
and
CMAKE_USE_WIN32_THREADS_INIT

should allow a pthread free win32 build

@yomnes0 yomnes0 self-assigned this May 13, 2024
@ethouris
Copy link
Collaborator

See the ThreadName facility. Although as I remember it should be turned off in case when pthreads are not in use. Maybe this slipped off at some point.

@ethouris
Copy link
Collaborator

Ok, I can see more-less what happened. The ENABLE_STDCXX_SYNC is not used at all in the threadname.h header file. Might be that this facility should be provided in a "stub" version in this case, unless some sensible implementation for thread names can be provided. Note that there's no portable way to do this, and this is something that serves development purposes only, so it should normally suffice if this is only supported on Linux. Note also that this WILL work also if you use C++11 threads on Linux, and most likely the C++11 thread implementation is done on top of POSIX, so I think the best approach would be to leave this as is on Linux (even if C++11 threads are requested), but turn this into a "stub version" on all other systems.

@maxsharabayko
Copy link
Collaborator

@Andarwinux could you please check PR #2952?

@Andarwinux
Copy link
Author

Still doesn't seem to solve the problem, unless I add -UHAVE_PTHREAD_GETNAME_NP -UHAVE_PTHREAD_SETNAME_NP to CXXFLAGS.
I suspect that this is where the real problem lies.

-- The C compiler identification is Clang 19.0.0
-- The CXX compiler identification is Clang 19.0.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /build/install/bin/x86_64-w64-mingw32-gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /build/install/bin/x86_64-w64-mingw32-g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found PkgConfig: /usr/sbin/pkgconf (found version "2.1.1")
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - found
-- Found Threads: TRUE
-- BUILD TYPE: Release
-- LOGGING: ENABLED
-- USE_BUSY_WAITING: OFF (default)
-- No WITH_COMPILER_PREFIX - using C++ compiler /build/install/bin/x86_64-w64-mingw32-g++
-- Looking for inet_pton
-- Looking for inet_pton - found
-- Checking for pthread_(g/s)etname_np in 'pthread_np.h':
-- Looking for pthread_getname_np
-- Looking for pthread_getname_np - not found
-- Looking for pthread_setname_np
-- Looking for pthread_setname_np - not found
-- Checking for pthread_(g/s)etname_np in 'pthread.h':
-- Looking for pthread_getname_np
-- Looking for pthread_getname_np - found
-- Looking for pthread_setname_np
-- Looking for pthread_setname_np - found
-- Checking for module 'openssl libcrypto'
--   Found openssl libcrypto, version 3.4.0-dev
-- SSL via pkg-config: -L /build/install/x86_64-w64-mingw32/lib;/build/install/x86_64-w64-mingw32/lib -I /build/install/x86_64-w64-mingw32/include -l;ssl;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32
-- ENCRYPTION: ENABLED, using: openssl libcrypto
-- SSL libraries: ssl;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32
-- ENCRYPTION AEAD API: DISABLED
-- MAXREXMITBW API: DISABLED
-- COMPILER: Clang (/build/install/bin/x86_64-w64-mingw32-g++) - GNU compat
-- NOTE: CLANG 19.0.0 detected, unsure if >=C++11 is default, forcing C++11 on applications
-- Looking for __atomic_fetch_add_8 in atomic
-- Looking for __atomic_fetch_add_8 in atomic - not found
-- Performing Test HAVE_LIBATOMIC_COMPILES
-- Performing Test HAVE_LIBATOMIC_COMPILES - Failed
-- Performing Test HAVE_GCCATOMIC_INTRINSICS
-- Performing Test HAVE_GCCATOMIC_INTRINSICS - Success
-- Performing Test HAVE_CXX_ATOMIC
-- Performing Test HAVE_CXX_ATOMIC - Success
-- Performing Test HAVE_CXX_ATOMIC_STATIC
-- Performing Test HAVE_CXX_ATOMIC_STATIC - Success
-- Checking for C++ 'std::put_time()':
-- Performing Test HAVE_CXX_STD_PUT_TIME
-- Performing Test HAVE_CXX_STD_PUT_TIME - Success
-- STDCXX_SYNC: ON
-- MONOTONIC_CLOCK: OFF
-- C++ STANDARD: using C++11 for all
-- C++: Setting C++ standard for gnu compiler: lib:  apps:
-- DETECTED SYSTEM: WINDOWS;  WIN32=1; PTW32_STATIC_LIB=1
-- ENABLE_BONDING: OFF
-- Pthread library: C++11
-- WINDOWS detected: adding compat sources: /build/src_packages/libsrt/common/win_time.cpp
-- APP: srt_virtual: using default C++ standard
-- ADDING TRANSITIVE LINK DEP to:srt_static :
-- INSTALL DIRS: bin=bin lib=lib shlib=lib include=include
-- APPS: DISABLED
--
========================================================================
= Project Configuration:
========================================================================
  SRT Version:
    SRT_VERSION: 1.5.3
    SRT_VERSION_BUILD:
  CMake Configuration:
    CMAKE_VERSION: 3.29.3
    CMAKE_INSTALL_PREFIX: /build/install/x86_64-w64-mingw32
    CMAKE_BUILD_TYPE: Release
  Target Configuration:
    CMAKE_SYSTEM_NAME: Windows
    CMAKE_SYSTEM_VERSION:
    CMAKE_SYSTEM_PROCESSOR: x86_64
    CMAKE_SIZEOF_VOID_P: 8
    DARWIN: 0
    LINUX: 0
    BSD: 0
    MICROSOFT: 0
    GNU: 0
    ANDROID: 0
    SUNOS: 0
    POSIX: 0
    SYMLINKABLE: 0
    APPLE:
    UNIX:
    WIN32: 1
    MINGW: 1
    CYGWIN:
    CYGWIN_USE_POSIX: OFF
  Toolchain Configuration:
    CMAKE_TOOLCHAIN_FILE: /build/toolchain.cmake
    CMAKE_CROSSCOMPILING: TRUE
    CMAKE_SYSROOT:
    CMAKE_C_COMPILER_ID: Clang
    CMAKE_C_COMPILER_VERSION: 19.0.0
    CMAKE_C_COMPILER: /build/install/bin/x86_64-w64-mingw32-gcc
    CMAKE_C_FLAGS: '   -DENABLE_LOGGING=1 -Wall -Wextra'
    CMAKE_C_COMPILE_FEATURES: c_std_90;c_function_prototypes;c_std_99;c_restrict;c_variadic_macros;c_std_11;c_static_assert;c_std_17;c_std_23
    CMAKE_C_STANDARD: 11
    CMAKE_CXX_COMPILER_ID: Clang
    CMAKE_CXX_COMPILER_VERSION: 19.0.0
    CMAKE_CXX_COMPILER: /build/install/bin/x86_64-w64-mingw32-g++
    CMAKE_CXX_FLAGS: '   -DENABLE_LOGGING=1 -Wall -Wextra'
    CMAKE_CXX_COMPILE_FEATURES: cxx_std_98;cxx_template_template_parameters;cxx_std_11;cxx_alias_templates;cxx_alignas;cxx_alignof;cxx_attributes;cxx_auto_type;cxx_constexpr;cxx_decltype;cxx_decltype_incomplete_return_types;cxx_default_function_template_args;cxx_defaulted_functions;cxx_defaulted_move_initializers;cxx_delegating_constructors;cxx_deleted_functions;cxx_enum_forward_declarations;cxx_explicit_conversions;cxx_extended_friend_declarations;cxx_extern_templates;cxx_final;cxx_func_identifier;cxx_generalized_initializers;cxx_inheriting_constructors;cxx_inline_namespaces;cxx_lambdas;cxx_local_type_template_args;cxx_long_long_type;cxx_noexcept;cxx_nonstatic_member_init;cxx_nullptr;cxx_override;cxx_range_for;cxx_raw_string_literals;cxx_reference_qualified_functions;cxx_right_angle_brackets;cxx_rvalue_references;cxx_sizeof_member;cxx_static_assert;cxx_strong_enums;cxx_thread_local;cxx_trailing_return_types;cxx_unicode_literals;cxx_uniform_initialization;cxx_unrestricted_unions;cxx_user_literals;cxx_variadic_macros;cxx_variadic_templates;cxx_std_14;cxx_aggregate_default_initializers;cxx_attribute_deprecated;cxx_binary_literals;cxx_contextual_conversions;cxx_decltype_auto;cxx_digit_separators;cxx_generic_lambdas;cxx_lambda_init_captures;cxx_relaxed_constexpr;cxx_return_type_deduction;cxx_variable_templates;cxx_std_17;cxx_std_20;cxx_std_23
    CMAKE_CXX_STANDARD: 11
    CMAKE_LINKER: /build/install/bin/ld.lld
    CMAKE_NM: /build/install/bin/llvm-nm
    CMAKE_AR: /build/install/bin/x86_64-w64-mingw32-llvm-ar
    CMAKE_RANLIB: /build/install/bin/x86_64-w64-mingw32-llvm-ranlib
    HAVE_COMPILER_GNU_COMPAT: 1
    CMAKE_THREAD_LIBS:
    CMAKE_THREAD_LIBS_INIT: -lpthread
    ENABLE_THREAD_CHECK:
    USE_CXX_STD_APP:
    USE_CXX_STD_LIB:
    STDCXX: 11
    USE_CXX_STD: 11
    HAVE_CLOCK_GETTIME_IN:
    HAVE_CLOCK_GETTIME_LIBRT:
    HAVE_PTHREAD_GETNAME_NP_IN_PTHREAD_NP_H:
    HAVE_PTHREAD_SETNAME_NP_IN_PTHREAD_NP_H:
    HAVE_PTHREAD_GETNAME_NP: 1
    HAVE_PTHREAD_SETNAME_NP: 1
    HAVE_LIBATOMIC: 0
    HAVE_LIBATOMIC_COMPILES:
    HAVE_LIBATOMIC_COMPILES_STATIC: 0
    HAVE_GCCATOMIC_INTRINSICS: 1
    HAVE_GCCATOMIC_INTRINSICS_REQUIRES_LIBATOMIC:
    HAVE_CXX_ATOMIC: 1
    HAVE_CXX_ATOMIC_STATIC: 1
    HAVE_CXX_STD_PUT_TIME: 1
  Project Configuration:
    ENABLE_DEBUG: OFF
    ENABLE_CXX11: ON
    ENABLE_APPS: OFF
    ENABLE_EXAMPLES:
    ENABLE_BONDING: OFF
    ENABLE_TESTING: OFF
    ENABLE_PROFILE: OFF
    ENABLE_LOGGING: ON
    ENABLE_HEAVY_LOGGING: OFF
    ENABLE_HAICRYPT_LOGGING: OFF
    ENABLE_SHARED: OFF
    ENABLE_STATIC: ON
    ENABLE_RELATIVE_LIBPATH: OFF
    ENABLE_GETNAMEINFO: OFF
    ENABLE_UNITTESTS: OFF
    ENABLE_ENCRYPTION: ON
    ENABLE_CXX_DEPS: ON
    USE_STATIC_LIBSTDCXX: OFF
    ENABLE_INET_PTON: ON
    ENABLE_CODE_COVERAGE: OFF
    ENABLE_MONOTONIC_CLOCK: OFF
    ENABLE_STDCXX_SYNC: ON
    USE_OPENSSL_PC: ON
    OPENSSL_USE_STATIC_LIBS: OFF
    USE_BUSY_WAITING: OFF
    USE_GNUSTL: OFF
    ENABLE_SOCK_CLOEXEC: ON
    ENABLE_SHOW_PROJECT_CONFIG: ON
    ENABLE_CLANG_TSA: OFF
    ATOMIC_USE_SRT_SYNC_MUTEX: OFF
  Constructed Configuration:
    DISABLE_CXX11:
    HAVE_INET_PTON: 1
    PTHREAD_LIBRARY:
    USE_ENCLIB: openssl
    SSL Configuration:
      SSL_FOUND=1
      SSL_INCLUDE_DIRS=/build/install/x86_64-w64-mingw32/include
      SSL_LIBRARIES=ssl;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32
      SSL_VERSION=3.4.0-dev
    TARGET_srt: srt
    srt_libspec_static: ON
    srt_libspec_shared: OFF
    SRT_LIBS_PRIVATE:  -lwsock32 -lws2_32 -lc++ -lmingw32 -lunwind -lmoldname -lmingwex -lpthread -ladvapi32 -lshell32 -luser32 -lkernel32 -lmingw32 -lunwind -lmoldname -lmingwex -lkernel32
    Target Link Libraries:
      Static: ssl;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32;crypto;z;brotlienc;brotlidec;brotlicommon;m;zstd;ws2_32;gdi32;crypt32;wsock32;ws2_32
      Shared:
========================================================================

-- Configuring done (4.2s)
-- Generating done (0.0s)
-- Build files have been written to: /build/packages/libsrt-prefix/src/libsrt-build

@maxsharabayko
Copy link
Collaborator

I assume when HAVE_PTHREAD_GETNAME_NP and HAVE_PTHREAD_SETNAME_NP are detected on the platform, the thread name facility gets enabled and pthread is linked into the binary, as @ethouris has already suggested.

There are two cases for C++11 then:

  1. STDCXX_SYNC is enabled with POSIX threads - then there is no reason to not use the threadname facility,
  2. STDCXX_SYNC is enabled with WIN threads - then usage of the thread name must be disabled.

Maybe there should be an additional CMake variable and C++ definition indicating the threading library being used 🤔

@ethouris
Copy link
Collaborator

That's not exactly what I have suggested. I have suggested that this should be enabled even if C++11 threads are requested, but only if the C++11 threads implementation is based on POSIX threads. That should be the case on Linux. But in all other cases, the threadname.h should use a stub implementation, REGARDLESS OF AVAILABILITY of pthread_get/setname_np.

I also think that it is still safer to turn into stub threadname always if C++11 threads are requested - even on Linux - just reenable it on demand using an extra flag.

So, in total 3 things should be conditioned:

  1. Whether C++11 threads are enabled - ENABLE_STDCXX_SYNC
  2. Whether getname/setname are available - HAVE_GETSETNAME_NP (shortcut)
  3. Whether threadname should use getname/setname. - ENABLE_THREADNAME_NP (proposal)

And:

  • if ENABLE_STDCXX_SYNC: ENABLE_THREADNAME_NP = false, can be set to true only if HAVE_GETSETNAME_NP.
  • if NOT ENABLE_STDCXX_SYNC AND HAVE_GETSETNAME_NP, then ENABLE_THREADNAME_NP = true.

@jlsantiago0
Copy link
Contributor

jlsantiago0 commented May 30, 2024

NOTE: Other platforms besides linux currently use PThreads to set the thread name. For instance Android, MacOS, iOS, various BSDs, Solaris, etc. PR #2952 will break those platforms.

@ethouris
Copy link
Collaborator

We are talking about providing the thread names, which is strictly a development feature. It's not a problem then if the thread names are not supported if you requested C++11 threads, and it can be turned back on by an additional option by the developer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[build] Area: Changes in build files help wanted Indicates that a maintainer wants help on an issue or pull request Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants