-
Notifications
You must be signed in to change notification settings - Fork 13.7k
check before test for hardware capabilites in bits 32~63 of usize #146323
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
base: master
Are you sure you want to change the base?
Conversation
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.
Could you add a bit more to the PR description: a short summary of the problem, and of the solution.
Also, does this change actually fix the issue you're observing?
#[cfg(target_pointer_width = "64")] | ||
{ |
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.
This should at least get a comment, because I think it's quite reasonable to assume that aarch64 implies 64-bit pointers.
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.
also now you're basically saying that the features in this block cannot be queried for on ilp32, is that correct?
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.
Ok, I will add some comment to clarify it.
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.
also now you're basically saying that the features in this block cannot be queried for on ilp32, is that correct?
Yes, the libc::getauxval
returns 32 bit c_ulong
for aarch64 ILP32 target.
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.
also now you're basically saying that the features in this block cannot be queried for on ilp32, is that correct?
Well, to be more accurate, they can be queried, but the values will all be false
.
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.
This should at least get a comment, because I think it's quite reasonable to assume that aarch64 implies 64-bit pointers.
I have added some comments to clarify it.
I have updated the PR description.
It should. I will test it on the real hardware soon. |
Yes, it does fix the issue. I have verified that on the real hardware. The following code ran and printed fn main() {
println!("{:?}", std::arch::is_aarch64_feature_detected!("lse"));
} |
This commit tries to fix #146230.
std::arch::is_aarch64_feature_detected
panics on aarch64 ILP32 targets.After some digging, the real problem is
rust/library/std_detect/src/detect/os/linux/aarch64.rs
Lines 210 to 241 in 91edc3e
checks bits 32~63 of usize unconditionally on normal aarch64 LP64 target and aarch64 ILP32 target.
Here I propose to move these to a block guarded by
#[cfg(target_pointer_width="64")]
.See #146230 for more detailed analysis.
r? @Amanieu