Skip to content

Implement va_arg for AArch64 Linux #62207

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 114 additions & 17 deletions src/libcore/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,35 +268,73 @@ mod sealed_trait {
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "44930")]
pub trait VaArgSafe {}
pub trait VaArgSafe {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe VaArgSafe should be renamed to something more appropriate, like VaArgType.

#[doc(hidden)]
unsafe fn va_arg(ap: &mut super::VaListImpl<'_>) -> Self;
}
}

macro_rules! impl_va_arg_safe {
macro_rules! impl_va_arg_safe_integer {
($($t:ty),+) => {
$(
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "44930")]
impl sealed_trait::VaArgSafe for $t {}
impl sealed_trait::VaArgSafe for $t {
Copy link
Contributor

Choose a reason for hiding this comment

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

This patch adds a lot of this boilerplate, I'm wondering if there's a way to cut it down (maybe using the type system).

unsafe fn va_arg(ap: &mut VaListImpl<'_>) -> Self {
#[cfg(not(all(target_arch = "aarch64", not(target_os = "ios"), not(windows))))]
return va_arg(ap);
#[cfg(all(target_arch = "aarch64", not(target_os = "ios"), not(windows)))]
return ap.va_arg_gr();
}
}
)+
}
}

impl_va_arg_safe!{i8, i16, i32, i64, usize}
impl_va_arg_safe!{u8, u16, u32, u64, isize}
impl_va_arg_safe!{f64}
macro_rules! impl_va_arg_safe_float {
($($t:ty),+) => {
$(
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "44930")]
impl sealed_trait::VaArgSafe for $t {
unsafe fn va_arg(ap: &mut VaListImpl<'_>) -> Self {
#[cfg(not(all(target_arch = "aarch64", not(target_os = "ios"), not(windows))))]
return va_arg(ap);
#[cfg(all(target_arch = "aarch64", not(target_os = "ios"), not(windows)))]
return ap.va_arg_vr();
}
}
)+
}
}

#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "44930")]
impl<T> sealed_trait::VaArgSafe for *mut T {}
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "44930")]
impl<T> sealed_trait::VaArgSafe for *const T {}
macro_rules! impl_va_arg_safe_pointer {
($($t:ident),+) => {
$(
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "44930")]
impl<T> sealed_trait::VaArgSafe for *$t T {
unsafe fn va_arg(ap: &mut VaListImpl<'_>) -> Self {
#[cfg(not(all(target_arch = "aarch64", not(target_os = "ios"), not(windows))))]
return va_arg(ap);
#[cfg(all(target_arch = "aarch64", not(target_os = "ios"), not(windows)))]
return ap.va_arg_gr();
}
}
)+
}
}

impl_va_arg_safe_integer! {i8, i16, i32, i64, usize}
impl_va_arg_safe_integer! {u8, u16, u32, u64, isize}
impl_va_arg_safe_float! {f64}
impl_va_arg_safe_pointer! {mut, const}

#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
Expand All @@ -306,7 +344,7 @@ impl<'f> VaListImpl<'f> {
/// Advance to the next arg.
#[inline]
pub unsafe fn arg<T: sealed_trait::VaArgSafe>(&mut self) -> T {
va_arg(self)
T::va_arg(self)
}

/// Copies the `va_list` at the current location.
Expand Down Expand Up @@ -363,5 +401,64 @@ extern "rust-intrinsic" {

/// Loads an argument of type `T` from the `va_list` `ap` and increment the
/// argument `ap` points to.
#[cfg(not(all(target_arch = "aarch64", not(target_os = "ios"), not(windows))))]
fn va_arg<T: sealed_trait::VaArgSafe>(ap: &mut VaListImpl<'_>) -> T;
}

#[cfg(all(target_arch = "aarch64", not(target_os = "ios"), not(windows)))]
Copy link
Contributor

Choose a reason for hiding this comment

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

You could move this to the top of the file.

use crate::{mem, ptr};

// Implemenation for AArch64 linux (and others that follow the standard PCS)
// based on https://developer.arm.com/docs/ihi0055/d/procedure-call-standard-for-the-arm-64-bit-architecture
// APPENDIX Variable argument Lists -> The va_start() macro
#[cfg(all(target_arch = "aarch64", not(target_os = "ios"), not(windows)))]
impl<'f> VaListImpl<'f> {
unsafe fn get_begin_and_end<T>(current_pos: usize, reg_size: usize) -> (usize, usize) {
let mut begin = current_pos;
if mem::align_of::<T>() > 8 && reg_size == 8 {
begin = begin.wrapping_add(15) & !15; // round up
Copy link
Contributor

Choose a reason for hiding this comment

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

The spec uses -16. Is there a reason to use !15 instead.

}
let nreg = (mem::size_of::<T>() + reg_size - 1) / reg_size;
let end = begin.wrapping_add(nreg * reg_size);
#[cfg(target_endian = "big")]
{
if
/* classof(type) != "aggregate" && */
mem::size_of::<T>() < reg_size {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spacing. Could you change this comment to something like

FIXME(parched): When aggregate types are supported by `VaList::arg` we
should ensure the return type is not an aggregate type before this calculation

begin = begin.wrapping_add(reg_size - mem::size_of::<T>());
}
}
(begin, end)
}

unsafe fn va_arg_on_stack<T: sealed_trait::VaArgSafe>(stack: &mut *mut c_void) -> T {
let (begin, end) = Self::get_begin_and_end::<T>(*stack as usize, 8);
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this can give us different values than the spec if the stack isn't 8 byte aligned, but I suppose much worse things would be happening if that were the case.

*stack = end as *mut c_void;
ptr::read(begin as *const T)
}

unsafe fn va_arg_gr_or_vr<T: sealed_trait::VaArgSafe>(
Copy link
Member

Choose a reason for hiding this comment

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

I would rename this to va_arg_reg.

r_top: *mut c_void,
r_offs: &mut i32,
stack: &mut *mut c_void,
reg_size: usize,
) -> T {
if *r_offs >= 0 {
return Self::va_arg_on_stack(stack); // reg save area empty
}
let (begin, end) = Self::get_begin_and_end::<T>(*r_offs as usize, reg_size);
*r_offs = end as i32;
if *r_offs > 0 {
return Self::va_arg_on_stack(stack); // overflowed reg save area
}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be possible to do this check only once?

ptr::read((r_top as usize).wrapping_add(begin) as *const T)
}

unsafe fn va_arg_gr<T: sealed_trait::VaArgSafe>(&mut self) -> T {
Self::va_arg_gr_or_vr(self.gr_top, &mut self.gr_offs, &mut self.stack, 8)
}

unsafe fn va_arg_vr<T: sealed_trait::VaArgSafe>(&mut self) -> T {
Self::va_arg_gr_or_vr(self.vr_top, &mut self.vr_offs, &mut self.stack, 16)
}
}
30 changes: 30 additions & 0 deletions src/test/run-make-fulldeps/c-link-to-rust-va-list-fn/checkrust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,33 @@ pub unsafe extern "C" fn check_varargs_1(_: c_int, mut ap: ...) -> usize {
pub unsafe extern "C" fn check_varargs_2(_: c_int, _ap: ...) -> usize {
0
}

#[no_mangle]
pub unsafe extern "C" fn check_varargs_3(_: c_int, mut ap: ...) -> usize {
continue_if!(ap.arg::<c_int>() == 1);
continue_if!(ap.arg::<c_int>() == 2);
continue_if!(ap.arg::<c_int>() == 3);
continue_if!(ap.arg::<c_int>() == 4);
continue_if!(ap.arg::<c_int>() == 5);
continue_if!(ap.arg::<c_int>() == 6);
continue_if!(ap.arg::<c_int>() == 7);
continue_if!(ap.arg::<c_int>() == 8);
continue_if!(ap.arg::<c_int>() == 9);
continue_if!(ap.arg::<c_int>() == 10);
0
}

#[no_mangle]
pub unsafe extern "C" fn check_varargs_4(_: c_double, mut ap: ...) -> usize {
continue_if!(ap.arg::<c_double>() == 1.0);
continue_if!(ap.arg::<c_double>() == 2.0);
continue_if!(ap.arg::<c_double>() == 3.0);
continue_if!(ap.arg::<c_double>() == 4.0);
continue_if!(ap.arg::<c_double>() == 5.0);
continue_if!(ap.arg::<c_double>() == 6.0);
continue_if!(ap.arg::<c_double>() == 7.0);
continue_if!(ap.arg::<c_double>() == 8.0);
continue_if!(ap.arg::<c_double>() == 9.0);
continue_if!(ap.arg::<c_double>() == 10.0);
0
}
8 changes: 8 additions & 0 deletions src/test/run-make-fulldeps/c-link-to-rust-va-list-fn/test.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ extern size_t check_list_copy_0(va_list ap);
extern size_t check_varargs_0(int fixed, ...);
extern size_t check_varargs_1(int fixed, ...);
extern size_t check_varargs_2(int fixed, ...);
extern size_t check_varargs_3(int fixed, ...);
extern size_t check_varargs_4(double fixed, ...);

int test_rust(size_t (*fn)(va_list), ...) {
size_t ret = 0;
Expand All @@ -36,5 +38,11 @@ int main(int argc, char* argv[]) {

assert(check_varargs_2(0, "All", "of", "these", "are", "ignored", ".") == 0);

// make sure we overflow the argument registers on AArch64
assert(check_varargs_3(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10) == 0);

// make sure we overflow the argument registers on AArch64
assert(check_varargs_4(0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0) == 0);

return 0;
}