-
Notifications
You must be signed in to change notification settings - Fork 385
Android: Fixed tests for libc time API #3910
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
Conversation
5931300
to
ae3ff3b
Compare
src/shims/time.rs
Outdated
@@ -36,7 +36,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { | |||
let mut relative_clocks; | |||
|
|||
match this.tcx.sess.target.os.as_ref() { | |||
"linux" | "freebsd" => { | |||
"linux" | "freebsd" | "android" => { | |||
// Linux and FreeBSD have two main kinds of clocks. REALTIME clocks return the actual time since the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the comment here apply to Android as well? If yes, please make it say "Linux, Android, and FreeBSD"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it applies to Android as well. The thing with Android actually is it's based on LTS version of Linux, so technically it's Linux (source):
The Android kernel is based on an upstream Linux Long Term Supported (LTS) kernel. At Google, LTS kernels are combined with Android-specific patches to form what are known as Android Common Kernels (ACKs).
So, shouldn't the code be more unified? If you're okay, I can check all Linux specific foreign items to be supported by Android looking into its libc
, and then refactor under your guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know Android shares the kernel with Linux, but the userland is quite different. I don't know Android well enough to judge whether we'd better treat them as separate Unix flavors in Miri, or as a single flavor with some occasional special-casing.
One concern with treating them as one Unix flavor is that we don't have an Android expert in the team, so we'd want to avoid someone adding a new shim for Linux and accidentally enabling it on Android without testing that. If we treated shims more declaratively, that would be fairly easy to enforce... without that, the best we can do is probably a comment in the Linux file.
So yeah this seems worth exploring, if you think it has merit. In a future PR. :)
Ah, that's great, thanks. :) Unfortunately there are conflicts, can you please rebase? Also, I have a comment nit, see above. |
☔ The latest upstream changes (presumably #3913) made this pull request unmergeable. Please resolve the merge conflicts. |
ae3ff3b
to
3f876ab
Compare
3f876ab
to
d1f9caf
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Closes #3888.
There's nothing to do, really. All of the API is already supported by
miri
, but I just tweaked tests a bit to match the data structures and functions frombionic
.