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

WIP android64 support #12

Closed
wants to merge 1 commit into from
Closed

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Mar 2, 2022

This isn't done yet but I'm submitting this as a PR so that I can iterate on this in a place where the code is visible, and discuss the issues I'm running into.

Some temp garbage is in here as I try a few different things out.

// nix currently doesn't support PTRACE_GETFPREGS, so we have to do it ourselves
fn getfpregs(pid: Pid) -> Result<libc_user_fpsimd_struct> {
Self::ptrace_get_data::<libc_user_fpsimd_struct>(
ptrace::Request::PTRACE_GETFPREGS,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've almost got everything reasonable and compiling, but the last remaining blocker is this code here, where we get each thread's registers with ptrace-based register lookup.

For whatever reason it appears that PTRACE_GETREGS and PTRACE_GETFPREGS just don't exist on aarch64 (android?). The docs note that the APIs don't exist on all platforms but it is just Weird that this was the platform to do it?

Not fully clear on what's up with the breakpad version, ifdef hell...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible I want PTRACE_GETREGSET, but the nix ptrace stuff also doesn't have that for android. Not clear if that's a bug tho.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I fear, I have not much to add here. I'm most certainly not an Android expert :-/

@msirringhaus
Copy link
Collaborator

msirringhaus commented Mar 2, 2022 via email

@Gankra
Copy link
Contributor Author

Gankra commented Mar 9, 2022

Yeah no problem.

I went down a really deep rabbit-hole on the whole u128 issue, and I am happy to report that I've demonstrated that u128/i128 are actually totally sound for FFI on exactly aarch64 (android AND macos)!

rust-lang/libc#2524 (comment)

Copy link
Collaborator

@msirringhaus msirringhaus left a comment

Choose a reason for hiding this comment

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

LGTM so far.

@@ -37,7 +38,8 @@ pub mod imp;
pub mod imp;

#[cfg(target_arch = "aarch64")]
pub type fpstate_t = libc::fpsimd_context; // Currently not part of libc! This will produce an error.
#[allow(non_camel_case_types)]
pub type fpstate_t = libc_user_fpsimd_struct; // Currently not part of libc! This will produce an error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you created your own struct, this will not produce an error anymore? So the second half of the comment can be removed, I think.

// nix currently doesn't support PTRACE_GETFPREGS, so we have to do it ourselves
fn getfpregs(pid: Pid) -> Result<libc_user_fpsimd_struct> {
Self::ptrace_get_data::<libc_user_fpsimd_struct>(
ptrace::Request::PTRACE_GETFPREGS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I fear, I have not much to add here. I'm most certainly not an Android expert :-/

pub struct layout_only_ffi_u128(u128);

impl layout_only_ffi_u128 {
pub fn to_ne_bytes(self) -> [u8; 16] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this function used?

@msirringhaus msirringhaus mentioned this pull request Mar 11, 2022
@Gankra
Copy link
Contributor Author

Gankra commented Mar 16, 2022

Slowly yak-shaving... nix-rust/nix#1679

@luser
Copy link

luser commented Mar 22, 2022

Not fully clear on what's up with the breakpad version, ifdef hell...

Breakpad vendors a copy of Google's linux-syscall-support header, which provides C shims for calling Linux syscalls directly, FYI:
https://chromium.googlesource.com/linux-syscall-support/

That being said, it looks like PTRACE_GETREGSET is the proper syscall for aarch64. The Breakpad impl calls it in LinuxPtraceDumper::ReadRegisterSet, getting the parameters by calling ThreadInfo::GetGeneralPurposeRegisters, which uses a confusingly-ifdefed regs member of the struct whose aarch64 definition is here .

@gabrielesvelto
Copy link
Contributor

Closing this as PR #81 should give us everything we need

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants