From ec77a0af3d578214dff1110dfdbbf3a84c2072fb Mon Sep 17 00:00:00 2001 From: George Bateman Date: Sun, 29 Jan 2023 23:03:27 +0000 Subject: [PATCH] Block function bindings which pass extended floats by value --- bindgen/codegen/helpers.rs | 54 +++--- bindgen/codegen/mod.rs | 28 ++++ bindgen/ir/analysis/mod.rs | 2 + bindgen/ir/analysis/never_by_value.rs | 230 ++++++++++++++++++++++++++ bindgen/ir/context.rs | 36 +++- 5 files changed, 313 insertions(+), 37 deletions(-) create mode 100644 bindgen/ir/analysis/never_by_value.rs diff --git a/bindgen/codegen/helpers.rs b/bindgen/codegen/helpers.rs index 088c7f9363..6a9d3eb1b6 100644 --- a/bindgen/codegen/helpers.rs +++ b/bindgen/codegen/helpers.rs @@ -189,41 +189,35 @@ pub mod ast_ty { fk: FloatKind, layout: Option, ) -> TokenStream { - // TODO: we probably should take the type layout into account more - // often? - // - // Also, maybe this one shouldn't be the default? - match (fk, ctx.options().convert_floats) { - (FloatKind::Float, true) => quote! { f32 }, - (FloatKind::Double, true) => quote! { f64 }, - (FloatKind::Float, false) => raw_type(ctx, "c_float"), - (FloatKind::Double, false) => raw_type(ctx, "c_double"), - (FloatKind::LongDouble, _) => { - match layout { - Some(layout) => { - match layout.size { - 4 => quote! { f32 }, - 8 => quote! { f64 }, - // TODO(emilio): If rust ever gains f128 we should - // use it here and below. - _ => super::integer_type(ctx, layout) - .unwrap_or(quote! { f64 }), - } + let bits = layout.map(|l| l.size * 8); + match (fk, ctx.options().convert_floats, bits) { + // TODO: What about narrower floats? + (_, true, Some(32)) => quote! { f32 }, + (_, true, Some(64)) => quote! { f64 }, + (FloatKind::Float, ..) => raw_type(ctx, "c_float"), + (FloatKind::Double, ..) => raw_type(ctx, "c_double"), + (FloatKind::LongDouble, ..) => { + // TODO(emilio): If rust ever gains f80/f128 we should + // use it here and below. + if ctx.options().enable_cxx_namespaces { + quote! { + root::__BindgenLongDouble } - None => { - debug_assert!( - false, - "How didn't we know the layout for a primitive type?" - ); - quote! { f64 } + } else { + quote! { + __BindgenLongDouble } } } - (FloatKind::Float128, _) => { - if ctx.options().rust_features.i128_and_u128 { - quote! { u128 } + (FloatKind::Float128, ..) => { + if ctx.options().enable_cxx_namespaces { + quote! { + root::__BindgenFloat128 + } } else { - quote! { [u64; 2] } + quote! { + __BindgenFloat128 + } } } } diff --git a/bindgen/codegen/mod.rs b/bindgen/codegen/mod.rs index 6b24ae1bd0..9c472153e3 100644 --- a/bindgen/codegen/mod.rs +++ b/bindgen/codegen/mod.rs @@ -217,6 +217,9 @@ struct CodegenResult<'a> { /// Whether a bitfield allocation unit has been seen at least once. saw_bitfield_unit: bool, + /// Whether a long double has been seen at least once. + saw_long_double: bool, + items_seen: HashSet, /// The set of generated function/var names, needed because in C/C++ is /// legal to do something like: @@ -253,6 +256,7 @@ impl<'a> CodegenResult<'a> { saw_objc: false, saw_block: false, saw_bitfield_unit: false, + saw_long_double: false, codegen_id, items_seen: Default::default(), functions_seen: Default::default(), @@ -285,6 +289,10 @@ impl<'a> CodegenResult<'a> { self.saw_bitfield_unit = true; } + fn saw_long_double(&mut self) { + self.saw_long_double = true; + } + fn seen>(&self, item: Id) -> bool { self.items_seen.contains(&item.into()) } @@ -2454,6 +2462,10 @@ impl Method { _ => panic!("How in the world?"), }; + if utils::sig_unsupported_types(ctx, signature) { + return; + } + let supported_abi = match signature.abi(ctx, Some(&*name)) { ClangAbi::Known(Abi::ThisCall) => { ctx.options().rust_features().thiscall_abi @@ -4107,6 +4119,11 @@ impl CodeGenerator for Function { abi => abi, }; + if utils::sig_unsupported_types(ctx, signature) { + warn!("Skipping function which passes or returns by value types not available in Rust."); + return None; + } + // Handle overloaded functions by giving each overload its own unique // suffix. let times_seen = result.overload_number(&canonical_name); @@ -5058,4 +5075,15 @@ pub mod utils { true } + + pub fn sig_unsupported_types( + ctx: &BindgenContext, + sig: &FunctionSig + ) -> bool { + sig.argument_types().iter() + .map(|(_, ty_id)| ty_id) + .chain(std::iter::once(&sig.return_type())) + .find(|ty_id| ctx.lookup_never_by_value(*ty_id)) + .is_some() + } } diff --git a/bindgen/ir/analysis/mod.rs b/bindgen/ir/analysis/mod.rs index 40dfc6d644..666f3f8299 100644 --- a/bindgen/ir/analysis/mod.rs +++ b/bindgen/ir/analysis/mod.rs @@ -50,6 +50,8 @@ mod has_type_param_in_array; pub use self::has_type_param_in_array::HasTypeParameterInArray; mod has_float; pub use self::has_float::HasFloat; +mod never_by_value; +pub use self::never_by_value::NeverByValue; mod sizedness; pub use self::sizedness::{Sizedness, SizednessAnalysis, SizednessResult}; diff --git a/bindgen/ir/analysis/never_by_value.rs b/bindgen/ir/analysis/never_by_value.rs new file mode 100644 index 0000000000..d8f813ffd9 --- /dev/null +++ b/bindgen/ir/analysis/never_by_value.rs @@ -0,0 +1,230 @@ +//! Determining which types cannot be passed by value. + +use super::{generate_dependencies, ConstrainResult, MonotoneFramework}; +use crate::ir::comp::Field; +use crate::ir::comp::FieldMethods; +use crate::ir::context::{BindgenContext, ItemId}; +use crate::ir::traversal::EdgeKind; +use crate::ir::ty::TypeKind; +use crate::{HashMap, HashSet}; + +/// An analysis that finds each IR item which is a type which cannot be passed by value in Rust. +/// +/// This is defined as follows: +/// +/// * If T is float or complex float with size not 32 or 64 bit (twice that for complex), it cannot +/// be passed by value. +/// * If T is a type alias, a templated alias or an indirection to another type, +/// it matches if the type T refers to does +/// * If T is a compound type, it matches if any of base memter or field does. +/// * If T is an instantiation of an abstract template definition, T matches if any of the template +/// arguments or template definition do. +/// +/// A more advanced implementation might be aware of which registers arguments will actually end up +/// in and permit some signatures this rejects on a platform-specific basis. +#[derive(Debug, Clone)] +pub struct NeverByValue<'ctx> { + ctx: &'ctx BindgenContext, + + // The incremental result of this analysis's computation. Everything in this + // set has float. + never_by_value: HashSet, + + // Dependencies saying that if a key ItemId has been inserted into the + // `never_by_value` set, then each of the ids in Vec need to be + // considered again. + // + // This is a subset of the natural IR graph with reversed edges, where we + // only include the edges from the IR graph that can affect whether a type + // has float or not. + dependencies: HashMap>, +} + +impl<'ctx> NeverByValue<'ctx> { + fn consider_edge(kind: EdgeKind) -> bool { + match kind { + EdgeKind::BaseMember | + EdgeKind::Field | + EdgeKind::TypeReference | + EdgeKind::VarType | + EdgeKind::TemplateArgument | + EdgeKind::TemplateDeclaration | + EdgeKind::TemplateParameterDefinition => true, + + EdgeKind::Constructor | + EdgeKind::Destructor | + EdgeKind::FunctionReturn | + EdgeKind::FunctionParameter | + EdgeKind::InnerType | + EdgeKind::InnerVar | + EdgeKind::Method => false, + EdgeKind::Generic => false, + } + } + + fn insert>(&mut self, id: Id) -> ConstrainResult { + let id = id.into(); + info!("inserting {id:?} into the never_by_value set"); + + let was_not_already_in_set = self.never_by_value.insert(id); + assert!( + was_not_already_in_set, + "We shouldn't try and insert {:?} twice because if it was \ + already in the set, `constrain` should have exited early.", + id + ); + + ConstrainResult::Changed + } +} + +impl<'ctx> MonotoneFramework for NeverByValue<'ctx> { + type Node = ItemId; + type Extra = &'ctx BindgenContext; + type Output = HashSet; + + fn new(ctx: &'ctx BindgenContext) -> Self { + let never_by_value = HashSet::default(); + let dependencies = generate_dependencies(ctx, Self::consider_edge); + + NeverByValue { + ctx, + never_by_value, + dependencies, + } + } + + fn initial_worklist(&self) -> Vec { + self.ctx.allowlisted_items().iter().cloned().collect() + } + + fn constrain(&mut self, id: ItemId) -> ConstrainResult { + info!("constrain: {id:?}"); + + if self.never_by_value.contains(&id) { + info!(" already in set"); + return ConstrainResult::Same; + } + + let item = self.ctx.resolve_item(id); + if let Some(ty) = item.kind().as_type() { + match *ty.kind() { + TypeKind::Void | + TypeKind::NullPtr | + TypeKind::Int(..) | + TypeKind::Function(..) | + TypeKind::Enum(..) | + TypeKind::Reference(..) | + TypeKind::TypeParam | + TypeKind::Opaque | + TypeKind::Pointer(..) | + TypeKind::UnresolvedTypeRef(..) | + TypeKind::ObjCInterface(..) | + TypeKind::ObjCId | + TypeKind::ObjCSel | + TypeKind::BlockPointer(_) => { + info!(" simple type that is not float"); + ConstrainResult::Same + } + + TypeKind::Float(..) | TypeKind::Complex(..) => { + let size = ty.layout(self.ctx).expect("float with unknown layout").size; + match (ty.kind(), size) { + (TypeKind::Float(..), 4 | 8) | (TypeKind::Complex(..), 8 | 16) => { + info!(" skipped f32 or f64"); + ConstrainResult::Same + } + _ => { + info!(" extended float size {size}"); + self.insert(id) + } + } + } + + TypeKind::Alias(t) | + TypeKind::Array(t, _) | + TypeKind::ResolvedTypeRef(t) | + TypeKind::TemplateAlias(t, _) | + TypeKind::Vector(t, _) => { + if self.never_by_value.contains(&t.into()) { + info!(" contains/aliases matching type, so matches"); + self.insert(id) + } else { + info!(" does not contain/alias matching type"); + ConstrainResult::Same + } + } + + TypeKind::Comp(ref info) => { + let bases_have = info + .base_members() + .iter() + .any(|base| self.never_by_value.contains(&base.ty.into())); + if bases_have { + info!(" bases have float, so we also have"); + return self.insert(id); + } + let fields_have = info.fields().iter().any(|f| match *f { + Field::DataMember(ref data) => { + self.never_by_value.contains(&data.ty().into()) + } + Field::Bitfields(ref bfu) => bfu + .bitfields() + .iter() + .any(|b| self.never_by_value.contains(&b.ty().into())), + }); + if fields_have { + info!(" fields have float, so we also have"); + return self.insert(id); + } + + info!(" comp doesn't have float"); + ConstrainResult::Same + } + + TypeKind::TemplateInstantiation(ref template) => { + let args_have = template + .template_arguments() + .iter() + .any(|arg| self.never_by_value.contains(&arg.into())); + if args_have { + info!(" template args match, so instantiation also matches"); + return self.insert(id); + } + + let def_has = self + .never_by_value + .contains(&template.template_definition().into()); + if def_has { + info!(" template definition has float, so instantiation also has"); + return self.insert(id); + } + + info!(" template instantiation does not match"); + ConstrainResult::Same + } + } + } else { + info!(" not a type; skipped"); + ConstrainResult::Same + } + } + + fn each_depending_on(&self, id: ItemId, mut f: F) + where + F: FnMut(ItemId), + { + if let Some(edges) = self.dependencies.get(&id) { + for item in edges { + info!("enqueue {item:?} into worklist"); + f(*item); + } + } + } +} + +impl<'ctx> From> for HashSet { + fn from(analysis: NeverByValue<'ctx>) -> Self { + analysis.never_by_value + } +} diff --git a/bindgen/ir/context.rs b/bindgen/ir/context.rs index c5df37d7e9..68864a37d5 100644 --- a/bindgen/ir/context.rs +++ b/bindgen/ir/context.rs @@ -4,8 +4,8 @@ use super::super::time::Timer; use super::analysis::{ analyze, as_cannot_derive_set, CannotDerive, DeriveTrait, HasDestructorAnalysis, HasFloat, HasTypeParameterInArray, - HasVtableAnalysis, HasVtableResult, SizednessAnalysis, SizednessResult, - UsedTemplateParameters, + HasVtableAnalysis, HasVtableResult, NeverByValue, + SizednessAnalysis, SizednessResult, UsedTemplateParameters, }; use super::derive::{ CanDerive, CanDeriveCopy, CanDeriveDebug, CanDeriveDefault, CanDeriveEq, @@ -438,7 +438,7 @@ pub struct BindgenContext { /// and is always `None` before that and `Some` after. cannot_derive_hash: Option>, - /// The map why specified `ItemId`s of) types that can't derive hash. + /// The map why specified `ItemId`s of types that can't derive hash. /// /// This is populated when we enter codegen by /// `compute_cannot_derive_partialord_partialeq_or_eq` and is always `None` @@ -451,30 +451,38 @@ pub struct BindgenContext { /// that function is invoked and `Some` afterwards. sizedness: Option>, - /// The set of (`ItemId's of`) types that has vtable. + /// The set of (`ItemId`s of) types that have vtable. /// /// Populated when we enter codegen by `compute_has_vtable`; always `None` /// before that and `Some` after. have_vtable: Option>, - /// The set of (`ItemId's of`) types that has destructor. + /// The set of (`ItemId`s of) types that have destructor. /// /// Populated when we enter codegen by `compute_has_destructor`; always `None` /// before that and `Some` after. have_destructor: Option>, - /// The set of (`ItemId's of`) types that has array. + /// The set of (`ItemId`s of) types that have array. /// /// Populated when we enter codegen by `compute_has_type_param_in_array`; always `None` /// before that and `Some` after. has_type_param_in_array: Option>, - /// The set of (`ItemId's of`) types that has float. + /// The set of (`ItemId`s of) types that have float. /// /// Populated when we enter codegen by `compute_has_float`; always `None` /// before that and `Some` after. has_float: Option>, + /// The set of (`ItemId` of) types that cannot be reliably passed by value. + /// + /// These are or contain an extended float, which cannot be represented correctly for passing + /// by value in Rust. (In many contexts, anything of the right size and alignment will do, but + /// calling conventions depend on specifying the actual type.) The definition of extended float + /// used is simply not being 32 or 64 bits wide. + never_by_value: Option>, + /// The set of warnings raised during binding generation. warnings: Vec, } @@ -592,6 +600,7 @@ If you encounter an error missing from this list, please file an issue or a PR!" have_destructor: None, has_type_param_in_array: None, has_float: None, + never_by_value: None, warnings: Vec::new(), } } @@ -1180,6 +1189,7 @@ If you encounter an error missing from this list, please file an issue or a PR!" self.compute_cannot_derive_copy(); self.compute_has_type_param_in_array(); self.compute_has_float(); + self.compute_never_by_value(); self.compute_cannot_derive_hash(); self.compute_cannot_derive_partialord_partialeq_or_eq(); @@ -2749,6 +2759,18 @@ If you encounter an error missing from this list, please file an issue or a PR!" self.has_float.as_ref().unwrap().contains(&id.into()) } + /// Compute whether the type has float. + fn compute_never_by_value(&mut self) { + let _t = self.timer("compute_never_by_value"); + assert!(self.never_by_value.is_none()); + self.never_by_value = Some(analyze::(self)); + } + + /// Look up whether the item with `id` may never be passed by value. + pub fn lookup_never_by_value>(&self, id: Id) -> bool { + self.never_by_value.as_ref().unwrap().contains(&id.into()) + } + /// Check if `--no-partialeq` flag is enabled for this item. pub fn no_partialeq_by_name(&self, item: &Item) -> bool { let name = item.path_for_allowlisting(self)[1..].join("::");