Erase late bound regions for dropped instances

[Rattenkrieg]

Fixes #107205

It may happen that a symbol for vtable shim will be instantiated with ReLateBound regions, which will affect generation of symbol name in get_fn:

rust/compiler/rustc_codegen_llvm/src/callee.rs

Line 37 in 8dabf5d

let sym = tcx.symbol_name(instance).name;

Thus, the name used to refere to the function will be inconsistent with the actual name emitted for monomorphized item:

rust/compiler/rustc_codegen_ssa/src/mono_item.rs

Line 116 in 8dabf5d

let symbol_name = self.symbol_name(cx.tcx()).name;

This PR introduces late bounds erasing pass for DropGlue shims before its instantiations.

Open questions:

• Should we erase other TyKinds other than FnPtr? If so, should we introduce generic logic, say in terms of TypeFolders? I found one suitable for such needs in the hir_analyis sources and gave it a try only to see how stdlib fails to link - I'm going to craft some reproducers for that later.

[rustbot]

r? @michaelwoerister

(rustbot has picked a reviewer for you, use r? to override)

[cjgillot]

More simply, should resolve_drop_in_place call tcx.erase_regions on its argument?

[Rattenkrieg]

This is something I thought about adding to the list of open questions, but forgot as soon as I wrote the first one 🐟.

[Rattenkrieg]

Apparently there are issues building stdlb on linux that have been introduced by the changes made in this PR. I didn't encounter this on apple aarch64 while working on this PR. I will try to reproduce this on a linux machine.

[bugadani]

This test case is different from the original issue reproducer and compiles on current stable unless I'm missing something important: https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=0bb75ef9d55af8608afe6ddfaa054bd3

[Rattenkrieg]

I was trying to reduce repro so that there would be no dependencies on stdlib traits. By the time I started working on this ~2 weeks ago this code was failing on current master d117135 on aarch64 apple. I can't recall whether I tested it in playground or not though. I've checked that original repro still causing linker to fail, so I'm going to deal with linux issues first and then get back to the repro. Thanks for pointing this out!

[Rattenkrieg]

I was able to reproduce it with simple ./x.py build --stage 2 compiler/rustc on both linux and mac. I was using simple ./x.py build library before, that happens to build only std with the stage 1.

[michaelwoerister]

Thanks for the PR! I'll take a closer look shortly.

[bors]

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

[rustbot]

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

Some changes occurred in src/tools/cargo

cc @ehuss

[michaelwoerister]

This looks good to me -- but I have no good intuition about the question whether other types should be handled here too. I can certainly imagine a similar situation can arise for trait objects.

[michaelwoerister]

Can you try to turn this into a build-pass UI test? That would be more lightweight than a run-make test. Here is an example:

rust/tests/ui/associated-types/issue-64848.rs

Line 1 in 9556b56

// build-pass

[Rattenkrieg]

Moved it under tests/ui/coercion/ directory.

[michaelwoerister]

r? types -- I don't have the context to answer the open question.

[cjgillot]

Suggested change

	let ty_erased =
	if let (true, TyKind::FnPtr(poly_fn_sig)) = (ty.has_late_bound_regions(), ty.kind()) {
	let re_erased = tcx.erase_late_bound_regions(*poly_fn_sig);
	let rebound = Binder::dummy(re_erased);
	let reassembled = tcx.mk_fn_ptr(rebound);
	reassembled
	} else {
	ty
	};
	// Erase regions before calling resolve, as they must not change the resolution.
	let ty_erased = tcx.erase_regions(ty);

[michaelwoerister]

I think I tried that but normalizing late bound regions is not enough in this case. There's still a mismatch because one symbol looks like core::ptr::drop_in_place::<for<'a> fn(&'a ())> and the other like core::ptr::drop_in_place::<fn(&())>.

[cjgillot]

Changed my suggestion to use erase_regions that should erase everything.

[cjgillot]

Well, not sure actually. It's probably the instance resolution code that needs to be fortified against leaking regions.

[Rattenkrieg]

Changed my suggestion to use erase_regions that should erase everything.

I believe this already happening without desired effect wrt to this bug: Instance::expect_resolve -> Instance::resolve -> Instance::resolve_opt_const_arg -> tcx.erase_regions(substs). Anyway, I'm going to give it a try.

[michaelwoerister]

Changed my suggestion to use erase_regions that should erase everything.

That's what I thought too, but it doesn't seem to be the case:

/// Returns an equivalent value with all free regions removed (note
/// that late-bound regions remain, because they are important for
/// subtyping, but they are anonymized and normalized as well)..
pub fn erase_regions(self, value: T) -> T

[Rattenkrieg]

Just checked, it doesn't erase late bound regions.

[Rattenkrieg]

rust/compiler/rustc_middle/src/ty/erase_regions.rs

Lines 54 to 64 in b5c8c32

	fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> {
	// because late-bound regions affect subtyping, we can't
	// erase the bound/free distinction, but we can replace
	// all free regions with 'erased.
	//
	// Note that we *CAN* replace early-bound regions -- the
	// type system never "sees" those, they get substituted
	// away. In codegen, they will always be erased to 'erased
	// whenever a substitution occurs.
	match *r {
	ty::ReLateBound(..) => r,

this is the TypeFolder invoked during erase_regions

[jackh726]

I'm not obviously sure if this is the correct approach.

Fn pointers with and without higher-ranked types are definitely different types. I'm not actually sure if we expect different vtables for them or not. If so, then I would imagine this is not correct. If not, then I suppose this is fine.

[bors]

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

[Rattenkrieg]

I'm not obviously sure if this is the correct approach.

Fn pointers with and without higher-ranked types are definitely different types. I'm not actually sure if we expect different vtables for them or not. If so, then I would imagine this is not correct. If not, then I suppose this is fine.

To my knowledge, lifetime regions are just a matter of type/borrow checker, and their bounds meant not to be a part of codegen stage. Otherwise monomorphization would have taken care of that, and as I mentioned in the original message, there is no special logic wrt regions in mono item symbol name generation.

rust/compiler/rustc_codegen_ssa/src/mono_item.rs

Line 116 in 8dabf5d

let symbol_name = self.symbol_name(cx.tcx()).name;

I could be completely wrong in my understanding, but regardless of my assumptions, there is a mismatch between the callee and callsite names of the vtable functions.

[pnkfelix]

Nominating for discussion at T-types meeting, to resolve the questions raised about how to handle cases like #107205

@rustbot label: I-types-nominated

[BoxyUwU]

This PR is incorrect as is:
Erasing late bound regions is wrong, for<'a> fn(&'a ()) and fn(&'static ()) are different types not just from lifetimes but also to hir typeck and and TypeId. it is possible to implement the same trait for both types and have differing behaviour. In this specific case of fn ptrs they have no drop glue so its not observable

A targetted fix just for drop in place of fn ptrs is not enough, this problem is not restricted to fn ptrs or drop in place. From looking at rustc_codegen_ssa for a bit (all day 😭) this is a problem for all cases where we have mir like _7 = _8 and typeof(_8) is a supertype of typeof(_7) rather than equal to it. If we then unsize _7 we will create a vtable as if it had _8's type which is exploitable to create unsoundness (which is demonstrated in the original issue now)

[jackh726]

It would be helpful to create a MCVE of the program @BoxyUwU is talking about. (This is precisely the type of program I was thinking about originally)

That being said, I think marking this as waiting on team is the best label here, since it's types-nominated. It might be worth doing a deep dive to discuss (if nobody individually speaks up with a solution).

[BoxyUwU]

a code example for unsoundness caused by this bug is present in the issue as a comment: #107205 (comment)

[lcnr]

as stated above, erasing late bound regions is unsound as it can influence trait solving.

the underlying issue here seems to be is that we incorrectly track the types of values without updating the type when the value flows over a "subtyping edge".

looking into a fix for that and discussing it in #107205. Given that we will have to go with a completely separate approach to this PR I am going to remove the types nomination and close this issue.

Thank you for @Rattenkrieg for working on this, even if it didn't work out in the end.