From 3ee898cb2c1d29ed412ed01ee3f9ecc435f996da Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Thu, 7 Jan 2021 21:59:42 -0800 Subject: [PATCH 01/14] x64: support PC-rel symbol references using the GOT when in PIC mode. --- cranelift/codegen/src/isa/x64/inst/emit.rs | 43 ++++++++++++++---- .../codegen/src/isa/x64/inst/emit_tests.rs | 44 ++++++++++++++++++- cranelift/codegen/src/isa/x64/inst/mod.rs | 7 ++- 3 files changed, 82 insertions(+), 12 deletions(-) diff --git a/cranelift/codegen/src/isa/x64/inst/emit.rs b/cranelift/codegen/src/isa/x64/inst/emit.rs index b655178bdf..e4b63bcec7 100644 --- a/cranelift/codegen/src/isa/x64/inst/emit.rs +++ b/cranelift/codegen/src/isa/x64/inst/emit.rs @@ -2690,16 +2690,41 @@ pub(crate) fn emit( } Inst::LoadExtName { dst, name, offset } => { - // The full address can be encoded in the register, with a relocation. - // Generates: movabsq $name, %dst - let enc_dst = int_reg_enc(dst.to_reg()); - sink.put1(0x48 | ((enc_dst >> 3) & 1)); - sink.put1(0xB8 | (enc_dst & 7)); - emit_reloc(sink, state, Reloc::Abs8, name, *offset); - if info.flags().emit_all_ones_funcaddrs() { - sink.put8(u64::max_value()); + if info.flags().is_pic() { + // Generates: movq symbol@GOTPCREL(%rip), %dst + let enc_dst = int_reg_enc(dst.to_reg()); + sink.put1(0x48 | ((enc_dst >> 3) & 1) << 2); + sink.put1(0x8B); + sink.put1(0x05 | ((enc_dst & 7) << 3)); + emit_reloc(sink, state, Reloc::X86GOTPCRel4, name, -4); + sink.put4(0); + // Offset in the relocation above applies to the address of the *GOT entry*, not + // the loaded address; so we emit a separate add or sub instruction if needed. + if *offset < 0 { + assert!(*offset >= -i32::MAX as i64); + sink.put1(0x48 | ((enc_dst >> 3) & 1)); + sink.put1(0x81); + sink.put1(0xe8 | (enc_dst & 7)); + sink.put4((-*offset) as u32); + } else if *offset > 0 { + assert!(*offset <= i32::MAX as i64); + sink.put1(0x48 | ((enc_dst >> 3) & 1)); + sink.put1(0x81); + sink.put1(0xc0 | (enc_dst & 7)); + sink.put4(*offset as u32); + } } else { - sink.put8(0); + // The full address can be encoded in the register, with a relocation. + // Generates: movabsq $name, %dst + let enc_dst = int_reg_enc(dst.to_reg()); + sink.put1(0x48 | ((enc_dst >> 3) & 1)); + sink.put1(0xB8 | (enc_dst & 7)); + emit_reloc(sink, state, Reloc::Abs8, name, *offset); + if info.flags().emit_all_ones_funcaddrs() { + sink.put8(u64::max_value()); + } else { + sink.put8(0); + } } } diff --git a/cranelift/codegen/src/isa/x64/inst/emit_tests.rs b/cranelift/codegen/src/isa/x64/inst/emit_tests.rs index 0786746672..802a165566 100644 --- a/cranelift/codegen/src/isa/x64/inst/emit_tests.rs +++ b/cranelift/codegen/src/isa/x64/inst/emit_tests.rs @@ -2778,6 +2778,46 @@ fn test_x64_emit() { "call *321(%r10,%rdx,4)", )); + // ======================================================== + // LoadExtName + // N.B.: test harness below sets is_pic. + insns.push(( + Inst::LoadExtName { + dst: Writable::from_reg(r11), + name: Box::new(ExternalName::User { + namespace: 0, + index: 0, + }), + offset: 0, + }, + "4C8B1D00000000", + "load_ext_name u0:0+0, %r11", + )); + insns.push(( + Inst::LoadExtName { + dst: Writable::from_reg(r11), + name: Box::new(ExternalName::User { + namespace: 0, + index: 0, + }), + offset: 0x12345678, + }, + "4C8B1D000000004981C378563412", + "load_ext_name u0:0+305419896, %r11", + )); + insns.push(( + Inst::LoadExtName { + dst: Writable::from_reg(r11), + name: Box::new(ExternalName::User { + namespace: 0, + index: 0, + }), + offset: -0x12345678, + }, + "4C8B1D000000004981EB78563412", + "load_ext_name u0:0+-305419896, %r11", + )); + // ======================================================== // Ret insns.push((Inst::ret(), "C3", "ret")); @@ -3693,7 +3733,9 @@ fn test_x64_emit() { // ======================================================== // Actually run the tests! - let flags = settings::Flags::new(settings::builder()); + let mut flag_builder = settings::builder(); + flag_builder.enable("is_pic").unwrap(); + let flags = settings::Flags::new(flag_builder); use crate::settings::Configurable; let mut isa_flag_builder = x64::settings::builder(); diff --git a/cranelift/codegen/src/isa/x64/inst/mod.rs b/cranelift/codegen/src/isa/x64/inst/mod.rs index 806e8f276e..01c6858810 100644 --- a/cranelift/codegen/src/isa/x64/inst/mod.rs +++ b/cranelift/codegen/src/isa/x64/inst/mod.rs @@ -396,7 +396,10 @@ pub enum Inst { /// An instruction that will always trigger the illegal instruction exception. Ud2 { trap_code: TrapCode }, - /// Loads an external symbol in a register, with a relocation: movabsq $name, dst + /// Loads an external symbol in a register, with a relocation: + /// + /// movq $name@GOTPCREL(%rip), dst if PIC is enabled, or + /// movabsq $name, dst otherwise. LoadExtName { dst: Writable, name: Box, @@ -1692,7 +1695,7 @@ impl PrettyPrint for Inst { dst, name, offset, .. } => format!( "{} {}+{}, {}", - ljustify("movaps".into()), + ljustify("load_ext_name".into()), name, offset, show_ireg_sized(dst.to_reg(), mb_rru, 8), From 3580205f12445229ea5777bdd1880278aca31279 Mon Sep 17 00:00:00 2001 From: Yury Delendik Date: Fri, 8 Jan 2021 11:55:21 -0600 Subject: [PATCH 02/14] [Cranelift][Atomics] Add address folding for atomic notify/wait. (#2556) * fold address in wasm wait and notify ops * add atomics addr folding tests --- build.rs | 1 + cranelift/wasm/src/code_translator.rs | 38 ++++++ crates/cranelift/src/func_environ.rs | 119 +++++++++++++++--- crates/environ/src/builtin.rs | 12 ++ crates/runtime/src/libcalls.rs | 87 ++++++++++++- crates/runtime/src/vmcontext.rs | 12 ++ tests/all/wast.rs | 2 + .../threads/atomics_wait_address.wast | 52 ++++++++ 8 files changed, 307 insertions(+), 16 deletions(-) create mode 100644 tests/misc_testsuite/threads/atomics_wait_address.wast diff --git a/build.rs b/build.rs index 4258c27ca0..35257cdb7b 100644 --- a/build.rs +++ b/build.rs @@ -32,6 +32,7 @@ fn main() -> anyhow::Result<()> { test_directory_module(out, "tests/misc_testsuite/reference-types", strategy)?; test_directory_module(out, "tests/misc_testsuite/multi-memory", strategy)?; test_directory_module(out, "tests/misc_testsuite/module-linking", strategy)?; + test_directory_module(out, "tests/misc_testsuite/threads", strategy)?; Ok(()) })?; diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index 998a0c7417..5016bb8abb 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -1052,6 +1052,7 @@ pub fn translate_operator( let timeout = state.pop1(); // 64 (fixed) let expected = state.pop1(); // 32 or 64 (per the `Ixx` in `IxxAtomicWait`) let addr = state.pop1(); // 32 (fixed) + let addr = fold_atomic_mem_addr(addr, memarg, implied_ty, builder); assert!(builder.func.dfg.value_type(expected) == implied_ty); // `fn translate_atomic_wait` can inspect the type of `expected` to figure out what // code it needs to generate, if it wants. @@ -1070,6 +1071,7 @@ pub fn translate_operator( let heap = state.get_heap(builder.func, memarg.memory, environ)?; let count = state.pop1(); // 32 (fixed) let addr = state.pop1(); // 32 (fixed) + let addr = fold_atomic_mem_addr(addr, memarg, I32, builder); let res = environ.translate_atomic_notify(builder.cursor(), heap_index, heap, addr, count)?; state.push1(res); @@ -2142,6 +2144,42 @@ fn translate_icmp(cc: IntCC, builder: &mut FunctionBuilder, state: &mut FuncTran state.push1(builder.ins().bint(I32, val)); } +fn fold_atomic_mem_addr( + linear_mem_addr: Value, + memarg: &MemoryImmediate, + access_ty: Type, + builder: &mut FunctionBuilder, +) -> Value { + let access_ty_bytes = access_ty.bytes(); + let final_lma = if memarg.offset > 0 { + assert!(builder.func.dfg.value_type(linear_mem_addr) == I32); + let linear_mem_addr = builder.ins().uextend(I64, linear_mem_addr); + let a = builder + .ins() + .iadd_imm(linear_mem_addr, i64::from(memarg.offset)); + let cflags = builder.ins().ifcmp_imm(a, 0x1_0000_0000i64); + builder.ins().trapif( + IntCC::UnsignedGreaterThanOrEqual, + cflags, + ir::TrapCode::HeapOutOfBounds, + ); + builder.ins().ireduce(I32, a) + } else { + linear_mem_addr + }; + assert!(access_ty_bytes == 4 || access_ty_bytes == 8); + let final_lma_misalignment = builder + .ins() + .band_imm(final_lma, i64::from(access_ty_bytes - 1)); + let f = builder + .ins() + .ifcmp_imm(final_lma_misalignment, i64::from(0)); + builder + .ins() + .trapif(IntCC::NotEqual, f, ir::TrapCode::HeapMisaligned); + final_lma +} + // For an atomic memory operation, emit an alignment check for the linear memory address, // and then compute the final effective address. fn finalise_atomic_mem_addr( diff --git a/crates/cranelift/src/func_environ.rs b/crates/cranelift/src/func_environ.rs index aa89f2cfc4..6b94824f93 100644 --- a/crates/cranelift/src/func_environ.rs +++ b/crates/cranelift/src/func_environ.rs @@ -81,6 +81,10 @@ macro_rules! declare_function_signatures { AbiParam::new(I32).uext() } + fn i64(&self) -> AbiParam { + AbiParam::new(I64) + } + $( fn $name(&mut self, func: &mut Function) -> ir::SigRef { let sig = self.$name.unwrap_or_else(|| { @@ -258,6 +262,70 @@ impl<'module_environment> FuncEnvironment<'module_environment> { } } + fn get_memory_atomic_notify( + &mut self, + func: &mut Function, + memory_index: MemoryIndex, + ) -> (ir::SigRef, usize, BuiltinFunctionIndex) { + if let Some(defined_memory_index) = self.module.defined_memory_index(memory_index) { + ( + self.builtin_function_signatures.memory_atomic_notify(func), + defined_memory_index.index(), + BuiltinFunctionIndex::memory_atomic_notify(), + ) + } else { + ( + self.builtin_function_signatures + .imported_memory_atomic_notify(func), + memory_index.index(), + BuiltinFunctionIndex::imported_memory_atomic_notify(), + ) + } + } + + fn get_memory_atomic_wait( + &mut self, + func: &mut Function, + memory_index: MemoryIndex, + ty: ir::Type, + ) -> (ir::SigRef, usize, BuiltinFunctionIndex) { + match ty { + I32 => { + if let Some(defined_memory_index) = self.module.defined_memory_index(memory_index) { + ( + self.builtin_function_signatures.memory_atomic_wait32(func), + defined_memory_index.index(), + BuiltinFunctionIndex::memory_atomic_wait32(), + ) + } else { + ( + self.builtin_function_signatures + .imported_memory_atomic_wait32(func), + memory_index.index(), + BuiltinFunctionIndex::imported_memory_atomic_wait32(), + ) + } + } + I64 => { + if let Some(defined_memory_index) = self.module.defined_memory_index(memory_index) { + ( + self.builtin_function_signatures.memory_atomic_wait64(func), + defined_memory_index.index(), + BuiltinFunctionIndex::memory_atomic_wait64(), + ) + } else { + ( + self.builtin_function_signatures + .imported_memory_atomic_wait64(func), + memory_index.index(), + BuiltinFunctionIndex::imported_memory_atomic_wait64(), + ) + } + } + x => panic!("get_memory_atomic_wait unsupported type: {:?}", x), + } + } + fn get_memory_init_func(&mut self, func: &mut Function) -> (ir::SigRef, BuiltinFunctionIndex) { ( self.builtin_function_signatures.memory_init(func), @@ -1368,29 +1436,50 @@ impl<'module_environment> cranelift_wasm::FuncEnvironment for FuncEnvironment<'m fn translate_atomic_wait( &mut self, - _pos: FuncCursor, - _index: MemoryIndex, + mut pos: FuncCursor, + memory_index: MemoryIndex, _heap: ir::Heap, - _addr: ir::Value, - _expected: ir::Value, - _timeout: ir::Value, + addr: ir::Value, + expected: ir::Value, + timeout: ir::Value, ) -> WasmResult { - Err(WasmError::Unsupported( - "wasm atomics (fn translate_atomic_wait)".to_string(), - )) + let implied_ty = pos.func.dfg.value_type(expected); + let (func_sig, memory_index, func_idx) = + self.get_memory_atomic_wait(&mut pos.func, memory_index, implied_ty); + + let memory_index_arg = pos.ins().iconst(I32, memory_index as i64); + + let (vmctx, func_addr) = self.translate_load_builtin_function_address(&mut pos, func_idx); + + let call_inst = pos.ins().call_indirect( + func_sig, + func_addr, + &[vmctx, memory_index_arg, addr, expected, timeout], + ); + + Ok(*pos.func.dfg.inst_results(call_inst).first().unwrap()) } fn translate_atomic_notify( &mut self, - _pos: FuncCursor, - _index: MemoryIndex, + mut pos: FuncCursor, + memory_index: MemoryIndex, _heap: ir::Heap, - _addr: ir::Value, - _count: ir::Value, + addr: ir::Value, + count: ir::Value, ) -> WasmResult { - Err(WasmError::Unsupported( - "wasm atomics (fn translate_atomic_notify)".to_string(), - )) + let (func_sig, memory_index, func_idx) = + self.get_memory_atomic_notify(&mut pos.func, memory_index); + + let memory_index_arg = pos.ins().iconst(I32, memory_index as i64); + + let (vmctx, func_addr) = self.translate_load_builtin_function_address(&mut pos, func_idx); + + let call_inst = + pos.ins() + .call_indirect(func_sig, func_addr, &[vmctx, memory_index_arg, addr, count]); + + Ok(*pos.func.dfg.inst_results(call_inst).first().unwrap()) } fn translate_loop_header(&mut self, mut pos: FuncCursor) -> WasmResult<()> { diff --git a/crates/environ/src/builtin.rs b/crates/environ/src/builtin.rs index 2f7314dc28..f2bb330217 100644 --- a/crates/environ/src/builtin.rs +++ b/crates/environ/src/builtin.rs @@ -45,6 +45,18 @@ macro_rules! foreach_builtin_function { externref_global_get(vmctx, i32) -> (reference); /// Returns an index for Wasm's `global.get` instruction for `externref`s. externref_global_set(vmctx, i32, reference) -> (); + /// Returns an index for wasm's `memory.atomic.notify` for locally defined memories. + memory_atomic_notify(vmctx, i32, i32, i32) -> (i32); + /// Returns an index for wasm's `memory.atomic.notify` for imported memories. + imported_memory_atomic_notify(vmctx, i32, i32, i32) -> (i32); + /// Returns an index for wasm's `memory.atomic.wait32` for locally defined memories. + memory_atomic_wait32(vmctx, i32, i32, i32, i64) -> (i32); + /// Returns an index for wasm's `memory.atomic.wait32` for imported memories. + imported_memory_atomic_wait32(vmctx, i32, i32, i32, i64) -> (i32); + /// Returns an index for wasm's `memory.atomic.wait64` for locally defined memories. + memory_atomic_wait64(vmctx, i32, i32, i64, i64) -> (i32); + /// Returns an index for wasm's `memory.atomic.wait64` for imported memories. + imported_memory_atomic_wait64(vmctx, i32, i32, i64, i64) -> (i32); } }; } diff --git a/crates/runtime/src/libcalls.rs b/crates/runtime/src/libcalls.rs index f18392f4af..61e8ca4a21 100644 --- a/crates/runtime/src/libcalls.rs +++ b/crates/runtime/src/libcalls.rs @@ -58,7 +58,7 @@ use crate::externref::VMExternRef; use crate::table::Table; -use crate::traphandlers::raise_lib_trap; +use crate::traphandlers::{raise_lib_trap, Trap}; use crate::vmcontext::{VMCallerCheckedAnyfunc, VMContext}; use std::mem; use std::ptr::{self, NonNull}; @@ -496,3 +496,88 @@ pub unsafe extern "C" fn wasmtime_externref_global_set( let old = mem::replace((*global).as_externref_mut(), externref); drop(old); } + +#[derive(Debug)] +struct Unimplemented(&'static str); +impl std::error::Error for Unimplemented {} +impl std::fmt::Display for Unimplemented { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::result::Result<(), std::fmt::Error> { + write!(f, "unimplemented: {}", self.0) + } +} + +/// Implementation of `memory.atomic.notify` for locally defined memories. +pub unsafe extern "C" fn wasmtime_memory_atomic_notify( + _vmctx: *mut VMContext, + _memory_index: u32, + _addr: u32, + _count: u32, +) -> u32 { + raise_lib_trap(Trap::User(Box::new(Unimplemented( + "wasm atomics (fn wasmtime_memory_atomic_notify) unsupported", + )))); +} + +/// Implementation of `memory.atomic.notify` for imported memories. +pub unsafe extern "C" fn wasmtime_imported_memory_atomic_notify( + _vmctx: *mut VMContext, + _memory_index: u32, + _addr: u32, + _count: u32, +) -> u32 { + raise_lib_trap(Trap::User(Box::new(Unimplemented( + "wasm atomics (fn wasmtime_imported_memory_atomic_notify) unsupported", + )))); +} + +/// Implementation of `memory.atomic.wait32` for locally defined memories. +pub unsafe extern "C" fn wasmtime_memory_atomic_wait32( + _vmctx: *mut VMContext, + _memory_index: u32, + _addr: u32, + _expected: u32, + _timeout: u64, +) -> u32 { + raise_lib_trap(Trap::User(Box::new(Unimplemented( + "wasm atomics (fn wasmtime_memory_atomic_wait32) unsupported", + )))); +} + +/// Implementation of `memory.atomic.wait32` for imported memories. +pub unsafe extern "C" fn wasmtime_imported_memory_atomic_wait32( + _vmctx: *mut VMContext, + _memory_index: u32, + _addr: u32, + _expected: u32, + _timeout: u64, +) -> u32 { + raise_lib_trap(Trap::User(Box::new(Unimplemented( + "wasm atomics (fn wasmtime_imported_memory_atomic_wait32) unsupported", + )))); +} + +/// Implementation of `memory.atomic.wait64` for locally defined memories. +pub unsafe extern "C" fn wasmtime_memory_atomic_wait64( + _vmctx: *mut VMContext, + _memory_index: u32, + _addr: u32, + _expected: u64, + _timeout: u64, +) -> u32 { + raise_lib_trap(Trap::User(Box::new(Unimplemented( + "wasm atomics (fn wasmtime_memory_atomic_wait32) unsupported", + )))); +} + +/// Implementation of `memory.atomic.wait32` for imported memories. +pub unsafe extern "C" fn wasmtime_imported_memory_atomic_wait64( + _vmctx: *mut VMContext, + _memory_index: u32, + _addr: u32, + _expected: u64, + _timeout: u64, +) -> u32 { + raise_lib_trap(Trap::User(Box::new(Unimplemented( + "wasm atomics (fn wasmtime_imported_memory_atomic_wait64) unsupported", + )))); +} diff --git a/crates/runtime/src/vmcontext.rs b/crates/runtime/src/vmcontext.rs index 23d368802a..3eebdabd4e 100644 --- a/crates/runtime/src/vmcontext.rs +++ b/crates/runtime/src/vmcontext.rs @@ -600,6 +600,18 @@ impl VMBuiltinFunctionsArray { wasmtime_table_fill as usize; ptrs[BuiltinFunctionIndex::table_fill_funcref().index() as usize] = wasmtime_table_fill as usize; + ptrs[BuiltinFunctionIndex::memory_atomic_notify().index() as usize] = + wasmtime_memory_atomic_notify as usize; + ptrs[BuiltinFunctionIndex::imported_memory_atomic_notify().index() as usize] = + wasmtime_imported_memory_atomic_notify as usize; + ptrs[BuiltinFunctionIndex::memory_atomic_wait32().index() as usize] = + wasmtime_memory_atomic_wait32 as usize; + ptrs[BuiltinFunctionIndex::imported_memory_atomic_wait32().index() as usize] = + wasmtime_imported_memory_atomic_wait32 as usize; + ptrs[BuiltinFunctionIndex::memory_atomic_wait64().index() as usize] = + wasmtime_memory_atomic_wait64 as usize; + ptrs[BuiltinFunctionIndex::imported_memory_atomic_wait64().index() as usize] = + wasmtime_imported_memory_atomic_wait64 as usize; if cfg!(debug_assertions) { for i in 0..ptrs.len() { diff --git a/tests/all/wast.rs b/tests/all/wast.rs index 9227079a0f..362dca274b 100644 --- a/tests/all/wast.rs +++ b/tests/all/wast.rs @@ -14,6 +14,7 @@ fn run_wast(wast: &str, strategy: Strategy) -> anyhow::Result<()> { let multi_memory = wast.iter().any(|s| s == "multi-memory"); let module_linking = wast.iter().any(|s| s == "module-linking"); + let threads = wast.iter().any(|s| s == "threads"); let bulk_mem = multi_memory || wast.iter().any(|s| s == "bulk-memory-operations"); // Some simd tests assume support for multiple tables, which are introduced @@ -26,6 +27,7 @@ fn run_wast(wast: &str, strategy: Strategy) -> anyhow::Result<()> { .wasm_reference_types(reftypes || module_linking) .wasm_multi_memory(multi_memory || module_linking) .wasm_module_linking(module_linking) + .wasm_threads(threads) .strategy(strategy)? .cranelift_debug_verifier(true); diff --git a/tests/misc_testsuite/threads/atomics_wait_address.wast b/tests/misc_testsuite/threads/atomics_wait_address.wast new file mode 100644 index 0000000000..edffdc5545 --- /dev/null +++ b/tests/misc_testsuite/threads/atomics_wait_address.wast @@ -0,0 +1,52 @@ +;; From https://bugzilla.mozilla.org/show_bug.cgi?id=1684861. +;; + +(module + (type (;0;) (func)) + (func $main (type 0) + i32.const -64 + i32.const -63 + memory.atomic.notify offset=1 + unreachable) + (memory (;0;) 4 4) + (export "main" (func $main)) +) + +(assert_trap (invoke "main") "misaligned memory access") + + +(module + (type (;0;) (func)) + (func $main (type 0) + i32.const -64 + i32.const -63 + memory.atomic.notify offset=65536 + unreachable) + (memory (;0;) 4 4) + (export "main" (func $main)) +) + +(assert_trap (invoke "main") "out of bounds memory access") + + +(module + (type (;0;) (func)) + (func $wait32 (type 0) + i32.const -64 + i32.const 42 + i64.const 0 + memory.atomic.wait32 offset=1 + unreachable) + (func $wait64 (type 0) + i32.const -64 + i64.const 43 + i64.const 0 + memory.atomic.wait64 offset=3 + unreachable) + (memory (;0;) 4 4) + (export "wait32" (func $wait32)) + (export "wait64" (func $wait64)) +) + +(assert_trap (invoke "wait32") "misaligned memory access") +(assert_trap (invoke "wait64") "misaligned memory access") From bb2dd5b68b367df3305a98914eb9baeafa80bc03 Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Mon, 7 Dec 2020 13:59:31 -0800 Subject: [PATCH 03/14] [machinst x64]: implement load*_zero for x64 --- cranelift/codegen/src/isa/x64/lower.rs | 47 +++++++++++++++++++ .../isa/x64/simd-lane-access-compile.clif | 31 ++++++++++++ 2 files changed, 78 insertions(+) diff --git a/cranelift/codegen/src/isa/x64/lower.rs b/cranelift/codegen/src/isa/x64/lower.rs index 6b779a81d8..39d6f9fb57 100644 --- a/cranelift/codegen/src/isa/x64/lower.rs +++ b/cranelift/codegen/src/isa/x64/lower.rs @@ -4095,6 +4095,53 @@ fn lower_insn_to_regs>( emit_extract_lane(ctx, src, dst, lane, ty); } + Opcode::ScalarToVector => { + // When moving a scalar value to a vector register, we must be handle several + // situations: + // 1. a scalar float is already in an XMM register, so we simply move it + // 2. a scalar of any other type resides in a GPR register: MOVD moves the bits to an + // XMM register and zeroes the upper bits + // 3. a scalar (float or otherwise) that has previously been loaded from memory (e.g. + // the default lowering of Wasm's `load[32|64]_zero`) can be lowered to a single + // MOVSS/MOVSD instruction; to do this, we rely on `input_to_reg_mem` to sink the + // unused load. + let src = input_to_reg_mem(ctx, inputs[0]); + let src_ty = ctx.input_ty(insn, 0); + let dst = get_output_reg(ctx, outputs[0]); + let dst_ty = ty.unwrap(); + assert!(src_ty == dst_ty.lane_type() && dst_ty.bits() == 128); + match src { + RegMem::Reg { reg } => { + if src_ty.is_float() { + // Case 1: when moving a scalar float, we simply move from one XMM register + // to another, expecting the register allocator to elide this. Here we + // assume that the upper bits of a scalar float have not been munged with + // (the same assumption the old backend makes). + ctx.emit(Inst::gen_move(dst, reg, dst_ty)); + } else { + // Case 2: when moving a scalar value of any other type, use MOVD to zero + // the upper lanes. + let src_size = match src_ty.bits() { + 32 => OperandSize::Size32, + 64 => OperandSize::Size64, + _ => unimplemented!("invalid source size for type: {}", src_ty), + }; + ctx.emit(Inst::gpr_to_xmm(SseOpcode::Movd, src, src_size, dst)); + } + } + RegMem::Mem { .. } => { + // Case 3: when presented with `load + scalar_to_vector`, coalesce into a single + // MOVSS/MOVSD instruction. + let opcode = match src_ty.bits() { + 32 => SseOpcode::Movss, + 64 => SseOpcode::Movsd, + _ => unimplemented!("unable to move scalar to vector for type: {}", src_ty), + }; + ctx.emit(Inst::xmm_mov(opcode, src, dst)); + } + } + } + Opcode::Splat => { let ty = ty.unwrap(); assert_eq!(ty.bits(), 128); diff --git a/cranelift/filetests/filetests/isa/x64/simd-lane-access-compile.clif b/cranelift/filetests/filetests/isa/x64/simd-lane-access-compile.clif index f44dbd3b62..f451bd2a25 100644 --- a/cranelift/filetests/filetests/isa/x64/simd-lane-access-compile.clif +++ b/cranelift/filetests/filetests/isa/x64/simd-lane-access-compile.clif @@ -91,3 +91,34 @@ block0(v0: f64): ; check: uninit %xmm1 ; nextln: movsd %xmm0, %xmm1 ; nextln: movlhps %xmm0, %xmm1 + + + +;; load*_zero + +; Verify that a `load` followed by a `scalar_to_vector` (the CLIF translation of `load32_zero`) is +; lowered to a single MOVSS instruction. +function %load32_zero_coalesced(i64) -> i32x4 { +block0(v0: i64): + v1 = load.i32 v0 + v2 = scalar_to_vector.i32x4 v1 + ; check: movss 0(%rdi), %xmm0 + return v2 +} + +;; Verify that `scalar_to_vector` (used by `load32_zero`), lowers as expected. +function %load32_zero_int(i32) -> i32x4 { +block0(v0: i32): + v1 = scalar_to_vector.i32x4 v0 + ; check: movd %edi, %xmm0 + return v1 +} +function %load32_zero_float(f32) -> f32x4 { +block0(v0: f32): + v1 = scalar_to_vector.f32x4 v0 + ; regex: MOV=movap* + ; check: pushq + ; not: $MOV + ; check: ret + return v1 +} From 09a5b91b9dd42a96deb16448cc61b31cd2c288a6 Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Mon, 4 Jan 2021 16:09:28 -0800 Subject: [PATCH 04/14] x64: make several structures debuggable --- cranelift/codegen/src/isa/x64/inst/args.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cranelift/codegen/src/isa/x64/inst/args.rs b/cranelift/codegen/src/isa/x64/inst/args.rs index 680d0921ff..898134644f 100644 --- a/cranelift/codegen/src/isa/x64/inst/args.rs +++ b/cranelift/codegen/src/isa/x64/inst/args.rs @@ -145,7 +145,7 @@ impl PrettyPrint for Amode { /// A Memory Address. These denote a 64-bit value only. /// Used for usual addressing modes as well as addressing modes used during compilation, when the /// moving SP offset is not known. -#[derive(Clone)] +#[derive(Clone, Debug)] pub enum SyntheticAmode { /// A real amode. Real(Amode), @@ -286,7 +286,7 @@ impl PrettyPrintSized for RegMemImm { /// An operand which is either an integer Register or a value in Memory. This can denote an 8, 16, /// 32, 64, or 128 bit value. -#[derive(Clone)] +#[derive(Clone, Debug)] pub enum RegMem { Reg { reg: Reg }, Mem { addr: SyntheticAmode }, From b25a3c387ef4371bd77a9a84ab6118948bb59575 Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Fri, 8 Jan 2021 16:49:28 -0800 Subject: [PATCH 05/14] fix: `dst` should be `Writable`, not `ValueRegs` --- cranelift/codegen/src/isa/x64/lower.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cranelift/codegen/src/isa/x64/lower.rs b/cranelift/codegen/src/isa/x64/lower.rs index 39d6f9fb57..0b9784d59d 100644 --- a/cranelift/codegen/src/isa/x64/lower.rs +++ b/cranelift/codegen/src/isa/x64/lower.rs @@ -4107,7 +4107,7 @@ fn lower_insn_to_regs>( // unused load. let src = input_to_reg_mem(ctx, inputs[0]); let src_ty = ctx.input_ty(insn, 0); - let dst = get_output_reg(ctx, outputs[0]); + let dst = get_output_reg(ctx, outputs[0]).only_reg().unwrap(); let dst_ty = ty.unwrap(); assert!(src_ty == dst_ty.lane_type() && dst_ty.bits() == 128); match src { From 2adb0e8964901b0d967a8aa81188acf54bf6afa0 Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Fri, 8 Jan 2021 16:54:54 -0800 Subject: [PATCH 06/14] security: upgrade smallvec to 1.6.1 Fixes advisory https://rustsec.org/advisories/RUSTSEC-2021-0003. --- Cargo.lock | 4 ++-- cranelift/codegen/Cargo.toml | 2 +- cranelift/frontend/Cargo.toml | 2 +- cranelift/interpreter/Cargo.toml | 2 +- cranelift/reader/Cargo.toml | 2 +- cranelift/wasm/Cargo.toml | 2 +- crates/lightbeam/Cargo.toml | 2 +- crates/wasmtime/Cargo.toml | 2 +- 8 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4a5000807f..ed60ae353a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1966,9 +1966,9 @@ checksum = "7fdf1b9db47230893d76faad238fd6097fd6d6a9245cd7a4d90dbd639536bbd2" [[package]] name = "smallvec" -version = "1.5.1" +version = "1.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ae524f056d7d770e174287294f562e95044c68e88dec909a00d2094805db9d75" +checksum = "fe0f37c9e8f3c5a4a66ad655a93c74daac4ad00c441533bf5c6e7990bb42604e" [[package]] name = "souper-ir" diff --git a/cranelift/codegen/Cargo.toml b/cranelift/codegen/Cargo.toml index 6db2cf9700..d8a8baf384 100644 --- a/cranelift/codegen/Cargo.toml +++ b/cranelift/codegen/Cargo.toml @@ -22,7 +22,7 @@ log = { version = "0.4.6", default-features = false } serde = { version = "1.0.94", features = ["derive"], optional = true } bincode = { version = "1.2.1", optional = true } gimli = { version = "0.23.0", default-features = false, features = ["write"], optional = true } -smallvec = { version = "1.0.0" } +smallvec = { version = "1.6.1" } thiserror = "1.0.4" byteorder = { version = "1.3.2", default-features = false } peepmatic = { path = "../peepmatic", optional = true, version = "0.69.0" } diff --git a/cranelift/frontend/Cargo.toml b/cranelift/frontend/Cargo.toml index dada14fd5d..80dd4f2857 100644 --- a/cranelift/frontend/Cargo.toml +++ b/cranelift/frontend/Cargo.toml @@ -15,7 +15,7 @@ cranelift-codegen = { path = "../codegen", version = "0.69.0", default-features target-lexicon = "0.11" log = { version = "0.4.6", default-features = false } hashbrown = { version = "0.9.1", optional = true } -smallvec = { version = "1.0.0" } +smallvec = { version = "1.6.1" } [features] default = ["std"] diff --git a/cranelift/interpreter/Cargo.toml b/cranelift/interpreter/Cargo.toml index 10b525f34f..b919026047 100644 --- a/cranelift/interpreter/Cargo.toml +++ b/cranelift/interpreter/Cargo.toml @@ -15,7 +15,7 @@ cranelift-codegen = { path = "../codegen", version = "0.69.0", features = ["all- cranelift-entity = { path = "../entity", version = "0.69.0" } cranelift-reader = { path = "../reader", version = "0.69.0" } log = { version = "0.4.8", default-features = false } -smallvec = "1.4.2" +smallvec = "1.6.1" thiserror = "1.0.15" [dev-dependencies] diff --git a/cranelift/reader/Cargo.toml b/cranelift/reader/Cargo.toml index e9da0b581f..38662424a4 100644 --- a/cranelift/reader/Cargo.toml +++ b/cranelift/reader/Cargo.toml @@ -11,7 +11,7 @@ edition = "2018" [dependencies] cranelift-codegen = { path = "../codegen", version = "0.69.0" } -smallvec = "1.0.0" +smallvec = "1.6.1" target-lexicon = "0.11" thiserror = "1.0.15" diff --git a/cranelift/wasm/Cargo.toml b/cranelift/wasm/Cargo.toml index c8a9fdc4c2..413d2ee004 100644 --- a/cranelift/wasm/Cargo.toml +++ b/cranelift/wasm/Cargo.toml @@ -20,7 +20,7 @@ hashbrown = { version = "0.9.1", optional = true } itertools = "0.9.0" log = { version = "0.4.6", default-features = false } serde = { version = "1.0.94", features = ["derive"], optional = true } -smallvec = "1.0.0" +smallvec = "1.6.1" thiserror = "1.0.4" [dev-dependencies] diff --git a/crates/lightbeam/Cargo.toml b/crates/lightbeam/Cargo.toml index 89bd694397..89a18280fa 100644 --- a/crates/lightbeam/Cargo.toml +++ b/crates/lightbeam/Cargo.toml @@ -21,7 +21,7 @@ iter-enum = "0.2" itertools = "0.9.0" memoffset = "0.6.0" more-asserts = "0.2.1" -smallvec = "1.0.0" +smallvec = "1.6.1" thiserror = "1.0.9" typemap = "0.3" wasmparser = "0.71" diff --git a/crates/wasmtime/Cargo.toml b/crates/wasmtime/Cargo.toml index d8f21a6936..54da1d9873 100644 --- a/crates/wasmtime/Cargo.toml +++ b/crates/wasmtime/Cargo.toml @@ -26,7 +26,7 @@ rustc-demangle = "0.1.16" cpp_demangle = "0.3.2" log = "0.4.8" wat = { version = "1.0.18", optional = true } -smallvec = "1.4.0" +smallvec = "1.6.1" serde = { version = "1.0.94", features = ["derive"] } bincode = "1.2.1" indexmap = "1.6" From 07652ca0d497aa191951f376b275b40378a27175 Mon Sep 17 00:00:00 2001 From: Julian Seward Date: Fri, 8 Jan 2021 11:45:10 +0100 Subject: [PATCH 07/14] wasm->CLIF: fn translate_operator: Select/TypedSelect: add missing bitcasts The translation of Operator::Select and Operator::TypedSelect for vector-typed operands, lacks the relevant bitcasting of the operands to I8X16. This commit adds it. --- cranelift/wasm/src/code_translator.rs | 16 +++++++-- cranelift/wasmtests/pr2559.wat | 51 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 cranelift/wasmtests/pr2559.wat diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index 5016bb8abb..bb22a333ca 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -201,14 +201,26 @@ pub fn translate_operator( state.pop1(); } Operator::Select => { - let (arg1, arg2, cond) = state.pop3(); + let (mut arg1, mut arg2, cond) = state.pop3(); + if builder.func.dfg.value_type(arg1).is_vector() { + arg1 = optionally_bitcast_vector(arg1, I8X16, builder); + } + if builder.func.dfg.value_type(arg2).is_vector() { + arg2 = optionally_bitcast_vector(arg2, I8X16, builder); + } state.push1(builder.ins().select(cond, arg1, arg2)); } Operator::TypedSelect { ty: _ } => { // We ignore the explicit type parameter as it is only needed for // validation, which we require to have been performed before // translation. - let (arg1, arg2, cond) = state.pop3(); + let (mut arg1, mut arg2, cond) = state.pop3(); + if builder.func.dfg.value_type(arg1).is_vector() { + arg1 = optionally_bitcast_vector(arg1, I8X16, builder); + } + if builder.func.dfg.value_type(arg2).is_vector() { + arg2 = optionally_bitcast_vector(arg2, I8X16, builder); + } state.push1(builder.ins().select(cond, arg1, arg2)); } Operator::Nop => { diff --git a/cranelift/wasmtests/pr2559.wat b/cranelift/wasmtests/pr2559.wat new file mode 100644 index 0000000000..8cf50677c8 --- /dev/null +++ b/cranelift/wasmtests/pr2559.wat @@ -0,0 +1,51 @@ +(module + (type (;0;) (func (result v128 v128 v128))) + + (func $main1 (type 0) (result v128 v128 v128) + call $main1 + i8x16.add + call $main1 + i8x16.ge_u + i16x8.ne + i32.const 13 + br_if 0 (;@0;) + i32.const 43 + br_if 0 (;@0;) + i32.const 13 + br_if 0 (;@0;) + i32.const 87 + select + unreachable + i32.const 0 + br_if 0 (;@0;) + i32.const 13 + br_if 0 (;@0;) + i32.const 43 + br_if 0 (;@0;) + ) + (export "main1" (func $main1)) + + (func $main2 (type 0) (result v128 v128 v128) + call $main2 + i8x16.add + call $main2 + i8x16.ge_u + i16x8.ne + i32.const 13 + br_if 0 (;@0;) + i32.const 43 + br_if 0 (;@0;) + i32.const 13 + br_if 0 (;@0;) + i32.const 87 + select (result v128) + unreachable + i32.const 0 + br_if 0 (;@0;) + i32.const 13 + br_if 0 (;@0;) + i32.const 43 + br_if 0 (;@0;) + ) + (export "main2" (func $main2)) +) From 5ce6e009fc865ea57859011695306106a1ca467e Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Fri, 8 Jan 2021 08:55:27 -0800 Subject: [PATCH 08/14] Add Cargo.toml metadata to `peepmatic-test-operator` crate --- cranelift/peepmatic/crates/test-operator/Cargo.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cranelift/peepmatic/crates/test-operator/Cargo.toml b/cranelift/peepmatic/crates/test-operator/Cargo.toml index 66a5469b34..295610c1bc 100644 --- a/cranelift/peepmatic/crates/test-operator/Cargo.toml +++ b/cranelift/peepmatic/crates/test-operator/Cargo.toml @@ -1,5 +1,7 @@ [package] name = "peepmatic-test-operator" +description = "Operator for usage in peepmatic tests" +license = "Apache-2.0 WITH LLVM-exception" version = "0.69.0" authors = ["Nick Fitzgerald "] edition = "2018" From 3068d55fa1361a0e7d3151e4b12d0b06eb54c4f1 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Fri, 8 Jan 2021 08:57:52 -0800 Subject: [PATCH 09/14] wasi-nn: Fix keyword form in Cargo.toml metadata Keywords may not have spaces, apparently. --- crates/wasi-nn/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/wasi-nn/Cargo.toml b/crates/wasi-nn/Cargo.toml index e8067aba07..2e322f55e2 100644 --- a/crates/wasi-nn/Cargo.toml +++ b/crates/wasi-nn/Cargo.toml @@ -6,7 +6,7 @@ description = "Wasmtime implementation of the wasi-nn API" documentation = "https://docs.rs/wasmtime-wasi-nn" license = "Apache-2.0 WITH LLVM-exception" categories = ["wasm", "computer-vision"] -keywords = ["webassembly", "wasm", "neural network"] +keywords = ["webassembly", "wasm", "neural-network"] repository = "https://github.com/bytecodealliance/wasmtime" readme = "README.md" edition = "2018" From b4426be0729cbc372288dfabac4f11f3506472d8 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Fri, 8 Jan 2021 17:07:51 -0800 Subject: [PATCH 10/14] machinst lowering: update inst color when scanning across branch to allow more load-op merging. A branch is considered side-effecting and so updates the instruction color (which is our way of computing how far instructions can sink). However, in the lowering loop, we did not update current instruction color when scanning backward across branches, which are side-effecting. As a result, the color was stale and fewer load-op merges were permitted than are actually possible. Note that this would not have resulted in any correctness issues, as the stale color is too high (so no merges are permitted that should have been disallowed). Fixes #2562. --- cranelift/codegen/src/machinst/lower.rs | 10 ++++++---- cranelift/filetests/filetests/isa/x64/load-op.clif | 11 +++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/cranelift/codegen/src/machinst/lower.rs b/cranelift/codegen/src/machinst/lower.rs index 46b5fc1685..28e4edd0c7 100644 --- a/cranelift/codegen/src/machinst/lower.rs +++ b/cranelift/codegen/src/machinst/lower.rs @@ -683,10 +683,6 @@ impl<'func, I: VCodeInst> Lower<'func, I> { has_side_effect, value_needed, ); - // Skip lowering branches; these are handled separately. - if self.f.dfg[inst].opcode().is_branch() { - continue; - } // Update scan state to color prior to this inst (as we are scanning // backward). @@ -699,6 +695,12 @@ impl<'func, I: VCodeInst> Lower<'func, I> { self.cur_scan_entry_color = Some(entry_color); } + // Skip lowering branches; these are handled separately + // (see `lower_clif_branches()` below). + if self.f.dfg[inst].opcode().is_branch() { + continue; + } + // Normal instruction: codegen if the instruction is side-effecting // or any of its outputs its used. if has_side_effect || value_needed { diff --git a/cranelift/filetests/filetests/isa/x64/load-op.clif b/cranelift/filetests/filetests/isa/x64/load-op.clif index 6570a798c3..8fefaf6d42 100644 --- a/cranelift/filetests/filetests/isa/x64/load-op.clif +++ b/cranelift/filetests/filetests/isa/x64/load-op.clif @@ -59,3 +59,14 @@ block0(v0: i64, v1: i64): ; nextln: movq 0(%rax,%rdi,1), %rsi ; nextln: movq %rsi, %rax } + +function %merge_scalar_to_vector(i64) -> i32x4 { +block0(v0: i64): + v1 = load.i32 v0 + v2 = scalar_to_vector.i32x4 v1 + ; check: movss 0(%rdi), %xmm0 + + jump block1 +block1: + return v2 +} From 94467bcd9aa45980a6dac0add9a5d5c12a8f7b27 Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Mon, 11 Jan 2021 18:03:48 -0800 Subject: [PATCH 11/14] wiggle: bugfix, generated code should use Names::runtime_mod not wiggle as the crate from which these deps come. I worked around this in lucet, but I'll be able to revert that workaround. --- crates/wiggle/generate/src/funcs.rs | 4 ++-- crates/wiggle/generate/src/lib.rs | 2 +- crates/wiggle/generate/src/module_trait.rs | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/wiggle/generate/src/funcs.rs b/crates/wiggle/generate/src/funcs.rs index 5cdf59eda0..50f5eec640 100644 --- a/crates/wiggle/generate/src/funcs.rs +++ b/crates/wiggle/generate/src/funcs.rs @@ -169,7 +169,7 @@ pub fn define_func( let func_name = &func.name.as_str(); if func.noreturn { - quote!(pub fn #ident(#abi_args) -> Result<#abi_ret, wiggle::Trap> { + quote!(pub fn #ident(#abi_args) -> Result<#abi_ret, #rt::Trap> { let _span = #rt::tracing::span!( #rt::tracing::Level::TRACE, "wiggle abi", @@ -184,7 +184,7 @@ pub fn define_func( Err(trap) }) } else { - quote!(pub fn #ident(#abi_args) -> Result<#abi_ret, wiggle::Trap> { + quote!(pub fn #ident(#abi_args) -> Result<#abi_ret, #rt::Trap> { let _span = #rt::tracing::span!( #rt::tracing::Level::TRACE, "wiggle abi", diff --git a/crates/wiggle/generate/src/lib.rs b/crates/wiggle/generate/src/lib.rs index 1ba4b6ebae..b9a07f93ce 100644 --- a/crates/wiggle/generate/src/lib.rs +++ b/crates/wiggle/generate/src/lib.rs @@ -40,7 +40,7 @@ pub fn generate(doc: &witx::Document, names: &Names, errs: &ErrorTransform) -> T let abi_typename = names.type_ref(&errtype.abi_type(), anon_lifetime()); let user_typename = errtype.typename(); let methodname = names.user_error_conversion_method(&errtype); - quote!(fn #methodname(&self, e: super::#user_typename) -> Result<#abi_typename, wiggle::Trap>;) + quote!(fn #methodname(&self, e: super::#user_typename) -> Result<#abi_typename, #rt::Trap>;) }); let user_error_conversion = quote! { pub trait UserErrorConversion { diff --git a/crates/wiggle/generate/src/module_trait.rs b/crates/wiggle/generate/src/module_trait.rs index e42551a70b..a7a7289d09 100644 --- a/crates/wiggle/generate/src/module_trait.rs +++ b/crates/wiggle/generate/src/module_trait.rs @@ -69,7 +69,8 @@ pub fn define_module_trait(names: &Names, m: &Module, errxform: &ErrorTransform) .unwrap_or(quote!(())); quote!( Result<(#(#rets),*), #err> ) } else { - quote!(wiggle::Trap) + let rt = names.runtime_mod(); + quote!(#rt::Trap) }; if is_anonymous { From e2fb99af860174c5bf55d386381259fc8715958a Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Mon, 11 Jan 2021 18:04:43 -0800 Subject: [PATCH 12/14] wiggle: depend on bitflags, and re-export it. --- Cargo.lock | 1 + crates/wiggle/Cargo.toml | 1 + crates/wiggle/src/lib.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index ed60ae353a..c220f496b0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2815,6 +2815,7 @@ dependencies = [ name = "wiggle" version = "0.22.0" dependencies = [ + "bitflags", "proptest", "thiserror", "tracing", diff --git a/crates/wiggle/Cargo.toml b/crates/wiggle/Cargo.toml index 2255e8023d..3845fa71a6 100644 --- a/crates/wiggle/Cargo.toml +++ b/crates/wiggle/Cargo.toml @@ -15,6 +15,7 @@ thiserror = "1" witx = { path = "../wasi-common/WASI/tools/witx", version = "0.8.7", optional = true } wiggle-macro = { path = "macro", version = "0.22.0" } tracing = "0.1.15" +bitflags = "1.2" [badges] maintenance = { status = "actively-developed" } diff --git a/crates/wiggle/src/lib.rs b/crates/wiggle/src/lib.rs index 102bcb0e7b..4ecbf27da5 100644 --- a/crates/wiggle/src/lib.rs +++ b/crates/wiggle/src/lib.rs @@ -6,6 +6,7 @@ use std::slice; use std::str; use std::sync::Arc; +pub use bitflags; pub use wiggle_macro::from_witx; #[cfg(feature = "wiggle_metadata")] From ed44a19e5ef7c0fa056b2104aee3a195c3375b2d Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Mon, 11 Jan 2021 18:20:57 -0800 Subject: [PATCH 13/14] wiggle: use bitflags to generate flags more consistient with the rest of the ecosystem. --- crates/wiggle/generate/src/types/flags.rs | 93 +++-------------------- crates/wiggle/tests/flags.rs | 8 +- 2 files changed, 15 insertions(+), 86 deletions(-) diff --git a/crates/wiggle/generate/src/types/flags.rs b/crates/wiggle/generate/src/types/flags.rs index f87261abca..cda13a2bfa 100644 --- a/crates/wiggle/generate/src/types/flags.rs +++ b/crates/wiggle/generate/src/types/flags.rs @@ -27,102 +27,31 @@ pub(super) fn define_flags(names: &Names, name: &witx::Id, f: &witx::FlagsDataty } quote! { - #[repr(transparent)] - #[derive(Copy, Clone, Debug, ::std::hash::Hash, Eq, PartialEq)] - pub struct #ident(#repr); - - impl #ident { - #(pub const #names_: #ident = #ident(#values_);)* - - #[inline] - pub const fn empty() -> Self { - #ident(0) - } - - #[inline] - pub const fn all() -> Self { - #ident(#(#values_)|*) - } - - #[inline] - pub fn contains(&self, other: &#ident) -> bool { - !*self & *other == Self::empty() + #rt::bitflags::bitflags! { + pub struct #ident: #repr { + #(const #names_ = #values_;)* } } impl ::std::fmt::Display for #ident { fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result { - let mut first = true; - #( - if self.0 & #values_ == #values_ { - if !first { - f.write_str("|")?; - } - first = false; - f.write_fmt(format_args!("{}", stringify!(#names_).to_lowercase()))?; - } - )* - if first { - f.write_str("empty")?; - } - f.write_fmt(format_args!(" ({:#x})", self.0))?; + f.write_str(stringify!(#ident))?; + f.write_str("(")?; + ::std::fmt::Debug::fmt(self, f)?; + f.write_str(" (0x")?; + ::std::fmt::LowerHex::fmt(&self.bits, f)?; + f.write_str("))")?; Ok(()) } } - impl ::std::ops::BitAnd for #ident { - type Output = Self; - fn bitand(self, rhs: Self) -> Self::Output { - #ident(self.0 & rhs.0) - } - } - - impl ::std::ops::BitAndAssign for #ident { - fn bitand_assign(&mut self, rhs: Self) { - *self = *self & rhs - } - } - - impl ::std::ops::BitOr for #ident { - type Output = Self; - fn bitor(self, rhs: Self) -> Self::Output { - #ident(self.0 | rhs.0) - } - } - - impl ::std::ops::BitOrAssign for #ident { - fn bitor_assign(&mut self, rhs: Self) { - *self = *self | rhs - } - } - - impl ::std::ops::BitXor for #ident { - type Output = Self; - fn bitxor(self, rhs: Self) -> Self::Output { - #ident(self.0 ^ rhs.0) - } - } - - impl ::std::ops::BitXorAssign for #ident { - fn bitxor_assign(&mut self, rhs: Self) { - *self = *self ^ rhs - } - } - - impl ::std::ops::Not for #ident { - type Output = Self; - fn not(self) -> Self::Output { - #ident(!self.0) - } - } - impl ::std::convert::TryFrom<#repr> for #ident { type Error = #rt::GuestError; fn try_from(value: #repr) -> Result { if #repr::from(!#ident::all()) & value != 0 { Err(#rt::GuestError::InvalidFlagValue(stringify!(#ident))) } else { - Ok(#ident(value)) + Ok(#ident { bits: value }) } } } @@ -136,7 +65,7 @@ pub(super) fn define_flags(names: &Names, name: &witx::Id, f: &witx::FlagsDataty impl From<#ident> for #repr { fn from(e: #ident) -> #repr { - e.0 + e.bits } } diff --git a/crates/wiggle/tests/flags.rs b/crates/wiggle/tests/flags.rs index e12b64a457..1a2bb0019f 100644 --- a/crates/wiggle/tests/flags.rs +++ b/crates/wiggle/tests/flags.rs @@ -103,11 +103,11 @@ proptest! { #[test] fn flags_fmt() { let empty = format!("{}", types::CarConfig::empty()); - assert_eq!(empty, "empty (0x0)"); + assert_eq!(empty, "CarConfig((empty) (0x0))"); let one_flag = format!("{}", types::CarConfig::AWD); - assert_eq!(one_flag, "awd (0x2)"); + assert_eq!(one_flag, "CarConfig(AWD (0x2))"); let two_flags = format!("{}", types::CarConfig::AUTOMATIC | types::CarConfig::SUV); - assert_eq!(two_flags, "automatic|suv (0x5)"); + assert_eq!(two_flags, "CarConfig(AUTOMATIC | SUV (0x5))"); let all = format!("{}", types::CarConfig::all()); - assert_eq!(all, "automatic|awd|suv (0x7)"); + assert_eq!(all, "CarConfig(AUTOMATIC | AWD | SUV (0x7))"); } From 75a9dc7fe2b91e914b9c7a1757b775640bcbf50c Mon Sep 17 00:00:00 2001 From: Pat Hickey Date: Mon, 11 Jan 2021 18:27:43 -0800 Subject: [PATCH 14/14] wasi-common: wiggle flags are now bitflags! this mostly mechanical change is just getting rid of passing to `contains` by reference. --- crates/wasi-common/src/entry.rs | 4 +- crates/wasi-common/src/handle.rs | 6 +- crates/wasi-common/src/path.rs | 4 +- .../src/snapshots/wasi_snapshot_preview1.rs | 68 +++++++++---------- crates/wasi-common/src/sys/fd.rs | 8 +-- crates/wasi-common/src/sys/unix/mod.rs | 18 ++--- crates/wasi-common/src/sys/unix/path.rs | 8 +-- crates/wasi-common/src/virtfs.rs | 8 +-- 8 files changed, 62 insertions(+), 62 deletions(-) diff --git a/crates/wasi-common/src/entry.rs b/crates/wasi-common/src/entry.rs index a162b19f42..a8a5accaf0 100644 --- a/crates/wasi-common/src/entry.rs +++ b/crates/wasi-common/src/entry.rs @@ -78,7 +78,7 @@ impl Entry { /// `HandleRights` structure is a subset of rights attached to this `Entry`. The check is /// performed using `Entry::validate_rights` method. If the check fails, `Error::Notcapable` /// is returned. - pub(crate) fn as_handle(&self, rights: &HandleRights) -> Result { + pub(crate) fn as_handle(&self, rights: HandleRights) -> Result { self.validate_rights(rights)?; Ok(self.handle.get()) } @@ -87,7 +87,7 @@ impl Entry { /// rights attached to this `Entry` object are a superset. /// /// Upon unsuccessful check, `Error::Notcapable` is returned. - pub(crate) fn validate_rights(&self, rights: &HandleRights) -> Result<()> { + pub(crate) fn validate_rights(&self, rights: HandleRights) -> Result<()> { let this_rights = self.handle.get_rights(); if this_rights.contains(rights) { Ok(()) diff --git a/crates/wasi-common/src/handle.rs b/crates/wasi-common/src/handle.rs index ccb549703c..9bef53eacc 100644 --- a/crates/wasi-common/src/handle.rs +++ b/crates/wasi-common/src/handle.rs @@ -39,8 +39,8 @@ impl HandleRights { } /// Checks if `other` is a subset of those rights. - pub fn contains(&self, other: &Self) -> bool { - self.base.contains(&other.base) && self.inheriting.contains(&other.inheriting) + pub fn contains(&self, other: Self) -> bool { + self.base.contains(other.base) && self.inheriting.contains(other.inheriting) } /// Returns base rights. @@ -100,7 +100,7 @@ pub trait Handle { let file_type = self.get_file_type(); let rights = self.get_rights(); let required_rights = HandleRights::from_base(Rights::FD_SEEK | Rights::FD_TELL); - file_type == Filetype::CharacterDevice && rights.contains(&required_rights) + file_type == Filetype::CharacterDevice && rights.contains(required_rights) } // TODO perhaps should be a separate trait? // FdOps diff --git a/crates/wasi-common/src/path.rs b/crates/wasi-common/src/path.rs index b2e4b19ba0..df04e48f41 100644 --- a/crates/wasi-common/src/path.rs +++ b/crates/wasi-common/src/path.rs @@ -11,7 +11,7 @@ pub(crate) use crate::sys::path::{from_host, open_rights}; /// This is a workaround for not having Capsicum support in the OS. pub(crate) fn get( entry: &Entry, - required_rights: &HandleRights, + required_rights: HandleRights, dirflags: Lookupflags, path: &str, needs_final_component: bool, @@ -140,7 +140,7 @@ pub(crate) fn get( } continue; - } else if ends_with_slash || dirflags.contains(&Lookupflags::SYMLINK_FOLLOW) + } else if ends_with_slash || dirflags.contains(Lookupflags::SYMLINK_FOLLOW) { // if there's a trailing slash, or if `LOOKUP_SYMLINK_FOLLOW` is set, attempt // symlink expansion diff --git a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs index 8bb41a903a..794be79c9f 100644 --- a/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs +++ b/crates/wasi-common/src/snapshots/wasi_snapshot_preview1.rs @@ -60,7 +60,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let required_rights = HandleRights::from_base(types::Rights::FD_ADVISE); let entry = self.get_entry(fd)?; entry - .as_handle(&required_rights)? + .as_handle(required_rights)? .advise(advice, offset, len) } @@ -72,7 +72,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { ) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::FD_ALLOCATE); let entry = self.get_entry(fd)?; - entry.as_handle(&required_rights)?.allocate(offset, len) + entry.as_handle(required_rights)?.allocate(offset, len) } fn fd_close(&self, fd: types::Fd) -> Result<()> { @@ -89,12 +89,12 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { fn fd_datasync(&self, fd: types::Fd) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::FD_DATASYNC); let entry = self.get_entry(fd)?; - entry.as_handle(&required_rights)?.datasync() + entry.as_handle(required_rights)?.datasync() } fn fd_fdstat_get(&self, fd: types::Fd) -> Result { let entry = self.get_entry(fd)?; - let file = entry.as_handle(&HandleRights::empty())?; + let file = entry.as_handle(HandleRights::empty())?; let fs_flags = file.fdstat_get()?; let rights = entry.get_rights(); let fdstat = types::Fdstat { @@ -109,7 +109,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { fn fd_fdstat_set_flags(&self, fd: types::Fd, flags: types::Fdflags) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::FD_FDSTAT_SET_FLAGS); let entry = self.get_entry(fd)?; - entry.as_handle(&required_rights)?.fdstat_set_flags(flags) + entry.as_handle(required_rights)?.fdstat_set_flags(flags) } fn fd_fdstat_set_rights( @@ -120,7 +120,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { ) -> Result<()> { let rights = HandleRights::new(fs_rights_base, fs_rights_inheriting); let entry = self.get_entry(fd)?; - if !entry.get_rights().contains(&rights) { + if !entry.get_rights().contains(rights) { return Err(Error::Notcapable); } entry.set_rights(rights); @@ -130,14 +130,14 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { fn fd_filestat_get(&self, fd: types::Fd) -> Result { let required_rights = HandleRights::from_base(types::Rights::FD_FILESTAT_GET); let entry = self.get_entry(fd)?; - let host_filestat = entry.as_handle(&required_rights)?.filestat_get()?; + let host_filestat = entry.as_handle(required_rights)?.filestat_get()?; Ok(host_filestat) } fn fd_filestat_set_size(&self, fd: types::Fd, size: types::Filesize) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::FD_FILESTAT_SET_SIZE); let entry = self.get_entry(fd)?; - entry.as_handle(&required_rights)?.filestat_set_size(size) + entry.as_handle(required_rights)?.filestat_set_size(size) } fn fd_filestat_set_times( @@ -150,7 +150,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let required_rights = HandleRights::from_base(types::Rights::FD_FILESTAT_SET_TIMES); let entry = self.get_entry(fd)?; entry - .as_handle(&required_rights)? + .as_handle(required_rights)? .filestat_set_times(atim, mtim, fst_flags) } @@ -180,7 +180,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { .map(|s| io::IoSliceMut::new(&mut *s)) .collect::>>(); entry - .as_handle(&required_rights)? + .as_handle(required_rights)? .preadv(&mut buf, offset)? .try_into()? }; @@ -255,7 +255,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let buf: Vec = guest_slices.iter().map(|s| io::IoSlice::new(&*s)).collect(); entry - .as_handle(&required_rights)? + .as_handle(required_rights)? .pwritev(&buf, offset)? .try_into()? }; @@ -278,7 +278,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { .map(|s| io::IoSliceMut::new(&mut *s)) .collect(); entry - .as_handle(&required_rights)? + .as_handle(required_rights)? .read_vectored(&mut slices)? .try_into()? }; @@ -298,7 +298,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let mut bufused = 0; let mut buf = buf.clone(); - for pair in entry.as_handle(&required_rights)?.readdir(cookie)? { + for pair in entry.as_handle(required_rights)?.readdir(cookie)? { let (dirent, name) = pair?; let dirent_raw = dirent.as_bytes()?; let dirent_len: types::Size = dirent_raw.len().try_into()?; @@ -379,21 +379,21 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { types::Whence::End => SeekFrom::End(offset), types::Whence::Set => SeekFrom::Start(offset as u64), }; - let host_newoffset = entry.as_handle(&required_rights)?.seek(pos)?; + let host_newoffset = entry.as_handle(required_rights)?.seek(pos)?; Ok(host_newoffset) } fn fd_sync(&self, fd: types::Fd) -> Result<()> { let required_rights = HandleRights::from_base(types::Rights::FD_SYNC); let entry = self.get_entry(fd)?; - entry.as_handle(&required_rights)?.sync() + entry.as_handle(required_rights)?.sync() } fn fd_tell(&self, fd: types::Fd) -> Result { let required_rights = HandleRights::from_base(types::Rights::FD_TELL); let entry = self.get_entry(fd)?; let host_offset = entry - .as_handle(&required_rights)? + .as_handle(required_rights)? .seek(SeekFrom::Current(0))?; Ok(host_offset) } @@ -411,7 +411,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let slices: Vec = guest_slices.iter().map(|s| io::IoSlice::new(&*s)).collect(); entry - .as_handle(&required_rights)? + .as_handle(required_rights)? .write_vectored(&slices)? .try_into()? }; @@ -426,7 +426,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let path = path.as_str()?; let (dirfd, path) = path::get( &entry, - &required_rights, + required_rights, types::Lookupflags::empty(), path.deref(), false, @@ -443,9 +443,9 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let required_rights = HandleRights::from_base(types::Rights::PATH_FILESTAT_GET); let entry = self.get_entry(dirfd)?; let path = path.as_str()?; - let (dirfd, path) = path::get(&entry, &required_rights, flags, path.deref(), false)?; + let (dirfd, path) = path::get(&entry, required_rights, flags, path.deref(), false)?; let host_filestat = - dirfd.filestat_get_at(&path, flags.contains(&types::Lookupflags::SYMLINK_FOLLOW))?; + dirfd.filestat_get_at(&path, flags.contains(types::Lookupflags::SYMLINK_FOLLOW))?; Ok(host_filestat) } @@ -461,13 +461,13 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let required_rights = HandleRights::from_base(types::Rights::PATH_FILESTAT_SET_TIMES); let entry = self.get_entry(dirfd)?; let path = path.as_str()?; - let (dirfd, path) = path::get(&entry, &required_rights, flags, path.deref(), false)?; + let (dirfd, path) = path::get(&entry, required_rights, flags, path.deref(), false)?; dirfd.filestat_set_times_at( &path, atim, mtim, fst_flags, - flags.contains(&types::Lookupflags::SYMLINK_FOLLOW), + flags.contains(types::Lookupflags::SYMLINK_FOLLOW), )?; Ok(()) } @@ -487,7 +487,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let old_path = old_path.as_str()?; path::get( &old_entry, - &required_rights, + required_rights, types::Lookupflags::empty(), old_path.deref(), false, @@ -500,7 +500,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let new_path = new_path.as_str()?; path::get( &new_entry, - &required_rights, + required_rights, types::Lookupflags::empty(), new_path.deref(), false, @@ -510,7 +510,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { &old_path, new_dirfd, &new_path, - old_flags.contains(&types::Lookupflags::SYMLINK_FOLLOW), + old_flags.contains(types::Lookupflags::SYMLINK_FOLLOW), ) } @@ -535,7 +535,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let path = path.as_str()?; path::get( &entry, - &needed_rights, + needed_rights, dirflags, path.deref(), oflags & types::Oflags::CREAT != types::Oflags::empty(), @@ -581,7 +581,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let path = path.as_str()?; path::get( &entry, - &required_rights, + required_rights, types::Lookupflags::empty(), path.deref(), false, @@ -599,7 +599,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let path = path.as_str()?; path::get( &entry, - &required_rights, + required_rights, types::Lookupflags::empty(), path.deref(), true, @@ -621,7 +621,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let old_path = old_path.as_str()?; path::get( &entry, - &required_rights, + required_rights, types::Lookupflags::empty(), old_path.deref(), true, @@ -633,7 +633,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let new_path = new_path.as_str()?; path::get( &entry, - &required_rights, + required_rights, types::Lookupflags::empty(), new_path.deref(), true, @@ -654,7 +654,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let new_path = new_path.as_str()?; path::get( &entry, - &required_rights, + required_rights, types::Lookupflags::empty(), new_path.deref(), true, @@ -672,7 +672,7 @@ impl<'a> WasiSnapshotPreview1 for WasiCtx { let path = path.as_str()?; path::get( &entry, - &required_rights, + required_rights, types::Lookupflags::empty(), path.deref(), false, @@ -815,7 +815,7 @@ impl WasiCtx { } }; fd_events.push(sched::FdEventData { - handle: entry.as_handle(&required_rights)?, + handle: entry.as_handle(required_rights)?, r#type: types::Eventtype::FdRead, userdata: subscription.userdata, }); @@ -841,7 +841,7 @@ impl WasiCtx { } }; fd_events.push(sched::FdEventData { - handle: entry.as_handle(&required_rights)?, + handle: entry.as_handle(required_rights)?, r#type: types::Eventtype::FdWrite, userdata: subscription.userdata, }); diff --git a/crates/wasi-common/src/sys/fd.rs b/crates/wasi-common/src/sys/fd.rs index 8603c22a67..bcee2956bc 100644 --- a/crates/wasi-common/src/sys/fd.rs +++ b/crates/wasi-common/src/sys/fd.rs @@ -12,10 +12,10 @@ pub(crate) fn filestat_set_times( st_mtim: Timestamp, fst_flags: Fstflags, ) -> Result<()> { - let set_atim = fst_flags.contains(&Fstflags::ATIM); - let set_atim_now = fst_flags.contains(&Fstflags::ATIM_NOW); - let set_mtim = fst_flags.contains(&Fstflags::MTIM); - let set_mtim_now = fst_flags.contains(&Fstflags::MTIM_NOW); + let set_atim = fst_flags.contains(Fstflags::ATIM); + let set_atim_now = fst_flags.contains(Fstflags::ATIM_NOW); + let set_mtim = fst_flags.contains(Fstflags::MTIM); + let set_mtim_now = fst_flags.contains(Fstflags::MTIM_NOW); if (set_atim && set_atim_now) || (set_mtim && set_mtim_now) { return Err(Error::Inval); diff --git a/crates/wasi-common/src/sys/unix/mod.rs b/crates/wasi-common/src/sys/unix/mod.rs index 84d8ec581e..9c7fd5ece8 100644 --- a/crates/wasi-common/src/sys/unix/mod.rs +++ b/crates/wasi-common/src/sys/unix/mod.rs @@ -147,19 +147,19 @@ impl From for ClockId { impl From for OFlags { fn from(fdflags: Fdflags) -> Self { let mut nix_flags = Self::empty(); - if fdflags.contains(&Fdflags::APPEND) { + if fdflags.contains(Fdflags::APPEND) { nix_flags.insert(Self::APPEND); } - if fdflags.contains(&Fdflags::DSYNC) { + if fdflags.contains(Fdflags::DSYNC) { nix_flags.insert(Self::DSYNC); } - if fdflags.contains(&Fdflags::NONBLOCK) { + if fdflags.contains(Fdflags::NONBLOCK) { nix_flags.insert(Self::NONBLOCK); } - if fdflags.contains(&Fdflags::RSYNC) { + if fdflags.contains(Fdflags::RSYNC) { nix_flags.insert(O_RSYNC); } - if fdflags.contains(&Fdflags::SYNC) { + if fdflags.contains(Fdflags::SYNC) { nix_flags.insert(Self::SYNC); } nix_flags @@ -191,16 +191,16 @@ impl From for Fdflags { impl From for OFlags { fn from(oflags: Oflags) -> Self { let mut nix_flags = Self::empty(); - if oflags.contains(&Oflags::CREAT) { + if oflags.contains(Oflags::CREAT) { nix_flags.insert(Self::CREAT); } - if oflags.contains(&Oflags::DIRECTORY) { + if oflags.contains(Oflags::DIRECTORY) { nix_flags.insert(Self::DIRECTORY); } - if oflags.contains(&Oflags::EXCL) { + if oflags.contains(Oflags::EXCL) { nix_flags.insert(Self::EXCL); } - if oflags.contains(&Oflags::TRUNC) { + if oflags.contains(Oflags::TRUNC) { nix_flags.insert(Self::TRUNC); } nix_flags diff --git a/crates/wasi-common/src/sys/unix/path.rs b/crates/wasi-common/src/sys/unix/path.rs index bbe084d32b..d952790eeb 100644 --- a/crates/wasi-common/src/sys/unix/path.rs +++ b/crates/wasi-common/src/sys/unix/path.rs @@ -239,10 +239,10 @@ pub(crate) fn filestat_set_times_at( use std::time::{Duration, UNIX_EPOCH}; use yanix::filetime::*; - let set_atim = fst_flags.contains(&Fstflags::ATIM); - let set_atim_now = fst_flags.contains(&Fstflags::ATIM_NOW); - let set_mtim = fst_flags.contains(&Fstflags::MTIM); - let set_mtim_now = fst_flags.contains(&Fstflags::MTIM_NOW); + let set_atim = fst_flags.contains(Fstflags::ATIM); + let set_atim_now = fst_flags.contains(Fstflags::ATIM_NOW); + let set_mtim = fst_flags.contains(Fstflags::MTIM); + let set_mtim_now = fst_flags.contains(Fstflags::MTIM_NOW); if (set_atim && set_atim_now) || (set_mtim && set_mtim_now) { return Err(Error::Inval); diff --git a/crates/wasi-common/src/virtfs.rs b/crates/wasi-common/src/virtfs.rs index dae91de485..a219cff719 100644 --- a/crates/wasi-common/src/virtfs.rs +++ b/crates/wasi-common/src/virtfs.rs @@ -299,7 +299,7 @@ impl Handle for InMemoryFile { trace!("write_vectored(iovs={:?})", iovs); let mut data = self.data.borrow_mut(); - let append_mode = self.fd_flags.get().contains(&Fdflags::APPEND); + let append_mode = self.fd_flags.get().contains(Fdflags::APPEND); trace!(" | fd_flags={}", self.fd_flags.get()); // If this file is in append mode, we write to the end. @@ -353,7 +353,7 @@ impl Handle for InMemoryFile { oflags: Oflags, _fd_flags: Fdflags, ) -> Result> { - if oflags.contains(&Oflags::DIRECTORY) { + if oflags.contains(Oflags::DIRECTORY) { tracing::trace!( "InMemoryFile::openat was passed oflags DIRECTORY, but {:?} is a file.", path @@ -652,7 +652,7 @@ impl Handle for VirtualDir { return Err(Error::Exist); } - if oflags.contains(&Oflags::DIRECTORY) + if oflags.contains(Oflags::DIRECTORY) && e.get().get_file_type() != Filetype::Directory { tracing::trace!( @@ -665,7 +665,7 @@ impl Handle for VirtualDir { e.get().try_clone().map_err(Into::into) } Entry::Vacant(v) => { - if oflags.contains(&Oflags::CREAT) { + if oflags.contains(Oflags::CREAT) { if self.writable { // Enforce a hard limit at `u32::MAX - 2` files. // This is to have a constant limit (rather than target-dependent limit we