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

codegen generates vtables for a variable's supertype when unsizing sometimes #107205

Closed
Rattenkrieg opened this issue Jan 22, 2023 · 9 comments · Fixed by #112307
Closed

codegen generates vtables for a variable's supertype when unsizing sometimes #107205

Rattenkrieg opened this issue Jan 22, 2023 · 9 comments · Fixed by #112307
Labels
A-codegen Area: Code generation 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-high High 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

@Rattenkrieg
Copy link
Contributor

Rattenkrieg commented Jan 22, 2023

I was putting &'s here and there to make my program compile as beginners supposed to do and finally ended with:

  = note: Undefined symbols for architecture arm64:
            "core::ptr::drop_in_place$LT$fn$LP$$RF$$u5b$i64$u5d$$RP$$u20$.$GT$$u20$i64$GT$::h8f1379e9ceec7fb5", referenced from:
                l___unnamed_1 in wyas-3cfa3c0d2101b65b.6xaszv4ay9i42ky.rcgu.o
          ld: symbol(s) not found for architecture arm64
          clang: error: linker command failed with exit code 1 (use -v to see invocation)

To assure myself that this is unrelated to native/rosetta mess I've ran this in docker and in rust playground with the same outcome.

  = note: /usr/bin/ld: /playground/target/release/deps/playground-7e94a1292993c003.playground.0627c30c-cgu.0.rcgu.o:(.data.rel.ro..L__unnamed_3+0x0): undefined reference to `core::ptr::drop_in_place<fn(&[i64]) -> i64>'
          /usr/bin/ld: /playground/target/release/deps/playground-7e94a1292993c003: hidden symbol `_ZN4core3ptr66drop_in_place$LT$fn$LP$$RF$$u5b$i64$u5d$$RP$$u20$.$GT$$u20$i64$GT$17h8581d5a4605118bdE' isn't defined
          /usr/bin/ld: final link failed: bad value

This particular code breaks the linker

struct Wrapper(fn(val: &[i64]) -> i64);

impl Debug for Wrapper {
    fn fmt<'a>(&'a self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_tuple("Wrapper").field(&self.0 as &fn(&'a [i64]) -> i64).finish()
    }
}

fn main() {
    println!("{:?}", Wrapper(useful));
}

I've come up with that since deriving Debug or implementing it manually like that

impl Debug for Wrapper {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_tuple("Wrapper").field(&self.0).finish()
    }
}

leads to following type/borrow check error:

implementation of Debug is not general enough

Expected behavior: program prints

Wrapper(address)

Actual behavior: see above

rustc --version --verbose:

rustc 1.66.1 (90743e729 2023-01-10)
binary: rustc
commit-hash: 90743e7298aca107ddaa0c202a4d3604e29bfeb6
commit-date: 2023-01-10
host: aarch64-apple-darwin
release: 1.66.1
LLVM version: 15.0.2

I've tried both beta 1.67.0-beta.9 and nightly 1.68.0-nighlty at rust playground and both are failing during linking.


Edit

I've bruteforced working program 😂 :

impl Debug for Wrapper {
    fn fmt<'a>(&'a self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_tuple("Wrapper").field(&(self.0 as fn(&'a [i64]) -> i64)).finish()
    }
}
@Rattenkrieg Rattenkrieg added the C-bug Category: This is a bug. label Jan 22, 2023
@erikdesjardins
Copy link
Contributor

erikdesjardins commented Jan 24, 2023

This might be the shortest code sample* I've seen cause a link error. Bravo!

Marginally reduced: https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=e4cd76a3ddf3068e0d71ea19247915a9

* aside from obviously broken code, #[link_name], etc., which is cheating :)

@Noratrieb
Copy link
Member

The following reproduction reproduces on stable but not on nightly:

use std::fmt;

pub struct Wrapper(for<'b> fn(&'b ()));

impl fmt::Debug for Wrapper {
    fn fmt<'a>(&'a self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        let dyn_debug: &dyn fmt::Debug = &self.0 as &fn(&'static ());
        fmt::Debug::fmt(&dyn_debug, f)
    }
}

fn useful(_: &()) {}

fn main() {
    println!("{:?}", Wrapper(useful));
}

searched nightlies: from nightly-2023-02-01 to nightly-2023-03-16
regressed nightly: nightly-2023-02-17
searched commit range: 2d14db3...9a7cc6c
regressed commit: 639377e

bisected with cargo-bisect-rustc v0.6.5

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc 2023-02-01 --end 2023-03-16 --access github --regress success

Bisection points at a new optimization, so I guess the problematic code is simply optimized away. Using -Zmir-opt-level=0 to stop the opt makes it appear again even on nightly.

@BoxyUwU BoxyUwU 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 labels Mar 24, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 24, 2023
@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 24, 2023

Here is a code snippet that segfaults on stable because of this bug:

#![allow(coherence_leak_check)]

struct Foo<T: 'static>(T);

fn useful<'a>(_: &'a u8) {}

pub struct Wrapper(for<'b> fn(&'b u8));

trait GetInner {
    type Assoc;
    fn muahaha(&mut self) -> Self::Assoc;
}

impl GetInner for Foo<fn(&'static u8)> {
    type Assoc = String;
    fn muahaha(&mut self) -> String {
        panic!("cant do it boss")
    }
}

impl GetInner for Foo<for<'a> fn(&'a u8)> {
    type Assoc = [usize; 3];
    fn muahaha(&mut self) -> [usize; 3] {
        [100; 3]
    }
}

fn main() {
    let wrapper = Wrapper(useful);

    drop(Box::new(Foo(useful as fn(&'static u8))) as Box<dyn GetInner<Assoc = String>>);
    drop(Box::new(Foo(useful as fn(&u8))) as Box<dyn GetInner<Assoc = [usize; 3]>>);

    let hr_fnptr = Box::new(Foo::<for<'a> fn(&'a u8)>(wrapper.0));
    let lr_fnptr = hr_fnptr as Box<Foo<fn(&'static u8)>>;
    let mut any = lr_fnptr as Box<dyn GetInner<Assoc = String>>;

    let evil_string = any.muahaha();
    drop(evil_string);
}

if you remove the two statements that are drop(Box::new( you will get a linker error (what this issue is about), having them in this repro ensures that everything gets codegen'd to replace the linker error with calling the wrong function at runtime

@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 24, 2023

Here is also a super minimal repro of a linker error, although note that it requires -Zmir-opt-level=0 to repro (as nils pointed out previously):

pub struct Wrapper(for<'b> fn(&'b ()));

fn useful(_: &()) {}

fn main() {
    let wrapper = Wrapper(useful);
    let dyn_debug = &wrapper.0 as &fn(&'static ()) as &dyn std::fmt::Debug;
    println!("{:?}", dyn_debug);
}

@BoxyUwU BoxyUwU changed the title Linking fails for program that coercing lifetimes for borrowed function pointers codegen generates vtables for a variable's supertype when unsizing sometimes Mar 24, 2023
@BoxyUwU BoxyUwU added the A-codegen Area: Code generation label Mar 24, 2023
@apiraino

This comment was marked as outdated.

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 27, 2023
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added the P-critical Critical priority label Mar 27, 2023
@apiraino apiraino added P-high High priority and removed P-high High priority P-critical Critical priority labels Mar 27, 2023
@lcnr
Copy link
Contributor

lcnr commented Mar 27, 2023

this regressed in 1.17, the bug probably existed ever since we added mir: https://godbolt.org/z/a3b5Er71c

@workingjubilee workingjubilee added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label May 6, 2023
@lcnr
Copy link
Contributor

lcnr commented May 9, 2023

an even more minimal linker error happens with

// rustc src/main.rs -Zmir-opt-level=0
const X: for<'b> fn(&'b ()) = |&()| ();
fn main() {
    let dyn_debug = &X as &fn(&'static ()) as &dyn Send;
    drop(dyn_debug)
}

the bug here is caused by OperandRef having a TyAndLayout. The type of which may differ from the type of the place we write the OperandRef into.

This causes unsoundness when we do trait solving using the type of the OperandRef instead of the type of the place. It means that mir typeck and codegen can disagree on which impl to choose.

https://github.com/rust-lang/rust/compare/master...lcnr:rust:operand-ref?expand=1 tries to deal with that in some locals which is enough for that minimal linker error repro but does not yet fix the unsoudness as the status quo is very worrying to me:

  • we have multiple locations in the code which move an OperandRef "over a subtyping edge"
  • we have multiple locations in the code which use the type of the OperandRef instead of the type of the current mir place

Going to keep looking a bit more into this and may write some mentoring instructions or will look into fixing this myself. Also, thanks @BoxyUwU for guiding me towards my current understanding here

@lcnr
Copy link
Contributor

lcnr commented Jun 15, 2023

I have a fix for this in #112307, but given that this is somewhat fragile it would be better to instead make subtyping an explicit step in the MIR, opened #112651 for that if someone is interested in implementing that.

@bors bors closed this as completed in bb95b7d Jun 28, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 3, 2023
Make subtyping explicit in MIR

This adds new mir-opt that pushes new `ProjectionElem` called `ProjectionElem::Subtype(T)` to `Rvalue` of a subtyped assignment so we can unsoundness issues like rust-lang#107205

Addresses rust-lang#112651

r? `@lcnr`
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 3, 2023
Make subtyping explicit in MIR

This adds new mir-opt that pushes new `ProjectionElem` called `ProjectionElem::Subtype(T)` to `Rvalue` of a subtyped assignment so we can unsoundness issues like rust-lang#107205

Addresses rust-lang#112651

r? `@lcnr`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Oct 6, 2023
Make subtyping explicit in MIR

This adds new mir-opt that pushes new `ProjectionElem` called `ProjectionElem::Subtype(T)` to `Rvalue` of a subtyped assignment so we can unsoundness issues like rust-lang/rust#107205

Addresses rust-lang/rust#112651

r? `@lcnr`
RalfJung pushed a commit to RalfJung/miri that referenced this issue Oct 6, 2023
Make subtyping explicit in MIR

This adds new mir-opt that pushes new `ProjectionElem` called `ProjectionElem::Subtype(T)` to `Rvalue` of a subtyped assignment so we can unsoundness issues like rust-lang/rust#107205

Addresses rust-lang/rust#112651

r? `@lcnr`
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Oct 8, 2023
Make subtyping explicit in MIR

This adds new mir-opt that pushes new `ProjectionElem` called `ProjectionElem::Subtype(T)` to `Rvalue` of a subtyped assignment so we can unsoundness issues like rust-lang/rust#107205

Addresses rust-lang/rust#112651

r? `@lcnr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation 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-high High 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.
Projects
None yet
8 participants