-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Fix building on ARM #32503
Fix building on ARM #32503
Conversation
r? @jroesch (rust_highfive has picked a reviewer for you, use r? to override) |
We use a 64bit integer to pass the set of attributes that is to be removed, but the called C function expects a 32bit integer. On most platforms this doesn't cause any problems other than being unable to unset some attributes, but on ARM even the lower 32bit aren't handled correctly because the 64bit value is passed in different registers, so the C function actually sees random garbage. So we need to fix the relevant functions to use 32bit integers instead. Additionally we need an implementation that actually accepts 64bit integers because some attributes can only be unset that way. Fixes rust-lang#32360
This reverts commit afbbb74.
pub fn LLVMGetFunctionAttr(Fn: ValueRef) -> c_ulonglong; | ||
pub fn LLVMRemoveFunctionAttr(Fn: ValueRef, val: c_ulonglong); | ||
pub fn LLVMGetFunctionAttr(Fn: ValueRef) -> c_uint; | ||
pub fn LLVMRemoveFunctionAttr(Fn: ValueRef, val: c_uint); |
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.
Ouch, #17795 strikes again!
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.
@alexcrichton would it be possible to use ctest to check the validity of these LLVM bindings?
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.
In theory yeah, but may be nontrivial to integrate
@bors r+ |
📌 Commit 1eacb4a has been approved by |
@@ -964,9 +964,10 @@ extern { | |||
pub fn LLVMAddFunctionAttrStringValue(Fn: ValueRef, index: c_uint, | |||
Name: *const c_char, | |||
Value: *const c_char); | |||
pub fn LLVMRemoveFunctionAttributes(Fn: ValueRef, index: c_uint, attr: uint64_t); |
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.
how come attr
doens't need to be c_ulonglong
?
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 C side uses uint64_t
not unsigned long long
.
No description provided.