From ac6b90fb5e05e06ab635fc1ba81b528dbc644309 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 20 Nov 2023 13:00:42 -0800 Subject: [PATCH] Selectively enable `clippy::cast_sign_loss` This would have fixed #7558 so try to head off future issues with that by warning against this situation in a few crates. This lint is still quite noisy though for Cranelift for example so it's not worthwhile at this time to enable it for the whole workspace. --- Cargo.toml | 4 ++ cranelift/entity/src/lib.rs | 2 + cranelift/entity/src/unsigned.rs | 71 +++++++++++++++++++ crates/environ/src/lib.rs | 1 + crates/environ/src/module_environ.rs | 10 +-- crates/runtime/src/lib.rs | 3 +- crates/runtime/src/libcalls.rs | 4 +- crates/runtime/src/traphandlers/macos.rs | 2 +- crates/runtime/src/vmcontext.rs | 8 +-- crates/wasi-common/src/lib.rs | 3 + crates/wasi-common/src/snapshots/preview_1.rs | 4 +- crates/wasi/src/lib.rs | 2 + crates/wasi/src/preview2/preview1.rs | 4 +- 13 files changed, 103 insertions(+), 15 deletions(-) create mode 100644 cranelift/entity/src/unsigned.rs diff --git a/Cargo.toml b/Cargo.toml index f1273ff5c5a9..3b530c3eec4b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -135,12 +135,16 @@ edition = "2021" rust-version = "1.72.0" [workspace.lints.rust] +# Turn on some lints which are otherwise allow-by-default in rustc. unused_extern_crates = 'warn' trivial_numeric_casts = 'warn' unstable_features = 'warn' unused_import_braces = 'warn' [workspace.lints.clippy] +# The default set of lints in Clippy is viewed as "too noisy" right now so +# they're all turned off by default. Selective lints are then enabled below as +# necessary. all = 'allow' [workspace.dependencies] diff --git a/cranelift/entity/src/lib.rs b/cranelift/entity/src/lib.rs index 5c5cd2284eb1..b22eeb350f2f 100644 --- a/cranelift/entity/src/lib.rs +++ b/cranelift/entity/src/lib.rs @@ -202,6 +202,7 @@ mod map; mod primary; mod set; mod sparse; +mod unsigned; pub use self::boxed_slice::BoxedSlice; pub use self::iter::{Iter, IterMut}; @@ -211,6 +212,7 @@ pub use self::map::SecondaryMap; pub use self::primary::PrimaryMap; pub use self::set::EntitySet; pub use self::sparse::{SparseMap, SparseMapValue, SparseSet}; +pub use self::unsigned::Unsigned; /// A collection of tests to ensure that use of the different `entity_impl!` forms will generate /// `EntityRef` implementations that behave the same way. diff --git a/cranelift/entity/src/unsigned.rs b/cranelift/entity/src/unsigned.rs new file mode 100644 index 000000000000..2d8eb4c640ca --- /dev/null +++ b/cranelift/entity/src/unsigned.rs @@ -0,0 +1,71 @@ +/// Helper trait used to add `unsigned()` methods to primitive signed integer +/// types. +/// +/// The purpose of this trait is to signal the intent that the sign bit of a +/// signed integer is intended to be discarded and the value is instead +/// understood to be a "bag of bits" where the conversion to an unsigned number +/// is intended to be lossless. This can be used for example when converting a +/// signed integer into a larger width with zero-extension. +pub trait Unsigned { + /// The unsigned integer for this type which has the same width. + type Unsigned; + + /// View this signed integer as an unsigned integer of the same width. + /// + /// All bits are preserved. + fn unsigned(self) -> Self::Unsigned; +} + +impl Unsigned for i8 { + type Unsigned = u8; + + #[inline] + fn unsigned(self) -> u8 { + self as u8 + } +} + +impl Unsigned for i16 { + type Unsigned = u16; + + #[inline] + fn unsigned(self) -> u16 { + self as u16 + } +} + +impl Unsigned for i32 { + type Unsigned = u32; + + #[inline] + fn unsigned(self) -> u32 { + self as u32 + } +} + +impl Unsigned for i64 { + type Unsigned = u64; + + #[inline] + fn unsigned(self) -> u64 { + self as u64 + } +} + +impl Unsigned for i128 { + type Unsigned = u128; + + #[inline] + fn unsigned(self) -> u128 { + self as u128 + } +} + +impl Unsigned for isize { + type Unsigned = usize; + + #[inline] + fn unsigned(self) -> usize { + self as usize + } +} diff --git a/crates/environ/src/lib.rs b/crates/environ/src/lib.rs index 2aac53a100c5..af89c2830a06 100644 --- a/crates/environ/src/lib.rs +++ b/crates/environ/src/lib.rs @@ -4,6 +4,7 @@ //! linear memories. #![deny(missing_docs)] +#![warn(clippy::cast_sign_loss)] mod address_map; mod builtin; diff --git a/crates/environ/src/module_environ.rs b/crates/environ/src/module_environ.rs index 1339431427dd..f39a9b608db7 100644 --- a/crates/environ/src/module_environ.rs +++ b/crates/environ/src/module_environ.rs @@ -5,8 +5,8 @@ use crate::module::{ use crate::{ DataIndex, DefinedFuncIndex, ElemIndex, EntityIndex, EntityType, FuncIndex, GlobalIndex, GlobalInit, MemoryIndex, ModuleTypesBuilder, PrimaryMap, SignatureIndex, TableIndex, - TableInitialValue, Tunables, TypeConvert, TypeIndex, WasmError, WasmFuncType, WasmHeapType, - WasmResult, WasmType, + TableInitialValue, Tunables, TypeConvert, TypeIndex, Unsigned, WasmError, WasmFuncType, + WasmHeapType, WasmResult, WasmType, }; use cranelift_entity::packed_option::ReservedValue; use std::borrow::Cow; @@ -487,7 +487,7 @@ impl<'a, 'data> ModuleEnvironment<'a, 'data> { let table_index = TableIndex::from_u32(table_index.unwrap_or(0)); let mut offset_expr_reader = offset_expr.get_binary_reader(); let (base, offset) = match offset_expr_reader.read_operator()? { - Operator::I32Const { value } => (None, value as u32), + Operator::I32Const { value } => (None, value.unsigned()), Operator::GlobalGet { global_index } => { (Some(GlobalIndex::from_u32(global_index)), 0) } @@ -607,8 +607,8 @@ impl<'a, 'data> ModuleEnvironment<'a, 'data> { let memory_index = MemoryIndex::from_u32(memory_index); let mut offset_expr_reader = offset_expr.get_binary_reader(); let (base, offset) = match offset_expr_reader.read_operator()? { - Operator::I32Const { value } => (None, (value as u32).into()), - Operator::I64Const { value } => (None, value as u64), + Operator::I32Const { value } => (None, value.unsigned().into()), + Operator::I64Const { value } => (None, value.unsigned()), Operator::GlobalGet { global_index } => { (Some(GlobalIndex::from_u32(global_index)), 0) } diff --git a/crates/runtime/src/lib.rs b/crates/runtime/src/lib.rs index ce23f2500be4..d3cf7565abbf 100644 --- a/crates/runtime/src/lib.rs +++ b/crates/runtime/src/lib.rs @@ -1,6 +1,7 @@ //! Runtime library support for Wasmtime. #![deny(missing_docs)] +#![warn(clippy::cast_sign_loss)] use anyhow::{Error, Result}; use std::fmt; @@ -240,7 +241,7 @@ pub fn page_size() -> usize { #[cfg(unix)] fn get_page_size() -> usize { - unsafe { libc::sysconf(libc::_SC_PAGESIZE) as usize } + unsafe { libc::sysconf(libc::_SC_PAGESIZE).try_into().unwrap() } } } diff --git a/crates/runtime/src/libcalls.rs b/crates/runtime/src/libcalls.rs index f4c35e1bc59f..b43c11b33cee 100644 --- a/crates/runtime/src/libcalls.rs +++ b/crates/runtime/src/libcalls.rs @@ -66,7 +66,7 @@ use std::mem; use std::ptr::{self, NonNull}; use std::time::{Duration, Instant}; use wasmtime_environ::{ - DataIndex, ElemIndex, FuncIndex, GlobalIndex, MemoryIndex, TableIndex, Trap, + DataIndex, ElemIndex, FuncIndex, GlobalIndex, MemoryIndex, TableIndex, Trap, Unsigned, }; #[cfg(feature = "wmemcheck")] use wasmtime_wmemcheck::AccessError::{ @@ -233,7 +233,7 @@ unsafe fn table_grow( }; Ok(match instance.table_grow(table_index, delta, element)? { Some(r) => r, - None => -1_i32 as u32, + None => (-1_i32).unsigned(), }) } diff --git a/crates/runtime/src/traphandlers/macos.rs b/crates/runtime/src/traphandlers/macos.rs index 9cbe059bd9a2..34d707498e7b 100644 --- a/crates/runtime/src/traphandlers/macos.rs +++ b/crates/runtime/src/traphandlers/macos.rs @@ -31,7 +31,7 @@ //! function declarations. Many bits and pieces are copied or translated from //! the SpiderMonkey implementation and it should pass all the tests! -#![allow(non_snake_case)] +#![allow(non_snake_case, clippy::cast_sign_loss)] use crate::traphandlers::{tls, wasmtime_longjmp}; use mach::exception_types::*; diff --git a/crates/runtime/src/vmcontext.rs b/crates/runtime/src/vmcontext.rs index a46e59c3fde2..f79f063e3d80 100644 --- a/crates/runtime/src/vmcontext.rs +++ b/crates/runtime/src/vmcontext.rs @@ -12,7 +12,7 @@ use std::ptr::NonNull; use std::sync::atomic::{AtomicUsize, Ordering}; use std::u32; pub use vm_host_func_context::{VMArrayCallHostFuncContext, VMNativeCallHostFuncContext}; -use wasmtime_environ::{DefinedMemoryIndex, VMCONTEXT_MAGIC}; +use wasmtime_environ::{DefinedMemoryIndex, Unsigned, VMCONTEXT_MAGIC}; /// A function pointer that exposes the array calling convention. /// @@ -1038,7 +1038,7 @@ impl ValRaw { // `wasmtime` crate. Otherwise though all `ValRaw` constructors are // otherwise constrained to guarantee that the initial 64-bits are // always initialized. - ValRaw::u64((i as u32).into()) + ValRaw::u64(i.unsigned().into()) } /// Creates a WebAssembly `i64` value @@ -1112,13 +1112,13 @@ impl ValRaw { /// Gets the WebAssembly `i32` value #[inline] pub fn get_u32(&self) -> u32 { - self.get_i32() as u32 + self.get_i32().unsigned() } /// Gets the WebAssembly `i64` value #[inline] pub fn get_u64(&self) -> u64 { - self.get_i64() as u64 + self.get_i64().unsigned() } /// Gets the WebAssembly `f32` value diff --git a/crates/wasi-common/src/lib.rs b/crates/wasi-common/src/lib.rs index b6da9c1f984a..9428b2cc4df3 100644 --- a/crates/wasi-common/src/lib.rs +++ b/crates/wasi-common/src/lib.rs @@ -50,6 +50,9 @@ //! `WasiCtx::builder(...)` function. The //! `wasi_cap_std_sync::WasiCtxBuilder::new()` function uses this public //! interface to plug in its own implementations of each of these resources. + +#![warn(clippy::cast_sign_loss)] + pub mod clocks; mod ctx; pub mod dir; diff --git a/crates/wasi-common/src/snapshots/preview_1.rs b/crates/wasi-common/src/snapshots/preview_1.rs index 465af436da45..eab543b46817 100644 --- a/crates/wasi-common/src/snapshots/preview_1.rs +++ b/crates/wasi-common/src/snapshots/preview_1.rs @@ -554,7 +554,9 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { let whence = match whence { types::Whence::Cur => SeekFrom::Current(offset), types::Whence::End => SeekFrom::End(offset), - types::Whence::Set => SeekFrom::Start(offset as u64), + types::Whence::Set => { + SeekFrom::Start(offset.try_into().map_err(|_| Error::invalid_argument())?) + } }; let newoffset = self .table() diff --git a/crates/wasi/src/lib.rs b/crates/wasi/src/lib.rs index 70671b6172ba..a6a1e0b712a2 100644 --- a/crates/wasi/src/lib.rs +++ b/crates/wasi/src/lib.rs @@ -7,6 +7,8 @@ //! Individual snapshots are available through //! `wasmtime_wasi::snapshots::preview_{0, 1}::Wasi::new(&Store, Rc>)`. +#![warn(clippy::cast_sign_loss)] + #[cfg(feature = "preview2")] pub mod preview2; diff --git a/crates/wasi/src/preview2/preview1.rs b/crates/wasi/src/preview2/preview1.rs index b47a571954bf..dd9655183c95 100644 --- a/crates/wasi/src/preview2/preview1.rs +++ b/crates/wasi/src/preview2/preview1.rs @@ -1565,7 +1565,9 @@ impl< let position = position.clone(); drop(t); let pos = match whence { - types::Whence::Set if offset >= 0 => offset as _, + types::Whence::Set if offset >= 0 => { + offset.try_into().map_err(|_| types::Errno::Inval)? + } types::Whence::Cur => position .load(Ordering::Relaxed) .checked_add_signed(offset)