Skip to content

Commit a3c1656

Browse files
thunderseethecopybara-github
authored andcommitted
Refactor ffi_11 crate to use newtypes.
Previously when we could be confident a C++ and Rust type matched in bit width, it would be a type alias in ffi_11. Relying on aliases is problematic for the rmeta interface (aliases don't exist at the MIR level). Instead, use wrapper structs so our ffi types are considered distinct at the MIR level. PiperOrigin-RevId: 830498336
1 parent d4b521b commit a3c1656

File tree

7 files changed

+288
-129
lines changed

7 files changed

+288
-129
lines changed

cc_bindings_from_rs/generate_bindings/format_type.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -503,7 +503,7 @@ pub fn format_ty_for_cc<'tcx>(
503503
};
504504

505505
check_fn_sig(&sig)?;
506-
is_thunk_required(&sig).context("Function pointers can't have a thunk")?;
506+
is_thunk_required(tcx, &sig).context("Function pointers can't have a thunk")?;
507507

508508
// `is_thunk_required` check above implies `extern "C"` (or `"C-unwind"`).
509509
// This assertion reinforces that the generated C++ code doesn't need

cc_bindings_from_rs/generate_bindings/generate_function.rs

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,7 @@ fn cc_param_to_c_abi<'tcx>(
121121
includes: &mut BTreeSet<CcInclude>,
122122
statements: &mut TokenStream,
123123
) -> Result<TokenStream> {
124+
let tcx = db.tcx();
124125
Ok(if let Some(bridged_type) = is_bridged_type(db, ty.mid())? {
125126
match bridged_type {
126127
BridgedType::Legacy { cpp_type, .. } => {
@@ -146,7 +147,7 @@ fn cc_param_to_c_abi<'tcx>(
146147
quote! { #buffer_name }
147148
}
148149
}
149-
} else if is_c_abi_compatible_by_value(ty.mid()) {
150+
} else if is_c_abi_compatible_by_value(tcx, ty.mid()) {
150151
quote! { #cc_ident }
151152
} else if let Some(tuple_tys) = ty.as_tuple(db) {
152153
let n = tuple_tys.len();
@@ -243,7 +244,11 @@ fn cc_return_value_from_c_abi<'tcx>(
243244
storage_statements: &mut TokenStream,
244245
recursive: bool,
245246
) -> Result<ReturnConversion> {
247+
let tcx = db.tcx();
246248
let storage_name = &expect_format_cc_ident(&format!("__{ident}_storage"));
249+
// TODO: b/459482188 - The order of this check must align with the order in `generate_thunk_decl`.
250+
// We should centralize this logic so that the order exists in a singular location used by both
251+
// places.
247252
if let Some(bridged_type) = is_bridged_type(db, ty.mid())? {
248253
match bridged_type {
249254
BridgedType::Legacy { cpp_type, .. } => {
@@ -286,7 +291,7 @@ fn cc_return_value_from_c_abi<'tcx>(
286291
})
287292
}
288293
}
289-
} else if is_c_abi_compatible_by_value(ty.mid()) {
294+
} else if is_c_abi_compatible_by_value(tcx, ty.mid()) {
290295
let cc_type = &format_ty_for_cc_amending_prereqs(db, ty, prereqs)?;
291296
let local_name = &expect_format_cc_ident(&format!("__{ident}_ret_val_holder"));
292297
storage_statements.extend(quote! {
@@ -493,6 +498,7 @@ pub(crate) fn generate_thunk_call<'tcx>(
493498
has_self_param: bool,
494499
params: &[Param<'tcx>],
495500
) -> Result<CcSnippet> {
501+
let tcx = db.tcx();
496502
let mut prereqs = CcPrerequisites::default();
497503
let mut tokens = TokenStream::new();
498504

@@ -514,7 +520,6 @@ pub(crate) fn generate_thunk_call<'tcx>(
514520
tokens.extend(quote! { auto&& #cc_name = *this; });
515521
}
516522
}
517-
let tcx = db.tcx();
518523
cc_param_to_c_abi(
519524
db,
520525
cc_name.clone(),
@@ -552,7 +557,7 @@ pub(crate) fn generate_thunk_call<'tcx>(
552557
}
553558

554559
let return_body = if is_bridged_type(db, rs_return_type.mid())?.is_none()
555-
&& is_c_abi_compatible_by_value(rs_return_type.mid())
560+
&& is_c_abi_compatible_by_value(tcx, rs_return_type.mid())
556561
{
557562
// C++ compilers can emit diagnostics if a function marked [[noreturn]] looks like it
558563
// might return. In this scenario, we just call the (also [[noreturn]]) thunk.
@@ -601,7 +606,8 @@ pub fn generate_function(db: &dyn BindingsGenerator<'_>, def_id: DefId) -> Resul
601606
// TODO(b/262904507): Don't require thunks for mangled extern "C" functions.
602607
let (export_name, has_no_mangle) = export_name_and_no_mangle_attrs_of(tcx, def_id);
603608
let has_export_name = export_name.is_some();
604-
let needs_thunk = is_thunk_required(&sig_mid).is_err() || (!has_no_mangle && !has_export_name);
609+
let needs_thunk =
610+
is_thunk_required(tcx, &sig_mid).is_err() || (!has_no_mangle && !has_export_name);
605611
let thunk_name = thunk_name(db, def_id, export_name, needs_thunk);
606612

607613
let unqualified_fn_name = db

cc_bindings_from_rs/generate_bindings/generate_function_thunk.rs

Lines changed: 34 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ pub fn generate_thunk_decl<'tcx>(
8686
thunk_name: &Ident,
8787
has_self_param: bool,
8888
) -> Result<CcSnippet> {
89+
let tcx = db.tcx();
8990
let mut prereqs = CcPrerequisites::default();
9091
let main_api_ret_type = format_ret_ty_for_cc(db, sig_mid, sig_hir)?.into_tokens(&mut prereqs);
9192

@@ -110,7 +111,7 @@ pub fn generate_thunk_decl<'tcx>(
110111
}
111112
BridgedType::Composable(_) => Ok(quote! { unsigned char* }),
112113
}
113-
} else if is_c_abi_compatible_by_value(ty) {
114+
} else if is_c_abi_compatible_by_value(tcx, ty) {
114115
Ok(quote! { #cpp_type })
115116
} else if let Some(tuple_abi) = tuple_c_abi_c_type(ty) {
116117
Ok(tuple_abi)
@@ -130,22 +131,32 @@ pub fn generate_thunk_decl<'tcx>(
130131
};
131132

132133
// Types which are not C-ABI compatible by-value are returned via out-pointer parameters.
133-
let thunk_ret_type: TokenStream;
134-
if is_c_abi_compatible_by_value(sig_mid.output()) {
135-
thunk_ret_type = main_api_ret_type;
134+
// TODO: b/ 459482188 - The order of this check must align with the order in `cc_return_value_from_c_abi`.
135+
// We should centralize this logic so that the order exists in a singular location used by both
136+
// places.
137+
let thunk_ret_type = if let Some(briging) = is_bridged_type(db, sig_mid.output())? {
138+
match briging {
139+
BridgedType::Legacy { .. } => {
140+
thunk_params.push(quote! { #main_api_ret_type* __ret_ptr });
141+
quote! { void }
142+
}
143+
BridgedType::Composable(_) => {
144+
thunk_params.push(quote! { unsigned char * __ret_ptr });
145+
quote! { void }
146+
}
147+
}
148+
} else if is_c_abi_compatible_by_value(tcx, sig_mid.output()) {
149+
main_api_ret_type
136150
} else if let Some(tuple_abi) = tuple_c_abi_c_type(sig_mid.output()) {
137-
thunk_ret_type = quote! { void };
138151
thunk_params.push(quote! { #tuple_abi __ret_ptr });
152+
quote! { void }
139153
} else if let ty::TyKind::Array(inner_ty, _) = sig_mid.output().kind() {
140154
let c_type = array_c_abi_c_type(db.tcx(), *inner_ty)?;
141-
thunk_ret_type = quote! { void };
142155
thunk_params.push(quote! { #c_type __ret_ptr });
143-
} else if let Some(BridgedType::Composable(_)) = is_bridged_type(db, sig_mid.output())? {
144-
thunk_ret_type = quote! { void };
145-
thunk_params.push(quote! { unsigned char * __ret_ptr });
156+
quote! { void }
146157
} else {
147-
thunk_ret_type = quote! { void };
148158
thunk_params.push(quote! { #main_api_ret_type* __ret_ptr });
159+
quote! { void }
149160
};
150161

151162
let mut attributes = vec![];
@@ -266,7 +277,8 @@ fn convert_value_from_c_abi_to_rust<'tcx>(
266277
extern_c_decls,
267278
);
268279
}
269-
if is_c_abi_compatible_by_value(ty) {
280+
let tcx = db.tcx();
281+
if is_c_abi_compatible_by_value(tcx, ty) {
270282
return Ok(quote! {});
271283
}
272284
if let ty::TyKind::Tuple(tuple_tys) = ty.kind() {
@@ -281,12 +293,13 @@ fn c_abi_for_param_type<'tcx>(
281293
db: &dyn BindingsGenerator<'tcx>,
282294
ty: ty::Ty<'tcx>,
283295
) -> Result<TokenStream> {
296+
let tcx = db.tcx();
284297
if let Some(bridged) = is_bridged_type(db, ty)? {
285298
match bridged {
286299
BridgedType::Legacy { .. } => Ok(quote! { *const core::ffi::c_void }),
287300
BridgedType::Composable(_) => Ok(quote! { *const core::ffi::c_uchar }),
288301
}
289-
} else if is_c_abi_compatible_by_value(ty) {
302+
} else if is_c_abi_compatible_by_value(tcx, ty) {
290303
let rs_type = db.format_ty_for_rs(ty)?;
291304
Ok(quote! { #rs_type })
292305
} else if let Some(tuple_abi) = tuple_c_abi_rs_type(ty) {
@@ -375,6 +388,7 @@ fn write_rs_value_to_c_abi_ptr<'tcx>(
375388
let rs_type_tokens = db.format_ty_for_rs(rs_type)?;
376389
Ok(quote! { (#c_ptr as *mut #rs_type_tokens).write(#rs_value); })
377390
};
391+
let tcx = db.tcx();
378392
Ok(if let Some(bridged_type) = is_bridged_type(db, rs_type)? {
379393
match bridged_type {
380394
BridgedType::Legacy { conversion_info, .. } => match conversion_info {
@@ -412,7 +426,7 @@ fn write_rs_value_to_c_abi_ptr<'tcx>(
412426
}
413427
}
414428
}
415-
} else if is_c_abi_compatible_by_value(rs_type) {
429+
} else if is_c_abi_compatible_by_value(tcx, rs_type) {
416430
write_directly()?
417431
} else if let ty::TyKind::Tuple(tuple_tys) = rs_type.kind() {
418432
let num_elements = tuple_tys.len();
@@ -525,7 +539,7 @@ pub fn generate_thunk_impl<'tcx>(
525539

526540
let thunk_return_type;
527541
let thunk_return_expression;
528-
if output_is_bridged.is_none() && is_c_abi_compatible_by_value(sig.output()) {
542+
if output_is_bridged.is_none() && is_c_abi_compatible_by_value(tcx, sig.output()) {
529543
// The output is not bridged and is C ABI compatible by-value, so we can just return
530544
// the result directly, and no out-param is needed.
531545
thunk_return_type = db.format_ty_for_rs(sig.output())?;
@@ -576,7 +590,7 @@ pub fn generate_thunk_impl<'tcx>(
576590

577591
/// Returns `Ok(())` if no thunk is required.
578592
/// Otherwise returns an error the describes why the thunk is needed.
579-
pub fn is_thunk_required(sig: &ty::FnSig) -> Result<()> {
593+
pub fn is_thunk_required(tcx: TyCtxt<'_>, sig: &ty::FnSig) -> Result<()> {
580594
match sig.abi {
581595
// "C" ABI is okay: since https://rust-lang.github.io/rfcs/2945-c-unwind-abi.html has been
582596
// accepted, a Rust panic that "escapes" a "C" ABI function is a defined crash. See
@@ -594,9 +608,12 @@ pub fn is_thunk_required(sig: &ty::FnSig) -> Result<()> {
594608
_ => bail!("Any calling convention other than `extern \"C\"` requires a thunk"),
595609
};
596610

597-
ensure!(is_c_abi_compatible_by_value(sig.output()), "Return type requires a thunk");
611+
ensure!(is_c_abi_compatible_by_value(tcx, sig.output()), "Return type requires a thunk");
598612
for (i, param_ty) in sig.inputs().iter().enumerate() {
599-
ensure!(is_c_abi_compatible_by_value(*param_ty), "Type of parameter #{i} requires a thunk");
613+
ensure!(
614+
is_c_abi_compatible_by_value(tcx, *param_ty),
615+
"Type of parameter #{i} requires a thunk"
616+
);
600617
}
601618

602619
Ok(())

cc_bindings_from_rs/generate_bindings/query_compiler.rs

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -32,17 +32,17 @@ use std::rc::Rc;
3232

3333
/// Whether functions using `extern "C"` ABI can safely handle values of type
3434
/// `ty` (e.g. when passing by value arguments or return values of such type).
35-
pub fn is_c_abi_compatible_by_value(ty: Ty) -> bool {
35+
pub fn is_c_abi_compatible_by_value(tcx: TyCtxt<'_>, ty: Ty) -> bool {
3636
match ty.kind() {
3737
// `improper_ctypes_definitions` warning doesn't complain about the following types:
38-
ty::TyKind::Bool |
39-
ty::TyKind::Float{..} |
40-
ty::TyKind::Int{..} |
41-
ty::TyKind::Uint{..} |
42-
ty::TyKind::Never |
43-
ty::TyKind::RawPtr{..} |
44-
ty::TyKind::Ref{..} |
45-
ty::TyKind::FnPtr{..} => true,
38+
ty::TyKind::Bool
39+
| ty::TyKind::Float { .. }
40+
| ty::TyKind::Int { .. }
41+
| ty::TyKind::Uint { .. }
42+
| ty::TyKind::Never
43+
| ty::TyKind::RawPtr { .. }
44+
| ty::TyKind::Ref { .. }
45+
| ty::TyKind::FnPtr { .. } => true,
4646
ty::TyKind::Tuple(types) if types.is_empty() => true,
4747

4848
// Crubit assumes that `char` is compatible with a certain `extern "C"` ABI.
@@ -55,7 +55,7 @@ pub fn is_c_abi_compatible_by_value(ty: Ty) -> bool {
5555
// - In general `TyKind::Ref` should have the same ABI as `TyKind::RawPtr`
5656
// - References to slices (`&[T]`) or strings (`&str`) rely on assumptions
5757
// spelled out in `rust_builtin_type_abi_assumptions.md`.
58-
ty::TyKind::Slice{..} => false,
58+
ty::TyKind::Slice { .. } => false,
5959

6060
// Crubit's C++ bindings for tuples, structs, and other ADTs may not preserve
6161
// their ABI (even if they *do* preserve their memory layout). For example:
@@ -70,13 +70,23 @@ pub fn is_c_abi_compatible_by_value(ty: Ty) -> bool {
7070
// returning `true` in a few limited cases (this may require additional complexity to
7171
// ensure that `generate_adt` never injects explicit padding into such structs):
7272
// - `#[repr(C)]` structs and unions,
73-
// - `#[repr(transparent)]` struct that wraps an ABI-safe type,
7473
// - Discriminant-only enums (b/259984090).
75-
ty::TyKind::Tuple{..} | // An empty tuple (`()` - the unit type) is handled above.
76-
ty::TyKind::Adt{..} => false,
74+
ty::TyKind::Tuple { .. } => false, // An empty tuple (`()` - the unit type) is handled above.
75+
ty::TyKind::Adt(adt, _) => {
76+
if !adt.repr().transparent() {
77+
// If our adt is not transparent, it is not abi compatible by value.
78+
return false;
79+
}
80+
let Some(field) = adt.all_fields().next() else {
81+
// TODO: b/258259459 - Support zero sized types.
82+
return false;
83+
};
84+
is_c_abi_compatible_by_value(tcx, tcx.type_of(field.did).instantiate_identity())
85+
}
7786

7887
// Arrays are explicitly not ABI-compatible (though they are layout-compatible).
79-
ty::TyKind::Array{..} => false,
88+
ty::TyKind::Array { .. } => false,
89+
ty::TyKind::Alias { .. } => false,
8090

8191
// These kinds of reference-related types are not implemented yet - `is_c_abi_compatible_by_value`
8292
// should never need to handle them, because `format_ty_for_cc` fails for such types.
@@ -85,7 +95,7 @@ pub fn is_c_abi_compatible_by_value(ty: Ty) -> bool {
8595
// `format_ty_for_cc` is expected to fail for other kinds of types
8696
// and therefore `is_c_abi_compatible_by_value` should never be called for
8797
// these other types
88-
_ => unimplemented!(),
98+
ty => panic!("unsupported type kind: {ty:?}"),
8999
}
90100
}
91101

0 commit comments

Comments
 (0)