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

Imperfect vtable layout with an empty super trait comming after non-empty one #114942

Closed
WaffleLapkin opened this issue Aug 17, 2023 · 1 comment · Fixed by #131864
Closed

Imperfect vtable layout with an empty super trait comming after non-empty one #114942

WaffleLapkin opened this issue Aug 17, 2023 · 1 comment · Fixed by #131864
Labels
A-trait-objects Area: trait objects, vtable layout C-enhancement Category: An issue proposing an enhancement or a PR with one. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such F-trait_upcasting `#![feature(trait_upcasting)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@WaffleLapkin
Copy link
Member

I tried this code:

#![feature(rustc_attrs)]
#![allow(internal_features)]

#[rustc_dump_vtable]
trait Marker {}
trait Pencil { fn f(&self) {} }

#[rustc_dump_vtable]
trait Imperfection: Pencil + Marker {}

struct T;
impl Marker for T {}
impl Pencil for T {}
impl Imperfection for T {}

fn main() {
    (&T as &dyn Imperfection).f();
    let _a = &T as &dyn Marker;
}

(play)

I expected Imperfection to have vtable layout which is DSA, Pencil::f.

Instead, it also includes vptr(Marker). Note that this is unnecessary as Marker's vtable only contains DSA triplet and Imperfection's vtable can be casted to Marker's. Ideally we should never emit vptrs to empty traits.

error: vtable entries for `<T as Imperfection>`: [
           MetadataDropInPlace,
           MetadataSize,
           MetadataAlign,
           Method(<T as Pencil>::f),
           TraitVPtr(<T as Marker>),
       ]
 --> src/main.rs:9:1
  |
9 | trait Imperfection: Pencil + Marker {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: vtable entries for `<T as Marker>`: [
           MetadataDropInPlace,
           MetadataSize,
           MetadataAlign,
       ]
 --> src/main.rs:5:1
  |
5 | trait Marker {}
  | ^^^^^^^^^^^^

This is a continuation of #113840, inspired by #113856 (comment); #113856 missed this case.

Meta

rustc version:

1.73.0-nightly (2023-08-16 07438b0928c6691d6ee7)
@WaffleLapkin WaffleLapkin added C-bug Category: This is a bug. F-trait_upcasting `#![feature(trait_upcasting)]` A-trait-objects Area: trait objects, vtable layout labels Aug 17, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 17, 2023
@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 18, 2023
@crlf0710 crlf0710 added C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed C-bug Category: This is a bug. labels Aug 19, 2023
@workingjubilee workingjubilee added the C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such label Oct 8, 2023
@lcnr
Copy link
Contributor

lcnr commented Oct 17, 2024

This also affects auto-traits where it seems the most clearly wrong, cc #131820

@bors bors closed this as completed in 765e8c7 Oct 18, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 18, 2024
Rollup merge of rust-lang#131864 - lrh2000:upcast_reorder, r=WaffleLapkin

Never emit `vptr` for empty/auto traits

Emiting `vptr`s for empty/auto traits is unnecessary (rust-lang#114942) and causes unsoundness in `trait_upcasting` (rust-lang#131813). This PR should ensure that we never emit vtables for such traits. See the linked issues for more details.

I'm not sure if I can add tests for the vtable layout. So this PR only adds tests for the soundness hole (i.e., the segmentation fault will disappear after this PR).

Fixes rust-lang#114942
Fixes rust-lang#131813

Cc rust-lang#65991 (tracking issue for `trait_upcasting`)

r? `@WaffleLapkin`  (per rust-lang#131813 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-objects Area: trait objects, vtable layout C-enhancement Category: An issue proposing an enhancement or a PR with one. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such F-trait_upcasting `#![feature(trait_upcasting)]` 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.

6 participants