Skip to content

Mark simplify_aggregate_to_copy mir-opt as unsound #132356

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 31, 2024
Merged

Mark simplify_aggregate_to_copy mir-opt as unsound #132356

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d8bda3
Add a regression test for #132353
jieyouxu Oct 30, 2024
10b8ba4
Mark `simplify_aggregate_to_copy` mir-opt as unsound
jieyouxu Oct 30, 2024
cfb4c05
Add a mir-opt GVN test for #128299
jieyouxu Oct 30, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add a regression test for #132353 
To catch at least one pattern that was miscompiled. The test is a minimization of the MCVE reported in <#132353>.
  • Loading branch information
@jieyouxu
jieyouxu committed Oct 31, 2024
commit 4d8bda335e23b7ff10ff0c645c825a90fc2646bb
25 changes: 25 additions & 0 deletions tests/ui/mir/clone-canonicalization-miscompile-132353.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//! The mir-opt added in <https://github.com/rust-lang/rust/pull/128299> unfortunately seems to lead
//! to a miscompile (reported in <https://github.com/rust-lang/rust/issues/132353>, minimization
//! reproduced in this test file).
//@ revisions: release debug
// Note: it's not strictly cargo's release profile, but any non-zero opt-level was sufficient to
// reproduce the miscompile.
//@[release] compile-flags: -C opt-level=1
//@[debug] compile-flags: -C opt-level=0
//@ run-pass
Copy link
Contributor

@cjgillot cjgillot Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you copy this into mir-opt test which shows pop_min.GVN.diff?
Copy link
Member Author

@jieyouxu jieyouxu Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, do you mean retain this ui test (for now), but also create a new mir-opt test which shows the GVN diff?
Copy link
Member

@dianqk dianqk Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use this for a mir-opt test if you like:

#![allow(internal_features)] #![feature(rustc_attrs, core_intrinsics)] #[no_mangle] fn foo(v: &mut Option<i32>) -> Option<i32> { if let &Some(col) = get(&v) { *v = None; return Some(col); } else { unsafe { std::intrinsics::unreachable() } } } #[no_mangle] #[inline(never)] #[rustc_nounwind] fn get(v: &Option<i32>) -> &Option<i32> { v }
Copy link
Member Author

@jieyouxu jieyouxu Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I tried to add a GVN.diff test w/ this snippet, lmw if the test needs adjustments.

fn pop_min(mut score2head: Vec<Option<usize>>) -> Option<usize> {
loop {
if let Some(col) = score2head[0] {
score2head[0] = None;
return Some(col);
}
}
}

fn main() {
let min = pop_min(vec![Some(1)]);
println!("min: {:?}", min);
// panic happened on 1.83.0 beta in release mode but not debug mode.
let _ = min.unwrap();
}