Skip to content

[core]:fix pthread_condattr_setclock on lower versions of Android #3149

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

lilinxiong
Copy link
Contributor

No description provided.

1. Move Android pthread_condattr_setclock check to STDCXX_SYNC=OFF block.
2. Enforce ENABLE_STDCXX_SYNC or MONOTONIC_CLOCK to prevent unsafe builds.
@cl-ment
Copy link
Collaborator

cl-ment commented Mar 20, 2025

Thank you, lilinxiong, for reopening this pull request. It is related to #3138, which I mistakenly closed. My apologies again.

@ethouris
Copy link
Collaborator

Ok, very good, just please @lilinxiong do the fixes:

  1. Remove ANDROID from checks. The existence of pthread_condattr_setclock is ALWAYS required, if you set monotonic clock, regardless of the system and regardless of the fact that only older version of Android have problem with it. The informative message that the checks is done is also unnecessary.
  2. In Travis (.travis.yml) there is a build definition which will be done also on systems that require also posix threads and monotonic clock. Therefore in the cmake command that is called there the -DENABLE_MONOTONIC_CLOCK=ON must be added, otherwise some builds will fail.

@maxsharabayko maxsharabayko added this to the v1.5.5 milestone Mar 20, 2025
@maxsharabayko maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Mar 20, 2025
@lilinxiong
Copy link
Contributor Author

Ok, very good, just please @lilinxiong do the fixes:

  1. Remove ANDROID from checks. The existence of pthread_condattr_setclock is ALWAYS required, if you set monotonic clock, regardless of the system and regardless of the fact that only older version of Android have problem with it. The informative message that the checks is done is also unnecessary.
  2. In Travis (.travis.yml) there is a build definition which will be done also on systems that require also posix threads and monotonic clock. Therefore in the cmake command that is called there the -DENABLE_MONOTONIC_CLOCK=ON must be added, otherwise some builds will fail.

I feel that it’s clearer to look at the CMake logs; words aren’t very good at expressing this. Now I’ve added a message in CMake, which is to print the current system name.
image

Then I will remove the judgment for the platform, which means I will only check if ENABLE_MONOTONIC_CLOCK is set to ON. After that, I will re-run the configuration on Darwin, and I will get the following results.
image

That is to say, this is not in line with our expectations, and in actual usage on Darwin, we don’t need to enable ENABLE_MONOTONIC_CLOCK at all. However, only POSIX is true:
image

Then, when POSIX is true, it will proceed to test_requires_clock_gettime(ENABLE_MONOTONIC_CLOCK_DEFAULT MONOTONIC_CLOCK_LINKLIB). Here, ENABLE_MONOTONIC_CLOCK_DEFAULT will be true, and ultimately ENABLE_MONOTONIC_CLOCK will be true.
image

So, we actually can’t just determine if ENABLE_MONOTONIC_CLOCK is true. At least that’s how it seems for now. We either need to adjust the condition for setting ENABLE_MONOTONIC_CLOCK_DEFAULT to true. That is, it will only be enabled when ENABLE_MONOTONIC_CLOCK is needed.

@ethouris
Copy link
Collaborator

There's quite a mess with this definition here, but as long as I look at it, ANDROID is included in POSIX, so this should be checked anyway.

In 1.6.0 we'll simplify this, but CMakeLists.txt will be reworked upside down and won't be mergeable with whatever next is added to 1.5.5 (I'll have to add changes through distillation). Conditions will be also changed, some deprecated configurations will be no longer supported. I also added these changes you made here a little differently. Still, this fix better go to 1.5.5 and in the form that doesn't change fundamentally the configuration rules (which I did in the development for 1.6.0).

Therefore all we need is that if monotonic clock is requested and available, this *_setclock function must be available, otherwise it should make a fatal error. And all we need to make sure is that it detects this situation correctly on old Android version.

@lilinxiong
Copy link
Contributor Author

Ok, very good, just please @lilinxiong do the fixes:

  1. Remove ANDROID from checks. The existence of pthread_condattr_setclock is ALWAYS required, if you set monotonic clock, regardless of the system and regardless of the fact that only older version of Android have problem with it. The informative message that the checks is done is also unnecessary.
  2. In Travis (.travis.yml) there is a build definition which will be done also on systems that require also posix threads and monotonic clock. Therefore in the cmake command that is called there the -DENABLE_MONOTONIC_CLOCK=ON must be added, otherwise some builds will fail.

Regarding the second point here, should I go and modify the .travis.yml file to set ENABLE_MONOTONIC_CLOCK to OFF for those that don’t need it, such as when the operating system is: osx?

@lilinxiong lilinxiong force-pushed the fix/android/low_version branch from 61ef1d4 to 9e07c8b Compare March 20, 2025 12:57
@ethouris
Copy link
Collaborator

Regarding the second point here, should I go and modify the .travis.yml file to set ENABLE_MONOTONIC_CLOCK to OFF for those that don’t need it, such as when the operating system is: osx?

Not sure if there are any configurations where monotonic clock isn't available - I don't think so. So you should be able to add this simply to the overall call to cmake command; worst case it will be ignored. Hard to say, how it will turn out, we'll fix that later if need be.

.travis.yml Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you disable the monotonic clock in CI builds?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be enabled explicitly if we want to have monotonic clock enabled by default. Unless you think it is still better to just go on with default non-monotonic, if it was detected as not available (as it was so previously), but if so, then the check for this function must be also included in the check resulting in default monotonic clock OFF.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here!

Of course, you can also reopen it and give it a try…

Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>
Co-authored-by: Sektor van Skijlen <ethouris@gmail.com>
@cl-ment
Copy link
Collaborator

cl-ment commented Apr 8, 2025

@lilinxiong could you please try #3152 and let us know if it fixes your issue and compiles in your environment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants