CFI: Fix encode_region: unexpected ReEarlyBound(0, 'a) #111851
Conversation
|
r? @b-naber (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
r? @bjorn3 |
rcvalle
changed the title
rcvalle
force-pushed
the
rust-cfi-fix-111515
branch
3 times, most recently
from
d4a3915
to
03df1cc
Compare
This comment has been minimized.
This comment has been minimized.
rcvalle
force-pushed
the
rust-cfi-fix-111515
branch
from
03df1cc
to
1a40c78
Compare
This comment has been minimized.
This comment has been minimized.
Fixes rust-lang#111515 and complements rust-lang#106547 by adding support for encoding early bound regions and also excluding projections when transforming trait objects' traits into their identities before emitting type checks.
rcvalle
force-pushed
the
rust-cfi-fix-111515
branch
from
1a40c78
to
9bbdfea
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#111427 ([rustdoc][JSON] Use exclusively externally tagged enums in the JSON representation) - rust-lang#111486 (Pretty-print inherent projections correctly) - rust-lang#111722 (Document stack-protector option) - rust-lang#111761 (fix(resolve): not defined `extern crate shadow_name`) - rust-lang#111845 (Update books) - rust-lang#111851 (CFI: Fix encode_region: unexpected ReEarlyBound(0, 'a)) - rust-lang#111871 (Migrate GUI colors test to original CSS color format) r? `@ghost` `@rustbot` modify labels: rollup
| @@ -272,12 +272,11 @@ fn encode_region<'tcx>( | |||
| s.push('E'); | |||
| compress(dict, DictKey::Region(region), &mut s); | |||
| } | |||
| RegionKind::ReErased => { | |||
| RegionKind::ReEarlyBound(..) | RegionKind::ReErased => { | |||
There was a problem hiding this comment.
Why are we not just erasing regions somewhere up the callstack? I don't think we should be handing early-bound vars here.
| @@ -704,14 +703,15 @@ fn transform_predicates<'tcx>( | |||
| ) -> &'tcx List<ty::PolyExistentialPredicate<'tcx>> { | |||
| let predicates: Vec<ty::PolyExistentialPredicate<'tcx>> = predicates | |||
| .iter() | |||
| .map(|predicate| match predicate.skip_binder() { | |||
| .filter_map(|predicate| match predicate.skip_binder() { | |||
There was a problem hiding this comment.
Unrelated, but why are we erasing projection predicates here? Those distinguish object types in a meaningful way?
There was a problem hiding this comment.
The reason for this is when transforming the concrete self into a reference to a trait object before emitting type metadata identifiers for trait methods (in typeid_for_instance, called in predefine_fn), the trait resolved from the method is the identity trait (i.e., early bound), and this is also necessary for trait objects with generic type parameters (see Trait3 in tests).
Unless there is a way to bind the traits at that time, we'll need to work with identity traits (i.e., early bound, and that is the reason why predicates are excluded from the encoding) and add support for encoding early bound types both when emitting type metadata identifiers for trait methods and when emitting type checks.
It's okay to work with early bound types for CFI since we're not encoding/mangling names for linking. (Surely it'd be better if we could bind those traits when doing the method to trait resolution in typeid_for_instance so we'd get better granularity, but I haven't found a way to do that.)
There was a problem hiding this comment.
I'm sorry, I'm not really sure if I follow.
What I mean is that you should be passing this trait ref through tcx.erase_regions:
There was a problem hiding this comment.
what is an "identity trait" I have never heard this terminology before.
There was a problem hiding this comment.
Would you mind providing more information? I think that for CFI we want to know and encode that there was an (erased) region there so trait methods are properly grouped together.
There was a problem hiding this comment.
what is an "identity trait" I have never heard this terminology before.
The trait (reference) returned by https://doc.rust-lang.org/beta/nightly-rustc/rustc_middle/ty/struct.TraitRef.html#method.identity (which is early bound). (I'm not sure if identity trait is the actual correct name/terminology, but I've been using that to refer to it.)
There was a problem hiding this comment.
Why are you using the trait ref without substituting it?
This seems like it's accidentally throwing away the substs that are carried by the instance:
There was a problem hiding this comment.
It's intentional--but probably due to my lack of understanding--for grouping trait methods together for CFI. It's currently grouping methods per trait identity (i.e., early bound), but it surely would be better to group them per trait implementation and specialization (i.e., bound).
However, the instance has a concrete self (i.e., it was monomorphized already), and I'm transforming the concrete self into a reference to a trait object (for grouping trait methods together for CFI) by doing the method to trait resolution in typeid_for_instance. Are the subts from the instance there, the substs expected by the trait identity resolved from the method to trait resolution also there?
There was a problem hiding this comment.
Even though this was merged, lets continue the discussion because I'm interested in finding out a way to group trait methods more granularly, and if the substs from the monomorphized instance can be used in the trait ref returned from the method to trait resolution that would be great--and I can follow up with a PR to increase the granularity for trait objects using that.
Fixes #111515 and complements #106547 by adding support for encoding early bound regions and also excluding projections when transforming trait objects' traits into their identities before emitting type checks.