support scalar pairs properly - #640
Conversation
eddyb
left a comment
eddyb
left a comment
There was a problem hiding this comment.
LGTM, modulo trying to simplify entry.rs using the higher-level helpers that already exist.
| let b_offset = a | ||
| .primitive() | ||
| .size(self) | ||
| .align_to(b.primitive().align(self).abi); | ||
|
|
||
| let elem0_ty = self.scalar_pair_element_backend_type(layout, 0, false); | ||
| let elem1_ty = self.scalar_pair_element_backend_type(layout, 1, false); | ||
|
|
||
| let base_ptr = value_ptr.unwrap(); | ||
| let ptr1 = bx.inbounds_ptradd(base_ptr, self.const_usize(b_offset.bytes())); | ||
|
|
||
| let v0 = bx.load(elem0_ty, base_ptr, layout.align.abi); | ||
| let v1 = bx.load(elem1_ty, ptr1, layout.align.restrict_for_offset(b_offset)); |
There was a problem hiding this comment.
It might be possible to replace this with a call to load_operand.
There was a problem hiding this comment.
Pushed a cleanup, definitely a lot cleaner
| } else if let Some( | ||
| &[ | ||
| Inst::Call(call_ret_id, callee_id, ref call_args), | ||
| Inst::CompositeExtract(extracted0, from0, 0), | ||
| Inst::CompositeExtract(extracted1, from1, 1), | ||
| ], | ||
| ) = try_rev_take(-3).as_deref() | ||
| && [from0, from1] == [call_ret_id; 2] | ||
| && [extracted0, extracted1] == [template_id, rt_args_ptr_id] | ||
| { | ||
| // Newer rustc, since `BackendRepr::ScalarPair` args are no | ||
| // longer forced to `PassMode::Direct`, returns the whole | ||
| // `fmt::Arguments` from its `new_*` constructor as a scalar | ||
| // pair, and splits it (via `OpCompositeExtract`s) into the | ||
| // two scalar values passed to the panic entry-point. | ||
| // | ||
| // The constructor's own arguments (i.e. `pieces`/`template` | ||
| // and the `rt::Argument` slice pointers) still carry the | ||
| // recoverable const data, so use those, like the aggregate | ||
| // (non-split) `Call`+`extract`+`insert` case does below. | ||
| let call_args_storage = call_args.iter().copied().collect(); | ||
| // Consume the matched call + both `OpCompositeExtract`s. | ||
| try_rev_take(3).unwrap(); | ||
| (lookup_fmt_args_ctor(callee_id)?, call_args_storage) |
There was a problem hiding this comment.
The only reason I don't like this has to do with it feeling misplaced, likely a consequence of the split_fmt_args changes from months ago, which I might eventually revisit (and shouldn't block this PR).
e0b67f1 to
60c9476
Compare
60c9476 to
2b221f2
Compare
| let value_len = if is_pair { | ||
| // We've already emitted an error, fill in a placeholder value | ||
| let value_len = if is_pair && is_unsized { | ||
| // For wide references (e.g., slices), the second component is a length. |
There was a problem hiding this comment.
Losing the old comment makes this look unsound (undef is only being used because this situation is illegal).
Also, the variables make this additionally confusing, because is_... refer to two different aspects.
is_pair is "pass as pair" or "expects pair", whereas is_unsized should probably be renamed to pointee_is_unsized (it only really makes sense with refs, i.e. "ref to unsized").
Not to mention that the condition for "value_len is needed" is ty::Ref(..)+is_pair, with is_unsized being related but not necessarily the same thing.
Also, self.type_isize() isn't the right thing to use in the general case, and could lead to panics elsewhere if the unsized type is not a slice (though today that's only &dyn Trait, so I'm not sure it's testable at all).
Minimal fix: include the error justification in a comment, use scalar_pair_element_backend_type w/ entry_arg_abi.layout, instead of type_isize, maybe gate by ty::Ref instead of is_unsized.
| @@ -490,7 +481,7 @@ pub fn scalar_pair_element_backend_type<'tcx>( | |||
| ty: TyAndLayout<'tcx>, | |||
There was a problem hiding this comment.
This reminds me that a longstanding refactor has been replacing all ty: TyAndLayout to layout: TyAndLayout (because it derefs to the layout, but not the type, so e.g. layout.ty and layout.size, instead of ty.ty and ty.size or the redundant ty.layout.size).
IIRC I regretted not naming it LayoutWithTy or similar, to make clearer the intent.
| } = bx.load_operand(PlaceRef::new_sized( | ||
| value_ptr.unwrap(), | ||
| entry_arg_abi.layout, | ||
| )) |
There was a problem hiding this comment.
I think load_operand should be used uniformly (in this else {...}), when storage_class is Ok, and then pattern-matching on (operand.val, entry_arg_abi.mode).
This would also force SpecConstants to require PassMode::Direct (right now they technically allow PassMode::Ignore, not sure if anything else catches that).
3 participants
Supersedes #381, see discussions over there.
Removes the "abi readjustment" aka hack to treat ScalarPairs as Scalars and adds proper support for ScalarPairs where necessary. Likely required to update beyond
nightly-2025-07-19, see discussion in PR #630 (comment).Reviewers: the fmt args decompiler adjustments are AI written, I have no idea what's going on in that module.