CFI: Support calling methods on supertraits

[maurer]

Automatically adjust Virtual calls to supertrait functions to use the supertrait's trait object type as the receiver rather than the child trait.

cc @compiler-errors - this is the next usage of trait_object_ty I intend to have, so I thought it might be relevant while reviewing the existing one.

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[maurer]

r? @ghost

(Sorry for the noise, I didn't think a draft PR would auto-assign anyone)

[rustbot]

Failed to set assignee to ghost: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[workingjubilee]

@maurer It's not possible to unassign people by calling rustbot, sorry!

[workingjubilee]

@maurer I can't find this commit or a close equivalent in either of your "source" PRs, I assume this was an even-further-deferred followup?

[maurer]

@maurer I can't find this commit or a close equivalent in either of your "source" PRs, I assume this was an even-further-deferred followup?

This is a different approach. My source PR originally shimmed each vtable to be callable at a its vtable type. For example, if you had Child and Parent1 from the attached test, you'd get p1, CfiShim(p1, dyn Child), and CfiShim(p1, dyn Parent1). This instead adjusts each callsite (the Virtual) to go create a label with a label that matches the minimum trait. This is the approach I described for getting LLVM CFI shimless and reducing the number of shims in KCFI. In this case, there is only p1, it gets assigned the label that would previously have been given to CfiShim(p1, dyn Parent1), and Virtual(p1, _) gets its Self arg rewritten from dyn Child to dyn Parent1. This covers the vtable case, but you'll note that p1 at concrete type no longer exists. This can be addressed in LLVM CFI by assigning both typeids, but in KCFI, indirect calls to concrete p1 will now need a shim.

I was previously thinking of this as a follow-on, but since shims are the most contentious part of my plan, I implemented it early to make it possible to do KCFI trait objects with supertraits, even if concrete function pointers to drop_in_place and pointers to concrete trait methods will fail type tests until there are shims.

I do think at this point after breaking things down into pieces that I may be able to avoid CfiShim though, and lean on conditional injection of Reify to resolve this for methods. drop_in_place may be a bit more complicated to get right without widening the alias set too much.

[workingjubilee]

Aha! If we can avoid CfiShim, that would probably be for the best.

[maurer]

Aha! If we can avoid CfiShim, that would probably be for the best.

The one place I'm still iffy about is that track_caller generates vtable entries with ReifyShim for functions which are otherwise directly accessible. I think I can get away with inverting the behavior if it's one of these cases (ReifyShim is abstract, non-shimmed is concrete if you set #[track_caller]), but I haven't actually written the code for this yet. (And it still might go wrong if you wanted to set it to a non-track-caller function pointer? It's awkward.) It would still involve modifying the shimming behavior of the compiler, specifically going to resolve_for_fn_ptr, and if cfi-shimming is enabled, we'd always ReifyShim the target unless it was track_caller.

Then, over here in the typeid code, if we see ReifyShim and it's not track_caller, we don't abstract the type. If we see ReifyShim and it is track_caller, we do abstract it.

We shouldn't need to worry about the original use of ReifyShim (converting a virtual call to a function pointer), because a virtual call can't be concretized any further, so its "concrete" type will remain abstract.

Note that this still leaves drop_in_place open. Unless we want all vtables swappable when it comes to calls to drop_in_place, something interesting is going to need to happen there. My previous strategy was CfiShim, but we might be able to use either an extra argument to DropGlue or just (ick) being OK with all vtable swappability for drop calls.

[rustbot]

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

Some changes occurred in compiler/rustc_symbol_mangling/src/typeid

cc @rust-lang/project-exploit-mitigations, @rcvalle

[rcvalle]

Is this always resolving it to the trait that implements the method (similarly to what I was doing by resolving to the identity of the trait that implements the method), but with args substitution instead?

[maurer]

Is this always resolving it to the trait that implements the method (similarly to what I was doing by resolving to the identity of the trait that implements the method), but with args substitution instead?

Resolving to the trait that implements the method is already done on the declaration with the existing code, and I believe is what your previous implementation did.

This adjusts callsites in the same way, so that the type they test against is the one that we put on the declaration, even if they are calling a method on an trait bound of the real dyn object they're working with. I don't think that the previous implementation did this, as until I plumbed Instance through call, we wouldn't have had access to know whether or not these were trait methods/virtual calls vs regular old function pointers.

[rcvalle]

I didn't know bringing the Instance down to rustc_codegen_llvm would be okay--I did bring fn_attrs, which we should probably remove later, since now we've the Instance--so that is one of the reasons I used the identity of the trait that implemented the method instead both when declaring/defining functions and methods and during code generation at call sites.

[rcvalle]

LGTM.

[bors]

☔ The latest upstream changes (presumably #123147) made this pull request unmergeable. Please resolve the merge conflicts.

[compiler-errors]

@bors r+

[bors]

📌 Commit d301f40 has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit d301f40 with merge 50e3d62...

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 50e3d62 to master...

[rust-timer]

Finished benchmarking commit (50e3d62): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[1.2%, 1.2%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.117s -> 669.053s (-0.01%)
Artifact size: 315.74 MiB -> 315.73 MiB (-0.00%)