Skip to content

Erase all regions before constructing an LLVM type #77196

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

Closed
wants to merge 1 commit into from

Conversation

Aaron1011
Copy link
Member

Fixes #55976

Previously, we only erased early-bound regions. However, two types that
differ only in their regions (including late-bound regions) are
indistinguishable to LLVM, so it's safe to erase all regions.

Fixes rust-lang#55976

Previously, we only erased early-bound regions. However, two types that
differ only in their regions (including late-bound regions) are
indistinguishable to LLVM, so it's safe to erase all regions.
@rust-highfive
Copy link
Contributor

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2020
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Sep 25, 2020

⌛ Trying commit 0446a73 with merge da15df4c276090851136ca9a686285f76cbaaf98...

@bors
Copy link
Collaborator

bors commented Sep 25, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: da15df4c276090851136ca9a686285f76cbaaf98 (da15df4c276090851136ca9a686285f76cbaaf98)

@rust-timer
Copy link
Collaborator

Queued da15df4c276090851136ca9a686285f76cbaaf98 with parent b984ef6, future comparison URL.

@petrochenkov
Copy link
Contributor

None of the most relevant people seem to do reviews right now.
r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned petrochenkov Sep 25, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (da15df4c276090851136ca9a686285f76cbaaf98): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@Aaron1011
Copy link
Member Author

@eddyb: Are there any changes that you'd like me to make?

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2020
@eddyb
Copy link
Member

eddyb commented Oct 31, 2020

Sorry for not seeing this earlier (please PM on e.g. Zulip if you're blocked on me for anything).

Traditionally we've avoided this because projections can observe a difference in bound lifetimes. In other words, this PR introduces unsoundness: it should be possible to cause some transmuting to happen (through safe code) by having the same trait implemented for fn(&()) and fn(&'a ()) (but with different types for an associated type), as the erasure can turn the former into the latter, though maybe LLVM assertions would prevent most of the simpler exploits.

What we've done instead is aggressive LLVM casting everywhere to handle subtyping (I recall @pnkfelix was involved).

At some point (while @RalfJung was trying to do some type checks in miri) I've suggested doing "variance-based erasing of bound lifetimes", along the lines of "projections will introduce invariance and therefore bound lifetimes nested in covariant/contravariant positions should be safe to erase" (assuming the type then doesn't touch the trait system, maybe we should have a variation on re_erased that the trait system instantly ICEs if seen anywhere).

I'm tempted to close this, unless you plan to reuse the PR for a different implementation. Feel free to PM me if you want to discuss this further or come up with a test that shows the unsoundness full erasure introduces.

r? @nikomatsakis for the time being

@eddyb
Copy link
Member

eddyb commented Nov 3, 2020

In Zulip PMs with @Aaron1011 I came up with this sketch, but I don't think it was ever tested:

trait Trait<T> {
    type Assoc;
}
impl<T> Trait<T> for fn(&()) {
    type Assoc = T;
}
impl<'a, T> Trait<T> for fn(&'a ()) {
    type Assoc = ();
}
#[repr(C)]
struct Union<T, U, F: Trait<U>> {
    to: F::Assoc,
    from: T,
}
pub fn transmute<T, U>(x: T) -> U {
    Union::<Option<T>, Option<U>, fn(&())> { to: None, from: Some(x) }.to.unwrap()
}

With full erasure, layout_of(Union<Option<T>, Option<U>, fn(&())>) would normalize to's type to () and therefore the to and from fields would overlap, i.e. Some(x) would be written to offset 0, and the same offset 0 is where .to.unwrap() would extract x from (but read as type U).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustc seemingly generates invalid IR
8 participants