Skip to content

Rust appears to create (duplicate?) vtables at runtime that could have been emitted at compile-time. #8591

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

Closed
Thiez opened this issue Aug 18, 2013 · 8 comments
Labels
A-codegen Area: Code generation A-trait-system Area: Trait system A-type-system Area: Type system E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.

Comments

@Thiez
Copy link
Contributor

Thiez commented Aug 18, 2013

The code below outputs ((6340760, 140201758297392), (6340840, 140201758297424), (6340856, 140201758297472), (6340872, 140201758297504)) (exact numbers may vary). The important part is that the vtables seem to get newly created for ~1 and ~2 (and also @1 and @2) even though they could easily use the same vtables.

use std::cast::transmute;
trait Foo {
  fn foo(&self) {}
}
impl Foo for uint {}
impl Foo for int {}
unsafe {
  (transmute::<~Foo, (uint,uint)>(~1i as ~Foo),
   transmute::<@Foo, (uint,uint)>(@1i as @Foo),
   transmute::<~Foo, (uint,uint)>(~2i as ~Foo),
   transmute::<@Foo, (uint,uint)>(@2i as @Foo))
}

The code was fed to rusti, original below (thanks to dbaupp):

13:55 < dbaupp> rusti: use std::cast::transmute; trait Foo { fn foo(&self) {} } impl Foo for uint {} impl Foo for int {} unsafe {(transmute::<~Foo, (uint,uint)>(~1i as ~Foo),transmute::<@Foo, (uint,uint)>(@1i as @Foo),transmute::<~Foo, (uint,uint)>(~2i as ~Foo),transmute::<@Foo, (uint,uint)>(@2i as @Foo))}
13:55 -rusti:#rust- ((6340760, 140201758297392), (6340840, 140201758297424), (6340856, 140201758297472), (6340872, 140201758297504))

Swapping vtables appears to work just fine, even between ~Trait and &Trait (but not between ~Trait and @trait):

use std::cast::transmute;
trait Foo{
  fn foo(&mut self);
  fn bar(&self)->int;
}
impl Foo for int{
  fn foo(&mut self) {
    *self=*self+1
  }
  fn bar(&self)->int{
    *self
  }
};
unsafe{
  let (a1,a2) = transmute::<~Foo,(uint,uint)>(~1 as ~Foo);
  let (b1,b2) = transmute::<&mut Foo,(uint,uint)>(&mut 2 as &mut Foo);
  let mut a = transmute::<(uint,uint),~Foo>((b1,a2));
  let b = transmute::<(uint,uint),&mut Foo>((a1,b2));
  a.foo();b.foo();
  (a.bar(),b.bar() )
}

Outputs (2,3) in rusti.

14:24 < Thiez> rusti: use std::cast::transmute; trait Foo{fn foo(&mut self);fn bar(&self)->int;} impl Foo for int{fn foo(&mut self){*self=*self+1}fn bar(&self)->int{*self}}; unsafe{ let
               (a1,a2)=transmute::<~Foo,(uint,uint)>(~1 as ~Foo); let (b1,b2)=transmute::<&mut Foo,(uint,uint)>(&mut 2 as &mut Foo); let mut a=transmute::<(uint,uint),~Foo>((b1,a2));let b =
               transmute::<(uint,uint),&mut Foo>((a1,b2)); a.foo();b.foo();(a.bar(),b.bar()) }
14:24 -rusti:#rust- (2, 3)

It seems to me we should generate the vtables at compile time.

@emberian
Copy link
Member

emberian commented Mar 3, 2014

cc @alexcrichton

@steveklabnik
Copy link
Member

Triage: now that @ is removed form the language, I'm not entirely sure how to reproduce this bug.

I guess the second example doesn't use @....

@steveklabnik
Copy link
Member

New code:

use std::mem;

trait Foo{
    fn foo(&mut self);
    fn bar(&self)->isize;
}
impl Foo for isize{
    fn foo(&mut self) {
        *self=*self+1
    }
    fn bar(&self)->isize{
        *self
    }
}

fn main() {
unsafe{
    let (a1,a2) = mem::transmute::<Box<Foo>,(usize,usize)>(Box::new(1isize) as Box<Foo>);
    let (b1,b2) = mem::transmute::<&mut Foo,(usize,usize)>(&mut 2isize as &mut Foo);
    let mut a = mem::transmute::<(usize,usize),Box<Foo>>((b1,a2));
    let b = mem::transmute::<(usize,usize),&mut Foo>((a1,b2));
    a.foo();b.foo();
    println!("{:?}", (a.bar(),b.bar()))
}
}

ouputs: (3, 2)

@emberian
Copy link
Member

Modernized:

use std::mem::transmute;
trait Foo {
    fn foo(&self) {}
}
impl Foo for u32 {}
impl Foo for i32 {}

fn main() {
unsafe {
    println!("{:?}", (transmute::<Box<Foo>, (usize,usize)>(Box::new(1) as Box<Foo>),
    transmute::<&Foo, (usize,usize)>(&1 as &Foo),
    transmute::<Box<Foo>, (usize,usize)>(Box::new(2) as Box<Foo>),
    transmute::<&Foo, (usize,usize)>(&2 as &Foo)));
}
}

outputs:

((140238844473352, 140238875981712), (140238873824724, 140238875981712), (140238844473368, 140238875981712), (140238873824728, 140238875981712))

@emberian
Copy link
Member

(That is, vtables are only emitted only once where identical, I suspect we've modified the linkage such that LLVM merges them? Or maybe the compiler just got smarter)

@emberian emberian reopened this Apr 16, 2015
@emberian emberian added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Apr 16, 2015
@emberian
Copy link
Member

I'm not sure if we want to have a test for this sort of optimization to be honest, but I'll leave that up to someone else.

@steveklabnik
Copy link
Member

When we start doing codegen tests like this, we'll need to decide what all we cover, so I think I'll make that decision for now, to give this a close 😉

@steveklabnik
Copy link
Member

(and thanks for the backup! ❤️ )

flip1995 pushed a commit to flip1995/rust that referenced this issue Apr 7, 2022
…raffate

add `empty_structs_with_brackets`

<!-- Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only includes internal changes, you can just write
`changelog: none`. Otherwise, please write a short comment
explaining your change. Also, it's helpful for us that
the lint name is put into brackets `[]` and backticks `` ` ` ``,
e.g. ``[`lint_name`]``.

If your PR fixes an issue, you can add "fixes #issue_number" into this
PR description. This way the issue will be automatically closed when
your PR is merged.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

- \[ ] Followed [lint naming conventions][lint_naming]
- \[ ] Added passing UI tests (including committed `.stderr` file)
- \[ ] `cargo test` passes locally
- \[ ] Executed `cargo dev update_lints`
- \[ ] Added lint documentation
- \[ ] Run `cargo dev fmt`

[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR.

--

*Please write a short comment explaining your change (or "none" for internal only changes)*
-->
Closes rust-lang#8591

I'm already sorry for the massive diff 😅

changelog: New lint [`empty_structs_with_brackets`]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-trait-system Area: Trait system A-type-system Area: Type system E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.
Projects
None yet
Development

No branches or pull requests

3 participants