Repository navigation
Fix PassMode::Cast ABI argument lowering - #1659
1sgtpepper wants to merge 4 commits into
Conversation
|
It would be nice if we had a concrete test for a case that goes wrong without this change. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
|
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. |
|
@bjorn3 rebased, enabled the ABI-cafe regression, and CI is green. Could you re-review when convenient? |
| cast.rest.total.bytes() % cast.rest.unit.size.bytes(), | ||
| ) | ||
| assert_ne!(cast.rest.unit.size, Size::ZERO); | ||
| if cast.rest.total.bytes() % cast.rest.unit.size.bytes() != 0 { |
There was a problem hiding this comment.
| if cast.rest.total.bytes() % cast.rest.unit.size.bytes() != 0 { | |
| if !cast.rest.total.bytes().is_multiple_of(cast.rest.unit.size.bytes()) { |
| let scratch = | ||
| fx.create_stack_slot(u32::try_from(slot_size).unwrap(), u32::try_from(slot_align).unwrap()); | ||
| // A partial final integer is passed in a full register. Initialize every byte that can be | ||
| // loaded as an ABI parameter before copying the value bytes into the scratch slot. |
There was a problem hiding this comment.
Why is this necessary? Cranelift doesn't have any concept of undef bytes. And I would expect the callee to just ignore the uninitialized part.
| true, | ||
| MemFlagsData::new(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Why this extra copy? force_stack already writes to the stack if the argument isn't already on the stack. If this is for the case where the abi_param_size is larger than the argument type, I think a better option would be to special case the last load to use the smaller type and zero-extend it instead.
| let ptr = fx.create_stack_slot( | ||
| // Stack slot size may be bigger for example `[u8; 3]` which is packed into an `i32`. | ||
| // It may also be smaller for example when the type is a wrapper around an integer with a | ||
| // larger alignment than the integer. |
| let abi_param_size = abi_params_size(&abi_params); | ||
| let cast_align = cast.align(fx).bytes(); | ||
| let layout_size = layout.size.bytes(); | ||
| let layout_align = layout.align.bytes(); |
There was a problem hiding this comment.
I think these variables could be inlined without making things less readable.
3 participants
Fixes #1477.
Updates
PassMode::Castlowering to pass rounded full ABI units for integer rest fields, matchingCastTarget's rounded ABI layout. Casted arguments now use a zero-initialized ABI-sized scratch slot before loading ABI parameters, preventing padding from being read from the Rust-layout representation.The previously waived
SingleVariantUnion::conv_c::repr_cABI-cafe case is now enabled on macOS AArch64.Verification:
cargo fmt --checkcargo check --locked