Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CFI: Fix encode_region: unexpected ReEarlyBound(0, 'a) #111851

Merged
merged 1 commit into from
May 23, 2023
Merged

CFI: Fix encode_region: unexpected ReEarlyBound(0, 'a) #111851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,12 +272,11 @@ fn encode_region<'tcx>(
s.push('E');
compress(dict, DictKey::Region(region), &mut s);
}
RegionKind::ReErased => {
RegionKind::ReEarlyBound(..) | RegionKind::ReErased => {
Copy link
Member

@compiler-errors compiler-errors May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we not just erasing regions somewhere up the callstack? I don't think we should be handing early-bound vars here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below.

s.push_str("u6region");
compress(dict, DictKey::Region(region), &mut s);
}
RegionKind::ReEarlyBound(..)
| RegionKind::ReFree(..)
RegionKind::ReFree(..)
| RegionKind::ReStatic
| RegionKind::ReError(_)
| RegionKind::ReVar(..)
Expand Down Expand Up @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but why are we erasing projection predicates here? Those distinguish object types in a meaningful way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

@compiler-errors compiler-errors May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

rust/compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs

Line 1067 in 52dd1cd

if let Some(trait_ref) = tcx.impl_trait_ref(impl_def_id.unwrap()) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is an "identity trait" I have never heard this terminology before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using the trait ref without substituting it?

rust/compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs

Line 1075 in 52dd1cd

existential_predicate.skip_binder(),

This seems like it's accidentally throwing away the substs that are carried by the instance:

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.Instance.html#structfield.substs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ty::ExistentialPredicate::Trait(trait_ref) => {
let trait_ref = ty::TraitRef::identity(tcx, trait_ref.def_id);
ty::Binder::dummy(ty::ExistentialPredicate::Trait(
Some(ty::Binder::dummy(ty::ExistentialPredicate::Trait(
ty::ExistentialTraitRef::erase_self_ty(tcx, trait_ref),
))
)))
}
_ => predicate,
ty::ExistentialPredicate::Projection(..) => None,
ty::ExistentialPredicate::AutoTrait(..) => Some(predicate),
})
.collect();
tcx.mk_poly_existential_predicates(&predicates)
Expand Down
18 changes: 9 additions & 9 deletions tests/codegen/sanitizer-cfi-emit-type-metadata-id-itanium-cxx-abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,15 +536,15 @@ pub fn foo149(_: Type14<Bar>, _: Type14<Bar>, _: Type14<Bar>) { }
// CHECK: ![[TYPE93]] = !{i64 0, !"_ZTSFvPFu3i32S_EE"}
// CHECK: ![[TYPE94]] = !{i64 0, !"_ZTSFvPFu3i32S_ES0_E"}
// CHECK: ![[TYPE95]] = !{i64 0, !"_ZTSFvPFu3i32S_ES0_S0_E"}
// CHECK: ![[TYPE96]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function2FnIu5paramEu{{[0-9]+}}NtNtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnce6OutputIu5tupleIu3i32EES1_u6regionEEE"}
// CHECK: ![[TYPE97]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function2FnIu5paramEu{{[0-9]+}}NtNtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnce6OutputIu5tupleIu3i32EES1_u6regionEES6_E"}
// CHECK: ![[TYPE98]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function2FnIu5paramEu{{[0-9]+}}NtNtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnce6OutputIu5tupleIu3i32EES1_u6regionEES6_S6_E"}
// CHECK: ![[TYPE99]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function5FnMutIu5paramEu{{[0-9]+}}NtNtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnce6OutputIu5tupleIu3i32EES1_u6regionEEE"}
// CHECK: ![[TYPE100]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function5FnMutIu5paramEu{{[0-9]+}}NtNtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnce6OutputIu5tupleIu3i32EES1_u6regionEES6_E"}
// CHECK: ![[TYPE101]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function5FnMutIu5paramEu{{[0-9]+}}NtNtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnce6OutputIu5tupleIu3i32EES1_u6regionEES6_S6_E"}
// CHECK: ![[TYPE102]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnceIu5paramEu{{[0-9]+}}NtNtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnce6OutputIu5tupleIu3i32EES1_u6regionEEE"}
// CHECK: ![[TYPE103]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnceIu5paramEu{{[0-9]+}}NtNtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnce6OutputIu5tupleIu3i32EES1_u6regionEES6_E"}
// CHECK: ![[TYPE104]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnceIu5paramEu{{[0-9]+}}NtNtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnce6OutputIu5tupleIu3i32EES1_u6regionEES6_S6_E"}
// CHECK: ![[TYPE96]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function2FnIu5paramEu6regionEEE"}
// CHECK: ![[TYPE97]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function2FnIu5paramEu6regionEES3_E"}
// CHECK: ![[TYPE98]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function2FnIu5paramEu6regionEES3_S3_E"}
// CHECK: ![[TYPE99]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function5FnMutIu5paramEu6regionEEE"}
// CHECK: ![[TYPE100]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function5FnMutIu5paramEu6regionEES3_E"}
// CHECK: ![[TYPE101]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function5FnMutIu5paramEu6regionEES3_S3_E"}
// CHECK: ![[TYPE102]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnceIu5paramEu6regionEEE"}
// CHECK: ![[TYPE103]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnceIu5paramEu6regionEES3_E"}
// CHECK: ![[TYPE104]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtNtC{{[[:print:]]+}}_4core3ops8function6FnOnceIu5paramEu6regionEES3_S3_E"}
// CHECK: ![[TYPE105]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtC{{[[:print:]]+}}_4core6marker4Sendu6regionEEE"}
// CHECK: ![[TYPE106]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtC{{[[:print:]]+}}_4core6marker4Sendu6regionEES2_E"}
// CHECK: ![[TYPE107]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtNtC{{[[:print:]]+}}_4core6marker4Sendu6regionEES2_S2_E"}
Expand Down
31 changes: 31 additions & 0 deletions tests/codegen/sanitizer-cfi-emit-type-metadata-trait-objects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,20 @@ impl<T, U> Trait3<U> for T {
}
}

pub trait Trait4<'a, T> {
type Output: 'a;
fn qux(&self, _: &T) -> Self::Output;
}

pub struct Type4;

impl<'a, T, U> Trait4<'a, U> for T {
type Output = &'a i32;
fn qux(&self, _: &U) -> Self::Output {
&0
}
}

pub fn foo1(a: &dyn Trait1) {
a.foo();
// CHECK-LABEL: define{{.*}}4foo1{{.*}}!type !{{[0-9]+}}
Expand Down Expand Up @@ -84,6 +98,23 @@ pub fn bar3() {
// CHECK: call i1 @llvm.type.test({{i8\*|ptr}} {{%f|%[0-9]}}, metadata !"[[TYPE3:[[:print:]]+]]")
}

pub fn foo4<'a>(a: &dyn Trait4<'a, Type4, Output = &'a i32>) {
let b = Type4;
a.qux(&b);
// CHECK-LABEL: define{{.*}}4foo4{{.*}}!type !{{[0-9]+}}
// CHECK: call i1 @llvm.type.test({{i8\*|ptr}} {{%f|%[0-9]}}, metadata !"[[TYPE4:[[:print:]]+]]")
}

pub fn bar4<'a>() {
let a = Type4;
foo4(&a);
let b = &a as &dyn Trait4<'a, Type4, Output = &'a i32>;
b.qux(&a);
// CHECK-LABEL: define{{.*}}4bar4{{.*}}!type !{{[0-9]+}}
// CHECK: call i1 @llvm.type.test({{i8\*|ptr}} {{%f|%[0-9]}}, metadata !"[[TYPE4:[[:print:]]+]]")
}

// CHECK: !{{[0-9]+}} = !{i64 0, !"[[TYPE1]]"}
// CHECK: !{{[0-9]+}} = !{i64 0, !"[[TYPE2]]"}
// CHECK: !{{[0-9]+}} = !{i64 0, !"[[TYPE3]]"}
// CHECK: !{{[0-9]+}} = !{i64 0, !"[[TYPE4]]"}
31 changes: 31 additions & 0 deletions tests/codegen/sanitizer-kcfi-emit-type-metadata-trait-objects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,20 @@ impl<T, U> Trait3<U> for T {
}
}

pub trait Trait4<'a, T> {
type Output: 'a;
fn qux(&self, _: &T) -> Self::Output;
}

pub struct Type4;

impl<'a, T, U> Trait4<'a, U> for T {
type Output = &'a i32;
fn qux(&self, _: &U) -> Self::Output {
&0
}
}

pub fn foo1(a: &dyn Trait1) {
a.foo();
// CHECK-LABEL: define{{.*}}4foo1{{.*}}!{{<unknown kind #36>|kcfi_type}} !{{[0-9]+}}
Expand Down Expand Up @@ -108,6 +122,23 @@ pub fn bar3() {
// CHECK: call void %{{[0-9]}}({{\{\}\*|ptr}} align 1 {{%[a-z]\.0|%_[0-9]}}, {{\{\}\*|ptr|%Type3\*}} align 1 {{%[a-z]\.0|%_[0-9]}}){{.*}}[ "kcfi"(i32 [[TYPE3:[[:print:]]+]]) ]
}

pub fn foo4<'a>(a: &dyn Trait4<'a, Type4, Output = &'a i32>) {
let b = Type4;
a.qux(&b);
// CHECK-LABEL: define{{.*}}4foo4{{.*}}!{{<unknown kind #36>|kcfi_type}} !{{[0-9]+}}
// CHECK: call align 4 {{ptr|i32\*}} %{{[0-9]}}({{\{\}\*|ptr}} align 1 {{%[a-z]\.0|%_[0-9]}}, {{\{\}\*|ptr|%Type4\*}} align 1 {{%[a-z]\.0|%_[0-9]}}){{.*}}[ "kcfi"(i32 [[TYPE4:[[:print:]]+]]) ]
}

pub fn bar4<'a>() {
let a = Type4;
foo4(&a);
let b = &a as &dyn Trait4<'a, Type4, Output = &'a i32>;
b.qux(&a);
// CHECK-LABEL: define{{.*}}4bar4{{.*}}!{{<unknown kind #36>|kcfi_type}} !{{[0-9]+}}
// CHECK: call align 4 {{ptr|i32\*}} %{{[0-9]}}({{\{\}\*|ptr}} align 1 {{%[a-z]\.0|%_[0-9]}}, {{\{\}\*|ptr|%Type4\*}} align 1 {{%[a-z]\.0|%_[0-9]}}){{.*}}[ "kcfi"(i32 [[TYPE4:[[:print:]]+]]) ]
}

// CHECK: !{{[0-9]+}} = !{i32 [[TYPE1]]}
// CHECK: !{{[0-9]+}} = !{i32 [[TYPE2]]}
// CHECK: !{{[0-9]+}} = !{i32 [[TYPE3]]}
// CHECK: !{{[0-9]+}} = !{i32 [[TYPE4]]}