Skip to content

Commit 5e6e31a

Browse files
RalfJungRalfJung
committed
move ABI sanity check from LLVM codegen backend to ABI computation logic
1 parent 86e717d commit 5e6e31a

File tree

3 files changed

+77
-43
lines changed

3 files changed

+77
-43
lines changed

‎compiler/rustc_codegen_llvm/src/abi.rs‎

Lines changed: 3 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -348,50 +348,18 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
348348
PassMode::Direct(_) => {
349349
// ABI-compatible Rust types have the same `layout.abi` (up to validity ranges),
350350
// and for Scalar ABIs the LLVM type is fully determined by `layout.abi`,
351-
// guarnateeing that we generate ABI-compatible LLVM IR. Things get tricky for
352-
// aggregates...
353-
if matches!(arg.layout.abi, abi::Abi::Aggregate { .. }) {
354-
assert!(
355-
arg.layout.is_sized(),
356-
"`PassMode::Direct` for unsized type: {}",
357-
arg.layout.ty
358-
);
359-
// This really shouldn't happen, since `immediate_llvm_type` will use
360-
// `layout.fields` to turn this Rust type into an LLVM type. This means all
361-
// sorts of Rust type details leak into the ABI. However wasm sadly *does*
362-
// currently use this mode so we have to allow it -- but we absolutely
363-
// shouldn't let any more targets do that.
364-
// (Also see <https://github.com/rust-lang/rust/issues/115666>.)
365-
//
366-
// The unstable abi `PtxKernel` also uses Direct for now.
367-
// It needs to switch to something else before stabilization can happen.
368-
// (See issue: https://github.com/rust-lang/rust/issues/117271)
369-
assert!(
370-
matches!(&*cx.tcx.sess.target.arch, "wasm32" | "wasm64")
371-
|| self.conv == Conv::PtxKernel,
372-
"`PassMode::Direct` for aggregates only allowed on wasm and `extern \"ptx-kernel\"` fns\nProblematic type: {:#?}",
373-
arg.layout,
374-
);
375-
}
351+
// guarnateeing that we generate ABI-compatible LLVM IR.
376352
arg.layout.immediate_llvm_type(cx)
377353
}
378354
PassMode::Pair(..) => {
379355
// ABI-compatible Rust types have the same `layout.abi` (up to validity ranges),
380356
// so for ScalarPair we can easily be sure that we are generating ABI-compatible
381357
// LLVM IR.
382-
assert!(
383-
matches!(arg.layout.abi, abi::Abi::ScalarPair(..)),
384-
"PassMode::Pair for type {}",
385-
arg.layout.ty
386-
);
387358
llargument_tys.push(arg.layout.scalar_pair_element_llvm_type(cx, 0, true));
388359
llargument_tys.push(arg.layout.scalar_pair_element_llvm_type(cx, 1, true));
389360
continue;
390361
}
391-
PassMode::Indirect { attrs: _, meta_attrs: Some(_), on_stack } => {
392-
// `Indirect` with metadata is only for unsized types, and doesn't work with
393-
// on-stack passing.
394-
assert!(arg.layout.is_unsized() && !on_stack);
362+
PassMode::Indirect { attrs: _, meta_attrs: Some(_), on_stack: _ } => {
395363
// Construct the type of a (wide) pointer to `ty`, and pass its two fields.
396364
// Any two ABI-compatible unsized types have the same metadata type and
397365
// moreover the same metadata value leads to the same dynamic size and
@@ -402,13 +370,8 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
402370
llargument_tys.push(ptr_layout.scalar_pair_element_llvm_type(cx, 1, true));
403371
continue;
404372
}
405-
PassMode::Indirect { attrs: _, meta_attrs: None, on_stack: _ } => {
406-
assert!(arg.layout.is_sized());
407-
cx.type_ptr()
408-
}
373+
PassMode::Indirect { attrs: _, meta_attrs: None, on_stack: _ } => cx.type_ptr(),
409374
PassMode::Cast { cast, pad_i32 } => {
410-
// `Cast` means "transmute to `CastType`"; that only makes sense for sized types.
411-
assert!(arg.layout.is_sized());
412375
// add padding
413376
if *pad_i32 {
414377
llargument_tys.push(Reg::i32().llvm_type(cx));

‎compiler/rustc_ty_utils/src/abi.rs‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,74 @@ fn adjust_for_rust_scalar<'tcx>(
327327
}
328328
}
329329

330+
/// Ensure that the ABI makes basic sense.
331+
fn fn_abi_sanity_check<'tcx>(cx: &LayoutCx<'tcx, TyCtxt<'tcx>>, fn_abi: &FnAbi<'tcx, Ty<'tcx>>) {
332+
fn fn_arg_sanity_check<'tcx>(
333+
cx: &LayoutCx<'tcx, TyCtxt<'tcx>>,
334+
fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
335+
arg: &ArgAbi<'tcx, Ty<'tcx>>,
336+
) {
337+
match &arg.mode {
338+
PassMode::Ignore => {}
339+
PassMode::Direct(_) => {
340+
// Here the Rust type is used to determine the actual ABI, so we have to be very
341+
// careful. Scalar/ScalarPair is fine, since backends will generally use
342+
// `layout.abi` and ignore everything else. We should just reject `Aggregate`
343+
// entirely here, but some targets need to be fixed first.
344+
if matches!(arg.layout.abi, Abi::Aggregate { .. }) {
345+
assert!(
346+
arg.layout.is_sized(),
347+
"`PassMode::Direct` for unsized type in ABI: {:#?}",
348+
fn_abi
349+
);
350+
// This really shouldn't happen, since `immediate_llvm_type` will use
351+
// `layout.fields` to turn this Rust type into an LLVM type. This means all
352+
// sorts of Rust type details leak into the ABI. However wasm sadly *does*
353+
// currently use this mode so we have to allow it -- but we absolutely
354+
// shouldn't let any more targets do that.
355+
// (Also see <https://github.com/rust-lang/rust/issues/115666>.)
356+
//
357+
// The unstable abi `PtxKernel` also uses Direct for now.
358+
// It needs to switch to something else before stabilization can happen.
359+
// (See issue: https://github.com/rust-lang/rust/issues/117271)
360+
assert!(
361+
matches!(&*cx.tcx.sess.target.arch, "wasm32" | "wasm64")
362+
|| fn_abi.conv == Conv::PtxKernel,
363+
"`PassMode::Direct` for aggregates only allowed on wasm and `extern \"ptx-kernel\"` fns\nProblematic type: {:#?}",
364+
arg.layout,
365+
);
366+
}
367+
}
368+
PassMode::Pair(_, _) => {
369+
// Similar to `Direct`, we need to make sure that backends use `layout.abi` and
370+
// ignore the rest of the layout.
371+
assert!(
372+
matches!(arg.layout.abi, Abi::ScalarPair(..)),
373+
"PassMode::Pair for type {}",
374+
arg.layout.ty
375+
);
376+
}
377+
PassMode::Cast { .. } => {
378+
// `Cast` means "transmute to `CastType`"; that only makes sense for sized types.
379+
assert!(arg.layout.is_sized());
380+
}
381+
PassMode::Indirect { meta_attrs: None, .. } => {
382+
// No metadata, must be sized.
383+
assert!(arg.layout.is_sized());
384+
}
385+
PassMode::Indirect { meta_attrs: Some(_), on_stack, .. } => {
386+
// With metadata. Must be unsized and not on the stack.
387+
assert!(arg.layout.is_unsized() && !on_stack);
388+
}
389+
}
390+
}
391+
392+
for arg in fn_abi.args.iter() {
393+
fn_arg_sanity_check(cx, fn_abi, arg);
394+
}
395+
fn_arg_sanity_check(cx, fn_abi, &fn_abi.ret);
396+
}
397+
330398
// FIXME(eddyb) perhaps group the signature/type-containing (or all of them?)
331399
// arguments of this method, into a separate `struct`.
332400
#[tracing::instrument(level = "debug", skip(cx, caller_location, fn_def_id, force_thin_self_ptr))]
@@ -453,6 +521,7 @@ fn fn_abi_new_uncached<'tcx>(
453521
};
454522
fn_abi_adjust_for_abi(cx, &mut fn_abi, sig.abi, fn_def_id)?;
455523
debug!("fn_abi_new_uncached = {:?}", fn_abi);
524+
fn_abi_sanity_check(cx, &fn_abi);
456525
Ok(cx.tcx.arena.alloc(fn_abi))
457526
}
458527

‎tests/ui/abi/compatibility.rs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,11 @@
3737
// revisions: wasi
3838
//[wasi] compile-flags: --target wasm32-wasi
3939
//[wasi] needs-llvm-components: webassembly
40-
// revisions: nvptx64
41-
//[nvptx64] compile-flags: --target nvptx64-nvidia-cuda
42-
//[nvptx64] needs-llvm-components: nvptx
40+
// FIXME: disabled on nvptx64 since the target ABI fails the sanity check
41+
/* revisions: nvptx64
42+
[nvptx64] compile-flags: --target nvptx64-nvidia-cuda
43+
[nvptx64] needs-llvm-components: nvptx
44+
*/
4345
#![feature(rustc_attrs, unsized_fn_params, transparent_unions)]
4446
#![cfg_attr(not(host), feature(no_core, lang_items), no_std, no_core)]
4547
#![allow(unused, improper_ctypes_definitions, internal_features)]

0 commit comments

Comments
 (0)