fix: disable pthread_condattr_setclock on lower versions of Android #3138
Conversation
|
I have a feeling this check should better be done by CMake, somewhere here Or here |
change test_requires_clock_gettime ? |
|
Hm, sorry, I got confused. You define |
There was a problem hiding this comment.
So it seems like
pthread_condattr_setclock(&CondAttribs, CLOCK_MONOTONIC) won't be called to configure a CV to use MONOTONIC clock.
But there will still be calls to clock_gettime(CLOCK_MONOTONIC, &timeout) to get the target timeout, while the CV won't probably use monotonic clock to wait...
So timeout will be with monotonic clock base, but CV will use system clock internally to wait...
timeout = us_to_timespec(now_us + count_microseconds(rel_time));
return pthread_cond_timedwait(&m_cv, &lock.mutex()->ref(), &timeout) != ETIMEDOUT;
|
It seems that we forgot to call |
|
May I suggest to modify I'm not sure about the changes made to |
Good idea. I have already made the changes. |
|
That error message should rather advice compiling with C++11, as compiling without monotonic clock is dangerous. |
So the modification that should be made here is, if both |
|
Well, it could be quite complicated and "reinventing" if we set default C++11 for Android in this case, so my stance on it is that:
|
|
BTW These last CI builds on Travis fail on the configuration level, but weirdly I see only the general error at the end of cmake, but no error message during configuration. |
cc? I have already made the changes. |
|
Look at sync.h#L30-L45 here. The platform and |
|
@lilinxiong you are right. 1. CLOCK_MONOTONICSRT uses The SRT_SYNC_CLOCK is defined in sync.h as follows: // Defile clock type to use
#ifdef IA32
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_IA32_RDTSC
#define SRT_SYNC_CLOCK_STR "IA32_RDTSC"
#elif defined(IA64)
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_IA64_ITC
#define SRT_SYNC_CLOCK_STR "IA64_ITC"
#elif defined(AMD64)
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_AMD64_RDTSC
#define SRT_SYNC_CLOCK_STR "AMD64_RDTSC"
#elif defined(_WIN32)
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_WINQPC
#define SRT_SYNC_CLOCK_STR "WINQPC"
#elif TARGET_OS_MAC
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_MACH_ABSTIME
#define SRT_SYNC_CLOCK_STR "MACH_ABSTIME"
#elif defined(ENABLE_MONOTONIC_CLOCK)
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_GETTIME_MONOTONIC
#define SRT_SYNC_CLOCK_STR "GETTIME_MONOTONIC"
#else
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_POSIX_GETTIMEOFDAY
#define SRT_SYNC_CLOCK_STR "POSIX_GETTIMEOFDAY"
#endif
2.
|
|
@maxsharabayko Sure, I totally agree with what you said. However, I have another question. // Defile clock type to use
#ifdef IA32 // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_IA32_RDTSC
#define SRT_SYNC_CLOCK_STR "IA32_RDTSC"
#elif defined(IA64) // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_IA64_ITC
#define SRT_SYNC_CLOCK_STR "IA64_ITC"
#elif defined(AMD64) // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_AMD64_RDTSC
#define SRT_SYNC_CLOCK_STR "AMD64_RDTSC"
#elif defined(_WIN32) // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_WINQPC
#define SRT_SYNC_CLOCK_STR "WINQPC"
#elif TARGET_OS_MAC // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_MACH_ABSTIME
#define SRT_SYNC_CLOCK_STR "MACH_ABSTIME"
#elif defined(ENABLE_MONOTONIC_CLOCK) // this is not platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_GETTIME_MONOTONIC
#define SRT_SYNC_CLOCK_STR "GETTIME_MONOTONIC"
#else
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_POSIX_GETTIMEOFDAY
#define SRT_SYNC_CLOCK_STR "POSIX_GETTIMEOFDAY"
#endifRegarding the definition here, could we possibly make some modifications, such as changing it to the following: ...
#elif TARGET_OS_MAC // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_MACH_ABSTIME
#define SRT_SYNC_CLOCK_STR "MACH_ABSTIME"
#elif defined(__ANDROID__) // platform
#if defined(ENABLE_MONOTONIC_CLOCK)
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_GETTIME_MONOTONIC
#define SRT_SYNC_CLOCK_STR "GETTIME_MONOTONIC"
#endif
#else
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_POSIX_GETTIMEOFDAY
#define SRT_SYNC_CLOCK_STR "POSIX_GETTIMEOFDAY"
#endifBecause here, the priority of the platform is higher than our self-defined Or to put it another way, in our check pthread_condattr_setclock So I’ve made a temporary adjustment to only check for the pthread_condattr_setclock symbol on Android. From 68b598ddede64632a149dcee8c8b2f1d5de335e2 Mon Sep 17 00:00:00 2001
From: lilinxiong <lilinxiong1997@gmail.com>
Date: Thu, 13 Mar 2025 17:57:23 +0800
Subject: [PATCH] only check Android platforom
---
CMakeLists.txt | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/CMakeLists.txt b/CMakeLists.txt
index b83b316..2035526 100755
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -142,7 +142,11 @@ elseif (POSIX)
test_requires_clock_gettime(ENABLE_MONOTONIC_CLOCK_DEFAULT MONOTONIC_CLOCK_LINKLIB)
endif()
-if (ENABLE_MONOTONIC_CLOCK_DEFAULT)
+# If current platform is android both ENABLE_MONOTONIC_CLOCK_DEFAULT is ON so check
+# todo: This is not a very good approach, but in the current project,
+# todo: the platform is not defined independently, and there is a mix of platform and ENABLE_MONOTONIC_CLOCK in src/core/sync.h,
+# todo: which should not be used together. Therefore, for now, we have to write it this way.
+if (ANDROID AND ENABLE_MONOTONIC_CLOCK_DEFAULT)
message(STATUS "because enable monotonic clock so start check needed symbol.")
list(PREPEND CMAKE_REQUIRED_LIBRARIES "${CMAKE_THREAD_LIBS_INIT}")
check_symbol_exists(pthread_condattr_setclock "pthread.h" HAVE_PTHREAD_CONDATTR_SETCLOCK)
--
2.45.2However, this is not very elegant because now only Android is being checked, and none of the others. The reason is that in the current CMakeLists.txt, I seem to be unable to predict on which platform ENABLE_MONOTONIC_CLOCK will be used, because in the CMakeLists.txt, MacOS also belongs to POSIX. Its definition is here: ...
set_if(POSIX LINUX OR DARWIN OR BSD OR SUNOS OR ANDROID OR (CYGWIN AND CYGWIN_USE_POSIX) OR GNU_OS)
...ConclusionFor the definition of |
CMakeLists.txt
Outdated
| if (POSIX) | ||
| # If current platform is POSIX, need check ENABLE_MONOTONIC_CLOCK is ON or ENABLE_STDCXX_SYNC ON | ||
| if (NOT ENABLE_STDCXX_SYNC AND NOT ENABLE_MONOTONIC_CLOCK) | ||
| message(FATAL_ERROR "Both ENABLE_STDCXX_SYNC and ENABLE_MONOTONIC_CLOCK are OFF. Exiting. pls Open ENABLE_STDCXX_SYNC ENABLE_MONOTONIC_CLOCK=OFF is DANGEROUS!!") |
There was a problem hiding this comment.
This message is not comprehensible.
The idea is:
- if ENABLE_STDXCC_SYNC is ON, don't check for ENABLE_MONOTONIC_CLOCK presence nor availability of symbol
- if ENABLE_STDCXX_SYNC is OFF, default ENABLE_MONOTONIC_CLOCK should be ON.
- if ENABLE_MONOTONIC_CLOCK is ON, and the symbol is not available, issue a fatal error.
There was a problem hiding this comment.
You're right, this logic was written earlier, and I forgot to adjust it later. I've already made the necessary changes.
if (ANDROID AND ENABLE_MONOTONIC_CLOCK_DEFAULT)
message(STATUS "because enable monotonic clock so start check needed symbol.")
list(PREPEND CMAKE_REQUIRED_LIBRARIES "${CMAKE_THREAD_LIBS_INIT}")
check_symbol_exists(pthread_condattr_setclock "pthread.h" HAVE_PTHREAD_CONDATTR_SETCLOCK)
if (NOT HAVE_PTHREAD_CONDATTR_SETCLOCK)
message(FATAL_ERROR "not find pthread_condattr_setclock monotonic clock not available on this system. pls open ENABLE_STDCXX_SYNC, as compiling without monotonic clock is dangerous.")
endif ()
endif ()When ENABLE_MONOTONIC_CLOCK_DEFAULT is ON, and it is running on the Android platform, it will check for the presence of the pthread_condattr_setclock symbol. If it is not found, the program will terminate fatally at this point and provide a detailed message.
CMakeLists.txt
Outdated
| # todo: This is not a very good approach, but in the current project, | ||
| # todo: the platform is not defined independently, and there is a mix of platform and ENABLE_MONOTONIC_CLOCK in src/core/sync.h, | ||
| # todo: which should not be used together. Therefore, for now, we have to write it this way. | ||
| if (ANDROID AND ENABLE_MONOTONIC_CLOCK_DEFAULT) |
There was a problem hiding this comment.
I don't think you need to make any special check if the problem is on Android. Sure, you need this above comment, but you can as well check BOTH things - availability of monotonic clock and availability of pthread_condattr_setclock in one step (the above if-branch)
Checks should be done elsewhere (after the option series, where you'd expect to define the ENABLE_MONOTONIC_CLOCK option). What you need to check is:
- if ENABLE_STDCXX_SYNC is OFF, you don't check anything
- Otherwise, if ENABLE_MONOTONIC_CLOCK is ON, you check if ENABLE_MONOTONIC_CLOCK_DEFAULT and HAVE_PTHREAD_CONDATTR_SETCLOCK are both ON. If not, you issue a FATAL_ERROR.
There was a problem hiding this comment.
hmmm,
"if ENABLE_STDCXX_SYNC is OFF, you don't check anything"
Is there a mistake here? Shouldn’t it be that when ENABLE_STDCXX_SYNC is ON, I don’t need to do any checks because when ENABLE_STDCXX_SYNC is ON, it will use C++11.
Only when ENABLE_STDCXX_SYNC is OFF, and the current platform is Android (because of the comment above), and ENABLE_MONOTONIC_CLOCK is ON, will I check ENABLE_MONOTONIC_CLOCK_DEFAULT and HAVE_PTHREAD_CONDATTR_SETCLOCK.
If my understanding is correct, then I will make the necessary modifications based on this understanding.
There was a problem hiding this comment.
if ENABLE_STDCXX_SYNC Is ON, of course, sorry, you're right :)
Still, you don't have to check for Android at all. You can mention in the comment that this situation is on some older versions of Android, but checking it isn't necessary.
So, if ENABLE_STDCXX_SYNC is ON, or if ENABLE_MONOTONIC_CLOCK is OFF, then monotonic clock is not requested.
If it is requested, need to check if both ENABLE_MONOTONIC_CLOCK_DEFAULT is ON and HAVE_PTHREAD_CONDATTR_SETCLOCK is ON, and if not, issue a fatal error. This check must be done after the declaration of the ENABLE_MONOTONIC_CLOCK option (because only there you can check its value as given by user or remaining as default).
There was a problem hiding this comment.
hmmm, Essentially, it should be consistent with what you said, but I’ve found that if my current platform is MacOS, then by default, ENABLE_MONOTONIC_CLOCK_DEFAULT is set to ON. However, when I check pthread_condattr_setclock, it indicates that the symbol is not found. But according to the definition in sync.h, even if ENABLE_STDCXX_SYNC is OFF, it does not use pthread on MacOS… At the root of the issue, it’s that we still define it when we are not using ENABLE_MONOTONIC_CLOCK…
There is detailed information in 3138#issuecomment-2721544858 here.
There was a problem hiding this comment.
hmmm, Essentially, it should be consistent with what you said, but I’ve found that if my current platform is MacOS, then by default, ENABLE_MONOTONIC_CLOCK_DEFAULT is set to ON. However, when I check pthread_condattr_setclock, it indicates that the symbol is not found. But according to the definition in sync.h, even if ENABLE_STDCXX_SYNC is OFF, it does not use pthread on MacOS… At the root of the issue, it’s that we still define it when we are not using ENABLE_MONOTONIC_CLOCK…
There is detailed information in 3138#issuecomment-2721544858 here.
@ethouris But, I want to confirm if my understanding here is correct, because it concerns the necessity of the judgment for ‘Android’ in the final if statement
37b026a to
a6b9959
Compare
|
@cl-ment Hm? What’s the reason for this closure? |
|
@lilinxiong We have some technical failure on our side, something like one of our tools has gone rogue. If you have this branch in your local repository, would you be able to push this branch - with |
|
It was actually worse - some tool has done this "in the background". Closure has likely happen from Github automatically because in result all commits on your branch have been deleted. |
|
My apologies ! @ethouris and I are trying to repare my mistake. |
okay~ |
|
Ok, thanks in advance. After you push, the PR could be likely reopen - you should be able to do it this time. |
|
@ethouris @cl-ment The resubmitted pull request is here-pull#3149. |
On Android OS versions equal to or less than 21,
pthread_condattr_setclockis not available, so we need to disable it to prevent compilation failures.