Cranelift: preserve_all: disallow return values. (#12447)

In #12399 a fuzzbug uncovered an issue whereby a function with
`preserve_all` and with many return values cannot be called, because
`emit_retval_loads` cannot codegen memory-to-memory moves (from on-stack
return value slots directly to spillslots) without a
temporary/clobberable register, and `preserve_all` implies no such
registers exist.

I thought about trying to support this by shuffling registers -- such a
case implies many return values, and at least some of them will be in
registers, so we might be able to temporarily spill one of those and use
it as a scratch to move other values memory-to-memory. But the
complexity doesn't seem worthwhile, and this path is not actually needed
at the moment.

Thinking more broadly, anyone using `preserve_all` for hooks meant to
minimally perturb register state will likely not want to use many return
values anyway (that defeats the point). We could allow *one* return
value, with the knowledge that this always fits in a register in our
existing ABIs, but that also feels somewhat arbitrary, and I could make
the argument that "preserve all" should really mean preserve *all*.
Someone wanting to return a value anyway from such a hook could use
indirect means (pass in a pointer to a stackslot, for example). I'm
happy to tweak this limit if others have more thoughts, however.

Fixes #12399.

Author: cfallin

cranelift/codegen/src/isa/call_conv.rs

@@ -53,7 +53,8 @@ pub enum CallConv {
     /// as little as possible.
     ///
     /// The ABI is based on the native register-argument ABI on each
-    /// respective platform. It does not support tail-calls.
+    /// respective platform. It does not support tail-calls. It also
+    /// does not support return values.
     PreserveAll,
 }
 

cranelift/codegen/src/machinst/abi.rs

@@ -2500,7 +2500,7 @@ impl<T> CallInfo<T> {
         // The temporary must be noted as clobbered unless there are
         // no returns (hence it isn't needed). The latter can only be
         // the case statically for an ABI when the ABI doesn't allow
-        // any returns at all (e.g., patchable-call ABI).
+        // any returns at all (e.g., preserve-all ABI).
         debug_assert!(
             self.defs.is_empty()
                 || M::get_regs_clobbered_by_call(self.callee_conv, self.try_call_info.is_some())

cranelift/codegen/src/verifier/mod.rs

@@ -67,7 +67,6 @@ use crate::dbg::DisplayList;
 use crate::dominator_tree::DominatorTree;
 use crate::entity::SparseSet;
 use crate::flowgraph::{BlockPredecessor, ControlFlowGraph};
-use crate::ir::ExceptionTableItem;
 use crate::ir::entities::AnyEntity;
 use crate::ir::instructions::{CallInfo, InstructionFormat, ResolvedConstraint};
 use crate::ir::{self, ArgumentExtension, BlockArg, ExceptionTable};
@@ -76,14 +75,16 @@ use crate::ir::{
     JumpTable, MemFlags, MemoryTypeData, Opcode, SigRef, StackSlot, Type, Value, ValueDef,
     ValueList, types,
 };
-use crate::isa::TargetIsa;
+use crate::ir::{ExceptionTableItem, Signature};
+use crate::isa::{CallConv, TargetIsa};
 use crate::print_errors::pretty_verifier_error;
 use crate::settings::FlagsOrIsa;
 use crate::timing;
 use alloc::collections::BTreeSet;
 use alloc::string::{String, ToString};
 use alloc::vec::Vec;
 use core::fmt::{self, Display, Formatter};
+use cranelift_entity::packed_option::ReservedValue;
 
 /// A verifier error.
 #[derive(Debug, PartialEq, Eq, Clone)]
@@ -2082,12 +2083,56 @@ impl<'a> Verifier<'a> {
         Ok(())
     }
 
+    fn verify_signature(
+        &self,
+        sig: &Signature,
+        entity: impl Into<AnyEntity>,
+        errors: &mut VerifierErrors,
+    ) -> VerifierStepResult {
+        match sig.call_conv {
+            CallConv::PreserveAll => {
+                if !sig.returns.is_empty() {
+                    errors.fatal((
+                        entity,
+                        "Signature with `preserve_all` ABI cannot have return values".to_string(),
+                    ))?;
+                }
+            }
+            _ => {}
+        }
+        Ok(())
+    }
+
+    fn verify_signatures(&self, errors: &mut VerifierErrors) -> VerifierStepResult {
+        // Verify this function's own signature.
+        self.verify_signature(&self.func.signature, AnyEntity::Function, errors)?;
+        // Verify signatures referenced by any extfunc, using that
+        // extfunc as the entity to which to attach the error.
+        for (func, funcdata) in &self.func.dfg.ext_funcs {
+            // Non-contiguous func entities result in placeholders
+            // with invalid signatures; skip them.
+            if !funcdata.signature.is_reserved_value() {
+                self.verify_signature(&self.func.dfg.signatures[funcdata.signature], func, errors)?;
+            }
+        }
+        // Verify all signatures, including those only used by
+        // e.g. indirect calls. Technically this re-verifies
+        // signatures verified above but we want the first pass to
+        // attach errors to funcrefs and we also need to verify all
+        // defined signatures.
+        for (sig, sigdata) in &self.func.dfg.signatures {
+            self.verify_signature(sigdata, sig, errors)?;
+        }
+        Ok(())
+    }
+
     pub fn run(&self, errors: &mut VerifierErrors) -> VerifierStepResult {
         self.verify_global_values(errors)?;
         self.verify_memory_types(errors)?;
         self.typecheck_entry_block_params(errors)?;
         self.check_entry_not_cold(errors)?;
         self.typecheck_function_signature(errors)?;
+        self.verify_signatures(errors)?;
 
         for block in self.func.layout.blocks() {
             if self.func.layout.first_inst(block).is_none() {

cranelift/filetests/filetests/verifier/preserve-all-abi.clif

@@ -0,0 +1,22 @@
+test verifier
+
+function %f0(i8) -> i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 preserve_all { ; error: Signature with `preserve_all` ABI cannot have return values
+block0(v0: i8):
+    return v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0, v0
+}
+
+function %f1(i64) {
+    sig0 = () -> i8, i8, i8, i8 preserve_all ; error: Signature with `preserve_all` ABI cannot have return values
+
+block0(v0: i64):
+    v1, v2, v3, v4 = call_indirect sig0, v0()
+    return
+}
+
+function %f2() {
+    fn0 = %g() -> i8, i8, i8, i8 preserve_all ; error: Signature with `preserve_all` ABI cannot have return values
+
+block0():
+    v1, v2, v3, v4 = call fn0()
+    return
+}