-
Notifications
You must be signed in to change notification settings - Fork 13k
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
interpret: change ABI-compat test to be type-based #115699
Conversation
r? @oli-obk (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri The Miri subtree was changed cc @rust-lang/miri Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
8773102
to
d45c7ec
Compare
…nsistent across targets
d45c7ec
to
e001209
Compare
@@ -2743,7 +2745,7 @@ impl<'tcx> Ty<'tcx> { | |||
| ty::Tuple(..) => (tcx.types.unit, false), | |||
|
|||
ty::Str | ty::Slice(_) => (tcx.types.usize, false), | |||
ty::Dynamic(..) => { | |||
ty::Dynamic(_, _, DynKind::Dyn) => { |
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.
This looks like a pretty serious bug in ptr_metadata_ty caused by Dyn and DynStar using the same type constructor.
fn unfold_npo(&self, ty: Ty<'tcx>) -> InterpResult<'tcx, Ty<'tcx>> { | ||
// Check if this is `Option` wrapping some type. | ||
let inner_ty = match ty.kind() { | ||
ty::Adt(def, args) if self.tcx.is_diagnostic_item(sym::Option, def.did()) => { |
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.
using a diagnostic_item
for this is somewhat fishy, but since it only decides whether code is ok or not, it seems ok. We just need to avoid using these for behaviour changes.
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (366dab1): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 631.339s -> 631.819s (0.08%) |
This makes the test consistent across targets. Otherwise the chances are very high that ABI mismatches get accepted on x86_64 but still fail on many other targets with more complicated ABIs.
This implements (most of) the rules described in #115476.