Miscompilation caused by incorrectly-deduced readonly on virtual call #137646
Assignees
Labels
A-LLVM
Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.
C-bug
Category: This is a bug.
I-miscompile
Issue: Correct Rust code lowers to incorrect machine code
I-unsound
Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness
P-critical
Critical priority
regression-from-stable-to-stable
Performance or correctness regression from one stable version to another.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
rustbot
added
the
needs-triage
johnbwang
changed the title
Copy types breaks expected Copy semantics
well… among those that’s probably the LLVM bump then, right?
|
|
Might also be #116454 |
saethlin
added
T-compiler
|
Interestingly, with the marked section uncommented (and fixed to refer to I don’t know, is that this one perhaps? |
This comment has been minimized.
This comment has been minimized.
|
It also isn’t “fixed” by uncommenting the indicated line, if |
saethlin
added
the
A-LLVM
|
It’s really interesting how it seems to be relevant whether or not the default method body does mutate its argument. E.g.: pub trait Trait {
fn method(&self, mut arr: [i32; 3]) {
println!("{:p}", &arr[0]);
// uncomment to "fix":
// println!("{:p}", &mut arr[0]);
}
}
impl Trait for () {
fn method(&self, mut arr: [i32; 3]) {
arr[0] = 42;
}
}
fn inspect_arr(arr: [i32; 3]) {
assert_eq!(format!("{}", arr[0]), "0");
}
pub fn run(trait_b: & dyn Trait, arr: [i32; 3]) {
inspect_arr(arr);
trait_b.method(arr);
inspect_arr(arr);
}use name_of_crate::*;
fn main() {
run(&(), [0, 0, 0]);
}here the |
|
With
|
|
That’s funny, my bisection (of my reduced example) with oh well… this might be because I use an array… most likely that regression point was related to because the regression was also mismatching between nightly and stable, suggesting a back-port… Ah yes, with a tuple instead of an array I do get |
|
I looked at LLVM IR, and realized I can't really read it myself, but I can at least share it here to save other folks some trouble: |
|
oh well… we can go back even further… with an unaligned type! edit: done: |
|
Now, this last bisection might be an LLVM upgrade, after all? |
|
So, if i'm reading this right (not sure about this one), we are working with this unoptimized llvm ir: define void @_ZN7example3run17h6794af29876d6ee8E(ptr align 1 %trait_b.0, ptr align 8 %trait_b.1, ptr %arr) unnamed_addr #0 {
%_5 = alloca [12 x i8], align 1
call void @llvm.memcpy.p0.p0.i64(ptr align 1 %_5, ptr align 1 %arr, i64 12, i1 false)
%0 = getelementptr inbounds ptr, ptr %trait_b.1, i64 3
%1 = load ptr, ptr %0, align 8, !invariant.load !2, !nonnull !2
call void %1(ptr align 1 %trait_b.0, ptr %_5)
ret void
}and llvm15 leaves it mostly as is define void @_ZN7example3run17h6794af29876d6ee8E(ptr noundef nonnull align 1 %trait_b.0, ptr noalias nocapture noundef readonly align 8 dereferenceable(24) %trait_b.1, ptr noalias nocapture noundef readonly dereferenceable(12) %arr) unnamed_addr #1 {
%_5 = alloca [12 x i8], align 1
call void @llvm.lifetime.start.p0(i64 12, ptr nonnull %_5)
call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 1 dereferenceable(12) %_5, ptr noundef nonnull align 1 dereferenceable(12) %arr, i64 12, i1 false)
%0 = getelementptr inbounds ptr, ptr %trait_b.1, i64 3
%1 = load ptr, ptr %0, align 8, !invariant.load !2, !nonnull !2
call void %1(ptr noundef nonnull align 1 %trait_b.0, ptr noalias nocapture noundef nonnull readonly dereferenceable(12) %_5)
call void @llvm.lifetime.end.p0(i64 12, ptr nonnull %_5)
ret void
}while llvm 16 elides the alloca+memcpy copy and transforms it into a tail call? Which means we are operating on the "original" data, and not on the copy anymore(?). define void @_ZN7example3run17h7445f22c4a90eb2aE(ptr noundef nonnull align 1 %trait_b.0, ptr noalias nocapture noundef readonly align 8 dereferenceable(24) %trait_b.1, ptr noalias nocapture noundef readonly dereferenceable(12) %arr) unnamed_addr #1 {
%0 = getelementptr inbounds ptr, ptr %trait_b.1, i64 3
%1 = load ptr, ptr %0, align 8, !invariant.load !2, !nonnull !2
tail call void %1(ptr noundef nonnull align 1 %trait_b.0, ptr noalias nocapture noundef nonnull readonly dereferenceable(12) %arr)
ret void
}At least that's what I get from staring at the godbolt, but i'm really out of my depths here... |
|
It seems we might not need to keep bisecting. It looks like parameters are being passed by reference, not by value. |
tail call void %1(ptr noundef nonnull align 1 %trait_b.0, ptr noalias nocapture noundef nonnull readonly dereferenceable(12) %arr)contains ptr noalias nocapture noundef nonnull readonly dereferenceable(12) %arrthe question is – where does this “ As far as I can tell, the actual method – which also makes it into the vtable – is instead ptr noalias nocapture noundef writeonly align 1 dereferenceable(12) %arrThat being said, these properties – readonly or not – of course cannot actually be known by the caller in the It thus also doesn’t surprise me if the trait method’s default implementation somehow influences this miscompilation. |
|
The |
|
Copy for the passed types is fairly irrelevant: Example without pub struct Foo(i32, i32, i32);
pub trait Trait {
fn methodd(&self, mut foo: Foo) {
println!("{:p}", &foo.0);
// uncomment to "fix":
//println!("{:p}", &mut foo.0);
}
}
impl Trait for () {
fn methodd(&self, mut foo: Foo) {
foo.0 = 42;
}
}
fn inspect_foo(foo: Foo) {
assert_eq!(format!("{}", foo.0), "1");
}
pub fn run(trait_b: &dyn Trait) {
inspect_foo(Foo(1, 2, 3));
trait_b.methodd(Foo(1, 2, 3));
inspect_foo(Foo(1, 2, 3));
}
pub fn call() {
run(&());
}use name_of_crate::*;
fn main() {
call();
} |
|
I want to dig into it. |
|
use std::hint::black_box;
type T = (i32, i32, i32);
pub trait Trait {
fn m(&self, _: T, _: T) {}
}
impl Trait for () {
fn m(&self, mut _foo: T, foo2: T) {
_foo = (0, 0, 0);
println!("{foo2:?}");
}
}
pub fn run(trait_b: &dyn Trait) {
trait_b.m((1, 1, 1), (1, 1, 1));
}
pub fn call() {
run(&());
}
fn main() {
black_box(run as fn(&dyn Trait));
call();
} |
|
Of note: steffahn's reproducer above changed the behavior from "value that shouldn't be modified is modified" to "a garbage (uninitialized?) value is printed". Also, |
rustbot
added
regression-from-stable-to-stable
Noratrieb
added
I-unsound
|
WG-prioritization assigning priority (Zulip discussion). @rustbot label -I-prioritize +P-critical |
rustbot
added
P-critical
DianQK
added a commit
to DianQK/rust
that referenced
this issue
Feb 26, 2025
saethlin
changed the title
Copy types breaks expected Copy semantics
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Feb 27, 2025
Don't infer attributes of virtual calls based on the function body Fixes (after backport) rust-lang#137646. Since we don't know the exact implementation of the virtual call, it might write to parameters, we can't infer the readonly attribute.
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Feb 28, 2025
Don't infer attributes of virtual calls based on the function body Fixes (after backport) rust-lang#137646. Since we don't know the exact implementation of the virtual call, it might write to parameters, we can't infer the readonly attribute.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I tried the following code. It requires a binary crate and a library crate.
main.rs
main's Cargo.toml
lib.rs
lib's Cargo.toml
Since
struct_aconforms toCopy, passingstruct_atoget_trait_a()should make a copy. Instead, when modifyingstruct_ainside ofget_trait_a(), it modifies the original. When printing the struct a second time, the struct has been modified. To repro the bug, it requiresopt-level >= 1andincremental = false.I expected the output in my command line to be:
Instead, this happened:
Meta
rustc --version --verbose:The text was updated successfully, but these errors were encountered: