Skip to content
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

VTable-related miscompilation with incremental compilation #89598

Closed
michaelwoerister opened this issue Oct 6, 2021 · 3 comments · Fixed by #89619
Closed

VTable-related miscompilation with incremental compilation #89598

michaelwoerister opened this issue Oct 6, 2021 · 3 comments · Fixed by #89619
Labels
A-incr-comp Area: Incremental compilation C-bug Category: This is a bug. 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-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@michaelwoerister
Copy link
Member

michaelwoerister commented Oct 6, 2021

PR #86475 introduced a global cache for vtable-layouts into the tcx which circumvents incremental compilation's dependency tracking. The effect is that object files might not get updated when methods within a trait get shuffled around, thus leading to miscompilations very similar to the one observed in #82920, i.e. trait object method calls will invoke the wrong method.

The following test (when added to src/test/incremental) shows this behavior:

// revisions:rpass1 rpass2

trait Foo {
    #[cfg(rpass1)]
    fn method1(&self) -> u32;

    fn method2(&self) -> u32;

    #[cfg(rpass2)]
    fn method1(&self) -> u32;
}

impl Foo for u32 {
    fn method1(&self) -> u32 { 17 }
    fn method2(&self) -> u32 { 42 }
}

fn main() {
    let x: &dyn Foo = &0u32;
    // This fails in `rpass2` because `method2` gets called instead of `method1`.
    assert_eq!(mod1::foo(x), 17);
}

mod mod1 {
    pub fn foo(x: &dyn super::Foo) -> u32 {
        x.method1()
    }
}

Global caches without dependency tracking are a sure way of introducing subtle incr. comp. bugs. Queries must be used instead.

I'm going to mark this as P-critical. We should fix this asap.

@michaelwoerister michaelwoerister added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-incr-comp Area: Incremental compilation C-bug Category: This is a bug. P-critical Critical priority labels Oct 6, 2021
@Aaron1011
Copy link
Member

Do we know why this didn't trigger a fingerprint ICE?

@michaelwoerister
Copy link
Member Author

Do we know why this didn't trigger a fingerprint ICE?

Good question! I suspect it is because the changed value never gets fingerprinted anywhere. It is directly used when generating codegen units in the backend and those don't get fingerprinted.

@Mark-Simulacrum Mark-Simulacrum added this to the 1.56.0 milestone Oct 6, 2021
@Mark-Simulacrum
Copy link
Member

Milestoning at 1.56. #86475 landed into 1.55, but we should try to fix this as soon as possible on stable as well.

@estebank estebank added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Oct 7, 2021
@bors bors closed this as completed in 44995f7 Oct 8, 2021
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Oct 9, 2021
Turn vtable_allocation() into a query

This PR removes the untracked vtable-const-allocation cache from the `tcx` and turns the `vtable_allocation()` method into a query.

The change is pretty straightforward and should be backportable without too much effort.

Fixes rust-lang/rust#89598.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation C-bug Category: This is a bug. 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-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants