Misc cleanups (#5014)

* Replace resize+copy_from_slice with extend_from_slice

Vec::resize initializes the new space, which is wasted effort if we're
just going to call `copy_from_slice` on it immediately afterward. Using
`extend_from_slice` is simpler, and very slightly faster.

If the new size were bigger than the buffer we're copying from, then it
would make sense to initialize the excess. But it isn't: it's always
exactly the same size.

* Move helpers from Context to CompiledCode

These methods only use information from Context::compiled_code, so they
should live on CompiledCode instead.

* Remove an unnecessary #[cfg_attr]

There are other uses of `#[allow(clippy::too_many_arguments)]` in this
file, so apparently it doesn't need to be guarded by the "cargo-clippy"
feature.

* Fix a few comments

Two of these were wrong/misleading:

- `FunctionBuilder::new` does not clear the provided func_ctx. It does
  debug-assert that the context is already clear, but I don't think
  that's worth a comment.

- `switch_to_block` does not "create values for the arguments." That's
  done by the combination of `append_block_params_for_function_params`
  and `declare_wasm_parameters`.

* wasmtime-cranelift: Misc cleanups

The main change is to use the `CompiledCode` reference we already had
instead of getting it out of `Context` repeatedly. This removes a bunch
of `unwrap()` calls.

* wasmtime-cranelift: Factor out uncached compile

cranelift/codegen/src/context.rs

@@ -119,10 +119,7 @@ impl Context {
         mem: &mut Vec<u8>,
     ) -> CompileResult<&CompiledCode> {
         let compiled_code = self.compile(isa)?;
-        let code_info = compiled_code.code_info();
+        mem.extend_from_slice(compiled_code.code_buffer());
-        let old_len = mem.len();
-        mem.resize(old_len + code_info.total_size as usize, 0);
-        mem[old_len..].copy_from_slice(compiled_code.code_buffer());
         Ok(compiled_code)
     }
 
@@ -194,32 +191,21 @@ impl Context {
     /// If available, return information about the code layout in the
     /// final machine code: the offsets (in bytes) of each basic-block
     /// start, and all basic-block edges.
+    #[deprecated = "use CompiledCode::get_code_bb_layout"]
     pub fn get_code_bb_layout(&self) -> Option<(Vec<usize>, Vec<(usize, usize)>)> {
-        if let Some(result) = self.compiled_code.as_ref() {
+        self.compiled_code().map(CompiledCode::get_code_bb_layout)
-            Some((
-                result.bb_starts.iter().map(|&off| off as usize).collect(),
-                result
-                    .bb_edges
-                    .iter()
-                    .map(|&(from, to)| (from as usize, to as usize))
-                    .collect(),
-            ))
-        } else {
-            None
-        }
     }
 
     /// Creates unwind information for the function.
     ///
     /// Returns `None` if the function has no unwind information.
     #[cfg(feature = "unwind")]
+    #[deprecated = "use CompiledCode::create_unwind_info"]
     pub fn create_unwind_info(
         &self,
         isa: &dyn TargetIsa,
     ) -> CodegenResult<Option<crate::isa::unwind::UnwindInfo>> {
-        let unwind_info_kind = isa.unwind_info_kind();
+        self.compiled_code().unwrap().create_unwind_info(isa)
-        let result = self.compiled_code.as_ref().unwrap();
-        isa.emit_unwind_info(result, unwind_info_kind)
     }
 
     /// Run the verifier on the function.

cranelift/codegen/src/isa/aarch64/inst/unwind/systemv.rs

@@ -91,9 +91,9 @@ mod tests {
             Some(StackSlotData::new(StackSlotKind::ExplicitSlot, 64)),
         ));
 
-        context.compile(&*isa).expect("expected compilation");
+        let code = context.compile(&*isa).expect("expected compilation");
 
-        let fde = match context
+        let fde = match code
             .create_unwind_info(isa.as_ref())
             .expect("can create unwind info")
         {
@@ -130,9 +130,9 @@ mod tests {
 
         let mut context = Context::for_function(create_multi_return_function(CallConv::SystemV));
 
-        context.compile(&*isa).expect("expected compilation");
+        let code = context.compile(&*isa).expect("expected compilation");
 
-        let fde = match context
+        let fde = match code
             .create_unwind_info(isa.as_ref())
             .expect("can create unwind info")
         {

cranelift/codegen/src/isa/mod.rs

@@ -56,7 +56,7 @@ use crate::CodegenResult;
 use alloc::{boxed::Box, vec::Vec};
 use core::fmt;
 use core::fmt::{Debug, Formatter};
-use target_lexicon::{triple, Architecture, OperatingSystem, PointerWidth, Triple};
+use target_lexicon::{triple, Architecture, PointerWidth, Triple};
 
 // This module is made public here for benchmarking purposes. No guarantees are
 // made regarding API stability.
@@ -360,18 +360,6 @@ impl<'a> dyn TargetIsa + 'a {
             pointer_width: self.pointer_width(),
         }
     }
-
-    /// Returns the flavor of unwind information emitted for this target.
-    pub(crate) fn unwind_info_kind(&self) -> UnwindInfoKind {
-        match self.triple().operating_system {
-            #[cfg(feature = "unwind")]
-            OperatingSystem::Windows => UnwindInfoKind::Windows,
-            #[cfg(feature = "unwind")]
-            _ => UnwindInfoKind::SystemV,
-            #[cfg(not(feature = "unwind"))]
-            _ => UnwindInfoKind::None,
-        }
-    }
 }
 
 impl Debug for &dyn TargetIsa {

cranelift/codegen/src/isa/riscv64/inst/unwind/systemv.rs

@@ -89,9 +89,9 @@ mod tests {
             Some(StackSlotData::new(StackSlotKind::ExplicitSlot, 64)),
         ));
 
-        context.compile(&*isa).expect("expected compilation");
+        let code = context.compile(&*isa).expect("expected compilation");
 
-        let fde = match context
+        let fde = match code
             .create_unwind_info(isa.as_ref())
             .expect("can create unwind info")
         {
@@ -129,9 +129,9 @@ mod tests {
 
         let mut context = Context::for_function(create_multi_return_function(CallConv::SystemV));
 
-        context.compile(&*isa).expect("expected compilation");
+        let code = context.compile(&*isa).expect("expected compilation");
 
-        let fde = match context
+        let fde = match code
             .create_unwind_info(isa.as_ref())
             .expect("can create unwind info")
         {

cranelift/codegen/src/isa/s390x/inst/unwind/systemv.rs

@@ -122,9 +122,9 @@ mod tests {
             Some(StackSlotData::new(StackSlotKind::ExplicitSlot, 64)),
         ));
 
-        context.compile(&*isa).expect("expected compilation");
+        let code = context.compile(&*isa).expect("expected compilation");
 
-        let fde = match context
+        let fde = match code
             .create_unwind_info(isa.as_ref())
             .expect("can create unwind info")
         {
@@ -164,9 +164,9 @@ mod tests {
             Some(StackSlotData::new(StackSlotKind::ExplicitSlot, 64)),
         ));
 
-        context.compile(&*isa).expect("expected compilation");
+        let code = context.compile(&*isa).expect("expected compilation");
 
-        let fde = match context
+        let fde = match code
             .create_unwind_info(isa.as_ref())
             .expect("can create unwind info")
         {

cranelift/codegen/src/isa/x64/inst/unwind/systemv.rs

@@ -118,9 +118,9 @@ mod tests {
             Some(StackSlotData::new(StackSlotKind::ExplicitSlot, 64)),
         ));
 
-        context.compile(&*isa).expect("expected compilation");
+        let code = context.compile(&*isa).expect("expected compilation");
 
-        let fde = match context
+        let fde = match code
             .create_unwind_info(isa.as_ref())
             .expect("can create unwind info")
         {
@@ -157,9 +157,9 @@ mod tests {
 
         let mut context = Context::for_function(create_multi_return_function(CallConv::SystemV));
 
-        context.compile(&*isa).expect("expected compilation");
+        let code = context.compile(&*isa).expect("expected compilation");
 
-        let fde = match context
+        let fde = match code
             .create_unwind_info(isa.as_ref())
             .expect("can create unwind info")
         {

cranelift/codegen/src/machinst/mod.rs

@@ -358,6 +358,36 @@ pub type CompiledCodeStencil = CompiledCodeBase<Stencil>;
 /// consumption.
 pub type CompiledCode = CompiledCodeBase<Final>;
 
+impl CompiledCode {
+    /// If available, return information about the code layout in the
+    /// final machine code: the offsets (in bytes) of each basic-block
+    /// start, and all basic-block edges.
+    pub fn get_code_bb_layout(&self) -> (Vec<usize>, Vec<(usize, usize)>) {
+        (
+            self.bb_starts.iter().map(|&off| off as usize).collect(),
+            self.bb_edges
+                .iter()
+                .map(|&(from, to)| (from as usize, to as usize))
+                .collect(),
+        )
+    }
+
+    /// Creates unwind information for the function.
+    ///
+    /// Returns `None` if the function has no unwind information.
+    #[cfg(feature = "unwind")]
+    pub fn create_unwind_info(
+        &self,
+        isa: &dyn crate::isa::TargetIsa,
+    ) -> CodegenResult<Option<crate::isa::unwind::UnwindInfo>> {
+        let unwind_info_kind = match isa.triple().operating_system {
+            target_lexicon::OperatingSystem::Windows => UnwindInfoKind::Windows,
+            _ => UnwindInfoKind::SystemV,
+        };
+        isa.emit_unwind_info(self, unwind_info_kind)
+    }
+}
+
 /// An object that can be used to create the text section of an executable.
 ///
 /// This primarily handles resolving relative relocations at

cranelift/filetests/src/test_unwind.rs

@@ -39,10 +39,10 @@ impl SubTest for TestUnwind {
         let isa = context.isa.expect("unwind needs an ISA");
         let mut comp_ctx = cranelift_codegen::Context::for_function(func.into_owned());
 
-        comp_ctx.compile(isa).expect("failed to compile function");
+        let code = comp_ctx.compile(isa).expect("failed to compile function");
 
         let mut text = String::new();
-        match comp_ctx.create_unwind_info(isa).expect("unwind info") {
+        match code.create_unwind_info(isa).expect("unwind info") {
             Some(UnwindInfo::WindowsX64(info)) => {
                 let mut mem = vec![0; info.emit_size()];
                 info.emit(&mut mem);

cranelift/wasm/src/environ/spec.rs

@@ -165,7 +165,7 @@ pub trait FuncEnvironment: TargetEnvironment {
     /// The signature `sig_ref` was previously created by `make_indirect_sig()`.
     ///
     /// Return the call instruction whose results are the WebAssembly return values.
-    #[cfg_attr(feature = "cargo-clippy", allow(clippy::too_many_arguments))]
+    #[allow(clippy::too_many_arguments)]
     fn translate_call_indirect(
         &mut self,
         builder: &mut FunctionBuilder,

cranelift/wasm/src/func_translator.rs

@@ -94,12 +94,11 @@ impl FuncTranslator {
         debug_assert_eq!(func.dfg.num_blocks(), 0, "Function must be empty");
         debug_assert_eq!(func.dfg.num_insts(), 0, "Function must be empty");
 
-        // This clears the `FunctionBuilderContext`.
         let mut builder = FunctionBuilder::new(func, &mut self.func_ctx);
         builder.set_srcloc(cur_srcloc(&reader));
         let entry_block = builder.create_block();
         builder.append_block_params_for_function_params(entry_block);
-        builder.switch_to_block(entry_block); // This also creates values for the arguments.
+        builder.switch_to_block(entry_block);
         builder.seal_block(entry_block); // Declare all predecessors known.
 
         // Make sure the entry block is inserted in the layout before we make any callbacks to
@@ -183,7 +182,7 @@ fn parse_local_decls<FE: FuncEnvironment + ?Sized>(
 
 /// Declare `count` local variables of the same type, starting from `next_local`.
 ///
-/// Fail of too many locals are declared in the function, or if the type is not valid for a local.
+/// Fail if too many locals are declared in the function, or if the type is not valid for a local.
 fn declare_locals<FE: FuncEnvironment + ?Sized>(
     builder: &mut FunctionBuilder,
     count: u32,

crates/cranelift/src/compiler.rs

@@ -142,8 +142,7 @@ impl Compiler {
     }
 
     fn get_function_address_map(
-        &self,
+        compiled_code: &CompiledCode,
-        context: &Context,
         body: &FunctionBody<'_>,
         body_len: u32,
         tunables: &Tunables,
@@ -162,9 +161,7 @@ impl Compiler {
         let instructions = if tunables.generate_address_map {
             collect_address_maps(
                 body_len,
-                context
+                compiled_code
-                    .compiled_code()
-                    .unwrap()
                     .buffer
                     .get_srclocs_sorted()
                     .into_iter()
@@ -270,46 +267,46 @@ impl wasmtime_environ::Compiler for Compiler {
         context.func.stack_limit = Some(stack_limit);
         let FunctionBodyData { validator, body } = input;
         let mut validator = validator.into_validator(validator_allocations);
-        func_translator.translate_body(
+        func_translator.translate_body(&mut validator, body, &mut context.func, &mut func_env)?;
-            &mut validator,
-            body.clone(),
-            &mut context.func,
-            &mut func_env,
-        )?;
-
-        let (code, code_buf) = compile_maybe_cached(&mut context, isa, cache_ctx.as_mut())?;
-        let alignment = code.alignment;
 
+        let (_, code_buf) = compile_maybe_cached(&mut context, isa, cache_ctx.as_mut())?;
+        // compile_maybe_cached returns the compiled_code but that borrow has the same lifetime as
+        // the mutable borrow of `context`, so the borrow checker prohibits other borrows from
+        // `context` while it's alive. Borrow it again to make the borrow checker happy.
         let compiled_code = context.compiled_code().unwrap();
+        let alignment = compiled_code.alignment;
+
         let func_relocs = compiled_code
             .buffer
             .relocs()
             .into_iter()
             .map(|item| mach_reloc_to_reloc(&context.func, item))
-            .collect::<Vec<_>>();
+            .collect();
 
         let traps = compiled_code
             .buffer
             .traps()
             .into_iter()
             .map(mach_trap_to_trap)
-            .collect::<Vec<_>>();
+            .collect();
 
         let stack_maps = mach_stack_maps_to_stack_maps(compiled_code.buffer.stack_maps());
 
         let unwind_info = if isa.flags().unwind_info() {
-            context
+            compiled_code
                 .create_unwind_info(isa)
                 .map_err(|error| CompileError::Codegen(pretty_error(&context.func, error)))?
         } else {
             None
         };
 
+        let length = u32::try_from(code_buf.len()).unwrap();
+
         let address_transform =
-            self.get_function_address_map(&context, &body, code_buf.len() as u32, tunables);
+            Self::get_function_address_map(compiled_code, &body, length, tunables);
 
         let ranges = if tunables.generate_native_debuginfo {
-            Some(context.compiled_code().unwrap().value_labels_ranges.clone())
+            Some(compiled_code.value_labels_ranges.clone())
         } else {
             None
         };
@@ -318,8 +315,6 @@ impl wasmtime_environ::Compiler for Compiler {
         log::debug!("{:?} translated in {:?}", func_index, timing.total());
         log::trace!("{:?} timing info\n{}", func_index, timing);
 
-        let length = u32::try_from(code_buf.len()).unwrap();
-
         let sized_stack_slots = std::mem::take(&mut context.func.sized_stack_slots);
 
         self.save_context(CompilerContext {
@@ -496,18 +491,9 @@ mod incremental_cache {
         isa: &dyn TargetIsa,
         cache_ctx: Option<&mut IncrementalCacheContext>,
     ) -> Result<(&'a CompiledCode, Vec<u8>), CompileError> {
-        let mut code_buf = Vec::new();
         let cache_ctx = match cache_ctx {
             Some(ctx) => ctx,
-            None => {
+            None => return compile_uncached(context, isa),
-                let compiled_code =
-                    context
-                        .compile_and_emit(isa, &mut code_buf)
-                        .map_err(|error| {
-                            CompileError::Codegen(pretty_error(&error.func, error.inner))
-                        })?;
-                return Ok((compiled_code, code_buf));
-            }
         };
 
         let mut cache_store = CraneliftCacheStore(cache_ctx.cache_store.clone());
@@ -521,10 +507,7 @@ mod incremental_cache {
             cache_ctx.num_cached += 1;
         }
 
-        code_buf.resize(compiled_code.code_info().total_size as _, 0);
+        Ok((compiled_code, compiled_code.code_buffer().to_vec()))
-        code_buf.copy_from_slice(compiled_code.code_buffer());
-
-        Ok((compiled_code, code_buf))
     }
 }
 
@@ -536,12 +519,19 @@ fn compile_maybe_cached<'a>(
     context: &'a mut Context,
     isa: &dyn TargetIsa,
     _cache_ctx: Option<&mut IncrementalCacheContext>,
+) -> Result<(&'a CompiledCode, Vec<u8>), CompileError> {
+    compile_uncached(context, isa)
+}
+
+fn compile_uncached<'a>(
+    context: &'a mut Context,
+    isa: &dyn TargetIsa,
 ) -> Result<(&'a CompiledCode, Vec<u8>), CompileError> {
     let mut code_buf = Vec::new();
     let compiled_code = context
         .compile_and_emit(isa, &mut code_buf)
         .map_err(|error| CompileError::Codegen(pretty_error(&error.func, error.inner)))?;
-    return Ok((compiled_code, code_buf));
+    Ok((compiled_code, code_buf))
 }
 
 fn to_flag_value(v: &settings::Value) -> FlagValue {
@@ -835,10 +825,10 @@ impl Compiler {
             .traps()
             .into_iter()
             .map(mach_trap_to_trap)
-            .collect::<Vec<_>>();
+            .collect();
 
         let unwind_info = if isa.flags().unwind_info() {
-            context
+            compiled_code
                 .create_unwind_info(isa)
                 .map_err(|error| CompileError::Codegen(pretty_error(&context.func, error)))?
         } else {
@@ -848,7 +838,7 @@ impl Compiler {
         Ok(CompiledFunction {
             body: code_buf,
             unwind_info,
-            relocations: Vec::new(),
+            relocations: Default::default(),
             sized_stack_slots: Default::default(),
             value_labels_ranges: Default::default(),
             info: Default::default(),