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 HAVE_PTHREAD_CONDATTR_SETCLOCK detection on Android #58701

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

MichalStrehovsky
Copy link
Member

Android puts pthread stuff into libc. Because this wasn't detected, we are using some iOS fallbacks in System.Native. I'm propagating how pthreads are detected in the CoreCLR PAL (that was fixed up for Android in the CoreCLR PAL a couple years ago).

Failure to properly detect it here was causing build breaks in CoreCLR on Android (that's how I found this).

Android puts pthread stuff into libc. Because this wasn't detected, we are using some iOS fallbacks in System.Native. I'm propagating how pthreads are detected in the CoreCLR PAL (that was fixed up for Android in the CoreCLR PAL a couple years ago).

Failure to properly detect it here was causing build breaks in CoreCLR on Android (that's how I found this).
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@marek-safar
Copy link
Contributor

@akoeplinger should we backport this?

@akoeplinger
Copy link
Member

akoeplinger commented Sep 6, 2021

@akoeplinger should we backport this?

Ideally yes, though it's not completely without risk.

Note that we still have this definition in mono-os-mutex.h which disables use of pthread_condattr_setclock, presumably because there was some issue on earlier Android versions:

#if !defined(CLOCK_MONOTONIC) || defined(HOST_DARWIN) || defined(HOST_ANDROID) || defined(HOST_WASM)
#define BROKEN_CLOCK_SOURCE
#endif

We might want to fix that as well. I also saw that we explicitly disable CLOCK_MONOTONIC for iOS since it was only available on iOS10+, but that is now our minimum target so we can likely remove that too.

@MichalStrehovsky
Copy link
Member Author

Ideally yes, though it's not completely without risk.

I filed #58737 and put it into 6.0 milestone. FWIW, the old commit that disabled it on Mono on Android just talks about fixing a build break and doesn't have much else to go off.

@ViktorHofer
Copy link
Member

ViktorHofer commented Sep 7, 2021

@MichalStrehovsky I think this change is causing the CoreCLR GCC Product Build Linux x64 leg to fail in all PRs: #58744.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants