-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ function run_tests { | |
fi | ||
|
||
./miri test --locked | ||
|
||
if [ -z "${MIRI_TEST_TARGET+exists}" ]; then | ||
# Only for host architecture: tests with optimizations (`-O` is what cargo passes, but crank MIR | ||
# optimizations up all the way). | ||
|
@@ -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 commentThe 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? |
||
;; | ||
x86_64-apple-darwin) | ||
MIRI_TEST_TARGET=mips64-unknown-linux-gnuabi64 run_tests # big-endian architecture | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -493,6 +493,28 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx | |||||
) | ||||||
} | ||||||
|
||||||
/// Helper function used inside the shims of foreign functions to assert that the target OS | ||||||
/// is based on the Linux kernel. It panics showing a message with the `name` of the foreign function | ||||||
/// if this is not the case. | ||||||
fn assert_target_os_is_linux_based(&self, name: &str) { | ||||||
assert!( | ||||||
matches!(self.eval_context_ref().tcx.sess.target.os.as_ref(), "linux" | "android"), | ||||||
"`{}` is only available for supported Linux-based targets", | ||||||
name, | ||||||
) | ||||||
} | ||||||
|
||||||
/// Helper function used inside the shims of foreign functions to assert that the target OS | ||||||
/// is part of the UNIX family. It panics showing a message with the `name` of the foreign function | ||||||
/// if this is not the case. | ||||||
fn assert_target_os_is_unix(&self, name: &str) { | ||||||
assert!( | ||||||
target_os_is_unix(self.eval_context_ref().tcx.sess.target.os.as_ref()), | ||||||
"`{}` is only available for supported UNIX family targets", | ||||||
name, | ||||||
); | ||||||
} | ||||||
|
||||||
/// Get last error variable as a place, lazily allocating thread-local storage for it if | ||||||
/// necessary. | ||||||
fn last_error_place(&mut self) -> InterpResult<'tcx, MPlaceTy<'tcx, Tag>> { | ||||||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
pub fn target_os_is_unix(target_os: &str) -> bool { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Elsewhere we are using So, please change this to take a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
matches!(target_os, "linux" | "macos" | "android") | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -430,6 +430,41 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> { | |||||||
this.write_scalar(Scalar::from_u8(0), &place.into())?; | ||||||||
Self::add_extern_static(this, "_tls_used", place.ptr); | ||||||||
} | ||||||||
"android" => { | ||||||||
// "environ" | ||||||||
Self::add_extern_static( | ||||||||
this, | ||||||||
"environ", | ||||||||
this.machine.env_vars.environ.unwrap().ptr, | ||||||||
); | ||||||||
// A couple zero-initialized pointer-sized extern statics. | ||||||||
// Most of them are for weak symbols, which we all set to null (indicating that the | ||||||||
// symbol is not supported, and triggering fallback code which ends up calling a | ||||||||
// syscall that we do support). | ||||||||
for name in &["__cxa_thread_atexit_impl", "getrandom", "statx"] { | ||||||||
let layout = this.machine.layouts.usize; | ||||||||
let place = this.allocate(layout, MiriMemoryKind::ExternStatic.into())?; | ||||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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:
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see what you mean. Can do. |
||||||||
let layout = this.machine.layouts.mut_raw_ptr; | ||||||||
RalfJung marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
let dlsym = Dlsym::from_str(symbol_name.as_bytes(), &this.tcx.sess.target.os)? | ||||||||
.unwrap_or_else(|| { | ||||||||
panic!( | ||||||||
"hardcoded `extern static` symbol {} has no dlsym handler", | ||||||||
symbol_name | ||||||||
) | ||||||||
}); | ||||||||
let ecx = this.eval_context_mut(); | ||||||||
let ptr = ecx.create_fn_alloc_ptr(FnVal::Other(dlsym)); | ||||||||
Comment on lines
+459
to
+460
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
|
||||||||
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 commentThe 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 |
||||||||
|
||||||||
Self::add_extern_static(this, symbol_name, place.ptr); | ||||||||
} | ||||||||
} | ||||||||
_ => {} // No "extern statics" supported on this target | ||||||||
} | ||||||||
Ok(()) | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
use helpers::check_arg_count; | ||
use log::trace; | ||
use rustc_middle::mir; | ||
|
||
use crate::*; | ||
|
||
#[derive(Debug, Copy, Clone)] | ||
#[allow(non_camel_case_types)] | ||
pub enum Dlsym { | ||
signal, | ||
} | ||
|
||
impl Dlsym { | ||
// Returns an error for unsupported symbols, and None if this symbol | ||
// should become a NULL pointer (pretend it does not exist). | ||
pub fn from_str<'tcx>(name: &str) -> InterpResult<'tcx, Option<Dlsym>> { | ||
Ok(match &*name { | ||
"__pthread_get_minstack" => None, | ||
"getrandom" => None, // std falls back to syscall(SYS_getrandom, ...) when this is NULL. | ||
"statx" => None, // std falls back to syscall(SYS_statx, ...) when this is NULL. | ||
"signal" | "bsd_signal" => Some(Dlsym::signal), // these have the same signature/implementation | ||
"android_set_abort_message" => None, // std falls back to just not doing anything when this is NULL. | ||
_ => throw_unsup_format!("unsupported Android dlsym: {}", name), | ||
}) | ||
} | ||
} | ||
|
||
impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} | ||
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { | ||
fn call_dlsym( | ||
&mut self, | ||
dlsym: Dlsym, | ||
args: &[OpTy<'tcx, Tag>], | ||
dest: &PlaceTy<'tcx, Tag>, | ||
ret: Option<mir::BasicBlock>, | ||
) -> InterpResult<'tcx> { | ||
let this = self.eval_context_mut(); | ||
assert!(this.tcx.sess.target.os == "android"); | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. The POSIX version of this is gated on |
||
let &[ref _sig, ref _func] = check_arg_count(args)?; | ||
this.write_null(dest)?; | ||
} | ||
} | ||
|
||
trace!("{:?}", this.dump_place(**dest)); | ||
this.go_to_block(ret); | ||
Ok(()) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
use rustc_middle::mir; | ||
use rustc_span::Symbol; | ||
use rustc_target::spec::abi::Abi; | ||
|
||
use crate::*; | ||
use shims::foreign_items::EmulateByNameResult; | ||
|
||
impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} | ||
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { | ||
fn emulate_foreign_item_by_name( | ||
&mut self, | ||
link_name: Symbol, | ||
abi: Abi, | ||
args: &[OpTy<'tcx, Tag>], | ||
dest: &PlaceTy<'tcx, Tag>, | ||
ret: mir::BasicBlock, | ||
) -> InterpResult<'tcx, EmulateByNameResult<'mir, 'tcx>> { | ||
let this = self.eval_context_mut(); | ||
|
||
match &*link_name.as_str() { | ||
_ => { | ||
// 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest copying enough of the Linux code to get |
||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
pub mod dlsym; | ||
pub mod foreign_items; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -168,6 +168,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx | |
} | ||
this.write_null(dest)?; | ||
} | ||
"memalign" => { | ||
let &[ref align, ref size] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; | ||
let align = this.read_scalar(align)?.to_machine_usize(this)?; | ||
let size = this.read_scalar(size)?.to_machine_usize(this)?; | ||
|
||
// Align must be power of 2. | ||
if !align.is_power_of_two() { | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Like |
||
Size::from_bytes(size), | ||
Align::from_bytes(align).unwrap(), | ||
MiriMemoryKind::C.into(), | ||
)?; | ||
this.write_pointer(ptr, dest)?; | ||
} | ||
|
||
// Dynamic symbol loading | ||
"dlsym" => { | ||
|
@@ -479,6 +496,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx | |
match this.tcx.sess.target.os.as_ref() { | ||
"linux" => return shims::posix::linux::foreign_items::EvalContextExt::emulate_foreign_item_by_name(this, link_name, abi, args, dest, ret), | ||
"macos" => return shims::posix::macos::foreign_items::EvalContextExt::emulate_foreign_item_by_name(this, link_name, abi, args, dest, ret), | ||
"android" => return shims::posix::android::foreign_items::EvalContextExt::emulate_foreign_item_by_name(this, link_name, abi, args, dest, ret), | ||
_ => unreachable!(), | ||
} | ||
} | ||
|
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.