more complete sparc64 ABI fix for aggregates with floating point members #94216
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,136 +1,212 @@ | ||
| // FIXME: This needs an audit for correctness and completeness. | ||
|
|
||
| use crate::abi::call::{ | ||
| ArgAbi, ArgAttribute, ArgAttributes, ArgExtension, CastTarget, FnAbi, Reg, Uniform, | ||
| }; | ||
| use crate::abi::{self, HasDataLayout, Scalar, Size, TyAbiInterface, TyAndLayout}; | ||
|
|
||
| #[derive(Clone, Debug)] | ||
| pub struct Sdata { | ||
| pub prefix: [Option<Reg>; 8], | ||
| pub prefix_index: usize, | ||
| pub last_offset: Size, | ||
| pub has_float: bool, | ||
| pub arg_attribute: ArgAttribute, | ||
| } | ||
|
|
||
| fn arg_scalar<C>(cx: &C, scalar: &Scalar, offset: Size, mut data: Sdata) -> Sdata | ||
| where | ||
| C: HasDataLayout, | ||
| { | ||
| let dl = cx.data_layout(); | ||
|
|
||
| if scalar.value != abi::F32 && scalar.value != abi::F64 { | ||
| return data; | ||
| } | ||
|
|
||
| data.has_float = true; | ||
|
|
||
| if !data.last_offset.is_aligned(dl.f64_align.abi) && data.last_offset < offset { | ||
| if data.prefix_index == data.prefix.len() { | ||
| return data; | ||
| } | ||
| data.prefix[data.prefix_index] = Some(Reg::i32()); | ||
| data.prefix_index += 1; | ||
| data.last_offset = data.last_offset + Reg::i32().size; | ||
| } | ||
|
|
||
| for _ in 0..((offset - data.last_offset).bits() / 64) | ||
| .min((data.prefix.len() - data.prefix_index) as u64) | ||
| { | ||
| data.prefix[data.prefix_index] = Some(Reg::i64()); | ||
| data.prefix_index += 1; | ||
| data.last_offset = data.last_offset + Reg::i64().size; | ||
| } | ||
|
|
||
| if data.last_offset < offset { | ||
| if data.prefix_index == data.prefix.len() { | ||
| return data; | ||
| } | ||
| data.prefix[data.prefix_index] = Some(Reg::i32()); | ||
| data.prefix_index += 1; | ||
| data.last_offset = data.last_offset + Reg::i32().size; | ||
| } | ||
|
|
||
| if data.prefix_index == data.prefix.len() { | ||
| return data; | ||
| } | ||
|
|
||
| if scalar.value == abi::F32 { | ||
| data.arg_attribute = ArgAttribute::InReg; | ||
| data.prefix[data.prefix_index] = Some(Reg::f32()); | ||
| data.last_offset = offset + Reg::f32().size; | ||
| } else { | ||
| data.prefix[data.prefix_index] = Some(Reg::f64()); | ||
| data.last_offset = offset + Reg::f64().size; | ||
| } | ||
| data.prefix_index += 1; | ||
| return data; | ||
| } | ||
|
|
||
| fn arg_scalar_pair<C>( | ||
| cx: &C, | ||
| scalar1: &Scalar, | ||
| scalar2: &Scalar, | ||
| mut offset: Size, | ||
| mut data: Sdata, | ||
| ) -> Sdata | ||
| where | ||
| C: HasDataLayout, | ||
| { | ||
| data = arg_scalar(cx, &scalar1, offset, data); | ||
| if scalar1.value == abi::F32 { | ||
| offset += Reg::f32().size; | ||
| } else if scalar2.value == abi::F64 { | ||
| offset += Reg::f64().size; | ||
| } else if let abi::Int(i, _signed) = scalar1.value { | ||
| offset += i.size(); | ||
| } else if scalar1.value == abi::Pointer { | ||
| offset = offset + Reg::i64().size; | ||
| } | ||
|
|
||
| if (offset.raw % 4) != 0 && (scalar2.value == abi::F32 || scalar2.value == abi::F64) { | ||
| offset.raw += 4 - (offset.raw % 4); | ||
| } | ||
| data = arg_scalar(cx, &scalar2, offset, data); | ||
| return data; | ||
| } | ||
|
|
||
| fn parse_structure<'a, Ty, C>( | ||
| cx: &C, | ||
| layout: TyAndLayout<'a, Ty>, | ||
| mut data: Sdata, | ||
| mut offset: Size, | ||
| ) -> Sdata | ||
| where | ||
| Ty: TyAbiInterface<'a, C> + Copy, | ||
| C: HasDataLayout, | ||
| { | ||
| if let abi::FieldsShape::Union(_) = layout.fields { | ||
| return data; | ||
| } | ||
|
|
||
| match layout.abi { | ||
| abi::Abi::Scalar(scalar) => { | ||
| data = arg_scalar(cx, &scalar, offset, data); | ||
| } | ||
| abi::Abi::Aggregate { .. } => { | ||
| for i in 0..layout.fields.count().clone() { | ||
| if offset < layout.fields.offset(i) { | ||
| offset = layout.fields.offset(i); | ||
| } | ||
|
Comment on lines
+118
to
121
There was a problem hiding this comment. This is a silent failure (it will cause data corruption AFAICT). If the fields aren't in ascending order, this is a non-C-friendly type and you have to either ICE (which is arguably unfriendly), or better just fall back to "this cannot be passed in any optimized way". Sadly SPARC64 seems to be one of the architectures unfortunate enough to use "turn But still, you could use something similar to the Actually, wait, this is wronger than I thought, it doesn't add field offsets to the enclosing There's no way the testcases added in this PR could work, I think it's just hitting all the silent failure modes around here and There was a problem hiding this comment. Thanks for looking at it! I'm sure there are problems with my fix. |
||
| data = parse_structure(cx, layout.field(cx, i).clone(), data.clone(), offset); | ||
| } | ||
| } | ||
| _ => { | ||
| if let abi::Abi::ScalarPair(scalar1, scalar2) = &layout.abi { | ||
| data = arg_scalar_pair(cx, scalar1, scalar2, offset, data); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return data; | ||
| } | ||
|
|
||
| fn classify_arg<'a, Ty, C>(cx: &C, arg: &mut ArgAbi<'a, Ty>, in_registers_max: Size) | ||
| where | ||
| Ty: TyAbiInterface<'a, C> + Copy, | ||
| C: HasDataLayout, | ||
| { | ||
| if !arg.layout.is_aggregate() { | ||
| arg.extend_integer_width_to(64); | ||
| return; | ||
| } | ||
|
|
||
| let total = arg.layout.size; | ||
| if total > in_registers_max { | ||
| arg.make_indirect(); | ||
| return; | ||
| } | ||
|
|
||
| match arg.layout.fields { | ||
| abi::FieldsShape::Primitive => unreachable!(), | ||
| abi::FieldsShape::Array { .. } => { | ||
| // Arrays are passed indirectly | ||
| arg.make_indirect(); | ||
| return; | ||
| } | ||
| abi::FieldsShape::Union(_) => { | ||
| // Unions and are always treated as a series of 64-bit integer chunks | ||
| } | ||
| abi::FieldsShape::Arbitrary { .. } => { | ||
| // Stuctures with floating point numbers need special care. | ||
|
|
||
| let mut data = parse_structure( | ||
| cx, | ||
| arg.layout.clone(), | ||
| Sdata { | ||
| prefix: [None; 8], | ||
| prefix_index: 0, | ||
| last_offset: Size::ZERO, | ||
| has_float: false, | ||
| arg_attribute: ArgAttribute::default(), | ||
| }, | ||
| Size { raw: 0 }, | ||
| ); | ||
|
|
||
| if data.has_float { | ||
| // Structure { float, int, int } doesn't like to be handled like | ||
| // { float, long int }. Other way around it doesn't mind. | ||
| if data.last_offset < arg.layout.size | ||
| && (data.last_offset.raw % 8) != 0 | ||
| && data.prefix_index < data.prefix.len() | ||
| { | ||
| data.prefix[data.prefix_index] = Some(Reg::i32()); | ||
| data.prefix_index += 1; | ||
| data.last_offset += Reg::i32().size; | ||
| } | ||
|
|
||
| let mut rest_size = arg.layout.size - data.last_offset; | ||
| if (rest_size.raw % 8) != 0 && data.prefix_index < data.prefix.len() { | ||
| data.prefix[data.prefix_index] = Some(Reg::i32()); | ||
| rest_size = rest_size - Reg::i32().size; | ||
| } | ||
|
|
||
| arg.cast_to(CastTarget { | ||
| prefix: data.prefix, | ||
| rest: Uniform { unit: Reg::i64(), total: rest_size }, | ||
| attrs: ArgAttributes { | ||
| regular: data.arg_attribute, | ||
| arg_ext: ArgExtension::None, | ||
| pointee_size: Size::ZERO, | ||
| pointee_align: None, | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| arg.cast_to(Uniform { unit: Reg::i64(), total }); | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
There's three issues here:
Abi::ScalarPairshould not be special-cased in call ABIs - it's justAbi::Aggregatewith extra Rust-only optimizations tacked on (none of the other issues matter if theparse_structurecodepath is taken)Abi::ScalarPair(scalar1.size(cx).align_to(scalar2.align(cx).abi)is more or less the typical formula, anything else risks being wrong for certain scalars)else if scalar2.value == abi::F64is probably a typo? certainly seems like the intention is to get the size ofscalar1(which is justscalar1.size(cx))As it stands, I believe this code passes
(u128, f64)incorrectly (thanks to alignment, I can't think of anything else that would be mismatched), so it was probably unobserved for that reason.