-
Notifications
You must be signed in to change notification settings - Fork 1k
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
netbsd adding mcontext related data for riscv64 #3468
Conversation
r? @JohnTitor (rustbot has picked a reviewer for you, use r? to override) |
5befa04
to
bbe2b2f
Compare
☔ The latest upstream changes (presumably #3525) made this pull request unmergeable. Please resolve the merge conflicts. |
Could you resolve conflicts and remove libc_union cfg? |
bbe2b2f
to
44fcc99
Compare
#[cfg(not(libc_union))] | ||
pub __qregs: [u128; 1], |
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 main branch doesn't need cfg anymore.
if #[cfg(libc_const_size_of)] { | ||
#[doc(hidden)] | ||
pub const _ALIGNBYTES: usize = ::mem::size_of::<::c_long>() - 1; | ||
} else { | ||
#[doc(hidden)] | ||
pub const _ALIGNBYTES: usize = 8 - 1; | ||
} |
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.
pub(crate)
can be used I think.
21e8f49
to
0b726ea
Compare
This probably needs a rebase at this point, but otherwise seems like it was waiting for review. @rustbot review |
0b726ea
to
966d3dd
Compare
Can this just be moved to netbsd/mod.rs? I'm not sure what is platform-specific here, if you have a header link that would help. This should also get the semver test |
well ucontext could possibly be make common but not mcontext which is very architecture dependent. |
c4957c2
to
0cf7b81
Compare
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 think the trait should be dropped as mentioned. Otherwise lgtm, please add sources to the PR description.
impl PartialEq for __c_anonymous__fpreg { | ||
fn eq(&self, other: &__c_anonymous__fpreg) -> bool { | ||
unsafe { | ||
self.u_u64 == other.u_u64 | ||
|| self.u_ud == other.u_ud | ||
} | ||
} | ||
} | ||
impl Eq for __c_anonymous__fpreg {} |
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.
These should probably just be dropped, it would be weird to use float equality to compare integers.
0cf7b81
to
fd6dd02
Compare
@rustbot review |
pub union __c_anonymous__fpreg { | ||
pub u_u64: u64, | ||
pub u_d: ::c_double, | ||
} |
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, one other thing - should this just be called __fpreg
? From https://github.com/NetBSD/src/blob/0465e5c825effb130eca95499f5ac6454fe0d5ab/sys/arch/riscv/include/mcontext.h#L44C7-L47.
It would probably also be good to define
pub const _NGREG: usize = 32;
pub const _NFREG: usize = 33;
pub type __gregset_t = [__greg_t; _NGREG];
pub type __fregset_t = [__fpreg; _NFREG];
and then use those in mcontext
fd6dd02
to
4660303
Compare
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.
Thanks!
4660303
to
fb52c7a
Compare
ref: https://github.com/NetBSD/src/blob/0465e5c825effb130eca95499f5ac6454fe0d5ab/sys/arch/riscv/include/mcontext.h#L44 (backport <rust-lang#3468>) (cherry picked from commit fb52c7a)
ref: https://github.com/NetBSD/src/blob/0465e5c825effb130eca95499f5ac6454fe0d5ab/sys/arch/riscv/include/mcontext.h#L44