From 2498a9d4642f66a931ab371ebeb316f59e49caef Mon Sep 17 00:00:00 2001 From: Ramon de C Valle Date: Thu, 4 Apr 2024 21:03:08 -0700 Subject: [PATCH] CFI: Restore typeid_for_instance default behavior Restore typeid_for_instance default behavior of performing self type erasure, since it's the most common case and what it does most of the time. Using concrete self (or not performing self type erasure) is for assigning a secondary type id, and secondary type ids are only assigned when they're unique and to methods, and also are only tested for when methods are used as function pointers. --- compiler/rustc_codegen_llvm/src/declare.rs | 6 ++-- compiler/rustc_symbol_mangling/src/typeid.rs | 30 +++++++++++++++---- .../src/typeid/typeid_itanium_cxx_abi.rs | 2 +- ...itanium-cxx-abi-method-secondary-typeid.rs | 4 +-- 4 files changed, 29 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/declare.rs b/compiler/rustc_codegen_llvm/src/declare.rs index f58dd4066ad..f86cdcaa6f7 100644 --- a/compiler/rustc_codegen_llvm/src/declare.rs +++ b/compiler/rustc_codegen_llvm/src/declare.rs @@ -147,7 +147,7 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> { for options in [ TypeIdOptions::GENERALIZE_POINTERS, TypeIdOptions::NORMALIZE_INTEGERS, - TypeIdOptions::ERASE_SELF_TYPE, + TypeIdOptions::USE_CONCRETE_SELF, ] .into_iter() .powerset() @@ -173,9 +173,7 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> { if self.tcx.sess.is_sanitizer_kcfi_enabled() { // LLVM KCFI does not support multiple !kcfi_type attachments - // Default to erasing the self type. If we need the concrete type, there will be a - // hint in the instance. - let mut options = TypeIdOptions::ERASE_SELF_TYPE; + let mut options = TypeIdOptions::empty(); if self.tcx.sess.is_sanitizer_cfi_generalize_pointers_enabled() { options.insert(TypeIdOptions::GENERALIZE_POINTERS); } diff --git a/compiler/rustc_symbol_mangling/src/typeid.rs b/compiler/rustc_symbol_mangling/src/typeid.rs index 862ba285db8..7bd998294dd 100644 --- a/compiler/rustc_symbol_mangling/src/typeid.rs +++ b/compiler/rustc_symbol_mangling/src/typeid.rs @@ -24,9 +24,14 @@ bitflags! { /// `-fsanitize-cfi-icall-experimental-normalize-integers` option for cross-language LLVM /// CFI and KCFI support. const NORMALIZE_INTEGERS = 4; - /// Generalize the instance by erasing the concrete `Self` type where possible. - /// Only has an effect on `{kcfi_,}typeid_for_instance`. - const ERASE_SELF_TYPE = 8; + /// Do not perform self type erasure for attaching a secondary type id to methods with their + /// concrete self so they can be used as function pointers. + /// + /// (This applies to typeid_for_instance only and should be used to attach a secondary type + /// id to methods during their declaration/definition so they match the type ids returned by + /// either typeid_for_instance or typeid_for_fnabi at call sites during code generation for + /// type membership tests when methods are used as function pointers.) + const USE_CONCRETE_SELF = 8; } } @@ -69,10 +74,23 @@ pub fn kcfi_typeid_for_instance<'tcx>( instance: Instance<'tcx>, mut options: TypeIdOptions, ) -> u32 { - // If we receive a `ReifyShim` intended to produce a function pointer, we need to remain - // concrete - abstraction is for vtables. + // KCFI support for Rust shares most of its implementation with the CFI support, with some key + // differences: + // + // 1. KCFI performs type tests differently and are implemented as different LLVM passes than CFI + // to not require LTO. + // 2. KCFI has the limitation that a function or method may have one type id assigned only. + // + // Because of the limitation listed above (2), the current KCFI implementation (not CFI) does + // reifying of types (i.e., adds shims/trampolines for indirect calls in these cases) for: + // + // * Supporting casting between function items, closures, and Fn trait objects. + // * Supporting methods being cast as function pointers. + // + // This was implemented for KCFI support in #123106 and #123052 (which introduced the + // ReifyReason). The tracking issue for KCFI support for Rust is #123479. if matches!(instance.def, InstanceDef::ReifyShim(_, Some(ReifyReason::FnPtr))) { - options.remove(TypeIdOptions::ERASE_SELF_TYPE); + options.insert(TypeIdOptions::USE_CONCRETE_SELF); } // A KCFI type metadata identifier is a 32-bit constant produced by taking the lower half of the // xxHash64 of the type metadata identifier. (See llvm/llvm-project@cff5bef.) diff --git a/compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs b/compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs index c632712f5a9..7f223f13250 100644 --- a/compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs +++ b/compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs @@ -1098,7 +1098,7 @@ pub fn typeid_for_instance<'tcx>( instance.args = tcx.mk_args_trait(invoke_ty, trait_ref.args.into_iter().skip(1)); } - if options.contains(EncodeTyOptions::ERASE_SELF_TYPE) { + if !options.contains(EncodeTyOptions::USE_CONCRETE_SELF) { if let Some(impl_id) = tcx.impl_of_method(instance.def_id()) && let Some(trait_ref) = tcx.impl_trait_ref(impl_id) { diff --git a/tests/codegen/sanitizer/cfi/emit-type-metadata-id-itanium-cxx-abi-method-secondary-typeid.rs b/tests/codegen/sanitizer/cfi/emit-type-metadata-id-itanium-cxx-abi-method-secondary-typeid.rs index 999fd292fe7..671db563dde 100644 --- a/tests/codegen/sanitizer/cfi/emit-type-metadata-id-itanium-cxx-abi-method-secondary-typeid.rs +++ b/tests/codegen/sanitizer/cfi/emit-type-metadata-id-itanium-cxx-abi-method-secondary-typeid.rs @@ -18,5 +18,5 @@ impl Trait1 for Type1 { } -// CHECK: ![[TYPE1]] = !{i64 0, !"_ZTSFvu3refIu{{[0-9]+}}NtC{{[[:print:]]+}}_{{[[:print:]]+}}5Type1EE"} -// CHECK: ![[TYPE2]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtC{{[[:print:]]+}}_{{[[:print:]]+}}6Trait1u6regionEEE"} +// CHECK: ![[TYPE1]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtC{{[[:print:]]+}}_{{[[:print:]]+}}6Trait1u6regionEEE"} +// CHECK: ![[TYPE2]] = !{i64 0, !"_ZTSFvu3refIu{{[0-9]+}}NtC{{[[:print:]]+}}_{{[[:print:]]+}}5Type1EE"}