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

Fix Build for MacOS, FreeBSD, GCC earlier than 4.7, and Clang earlier… #2104

Merged
merged 13 commits into from
Sep 1, 2021
Merged

Fix Build for MacOS, FreeBSD, GCC earlier than 4.7, and Clang earlier… #2104

merged 13 commits into from
Sep 1, 2021

Conversation

jlsantiago0
Copy link
Contributor

@jlsantiago0 jlsantiago0 commented Aug 26, 2021

… than 6.

There are two major problems, srt::sync::atomic implementation and usage of pthread_getname_np() and pthread_setname_np().

This fixes the build for MacOS for a number of versions. For instance XCode <7 uses Apple LLVM based on Clang 5 or earlier and XCode <5 uses GCC <4.5. These do not support GCC __atomic_* intrinsics which were introduced in GCC 4.7. This patch uses C++11 std::atomic to implement srt::sync::atomic for MacOS 10.7 and later, and provides an implementation of srt::sync::atomic using POSIX Mutexes for MacOS <10.7. Also some of the macros used to instrument the synchronous operations annotations are not available in LLVM <6. With this patch, OpenSRT is now building with XCode-3.2.6 (X86_64, i386, PPC, PPC64) targeting MacOS-10.5.x on MacOS 10.8, XCode-5.1.1 (X86_64, i386) on MacOS 10.8, XCode-9.4.2 (X86_64, i386) on MacOS 10.13, XCode-12.2 (X86_64) on MacOS-11.1. I have not yet had time to test MacOS-11.x for ARM64 builds.

This fixes MacOS builds for MacOS <10.6 as pthread_getname_np() and pthread_setname_np() was introduced in MacOS 10.6 and is not available in MacOS 10.5 and earlier.

While resolving the issues for MacOS builds, I was able to us the POSIX Mutex srt::sync::atomic implementation to fix build issues with GCC <4.7. This fixes the build for the MakitoX toolchain which is based on GCC-4.4.1, it also fixes the build for CentOS <7, Ubuntu <14 and for Slackware <14. NOTE It would be possible to support the older __sync_* intrinsics available in the older versions of GCC, but it might not be as simple as it seems as the synchronous memory model was changed in the GCC 4.7 implementation to match C++11 and C11 standards.

Also the rework of the pthread_getname_np() and pthread_setname_np() allows other operating systems that support the API to use it. For instance these functions are available in many of the BSD variants, but may be in a different header file or may have a slightly different name. I also fixed handling of the different function call parameters of the Apple and non-Apple implementations of pthread_setname_np(). Apple takes one parameter and the most implementations take 2. This was tested with FreeBSD-11.3 and OpenBSD-6.4.

NOTE That other than the Apple XCode toolchains, I assume that other Clang <6 users will not have support for the GCC __atomic_* intrinisics and will need to use POSIX Mutex implementation as well, if or until someone ports itto the older __sync_* intrinsics. But I havent specified that in this patch.

@maxsharabayko maxsharabayko added this to the v1.4.4 milestone Aug 27, 2021
@maxsharabayko maxsharabayko added [build] Area: Changes in build files Type: Bug Indicates an unexpected problem or unintended behavior labels Aug 27, 2021
@jeandube
Copy link
Collaborator

I would like to have this patch. I first found I can no longer build libsrt native for our build/test 'Indigo' system (C86_64 GCC4.6.3). From the detailed problem description above (thanks @jlsantiago0 ) I realized I cannot cross-compile for our X-series as well (arm GCC 4.4.1).
Only our most recent X4-series toolset (aarch64 GCC 6.2.1) can build it.

@jlsantiago0
Copy link
Contributor Author

I would like to have this patch. I first found I can no longer build libsrt native for our build/test 'Indigo' system (C86_64 GCC4.6.3). From the detailed problem description above (thanks @jlsantiago0 ) I realized I cannot cross-compile for our X-series as well (arm GCC 4.4.1).
Only our most recent X4-series toolset (aarch64 GCC 6.2.1) can build it.

I have one fix that I am testing for the test issue indicated by the CI tester. It should only effect MacOS. You may also need the patch for #2103 as well.

@DevSysEngineer
Copy link

DevSysEngineer commented Aug 27, 2021

@jlsantiago0 thanks for your pull request. This pull request make more easier to build libsrt for MacOS. This very handy for debugging and for personal use. I saw that the checks for MacOS failed. I don't know if that's a bad thing, maybe the Haivision team can answer this? Link: https://github.com/Haivision/srt/pull/2104/checks?check_run_id=3437542062.

@jlsantiago0
Copy link
Contributor Author

jlsantiago0 commented Aug 27, 2021

@KvanSteijn The latest commit seems to have resolved the MacOS CI test failure, Visual Studio Release builds are breaking, but the Debug builds are fine. I dont understand what is wrong.

@DevSysEngineer
Copy link

DevSysEngineer commented Aug 27, 2021

@jlsantiago0 I think it has to do with atomic. I'm seeing a lot of syntax errors that has to do with atomic.
syntax errors: c:\projects\srt\srtcore\atomic.h(345): error C2059: syntax error : '<'. It''

Edit: Link to pthreads repo that VS 2013 release CI are using: https://github.com/Cinegy/pthread-win32

Other name changes
------------------

All snapshots prior to and including snapshot 2000-08-13
used "_pthread_" as the prefix to library internal
functions, and "_PTHREAD_" to many library internal
macros. These have now been changed to "ptw32_" and "PTW32_"
respectively so as to not conflict with the ANSI standard's
reservation of identifiers beginning with "_" and "__" for
use by compiler implementations only.

If you have written any applications and you are linking
statically with the pthreads-win32 library then you may have
included a call to _pthread_processInitialize. You will
now have to change that to ptw32_processInitialize.

@maxsharabayko
Copy link
Collaborator

VS 2013 release CI build uses pthreads port for windows, while other builds use C++11 sync.

@jlsantiago0
Copy link
Contributor Author

jlsantiago0 commented Aug 27, 2021

@maxsharabayko It looks like the intent of the code was to use ATOMIC_USE_MSVC_INTRINSICS for Visual Studio if c++11 is not being used, but the definition for value_ for ATOMIC_USE_MSVC_INTRINSICS is incorrectly using std::atomic<T> unconditionally. My latest commit should resolve that issue. Looks like it has always been broken.

@jlsantiago0
Copy link
Contributor Author

jlsantiago0 commented Aug 27, 2021

@maxsharabayko and @KvanSteijn OK. I cleaned it up so that it is obvious that we have explicit definitions for each atomic implementation.

@jlsantiago0
Copy link
Contributor Author

jlsantiago0 commented Aug 27, 2021

@maxsharabayko BTW load() for ATOMIC_USE_MSVC_INTRINSICS is racy and should be fixed. I dont know anything about the Windows API. So someone with the proper knowledge should take that on.

@jlsantiago0
Copy link
Contributor Author

jlsantiago0 commented Aug 27, 2021

@maxsharabayko ATOMIC_USE_POSIX_MUTEX can be selected in the top level CMake build configuration by adding -DATOMIC_USE_POSIX_MUTEX=ON when performing the CMake configuration. That should make it easier to test, even when another implementation would be used normally for your build. A CLI option could be added to the configure script and the CMake configuration option should probably be referenced in the project configuration documentation. I will leave that to you as I will likely mess it up. Thank you for considering my patches.

@codecov-commenter

This comment has been minimized.

@maxsharabayko
Copy link
Collaborator

@maxsharabayko BTW load() for ATOMIC_USE_MSVC_INTRINSICS is racy and should be fixed. I dont know anything about the Windows API. So someone with the proper knowledge should take that on.

@jlsantiago0 Thanks for noting! Created issue #2108.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
srtcore/threadname.h Outdated Show resolved Hide resolved
srtcore/threadname.h Outdated Show resolved Hide resolved
srtcore/threadname.h Outdated Show resolved Hide resolved
@maxsharabayko
Copy link
Collaborator

Testing Windows POSIX build. Running for 2 hours (more than one TSBPD wrapping period), no issues. ✔️

Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>
jlsantiago0 and others added 2 commits August 31, 2021 09:27
Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>
srtcore/atomic.h Outdated Show resolved Hide resolved
srtcore/atomic.h Outdated Show resolved Hide resolved
@maxsharabayko maxsharabayko merged commit df95ecb into Haivision:master Sep 1, 2021
@jlsantiago0 jlsantiago0 deleted the build-fixery branch September 1, 2021 19:23
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 Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants