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

possible bug in vtable formation #113840

Closed
nikomatsakis opened this issue Jul 18, 2023 · 2 comments · Fixed by #113856
Closed

possible bug in vtable formation #113840

nikomatsakis opened this issue Jul 18, 2023 · 2 comments · Fixed by #113856
Assignees
Labels
A-trait-system Area: Trait system F-trait_upcasting `#![feature(trait_upcasting)]`

Comments

@nikomatsakis
Copy link
Contributor

While working on #112355 I think I found a bug. Given this program:

#![feature(rustc_attrs)]

#[rustc_dump_vtable]
pub trait What: Send + Sync {
    fn a(&self) {}
}

impl What for () {}

fn main() {
    (&() as &dyn What).a();
}

rustc outputs the following vtable layout:

 [
    MetadataDropInPlace,
    MetadataSize,
    MetadataAlign,
    TraitVPtr(<() as Sync>),
    Method(<() as What>::a),
]

Which is surprising, because TraitVPtr(<() as Sync>) is unnecessary — Send does not have any methods, so Sync vtable can be inlined...

I think we should be smarter here:

// Other than the left-most path, vptr should be emitted for each trait.
emit_vptr_on_new_entry = true;

(for the record, I'm pretty sure this happens in practice, I've found this with log::Log)

Originally posted by @WaffleLapkin in #65991 (comment)

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 18, 2023
@nikomatsakis
Copy link
Contributor Author

cc @crlf0710

@WaffleLapkin WaffleLapkin self-assigned this Jul 19, 2023
@WaffleLapkin WaffleLapkin added A-trait-system Area: Trait system F-trait_upcasting `#![feature(trait_upcasting)]` and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 19, 2023
@bors bors closed this as completed in 399b068 Jul 20, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jul 22, 2023
Refactor vtable encoding and optimize it for the case of multiple marker traits

This PR does two things
- Refactor `prepare_vtable_segments` (this was motivated by the other change, `prepare_vtable_segments` was quite hard to understand and while trying to edit it I've refactored it)
  - Mostly remove `loop`s labeled `break`s/`continue`s whenever there is a simpler solution
  - Also use `?`
- Make vtable format a bit more efficient wrt to marker traits
  - See the tests for an example

Fixes rust-lang/rust#113840
cc `@crlf0710`

----

Review wise it's probably best to review each commit individually, as then it's more clear why the refactoring is correct.

I can split the last two commits (which change behavior) into a separate PR if it makes reviewing easier
@crlf0710
Copy link
Member

sorry been slow a little recently... This is a nice improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system F-trait_upcasting `#![feature(trait_upcasting)]`
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants