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

[Android] Workaround for invalid return value from clock_nanosleep #64679

Conversation

simonrozsival
Copy link
Member

@simonrozsival simonrozsival commented Feb 2, 2022

There used to be a bug in Android libc implementation of clock_nanosleep. The return value should be errno on errors but instead in it returns -1 and sets errno. The libc (Bionic) bug has been fixed since Android 6 and newer but it causes problems to customers who are unable to update Android on their devices.

Fixes dotnet/android#6600

@ghost
Copy link

ghost commented Feb 2, 2022

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

There used to be a bug in Android libc implementation of clock_nanosleep. The return value should be errno on errors but instead in it returns -1 and sets errno. The libc (Bionic) bug has been fixed since Android 6 and newer but it causes problems to customers who are unable to update Android on their devices.

Fixes dotnet/android#6600

Author: simonrozsival
Assignees: -
Labels:

os-android

Milestone: -

@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

It's ok, but maybe we can have less copy/pasted code if we added a utility function to eglib

src/mono/mono/eglib/gdate-unix.c Outdated Show resolved Hide resolved
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

Good start. Needs couple of changes

src/mono/mono/eglib/gclock_nanosleep.c Outdated Show resolved Hide resolved
src/mono/mono/eglib/gclock_nanosleep.c Outdated Show resolved Hide resolved
src/mono/mono/eglib/gclock_nanosleep.c Outdated Show resolved Hide resolved
src/mono/mono/eglib/gclock_nanosleep.c Outdated Show resolved Hide resolved
src/mono/mono/utils/mono-threads.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-posix.c Outdated Show resolved Hide resolved
src/mono/mono/eglib/glib.h Show resolved Hide resolved
src/mono/mono/eglib/CMakeLists.txt Outdated Show resolved Hide resolved
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 7, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 7, 2022
@simonrozsival simonrozsival dismissed lambdageek’s stale review February 7, 2022 18:35

Thanks for the suggestions, @lambdageek!

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

Thanks!

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

Windows builds are unhappy. But we don't need clock_nanosleep there, anyway.

src/mono/mono/eglib/glib.h Show resolved Hide resolved
src/mono/mono/eglib/CMakeLists.txt Outdated Show resolved Hide resolved
@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@simonrozsival
Copy link
Member Author

There's a failing WASM test but it seems completely unrelated to this PR to me. I believe this PR could be merged.

@lambdageek
Copy link
Member

Yea the browser wasm windows System.Threading.Channels.Tests failures are #65012

@lambdageek lambdageek merged commit ac82c68 into dotnet:main Feb 9, 2022
@lambdageek
Copy link
Member

@steveisok should we backport to net6?

@akoeplinger
Copy link
Member

akoeplinger commented Feb 15, 2022

Yes.

@akoeplinger
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1846939447

@github-actions
Copy link
Contributor

@akoeplinger backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Account for incorrect implementation of clock_nanosleep in older Android libc
Applying: Shorten comments
Applying: Add g_clock_nanosleep function
error: sha1 information is lacking or useless (src/mono/mono/mini/mini-posix.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Add g_clock_nanosleep function
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@akoeplinger
Copy link
Member

@simonrozsival can you please look into doing a manual PR for 6.0? thanks

@simonrozsival
Copy link
Member Author

@akoeplinger Sure! I'll create one.

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Feb 15, 2022
…otnet#64679)

There used to be a bug in Android libc implementation of `clock_nanosleep`. The return value should be `errno` on errors  but instead in it returns `-1` and sets `errno`. The libc (Bionic) bug [has been fixed](https://android-review.googlesource.com/c/platform/bionic/+/110652/) since Android 6 and newer but it causes problems to [customers who are unable to update Android on their devices](dotnet/android#6600 (comment)).

Fixes dotnet/android#6600

* Account for incorrect implementation of clock_nanosleep in older Android libc

* Shorten comments

* Add g_clock_nanosleep function

* Add remap definition

* Fix build

* Make sure the extra check runs only on Android

* Make Windows builds happy

* Try making wasm builds happy
akoeplinger pushed a commit that referenced this pull request Feb 15, 2022
This is a follow-up of #64679 

I realized that one invocation of `clock_nanosleep` wasn't replaced with `g_clock_nanosleep`.
simonrozsival added a commit to simonrozsival/mono that referenced this pull request Feb 16, 2022
akoeplinger pushed a commit to akoeplinger/mono that referenced this pull request Feb 17, 2022
akoeplinger added a commit to mono/mono that referenced this pull request Feb 17, 2022
…osleep (#21435)

* [Android] Workaround for invalid return value from clock_nanosleep (#21433)

Fixes dotnet/android#6600

Backported from dotnet/runtime#64679 and dotnet/runtime#65373

(cherry picked from commit 36daf03)

* Fix Windows build

Co-authored-by: Simon Rozsival <simon@rozsival.com>
lewing pushed a commit that referenced this pull request Mar 10, 2022
…k_nanosleep (#65372)

* [Android] Workaround for invalid return value from clock_nanosleep (#64679)

There used to be a bug in Android libc implementation of `clock_nanosleep`. The return value should be `errno` on errors  but instead in it returns `-1` and sets `errno`. The libc (Bionic) bug [has been fixed](https://android-review.googlesource.com/c/platform/bionic/+/110652/) since Android 6 and newer but it causes problems to [customers who are unable to update Android on their devices](dotnet/android#6600 (comment)).

Fixes dotnet/android#6600

* Account for incorrect implementation of clock_nanosleep in older Android libc

* Shorten comments

* Add g_clock_nanosleep function

* Add remap definition

* Fix build

* Make sure the extra check runs only on Android

* Make Windows builds happy

* Try making wasm builds happy

* Fix leftover direct call to clock_nanosleep
@ghost ghost locked as resolved and limited conversation to collaborators Mar 17, 2022
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.

Native Crash (SIGABRT): monoeg_g_usleep: clock_nanosleep () returned -1
3 participants