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

Implement va_arg for AArch64 Linux #62207

Closed
wants to merge 1 commit into from

Conversation

@parched
Copy link
Contributor

commented Jun 28, 2019

Fixes #56475

WIP. I would like some feedback before I tidy it up and properly verify it.

Is this approach of implementing it in rust instead of an intrinsic fine? I did it this way because it was simpler for me to write and should be simpler to understand. Does it matter that I added a hidden method to VaArgSafe?

cc @dlrobertson, #59625 (@joshtriplett @eddyb @ahomescu)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2019

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@dlrobertson

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

Is this approach of implementing it in rust instead of an intrinsic fine?

It is something I wanted to experiment with. It certainly is much easier to read and less error prone than src/librustc_codegen_llvm/va_arg.rs

CC #56489

pub trait VaArgSafe {
#[doc(hidden)]
#[cfg(not(bootstrap))]
unsafe fn __va_arg(ap: &mut super::VaListImpl<'_>) -> Self;

This comment has been minimized.

Copy link
@ahomescu

ahomescu Jul 1, 2019

Contributor

I'm not sure about the double-underscore prefix, wouldn't va_arg_internal be a better name?

This comment has been minimized.

Copy link
@eddyb

eddyb Jul 1, 2019

Member

It's sealed so va_arg or va_arg_from_list would be fine.

($($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 {

This comment has been minimized.

Copy link
@ahomescu

ahomescu Jul 1, 2019

Contributor

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).

ptr::read((self.gr_top as usize).wrapping_add(offs as usize) as *const T)
}

// Rust doesn't support these yet

This comment has been minimized.

Copy link
@ahomescu

ahomescu Jul 1, 2019

Contributor

Would moving this implementation into the compiler (inside src/librustc_codegen_llvm/va_arg.rs) let you implement this?

This comment has been minimized.

Copy link
@ahomescu

ahomescu Jul 1, 2019

Contributor

Actually, are these SIMD types? There are some of those in nightly, even for AArch64.

This comment has been minimized.

Copy link
@parched

parched Jul 6, 2019

Author Contributor

Yeah it would and allow supporting aggregates in general. What I meant by this comment was rust doesn't support aggregates in variadic functions (yet) so it doesn't need to be implemented.

@@ -268,35 +268,86 @@ 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 {

This comment has been minimized.

Copy link
@ahomescu

ahomescu Jul 1, 2019

Contributor

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

@ahomescu

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

I added some comments (mostly just nitpicks).

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

This comment has been minimized.

Copy link
@eddyb

eddyb Jul 1, 2019

Member

You can put the #[cfg] on the expression inside the body, I think?

This comment has been minimized.

Copy link
@parched

parched Jul 6, 2019

Author Contributor

Ah yes, I think this is just left over from my first iteration where they had different signatures.

@eddyb

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

How much complexity is left after all the decision-making based on the type is done?
I'm tempted to suggest all the va_arg logic should be in rustc_target::abi::call, but I'm not sure how to reify the information (or whether the existing ArgType infra is useful at all).

let nreg = (mem::size_of::<T>() as i32 + 7) / 8;
self.gr_offs = offs + (nreg * 8);
if self.gr_offs > 0 {
return self.va_arg_on_stack(); // overflowed reg save area

This comment has been minimized.

Copy link
@eddyb

eddyb Jul 1, 2019

Member

This reminds me I wanted to change how rustc_target::abi::call works for a long time, and make it compute "designations" such as "on-stack" or "in register, if available, otherwise on-stack", with per-target "resources" (such as general-purpose registers for args/return and floating-point registers etc.), and put all of that together to compute final "assignments", afterwards (and we could even cache the per-type computation).
If we also tracked which registers were being used, that might even help Cranelift (cc @sunfishcode).

The relevant point here is making the per-type information context-free, so that it can be used for both signatures and va_arg.

This comment has been minimized.

Copy link
@parched

parched Jul 6, 2019

Author Contributor

That sounds ideal but outside my current knowledge/ability. Perhaps I could have a look at that after fixing it this way.

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

☔️ The latest upstream changes (presumably #62376) made this pull request unmergeable. Please resolve the merge conflicts.

@parched

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

@eddyb

How much complexity is left after all the decision-making based on the type is done?

Basically just checking whether you've overflowed the number of registers and need to go to the stack save area.

@parched parched force-pushed the parched:va-args branch from e0a5b40 to daa7185 Jul 6, 2019

@parched

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

I've tidied it up a bit and extended the tests a bit which now pass (x.py test log if anyone's interested)

@parched parched marked this pull request as ready for review Jul 6, 2019

fn va_arg<T: sealed_trait::VaArgSafe>(ap: &mut VaListImpl<'_>) -> T;
}

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

This comment has been minimized.

Copy link
@ahomescu

ahomescu Jul 10, 2019

Contributor

You could move this to the top of the file.

@dlrobertson
Copy link
Contributor

left a comment

After a review of this, this seems like a reasonable way to prototype support for new architectures. It is similar to how va_list-rs implements va_arg. The calculation also seems simple enough to implement in IR if we want to go down that road.

@joshtriplett @eddyb thoughts on implementing va_arg in pure rust? I'm not a fan of the sprawl of va_arg implementations we'd get, but that is the only drawback I can see so far. After this, we'd have some architectures rely on LLVMs va_arg (x86_64), others rely on custom LLVM IR we generate (i686), and then we'd have Aarch64 in libstd. Ultimately I'd like to pick one and try to support them all in one place.

{
if
/* classof(type) != "aggregate" && */
mem::size_of::<T>() < reg_size {

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Jul 10, 2019

Contributor

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
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

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Jul 10, 2019

Contributor

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

}

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);

This comment has been minimized.

Copy link
@dlrobertson

dlrobertson Jul 10, 2019

Contributor

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.

@eddyb

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Basically just checking whether you've overflowed the number of registers and need to go to the stack save area.

In that case, you should be able to make this pretty general by having an int/float register decision based on the type and then emitting the appropriate IR.
But I'll look at the implementation, maybe we can punt on the ideal solution.

ptr::read(begin as *const T)
}

unsafe fn va_arg_gr_or_vr<T: sealed_trait::VaArgSafe>(

This comment has been minimized.

Copy link
@eddyb

eddyb Jul 11, 2019

Member

I would rename this to va_arg_reg.

*r_offs = end as i32;
if *r_offs > 0 {
return Self::va_arg_on_stack(stack); // overflowed reg save area
}

This comment has been minimized.

Copy link
@eddyb

eddyb Jul 11, 2019

Member

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

@eddyb

eddyb approved these changes Jul 11, 2019

Copy link
Member

left a comment

r=me with @dlrobertson's, @ahomescu's and my nits

@hdhoang

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Ping from triage @parched, could you implement the reviewers' comments?

@JohnCSimon

This comment has been minimized.

Copy link

commented Aug 3, 2019

Pinging again @parched can you please address this? Thanks.
@kennytm

@kennytm

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned kennytm Aug 3, 2019

@parched

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Sorry, just on holiday at the moment, but will get on to it this coming week.

@parched

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Closing for now, until I have time to come back to this, hopefully that will be next week.

@parched parched closed this Aug 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.