-
Notifications
You must be signed in to change notification settings - Fork 14k
callconv: fix mips64 aggregate argument passing for C FFI #148294
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: main
Are you sure you want to change the base?
Conversation
|
rustbot has assigned @petrochenkov. Use |
This comment was marked as resolved.
This comment was marked as resolved.
fa68eff to
2b8161f
Compare
|
|
2b8161f to
4c08196
Compare
This comment has been minimized.
This comment has been minimized.
|
r? @workingjubilee |
|
|
|
Fair trade. |
|
@chenx97 re: your PR message: |
| let align = arg.layout.align.abi.max(dl.i64_align).min(dl.i128_align); | ||
| let pad_i32 = !offset.is_aligned(align); |
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.
So we take the max of align_of::<ArgType>() and align_of::<i64>(), guaranteeing it is at least align_of::<i64>(), and then we take the minimum of align_of::<i128>() and align_of::<ArgType>(), guaranteeing it is no more than align_of::<i128>()...
So, is there a reason this isn't just Ord::clamp(arg.layout.align.abi, dl.i64_align, dl.128_align)?
And then we ask if the offset is aligned to this? ... but we're changing the alignment so that u8, u16, etc. don't align...? You can easily convince me this PR is correct for MIPS, but I'm not sure how much this makes sense with respect to the PR description.
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.
So, is there a reason this isn't just
Ord::clamp(arg.layout.align.abi, dl.i64_align, dl.128_align)?
There is no reason other than that this line comes from existing code in mips.rs:28. I'll update this patch accordingly.
but we're changing the alignment so that u8, u16, etc. don't align...?
The offset starts from 0 and only increments in an aligned manner, and u8, u16 and u32 are always as aligned as u64: *offset = offset.align_to(align) + size.align_to(align);
You can easily convince me this PR is correct for MIPS, but I'm not sure how much this makes sense with respect to the PR description.
In that case, I need help with improving my description and commit message.
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.
Well, usually when I note a value is in a range, even though it's a commit message or PR, I deliberately use Rust's range syntax and say it's in the range instead of < or <=, because I make mistakes like the one you made if I don't.
Similar to mips, mips64 also adds a padding register when an aggregate argument is not passed with an aligned offset,
I do not think "add a padding register when an aggregate argument is not passed with an aligned offset" is quite right. That makes it sound a bit like when you pass a 1-byte struct, it uses 9 bytes, that byte plus an entire register! It's more like, "pads aggregates up to the 64-bit register size", right?
4c08196 to
e03382d
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
MIPS64 needs to put a padding argument before an aggregate argument when this argument is in an odd-number position, starting from 0, and has an alignment of 16 bytes or higher, e.g. `void foo(int a, max_align_t b);` is the same as `void foo(int a, long _padding, max_align_t b);` This fix uses an i32 padding, but it should work just fine because i32 is aligned like i64 for arguments.
e03382d to
a23c4cc
Compare
MIPS64 needs to put a padding argument before an aggregate argument when
this argument is in an odd-number position, starting from 0, and has an
alignment of 16 bytes or higher, e.g.
void foo(int a, max_align_t b);is the same asvoid foo(int a, long _padding, max_align_t b);