From 7be834f609d75a35137ddcb4d0bf800ba5017d90 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 1 Nov 2024 12:20:04 -0500 Subject: [PATCH] Enable some Clippy conversion lints for `wasmtime-cranelift` (#9536) Similar to how `wasmtime::runtime` has a few off-by-default lints turned on for it do the same for the compilation phase of `wasmtime-cranelift`. This is intended to help weed out lossy `as` casts and instead steer users to `from` or `try_from` conversions. --- cranelift/codegen/Cargo.toml | 4 ++-- crates/cranelift/src/compiled_function.rs | 4 ++-- crates/cranelift/src/compiler.rs | 4 ++-- crates/cranelift/src/debug.rs | 5 +++++ crates/cranelift/src/func_environ.rs | 2 +- crates/cranelift/src/lib.rs | 4 ++++ crates/cranelift/src/obj.rs | 8 ++++---- crates/cranelift/src/translate/code_translator.rs | 11 ++++------- crates/cranelift/src/translate/func_translator.rs | 4 ++-- crates/wasmtime/src/runtime/vm.rs | 2 ++ 10 files changed, 28 insertions(+), 20 deletions(-) diff --git a/cranelift/codegen/Cargo.toml b/cranelift/codegen/Cargo.toml index 324b5a2c9882..d52fe6790dd0 100644 --- a/cranelift/codegen/Cargo.toml +++ b/cranelift/codegen/Cargo.toml @@ -17,7 +17,7 @@ rust-version.workspace = true workspace = true [package.metadata.docs.rs] -# Ask Cargo to build docs with the feature `all-arch` +# Ask Cargo to build docs with the feature `all-arch` features = ["all-arch"] [dependencies] @@ -62,7 +62,7 @@ default = ["std", "unwind", "host-arch", "timing"] # The "std" feature enables use of libstd. The "core" feature enables use # of some minimal std-like replacement libraries. At least one of these two # features need to be enabled. -std = [] +std = ["serde?/std"] # The "core" feature used to enable a hashmap workaround, but is now # deprecated (we (i) always use hashbrown, and (ii) don't support a diff --git a/crates/cranelift/src/compiled_function.rs b/crates/cranelift/src/compiled_function.rs index 24732f003f9b..d5428cb2a1e3 100644 --- a/crates/cranelift/src/compiled_function.rs +++ b/crates/cranelift/src/compiled_function.rs @@ -111,7 +111,7 @@ impl CompiledFunction { .into_iter() .map(|&MachSrcLoc { start, end, loc }| (loc, start, (end - start))); let instructions = if with_instruction_addresses { - collect_address_maps(len as u32, srclocs) + collect_address_maps(len.try_into().unwrap(), srclocs) } else { Default::default() }; @@ -123,7 +123,7 @@ impl CompiledFunction { start_srcloc, end_srcloc, body_offset: 0, - body_len: len as u32, + body_len: len.try_into().unwrap(), }; self.metadata.address_map = address_map; diff --git a/crates/cranelift/src/compiler.rs b/crates/cranelift/src/compiler.rs index 487ece1ca972..e417fd1e001a 100644 --- a/crates/cranelift/src/compiler.rs +++ b/crates/cranelift/src/compiler.rs @@ -833,8 +833,8 @@ impl FunctionCompiler<'_> { let offset = data.original_position(); let len = data.bytes_remaining(); compiled_function.set_address_map( - offset as u32, - len as u32, + offset.try_into().unwrap(), + len.try_into().unwrap(), tunables.generate_address_map, ); } diff --git a/crates/cranelift/src/debug.rs b/crates/cranelift/src/debug.rs index 542a07092270..a072b082bb82 100644 --- a/crates/cranelift/src/debug.rs +++ b/crates/cranelift/src/debug.rs @@ -1,5 +1,10 @@ //! Debug utils for WebAssembly using Cranelift. +// FIXME: this whole crate opts-in to these two noisier-than-default lints, but +// this module has lots of hits on this warning which aren't the easiest to +// resolve. Ideally all warnings would be resolved here though. +#![allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] + use crate::CompiledFunctionMetadata; use cranelift_codegen::isa::TargetIsa; use object::write::SymbolId; diff --git a/crates/cranelift/src/func_environ.rs b/crates/cranelift/src/func_environ.rs index a68af7f2ecc3..d0713b70463d 100644 --- a/crates/cranelift/src/func_environ.rs +++ b/crates/cranelift/src/func_environ.rs @@ -952,7 +952,7 @@ impl<'module_environment> FuncEnvironment<'module_environment> { .name_section .func_names .get(&func_index) - .map(|s| *s) + .copied() } /// Proof-carrying code: create a memtype describing an empty diff --git a/crates/cranelift/src/lib.rs b/crates/cranelift/src/lib.rs index 73772ac5cb12..6a8d5d962e5a 100644 --- a/crates/cranelift/src/lib.rs +++ b/crates/cranelift/src/lib.rs @@ -3,6 +3,10 @@ //! This crate provides an implementation of the `wasmtime_environ::Compiler` //! and `wasmtime_environ::CompilerBuilder` traits. +// See documentation in crates/wasmtime/src/runtime.rs for why this is +// selectively enabled here. +#![warn(clippy::cast_possible_truncation, clippy::cast_sign_loss)] + use cranelift_codegen::{ binemit, cursor::FuncCursor, diff --git a/crates/cranelift/src/obj.rs b/crates/cranelift/src/obj.rs index c6f93bdb0037..b0fd17197fbe 100644 --- a/crates/cranelift/src/obj.rs +++ b/crates/cranelift/src/obj.rs @@ -26,7 +26,7 @@ use object::{Architecture, SectionKind, SymbolFlags, SymbolKind, SymbolScope}; use std::collections::HashMap; use std::ops::Range; use wasmtime_environ::obj::LibCall; -use wasmtime_environ::Compiler; +use wasmtime_environ::{Compiler, Unsigned}; const TEXT_SECTION_NAME: &[u8] = b".text"; @@ -341,9 +341,9 @@ impl<'a> UnwindInfoBuilder<'a> { let requires_extended_counts = code_words > (1 << 5); let encoded_function_len = function_len / 4; assert!(encoded_function_len < (1 << 18), "function too large"); - let mut word1 = encoded_function_len as u32; + let mut word1 = u32::try_from(encoded_function_len).unwrap(); if !requires_extended_counts { - word1 |= (code_words as u32) << 27; + word1 |= u32::from(code_words) << 27; } let unwind_address = self.windows_xdata.len(); self.windows_xdata.extend_from_slice(&word1.to_le_bytes()); @@ -548,7 +548,7 @@ impl<'a> UnwindInfoBuilder<'a> { // unwinders just use this constant for a relative addition with the // address of the FDE, which means that the sign doesn't actually // matter. - let fde = unwind_info.to_fde(Address::Constant(actual_offset as u64)); + let fde = unwind_info.to_fde(Address::Constant(actual_offset.unsigned())); table.add_fde(cie_id, fde); } let endian = match compiler.triple().endianness().unwrap() { diff --git a/crates/cranelift/src/translate/code_translator.rs b/crates/cranelift/src/translate/code_translator.rs index a1d3ba57f37c..fb1af58d073c 100644 --- a/crates/cranelift/src/translate/code_translator.rs +++ b/crates/cranelift/src/translate/code_translator.rs @@ -78,7 +78,6 @@ use crate::translate::state::{ControlStackFrame, ElseData, FuncTranslationState} use crate::translate::translation_utils::{ block_with_params, blocktype_params_results, f32_translation, f64_translation, }; -use core::{i32, u32}; use cranelift_codegen::ir::condcodes::{FloatCC, IntCC}; use cranelift_codegen::ir::immediates::Offset32; use cranelift_codegen::ir::types::*; @@ -93,8 +92,8 @@ use std::collections::{hash_map, HashMap}; use std::vec::Vec; use wasmparser::{FuncValidator, MemArg, Operator, WasmModuleResources}; use wasmtime_environ::{ - wasm_unsupported, DataIndex, ElemIndex, FuncIndex, GlobalIndex, MemoryIndex, TableIndex, - TypeIndex, WasmRefType, WasmResult, + wasm_unsupported, DataIndex, ElemIndex, FuncIndex, GlobalIndex, MemoryIndex, Signed, + TableIndex, TypeIndex, Unsigned, WasmRefType, WasmResult, }; /// Given a `Reachability`, unwrap the inner `T` or, when unreachable, set @@ -930,7 +929,7 @@ pub fn translate_operator( } /****************************** Nullary Operators ************************************/ Operator::I32Const { value } => { - state.push1(builder.ins().iconst(I32, *value as u32 as i64)) + state.push1(builder.ins().iconst(I32, i64::from(value.unsigned()))); } Operator::I64Const { value } => state.push1(builder.ins().iconst(I64, *value)), Operator::F32Const { value } => { @@ -3289,9 +3288,7 @@ fn align_atomic_addr( let effective_addr = if memarg.offset == 0 { addr } else { - builder - .ins() - .iadd_imm(addr, i64::from(memarg.offset as i32)) + builder.ins().iadd_imm(addr, memarg.offset.signed()) }; debug_assert!(loaded_bytes.is_power_of_two()); let misalignment = builder diff --git a/crates/cranelift/src/translate/func_translator.rs b/crates/cranelift/src/translate/func_translator.rs index 733520ae2673..da99ff737c31 100644 --- a/crates/cranelift/src/translate/func_translator.rs +++ b/crates/cranelift/src/translate/func_translator.rs @@ -278,6 +278,6 @@ fn parse_function_body( /// Get the current source location from a reader. fn cur_srcloc(reader: &BinaryReader) -> ir::SourceLoc { // We record source locations as byte code offsets relative to the beginning of the file. - // This will wrap around if byte code is larger than 4 GB. - ir::SourceLoc::new(reader.original_position() as u32) + // This will panic if bytecode is larger than 4 GB. + ir::SourceLoc::new(reader.original_position().try_into().unwrap()) } diff --git a/crates/wasmtime/src/runtime/vm.rs b/crates/wasmtime/src/runtime/vm.rs index a9373935a09b..d65ec713b06b 100644 --- a/crates/wasmtime/src/runtime/vm.rs +++ b/crates/wasmtime/src/runtime/vm.rs @@ -1,6 +1,8 @@ //! Runtime library support for Wasmtime. #![deny(missing_docs)] +// See documentation in crates/wasmtime/src/runtime.rs for why this is +// selectively enabled here. #![warn(clippy::cast_sign_loss)] use crate::prelude::*;