Add a `!type` annotation to a vtable for every supertrait that is a prefix of the vtable by folkertdev · Pull Request #165475 · rust-lang/rust

folkertdev · 2026-11-04T22:00:20Z

tracking issue: #68262

This fixes a miscompilation where a vtable was incorrectly eliminated because it was only used by a supertrait.

In https://godbolt.org/z/8TE1WMvac, call_super could be applied to both A and B, but it incorrectly just always calls the B method. The vtable for A has been eliminated.

The fix here only works for the first supertrait in the list, or later ones if they have no methods. So in trait Foo: A + B, there is still a bug when B actually has methods (and its vtable is not a prefix of Foos vtable).

In that case the vtable for B does not get any !type annotations, which provides incomplete information to VFE and it may therefore miscompile. That needs some further refactoring, which is blocked on #164808.

Best reviewed commit-by-commit.

folkertdev · 2026-11-05T18:46:39Z

On current nightly the drop does not run

https://godbolt.org/z/qs4v4Wbqf

View changes since the review

folkertdev · 2026-11-05T23:34:41Z

r? WaffleLapkin

rustbot · 2026-11-05T23:34:46Z

WaffleLapkin is not on the review rotation at the moment.
They may take a while to respond.

rustbot · 2026-11-05T23:34:55Z

Some changes occurred in compiler/rustc_codegen_llvm/src/debuginfo

cc @Walnut356

In `with_self_ty` the def_id that is used is just `self.def_id`, so this is always the same DefId

rustbot · 2026-11-12T12:31:34Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

WaffleLapkin

Some minor comments;

More importantly, the type metadata / vfe seems very confusing. I wish we had some better docs for it... But I think I understood the problem? Here is how I understand it:

We want llvm to be able to de-virtualize calls through trait objects. The tool that LLVM provides for this is "type metadata" (& typed loads & vcall visibility). Type metadata is just a pair of (offset, ident) which we attach to the vtable constants, which says "'object' ident starts at offset in this constant". Then, when we load function pointers from vtables, we use llvm.type.checked.load with the ident of the vtable we are loading from. From that LLVM assumes that this load only takes pointers to constants maked with the same ident (+ the offset, but we always make it 0), and can figure out what are all the options for the loaded value.

The problem then comes when we use vtable of subtraits as the super trait's one, because it can hide an option for what the virtual function is, in cases where we don't ever generate the super trait's vtable separately. And so this can only happen with upcasting (and only the case where it uses a prefix of the vtable), because that's the only case where we load pointer from a vtable X but it's backed by a constant of vtable Y.

@folkertdev does that all sound correct?

View changes since this review

WaffleLapkin · 2026-11-12T13:18:41Z

+}
+
+impl VCallVisibility {
+    fn for_def_id<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>, def_id: DefId) -> Self {


IMO this should take a trait ref, not just the def id (as otherwise it's unclear def id of what should be passed).

WaffleLapkin · 2026-11-12T13:21:11Z

+    let Some(vcall_visibility) = trait_hierarchy
+        .iter()
+        .map(|trait_ref| VCallVisibility::for_def_id(cx, trait_ref.def_id))
+        .min()
+    else {
+        return;
+    };


Could you add a comment describing what a "minimal" visibility here means?

WaffleLapkin · 2026-11-12T13:25:47Z

+    let mut entries = vec![];
+
+    let vtable_segment_callback = |segment| -> ControlFlow<()> {
+        if let VtblSegment::TraitOwnEntries { trait_ref, emit_vptr: false } = segment {


Could you add a comment explaining why we are checking for emit_vptr: false?

WaffleLapkin

r=me with the comments from my previous review addressed.

Follow up work would be to make sure that we always go through the codepath that adds the type metadata, rather than having something different for const eval / vptrs.

View changes since this review

For now it gets at most one trait, the next step is to (recursively) provide supertraits too

…en vtable

folkertdev · 2026-11-13T22:17:03Z

We discussed this in person, but just for posterity:

Type metadata is just a pair of (offset, ident) which we attach to the vtable constants, which says "'object' ident starts at offset in this constant".

Yes. For Rust vtables the offset is always 0, C++ vtables actually use this offset parameter.

Then, when we load function pointers from vtables, we use llvm.type.checked.load with the ident of the vtable we are loading from.

Yes, and it is important that we only ever use a checked load to load a function pointer from the vtable. A normal load is not allowed. We do use normal loads for the size and align fields of the vtable header. That is fine.

From that LLVM assumes that this load only takes pointers to constants marked with the same ident (+ the offset, but we always make it 0), and can figure out what are all the options for the loaded value.

Yes, again this only works if we give LLVM accurate information. When we don't, it may miscompile.

The problem then comes when we use vtable of subtraits as the super trait's one,

Yes, specifically when we only perform checked loads for methods from the supertrait, and never with the subtrait.

because it can hide an option for what the virtual function is, in cases where we don't ever generate the super trait's vtable separately.

Nit: generating the supertraits vtable separately by itself would not be sufficient. We'd also need to perform a type load for the specific functions that are used.

And so this can only happen with upcasting (and only the case where it uses a prefix of the vtable), because that's the only case where we load pointer from a vtable X but it's backed by a constant of vtable Y.

As discussed the case where the supertrait's vtable is not a prefix is also broken. But yeah the idea is just that before this PR the vtable for Trait could be removed (correctly, based on the information that we provided to LLVM) even though we might actually be using methods from the supertrait(s).

@bors r=WaffleLapkin

rust-bors · 2026-11-13T22:17:07Z

📌 Commit 4573464 has been approved by WaffleLapkin

It is now in the queue for this repository.

Add a `!type` annotation to a vtable for every supertrait that is a prefix of the vtable tracking issue: rust-lang#68262 This fixes a miscompilation where a vtable was incorrectly eliminated because it was only used by a supertrait. In https://godbolt.org/z/8TE1WMvac, `call_super` could be applied to both `A` and `B`, but it incorrectly just always calls the `B` method. The vtable for A has been eliminated. The fix here only works for the first supertrait in the list, or later ones if they have no methods. So in `trait Foo: A + B`, there is still a bug when B actually has methods (and its vtable is not a prefix of `Foo`s vtable). In that case the vtable for `B` does not get any `!type` annotations, which provides incomplete information to VFE and it may therefore miscompile. That needs some further refactoring, which is blocked on rust-lang#164808. Best reviewed commit-by-commit.

Rollup of 17 pull requests Successful merges: - #160618 (require a consistent codegen backend when -Zllvm-target-feature is present) - #164043 (alloc: Unstably add `Vec::repeat` and `Vec::repeat_in`) - #164808 (asm const ptr: move name generation to asm-specific logic) - #164823 (Use `DerefMut` and `IndexMut` before `&raw mut` autoborrow) - #165475 (Add a `!type` annotation to a vtable for every supertrait that is a prefix of the vtable) - #165568 (rustdoc: Suggest corrections for reference label typos) - #165924 (rustc_codegen_gcc subtree update 2026-11-13) - #165927 (Remove `const_struct` from `rustc_codegen_ssa`) - #164362 (Emit note for `non_exhaustive` within defining crate for `unreachable_patterns`) - #164997 (Fix as str suggestion in subpattern) - #165825 (make `Complex<x87_f80>` ABI-compatible with C) - #165868 (Small changes to dyn compatibility) - #165879 (Add `#[rustc_test_entrypoint_marker]` back !) - #165903 (Fix borrowck ICE when a borrow outlives a local in a const block) - #165914 (Use `trim_prefix` in bootstrap) - #165919 (tests: accept better behavior on underaligned accesses) - #165936 (Enhance documentation for try_clone File implementation)

Add a `!type` annotation to a vtable for every supertrait that is a prefix of the vtable tracking issue: rust-lang#68262 This fixes a miscompilation where a vtable was incorrectly eliminated because it was only used by a supertrait. In https://godbolt.org/z/8TE1WMvac, `call_super` could be applied to both `A` and `B`, but it incorrectly just always calls the `B` method. The vtable for A has been eliminated. The fix here only works for the first supertrait in the list, or later ones if they have no methods. So in `trait Foo: A + B`, there is still a bug when B actually has methods (and its vtable is not a prefix of `Foo`s vtable). In that case the vtable for `B` does not get any `!type` annotations, which provides incomplete information to VFE and it may therefore miscompile. That needs some further refactoring, which is blocked on rust-lang#164808. Best reviewed commit-by-commit.

Rollup of 16 pull requests Successful merges: - #160618 (require a consistent codegen backend when -Zllvm-target-feature is present) - #164043 (alloc: Unstably add `Vec::repeat` and `Vec::repeat_in`) - #164823 (Use `DerefMut` and `IndexMut` before `&raw mut` autoborrow) - #165475 (Add a `!type` annotation to a vtable for every supertrait that is a prefix of the vtable) - #165568 (rustdoc: Suggest corrections for reference label typos) - #165924 (rustc_codegen_gcc subtree update 2026-11-13) - #165927 (Remove `const_struct` from `rustc_codegen_ssa`) - #164362 (Emit note for `non_exhaustive` within defining crate for `unreachable_patterns`) - #164997 (Fix as str suggestion in subpattern) - #165825 (make `Complex<x87_f80>` ABI-compatible with C) - #165868 (Small changes to dyn compatibility) - #165879 (Add `#[rustc_test_entrypoint_marker]` back !) - #165903 (Fix borrowck ICE when a borrow outlives a local in a const block) - #165914 (Use `trim_prefix` in bootstrap) - #165919 (tests: accept better behavior on underaligned accesses) - #165936 (Enhance documentation for try_clone File implementation)

folkertdev force-pushed the vfe-upcast branch 2 times, most recently from 25055da to 7663f53 Compare November 5, 2026 12:37

This comment has been minimized.

Sign in to view

folkertdev force-pushed the vfe-upcast branch from 7663f53 to c36ddd6 Compare November 5, 2026 14:07

This comment has been minimized.

Sign in to view

folkertdev force-pushed the vfe-upcast branch 2 times, most recently from 3811141 to c638b49 Compare November 5, 2026 18:35

folkertdev commented Nov 5, 2026

View reviewed changes

rustbot assigned WaffleLapkin Nov 5, 2026

folkertdev marked this pull request as ready for review November 5, 2026 23:34

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2026

This comment has been minimized.

Sign in to view

folkertdev added 2 commits November 12, 2026 13:23

join 32-bit and 64-bit virtual function elimination tests

ea12d59

use trait_ref's def_id

00b1f62

In `with_self_ty` the def_id that is used is just `self.def_id`, so this is always the same DefId

folkertdev force-pushed the vfe-upcast branch from c638b49 to 1490fdb Compare November 12, 2026 12:31

WaffleLapkin reviewed Nov 13, 2026

View reviewed changes

WaffleLapkin approved these changes Nov 13, 2026

View reviewed changes

folkertdev added 3 commits November 13, 2026 21:45

document and refactor VCallVisibility

85f0143

support a slice of (super)traits in apply_vcall_visibility_metadata

9b8230b

For now it gets at most one trait, the next step is to (recursively) provide supertraits too

add !type annotations for each (super)trait that is a prefix of a giv…

4573464

…en vtable

folkertdev force-pushed the vfe-upcast branch from 1490fdb to 4573464 Compare November 13, 2026 20:45

rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2026

Zalathar mentioned this pull request Nov 13, 2026

Rollup of 17 pull requests #165944

Closed

Zalathar mentioned this pull request Nov 14, 2026

Rollup of 16 pull requests #165945

Open

Conversation

folkertdev commented Nov 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

folkertdev Nov 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

folkertdev commented Nov 5, 2026

Uh oh!

rustbot commented Nov 5, 2026

Uh oh!

rustbot commented Nov 5, 2026

Uh oh!

This comment has been minimized.

rustbot commented Nov 12, 2026

Uh oh!

WaffleLapkin left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WaffleLapkin Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

WaffleLapkin Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

WaffleLapkin Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

WaffleLapkin left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

folkertdev commented Nov 13, 2026

Uh oh!

rust-bors Bot commented Nov 13, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

folkertdev commented Nov 4, 2026 •

edited

Loading

folkertdev Nov 5, 2026 •

edited by rustbot

Loading

WaffleLapkin left a comment •

edited by rustbot

Loading

WaffleLapkin left a comment •

edited by rustbot

Loading