-
Notifications
You must be signed in to change notification settings - Fork 352
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
add basic support for android targets #2011
Conversation
7828dcc
to
8c0242d
Compare
Shouldn't it be enough to implement one of them?
Which target triple do you even use for Android? Do some simple demos work? If there is concurrency involved you might be seeing #1388. You can use Miri's measureme support to figure out where the time is spent. It would be good to add at least a smoke test to
except it should probably go through |
Oh, I just saw this:
Are you sure? Miri uses check-only builds of the stdlib. I can build the macOS and even Windows stdlib for Miri on my Linux without installing anything for those targets. The same should work for Android I think (and if not we should fix that). |
8c0242d
to
a5820cd
Compare
I'm not 100% sure on this. I'm not an Android expert, I just happen to be working on a project that targets Android at the moment. While trying to run Miri initially I get an stdlib build failure here: https://github.com/rust-lang/rust/blob/10dccdc7fcbdc64ee9efe2c1ed975ab8c1d61287/library/unwind/build.rs#L13
I use
A "Hello, world!" application works fine 🤷♂️ |
Ah, and it'll do that even for a check-only build. |
9fa12fb
to
494cde2
Compare
I think I misunderstood what was going on here since 0 tests were run. Still not sure how to avoid the UI test failure though. |
That is very odd... so |
Correct. I think the test runner performs this check for just the UI tests because it expects to get stdout/stderr from a process running on the Android device over ADB. The code assumes that is if it's Android, it'll always talk over ADB. |
Ah, compiletest special-cases android targets... that is annoying. :/ What happens when you just set Line 45 in 2da6bed
|
Perfect, that fixes the issue. What's better yet, after fixing a couple lingering issues it seems most tests pass:
The failures here for the most part seem to be related to |
Nice. :) Seems fine to make them |
Ok, I'm stopping there for the night. I've gone down the rabbit hole of adding new foreign items as they pop up while running tests. Seems like the min supported Android API for Rust is missing a lot of modern Linux APIs that are otherwise used.
well I guess by asking that question I'm admitting it's grown in scope. I'm going to go ahead and pull the later changes into a smaller PR. |
54f48ae
to
6023eb8
Compare
RIP, we hit the |
@RalfJung do you know enough about |
Sorry, no I don't. Maybe you could open an issue, then I can Cc the author of that support? |
Yeah, splitting things up into smaller chunks helps a lot with reviewing. Thanks. :) Now that you got more and more things to run, what is your thinking on treating Android as a separate Posix/Unix variant vs merging it with Linux? (Other ways to share code might be to factor things into common helper functions -- like how all the FS logic is shared between Linux and macOS.) |
All tests now pass, but you must still make the necessary change to the stdlib's |
@@ -214,6 +214,9 @@ degree documented below): | |||
supported on Windows. We also test `i686-pc-windows-msvc`, with the same | |||
reduced feature set. We might ship Miri with a nightly even when some features | |||
on these targets regress. | |||
- `aarch64-linux-android` has basic support and should work for simple targets. |
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.
What are "simple targets"?
Also it seems a bit premature to add this if we don't have anything on CI and people need to patch stdlib to make it work.
|
||
/// Helper function used inside the shims of foreign functions to check that the `target_os` is | ||
/// part of the UNIX family. | ||
pub fn target_os_is_unix(target_os: &str) -> bool { |
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.
Elsewhere we are using target.families.iter().any(|f| f == "unix")
. I think that probably makes more sense?
So, please change this to take a &Target
and also adjust the existing checks to call this function instead.
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.
Actually, thinking about it more -- I like explicitly listing the unix OSes that we do support. So let's go with your approach.
But please change the existing code that checks target.families
to call target_os_is_unix
instead.
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.
Could you or @InfRandomness make a separate PR that does nothing but add target_os_is_unix
and use it where appropriate? Then both the FreeBSD PR and this one can just adjust that function and don't need to change so many different places in the code.
this.write_scalar(Scalar::from_machine_usize(0, this), &place.into())?; | ||
Self::add_extern_static(this, name, place.ptr); | ||
} | ||
for symbol_name in &["signal", "bsd_signal"] { |
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.
for symbol_name in &["signal", "bsd_signal"] { | |
// Some weak symbols that we *do* (have to) support. | |
for symbol_name in &["signal", "bsd_signal"] { |
this.write_scalar(Scalar::from_machine_usize(0, this), &place.into())?; | ||
Self::add_extern_static(this, name, place.ptr); | ||
} | ||
for symbol_name in &["signal", "bsd_signal"] { |
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.
Shouldn't it be enough to implement one of them?
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.
Unfortunately not. If you try to run without both you get the following panic:
error: unsupported operation: `extern` static `std::sys::unix::android::signal::bsd_signal` from crate `std` is not supported by Miri
--> /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/unix/android.rs:76:5
|
76 | weak!(fn bsd_signal(c_int, sighandler_t) -> sighandler_t);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `extern` static `std::sys::unix::android::signal::bsd_signal` from crate `std` is not supported by Miri
|
= help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support
= note: inside `std::sys::unix::android::signal` at /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/unix/weak.rs:41:38
= note: inside `std::sys::unix::init::reset_sigpipe` at /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/unix/mod.rs:122:19
= note: inside `std::sys::unix::init` at /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/sys/unix/mod.rs:64:5
= note: inside `std::rt::init` at /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/rt.rs:78:9
= note: inside closure at /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/rt.rs:127:42
= note: inside `std::panicking::try::do_call::<[closure@std::rt::lang_start_internal::{closure#1}], ()>` at /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/panicking.rs:492:40
= note: inside `std::panicking::try::<(), [closure@std::rt::lang_start_internal::{closure#1}]>` at /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/panicking.rs:456:19
= note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{closure#1}], ()>` at /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/panic.rs:137:14
= note: inside `std::rt::lang_start_internal` at /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/rt.rs:127:5
= note: inside `std::rt::lang_start::<()>` at /Users/hvx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/rt.rs:144:17
= note: this error originates in the macro `weak` (in Nightly builds, run with -Z macro-backtrace for more info)
This is caused by the standard library weak linking both symbols https://github.com/rust-lang/rust/blob/1eb72580d076935a3e590deb6e5813a5aef3eca4/library/std/src/sys/unix/android.rs#L74-L81
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.
But you can make one of them NULL. Only one needs to be implemented.
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.
Ah I see what you mean. Can do.
let ecx = this.eval_context_mut(); | ||
let ptr = ecx.create_fn_alloc_ptr(FnVal::Other(dlsym)); |
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.
let ecx = this.eval_context_mut(); | |
let ptr = ecx.create_fn_alloc_ptr(FnVal::Other(dlsym)); | |
let ptr = this.create_fn_alloc_ptr(FnVal::Other(dlsym)); |
let ptr = ecx.create_fn_alloc_ptr(FnVal::Other(dlsym)); | ||
|
||
let place = this.allocate(layout, MiriMemoryKind::ExternStatic.into())?; | ||
this.write_pointer(ptr, &place.into())?; |
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.
Can you add a helper function that takes the symbol name and returns a Place
containing a pointer pointing to that function?
I'd like to keep init_extern_statics
as short and readable as possible.
@@ -883,3 +905,9 @@ impl std::fmt::Display for HexRange { | |||
write!(f, "[{:#x}..{:#x}]", self.0.start.bytes(), self.0.end().bytes()) | |||
} | |||
} | |||
|
|||
/// Helper function used inside the shims of foreign functions to check that the `target_os` is | |||
/// part of the UNIX family. |
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.
/// part of the UNIX family. | |
/// a supported OS in the UNIX family. |
throw_ub_format!("memalign: alignment must be a power of two, but is {}", align); | ||
} | ||
|
||
let ptr = this.allocate_ptr( |
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.
Like malloc
, this probably should return NULL if the size is 0.
@@ -134,7 +135,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx | |||
|
|||
let sys_getrandom = this.eval_libc("SYS_getrandom")?.to_machine_usize(this)?; | |||
|
|||
let sys_statx = this.eval_libc("SYS_statx")?.to_machine_usize(this)?; | |||
let sys_statx = if target_os == "android" { | |||
// libc's Android target currently does not support SYS_statx -- fail gracefully |
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.
Isn't this doing a syscall basically directly to the kernel?
If we need such an if
, this is a strong indication that we should not share this code between android and linux, IMO.
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.
That's correct. This special case is required as the libc
crate does not support a lot of constants and structures for Android even though they do exist, someone just has to make a PR. If you agree we should copy the Linux handlers to the Android foreign_items
to separate them then I can just remove this condition.
_ => { | ||
// By default we piggyback off the items emulated for Linux. For now this functions | ||
// serves to make adding android-specific syscalls easy | ||
return shims::posix::linux::foreign_items::EvalContextExt::emulate_foreign_item_by_name(this, link_name, abi, args, dest, ret); |
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 am not very happy about this fallback. The linux functions are not prepared for android use, and we didn't check that their implementation is correct against their android specification. Android has its completely separate libc, and the API we are implementing here is basically that of libc.
So, I am not convinced that we want to fall back like this here; to me this seems to carry a huge risk of accidentally using the wrong shim implementation.
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.
That makes sense. I originally went with this approach since Android is Linux-based, but you're right that if we're implementing the libc API, we should probably ensure these are separate.
What would you suggest, just copying the Linux code here?
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 would suggest copying enough of the Linux code to get hello.rs
working on Android -- and leaving everything else to a future PR.
@@ -1,4 +1,5 @@ | |||
// ignore-windows: Concurrency on Windows is not supported yet. | |||
// ignore-android: `libc` crate does not support `PR_SET_NAME` on Android |
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.
This code does not directly call libc
though. It just uses standard library APIs. So it should work on Android as well, right?
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.
Sorry, these comments in the tests were notes about the root cause of why the feature is not supported, and therefore why the test is ignored. I can reword this and the other locations to be more concise.
FWIW this is the (trimmed) panic you receive running this test:
thread '<unnamed>' panicked at 'Hello!', tests/pass/concurrency/simple.rs:55:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'rustc' panicked at 'failed to find required Rust item: ["libc", "PR_SET_NAME"]', src/helpers.rs:82:32
stack backtrace:
0: 0x1027845f4 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h34a40067a35bfbec
1: 0x1027def6b - core::fmt::write::h90d56a63b82c1d78
2: 0x102776c18 - std::io::Write::write_fmt::h9a11e832e4a1e526
3: 0x10278797d - std::panicking::default_hook::{{closure}}::h50f5199bcbe28750
4: 0x1027876f7 - std::panicking::default_hook::h95b189cf93ee7eab
5: 0x10fe9a05d - rustc_driver[8cbe49adf889e25c]::DEFAULT_HOOK::{closure#0}::{closure#0}
6: 0x102787fdb - std::panicking::rust_panic_with_hook::hde5cf82e5f0b952b
7: 0x102787e53 - std::panicking::begin_panic_handler::{{closure}}::h02042140a786e7a4
8: 0x102784a77 - std::sys_common::backtrace::__rust_end_short_backtrace::ha59ad93f482bafe0
9: 0x102787b2a - _rust_begin_unwind
10: 0x102808703 - core::panicking::panic_fmt::hd46cbc4cb5821886
11: 0x101e2404f - miri::helpers::EvalContextExt::resolve_path::{{closure}}::h0ec9ed0c1bb395ff
at /Users/hvx/dev/miri/src/helpers.rs:82:32
12: 0x101e2404f - core::option::Option<T>::unwrap_or_else::h0d7d7e05cfeea290
at /rustc/e71440575c930dcecac288b7c3536410d688b351/library/core/src/option.rs:825:21
13: 0x101e2404f - miri::helpers::EvalContextExt::resolve_path::h00886eb1a3d5bd0f
at /Users/hvx/dev/miri/src/helpers.rs:81:9
14: 0x101e2404f - miri::helpers::EvalContextExt::eval_path_scalar::h8283fff130397323
at /Users/hvx/dev/miri/src/helpers.rs:89:24
15: 0x101e65095 - miri::helpers::EvalContextExt::eval_libc::h5c2d4c27033a0621
at /Users/hvx/dev/miri/src/helpers.rs:98:9
16: 0x101e65095 - miri::helpers::EvalContextExt::eval_libc_i32::h506af73185e8afd7
at /Users/hvx/dev/miri/src/helpers.rs:104:9
17: 0x101e65095 - miri::shims::posix::thread::EvalContextExt::prctl::he65e14925b5442ba
at /Users/hvx/dev/miri/src/shims/posix/thread.rs:110:22
18: 0x101e65fb4 - miri::shims::posix::linux::foreign_items::EvalContextExt::emulate_foreign_item_by_name::h2a3aca9b29812087
at /Users/hvx/dev/miri/src/shims/posix/linux/foreign_items.rs:110:30
19: 0x101e65796 - miri::shims::posix::android::foreign_items::EvalContextExt::emulate_foreign_item_by_name::h2477024ea6b084a5
at /Users/hvx/dev/miri/src/shims/posix/android/foreign_items.rs:24:24
20: 0x101e4fc1c - miri::shims::posix::foreign_items::EvalContextExt::emulate_foreign_item_by_name::h972a07cae6d0156c
at /Users/hvx/dev/miri/src/shims/posix/foreign_items.rs:499:41
21: 0x101e2fb1c - miri::shims::foreign_items::EvalContextExt::emulate_foreign_item_by_name::hc7eb92b0e9675c1b
at /Users/hvx/dev/miri/src/shims/foreign_items.rs:705:63
22: 0x101df5e85 - miri::shims::foreign_items::EvalContextExt::emulate_foreign_item::hf8d1dc31e7e38b74
at /Users/hvx/dev/miri/src/shims/foreign_items.rs:302:15
23: 0x101df5e85 - miri::shims::EvalContextExt::find_mir_or_eval_fn::h67395dcac9bbe974
at /Users/hvx/dev/miri/src/shims/mod.rs:54:20
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.
Right, that is caused by you calling our linux-specific implementation of prctl
on a non-Linux target.
This confirms my stance that android should not just fall back to the Linux functions. That leads to the weirdest failures, in the best case -- or to silently wrong behavior in the worst case.
@@ -1,4 +1,5 @@ | |||
// ignore-windows: Concurrency on Windows is not supported yet. | |||
// ignore-android: `libc` crate does not support `gettimeofday()` on Android |
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.
Please reword this to document which stdlib API Miri does not support on Android. (I assume with rustc, this code here works fine on Android.)
@@ -1,3 +1,4 @@ | |||
// ignore-android: No foreign function support for __errno yet |
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.
What does this have to do with what the test is doing?
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.
Same as the test/pass/concurrency/simple.rs
comment this is relating to the panic raised when actually running the test. I won't blow up your notifications on each instance here, but that's the general theme with the ignore-android
comments.
@@ -1,3 +1,5 @@ | |||
// ignore-android: libc for Android does not have `SYS_statx` |
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.
What does the hashmap have to do with statx?
@@ -1,4 +1,5 @@ | |||
// ignore-windows: No libc on Windows | |||
// ignore-android: libc's Android target does not have support for __errno_location yet |
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.
And that affects all tests in this file?
let ret = ret.expect("we don't support any diverging dlsym"); | ||
|
||
match dlsym { | ||
Dlsym::signal => { |
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.
The POSIX version of this is gated on this.frame_in_std()
, probably we should do the same here? After all we don't actually do anything...
☔ The latest upstream changes (presumably #2198) made this pull request unmergeable. Please resolve the merge conflicts. |
@@ -50,6 +51,8 @@ case $HOST_TARGET in | |||
MIRI_TEST_TARGET=i686-unknown-linux-gnu run_tests | |||
MIRI_TEST_TARGET=aarch64-apple-darwin run_tests | |||
MIRI_TEST_TARGET=i686-pc-windows-msvc run_tests | |||
# TODO: Enable once https://github.com/rust-lang/rust/pull/94813 lands | |||
# MIRI_TEST_TARGET=aarch64-linux-android run_tests |
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.
Could you add a note that @landaire is maintaining this target -- so that we know whom to ping when something android-specific breaks?
Btw we have the concept of a "minimal" target now that only runs a subset of the tests. It's a great way to get started with a new target and then improve its support step-by-step. So IMO the first PR should just add |
I am going to close this PR due to inactivity; @landaire feel free to reopen if/when you get back to this. :) |
FWIW #2493 resurrects a part of this PR to get at least very basic android support. I don't plan to spend any more time on this though. |
@RalfJung I apologize for dragging my feet on this. It was on my TODO list to come back around but I've been too busy give it the necessary focus to make the changes necessary. Thanks for taking the lead. |
This PR fixes #2010 by adding platform support for Android. This does a couple of things:
signal
andbsd_signal
syscalls. This is handled similarly todlsym
(dynamically linked) items where we call into theDlsym::from_str
for each weakly linked symbol, allocate a function for them, and write the pointer to the function at the slot for the weakly linked item.android
as a supported platform forshims::posix::foreign_item
emulationshims::posix::linux::foreign_items
for foreign item emulationThis is marked as a draft until I figure out why my test (aka prod) application is hanging on one of its tests. I'm not sure if it's just really expensive and runs slow, or if one of the emulated syscalls works differently on Android.
Currently though this is enough support to run basic unit tests.
TODO:
check
builds rust#94813