From bf041e3ae2ee2080fa54e79ac2842c1f6ff7f65a Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Fri, 5 Oct 2018 06:43:22 -0700 Subject: [PATCH] Move `return_at_end` out of Settings and into the wasm FuncEnvironment. (#547) * Move `return_at_end` out of Settings and into the wasm FuncEnvironment. The `return_at_end` flag supports users that want to append a custom epilogue to Cranelift-produced functions. It arranges for functions to always return via a single return statement at the end, and users are expected to remove this return to append their code. This patch makes two changes: - First, introduce a `fallthrough_return` instruction and use that instead of adding a `return` at the end. That's simpler than having users remove the `return` themselves. - Second, move this setting out of the Settings and into the wasm FuncEnvironment. This flag isn't something the code generator uses, it's something that the wasm translator uses. The code generator needs to preserve the property, however we can give the `fallthrough_return` instruction properties to ensure this as needed, such as marking it non-cloneable. --- .../filetests/verifier/return_at_end.clif | 22 ----------- cranelift/src/wasm.rs | 11 ++++-- ..._at_end.wat => use_fallthrough_return.wat} | 0 lib/codegen/meta-python/base/instructions.py | 10 +++++ lib/codegen/meta-python/base/settings.py | 10 ----- lib/codegen/src/settings.rs | 1 - lib/codegen/src/verifier/mod.rs | 30 ++------------- lib/wasm/src/code_translator.rs | 11 +++--- lib/wasm/src/environ/dummy.rs | 37 +++++++++++++++---- lib/wasm/src/environ/mod.rs | 2 +- lib/wasm/src/environ/spec.rs | 14 +++++++ lib/wasm/src/func_translator.rs | 7 +++- lib/wasm/src/lib.rs | 3 +- lib/wasm/tests/wasm_testsuite.rs | 24 +++++++----- 14 files changed, 91 insertions(+), 91 deletions(-) delete mode 100644 cranelift/filetests/verifier/return_at_end.clif rename cranelift/wasmtests/{return_at_end.wat => use_fallthrough_return.wat} (100%) diff --git a/cranelift/filetests/verifier/return_at_end.clif b/cranelift/filetests/verifier/return_at_end.clif deleted file mode 100644 index fcbae57070..0000000000 --- a/cranelift/filetests/verifier/return_at_end.clif +++ /dev/null @@ -1,22 +0,0 @@ -test verifier -set return_at_end - -function %ok(i32) { -ebb0(v0: i32): - brnz v0, ebb1 - trap int_divz - -ebb1: - trapz v0, user5 - return -} - -function %bad(i32) { -ebb0(v0: i32): - brnz v0, ebb1 - return ; error: Internal return not allowed - -ebb1: - trapz v0, user6 - return -} diff --git a/cranelift/src/wasm.rs b/cranelift/src/wasm.rs index 11b859d52b..3e91c3e7b0 100644 --- a/cranelift/src/wasm.rs +++ b/cranelift/src/wasm.rs @@ -11,7 +11,9 @@ use cranelift_codegen::print_errors::{pretty_error, pretty_verifier_error}; use cranelift_codegen::settings::FlagsOrIsa; use cranelift_codegen::Context; use cranelift_entity::EntityRef; -use cranelift_wasm::{translate_module, DummyEnvironment, FuncIndex, ModuleEnvironment}; +use cranelift_wasm::{ + translate_module, DummyEnvironment, FuncIndex, ModuleEnvironment, ReturnMode, +}; use std::error::Error; use std::path::Path; use std::path::PathBuf; @@ -102,8 +104,11 @@ fn handle_module( } }; - let mut dummy_environ = - DummyEnvironment::with_triple_flags(isa.triple().clone(), fisa.flags.clone()); + let mut dummy_environ = DummyEnvironment::with_triple_flags( + isa.triple().clone(), + fisa.flags.clone(), + ReturnMode::NormalReturns, + ); translate_module(&module_binary, &mut dummy_environ).map_err(|e| e.to_string())?; let _ = terminal.fg(term::color::GREEN); diff --git a/cranelift/wasmtests/return_at_end.wat b/cranelift/wasmtests/use_fallthrough_return.wat similarity index 100% rename from cranelift/wasmtests/return_at_end.wat rename to cranelift/wasmtests/use_fallthrough_return.wat diff --git a/lib/codegen/meta-python/base/instructions.py b/lib/codegen/meta-python/base/instructions.py index c22b6b9b77..2993d6de49 100644 --- a/lib/codegen/meta-python/base/instructions.py +++ b/lib/codegen/meta-python/base/instructions.py @@ -234,6 +234,16 @@ x_return = Instruction( """, ins=rvals, is_return=True, is_terminator=True) +fallthrough_return = Instruction( + 'fallthrough_return', r""" + Return from the function by fallthrough. + + This is a specialized instruction for use where one wants to append + a custom epilogue, which will then perform the real return. This + instruction has no encoding. + """, + ins=rvals, is_return=True, is_terminator=True) + FN = Operand( 'FN', entities.func_ref, diff --git a/lib/codegen/meta-python/base/settings.py b/lib/codegen/meta-python/base/settings.py index e43e455693..4eb7541840 100644 --- a/lib/codegen/meta-python/base/settings.py +++ b/lib/codegen/meta-python/base/settings.py @@ -65,16 +65,6 @@ colocated_libcalls = BoolSetting( they can use more efficient addressing. """) -return_at_end = BoolSetting( - """ - Generate functions with at most a single return instruction at the - end of the function. - - This guarantees that functions do not have any internal return - instructions. Either they never return, or they have a single return - instruction at the end. - """) - avoid_div_traps = BoolSetting( """ Generate explicit checks around native division instructions to avoid diff --git a/lib/codegen/src/settings.rs b/lib/codegen/src/settings.rs index 743bd29268..c9374d9e47 100644 --- a/lib/codegen/src/settings.rs +++ b/lib/codegen/src/settings.rs @@ -370,7 +370,6 @@ mod tests { call_conv = \"fast\"\n\ is_pic = false\n\ colocated_libcalls = false\n\ - return_at_end = false\n\ avoid_div_traps = false\n\ enable_float = true\n\ enable_nan_canonicalization = false\n\ diff --git a/lib/codegen/src/verifier/mod.rs b/lib/codegen/src/verifier/mod.rs index 2daa1d5440..ba396a8b10 100644 --- a/lib/codegen/src/verifier/mod.rs +++ b/lib/codegen/src/verifier/mod.rs @@ -307,7 +307,6 @@ struct Verifier<'a> { func: &'a Function, expected_cfg: ControlFlowGraph, expected_domtree: DominatorTree, - flags: &'a Flags, isa: Option<&'a TargetIsa>, } @@ -319,7 +318,6 @@ impl<'a> Verifier<'a> { func, expected_cfg, expected_domtree, - flags: fisa.flags, isa: fisa.isa, } } @@ -1654,9 +1652,9 @@ impl<'a> Verifier<'a> { // Instructions with side effects are not allowed to be ghost instructions. let opcode = self.func.dfg[inst].opcode(); - // The `fallthrough` instruction is marked as a terminator and a branch, but it is not - // required to have an encoding. - if opcode == Opcode::Fallthrough { + // The `fallthrough` and `fallthrough_return` instructions are marked as terminators and + // branches, but they are not required to have an encoding. + if opcode == Opcode::Fallthrough || opcode == Opcode::FallthroughReturn { return Ok(()); } @@ -1696,24 +1694,6 @@ impl<'a> Verifier<'a> { Ok(()) } - /// Verify the `return_at_end` property which requires that there are no internal return - /// instructions. - fn verify_return_at_end(&self, errors: &mut VerifierErrors) -> VerifierStepResult<()> { - for ebb in self.func.layout.ebbs() { - let inst = self.func.layout.last_inst(ebb).unwrap(); - if self.func.dfg[inst].opcode().is_return() && Some(ebb) != self.func.layout.last_ebb() - { - report!( - errors, - inst, - "Internal return not allowed with return_at_end=1" - ); - } - } - - errors.as_result() - } - pub fn run(&self, errors: &mut VerifierErrors) -> VerifierStepResult<()> { self.verify_global_values(errors)?; self.verify_heaps(errors)?; @@ -1729,10 +1709,6 @@ impl<'a> Verifier<'a> { } } - if self.flags.return_at_end() { - self.verify_return_at_end(errors)?; - } - verify_flags(self.func, &self.expected_cfg, self.isa, errors)?; Ok(()) diff --git a/lib/wasm/src/code_translator.rs b/lib/wasm/src/code_translator.rs index 3c615568ca..152c20ca20 100644 --- a/lib/wasm/src/code_translator.rs +++ b/lib/wasm/src/code_translator.rs @@ -28,7 +28,7 @@ use cranelift_codegen::ir::{self, InstBuilder, JumpTableData, MemFlags}; use cranelift_codegen::packed_option::ReservedValue; use cranelift_entity::EntityRef; use cranelift_frontend::{FunctionBuilder, Variable}; -use environ::{FuncEnvironment, GlobalVariable, WasmError, WasmResult}; +use environ::{FuncEnvironment, GlobalVariable, ReturnMode, WasmError, WasmResult}; use state::{ControlStackFrame, TranslationState}; use std::collections::{hash_map, HashMap}; use std::vec::Vec; @@ -340,11 +340,10 @@ pub fn translate_operator( }; { let args = state.peekn(return_count); - if environ.flags().return_at_end() { - builder.ins().jump(br_destination, args); - } else { - builder.ins().return_(args); - } + match environ.return_mode() { + ReturnMode::NormalReturns => builder.ins().return_(args), + ReturnMode::FallthroughReturn => builder.ins().jump(br_destination, args), + }; } state.popn(return_count); state.reachable = false; diff --git a/lib/wasm/src/environ/dummy.rs b/lib/wasm/src/environ/dummy.rs index d1e6a8052c..ae0edb96e3 100644 --- a/lib/wasm/src/environ/dummy.rs +++ b/lib/wasm/src/environ/dummy.rs @@ -7,7 +7,7 @@ use cranelift_codegen::ir::types::*; use cranelift_codegen::ir::{self, InstBuilder}; use cranelift_codegen::settings; use cranelift_entity::{EntityRef, PrimaryMap}; -use environ::{FuncEnvironment, GlobalVariable, ModuleEnvironment, WasmResult}; +use environ::{FuncEnvironment, GlobalVariable, ModuleEnvironment, ReturnMode, WasmResult}; use func_translator::FuncTranslator; use std::string::String; use std::vec::Vec; @@ -105,38 +105,55 @@ pub struct DummyEnvironment { /// Vector of wasm bytecode size for each function. pub func_bytecode_sizes: Vec, + + /// How to return from functions. + return_mode: ReturnMode, } impl DummyEnvironment { /// Allocates the data structures with default flags. pub fn with_triple(triple: Triple) -> Self { - Self::with_triple_flags(triple, settings::Flags::new(settings::builder())) + Self::with_triple_flags( + triple, + settings::Flags::new(settings::builder()), + ReturnMode::NormalReturns, + ) } - /// Allocates the data structures with the given flags. - pub fn with_triple_flags(triple: Triple, flags: settings::Flags) -> Self { + /// Allocates the data structures with the given triple. + pub fn with_triple_flags( + triple: Triple, + flags: settings::Flags, + return_mode: ReturnMode, + ) -> Self { Self { info: DummyModuleInfo::with_triple_flags(triple, flags), trans: FuncTranslator::new(), func_bytecode_sizes: Vec::new(), + return_mode, } } /// Return a `DummyFuncEnvironment` for translating functions within this /// `DummyEnvironment`. pub fn func_env(&self) -> DummyFuncEnvironment { - DummyFuncEnvironment::new(&self.info) + DummyFuncEnvironment::new(&self.info, self.return_mode) } } /// The `FuncEnvironment` implementation for use by the `DummyEnvironment`. pub struct DummyFuncEnvironment<'dummy_environment> { pub mod_info: &'dummy_environment DummyModuleInfo, + + return_mode: ReturnMode, } impl<'dummy_environment> DummyFuncEnvironment<'dummy_environment> { - pub fn new(mod_info: &'dummy_environment DummyModuleInfo) -> Self { - Self { mod_info } + pub fn new(mod_info: &'dummy_environment DummyModuleInfo, return_mode: ReturnMode) -> Self { + Self { + mod_info, + return_mode, + } } // Create a signature for `sigidx` amended with a `vmctx` argument after the standard wasm @@ -321,6 +338,10 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ ) -> WasmResult { Ok(pos.ins().iconst(I32, -1)) } + + fn return_mode(&self) -> ReturnMode { + self.return_mode + } } impl<'data> ModuleEnvironment<'data> for DummyEnvironment { @@ -433,7 +454,7 @@ impl<'data> ModuleEnvironment<'data> for DummyEnvironment { fn define_function_body(&mut self, body_bytes: &'data [u8]) -> WasmResult<()> { let func = { - let mut func_environ = DummyFuncEnvironment::new(&self.info); + let mut func_environ = DummyFuncEnvironment::new(&self.info, self.return_mode); let func_index = FuncIndex::new(self.get_num_func_imports() + self.info.function_bodies.len()); let name = get_func_name(func_index); diff --git a/lib/wasm/src/environ/mod.rs b/lib/wasm/src/environ/mod.rs index 0ca34575f7..d44011fd8c 100644 --- a/lib/wasm/src/environ/mod.rs +++ b/lib/wasm/src/environ/mod.rs @@ -5,5 +5,5 @@ mod spec; pub use environ::dummy::DummyEnvironment; pub use environ::spec::{ - FuncEnvironment, GlobalVariable, ModuleEnvironment, WasmError, WasmResult, + FuncEnvironment, GlobalVariable, ModuleEnvironment, ReturnMode, WasmError, WasmResult, }; diff --git a/lib/wasm/src/environ/spec.rs b/lib/wasm/src/environ/spec.rs index 31640635b5..872ec28475 100644 --- a/lib/wasm/src/environ/spec.rs +++ b/lib/wasm/src/environ/spec.rs @@ -74,6 +74,15 @@ impl WasmError { /// A convenient alias for a `Result` that uses `WasmError` as the error type. pub type WasmResult = Result; +/// How to return from functions. +#[derive(Copy, Clone, PartialEq, Eq, Debug)] +pub enum ReturnMode { + /// Use normal return instructions as needed. + NormalReturns, + /// Use a single fallthrough return at the end of the function. + FallthroughReturn, +} + /// Environment affecting the translation of a single WebAssembly function. /// /// A `FuncEnvironment` trait object is required to translate a WebAssembly function to Cranelift @@ -216,6 +225,11 @@ pub trait FuncEnvironment { fn translate_loop_header(&mut self, _pos: FuncCursor) { // By default, don't emit anything. } + + /// Should the code be structured to use a single `fallthrough_return` instruction at the end + /// of the function body, rather than `return` instructions as needed? This is used by VMs + /// to append custom epilogues. + fn return_mode(&self) -> ReturnMode; } /// An object satisfying the `ModuleEnvironment` trait can be passed as argument to the diff --git a/lib/wasm/src/func_translator.rs b/lib/wasm/src/func_translator.rs index 0f946751db..0d99754c79 100644 --- a/lib/wasm/src/func_translator.rs +++ b/lib/wasm/src/func_translator.rs @@ -9,7 +9,7 @@ use cranelift_codegen::entity::EntityRef; use cranelift_codegen::ir::{self, Ebb, InstBuilder}; use cranelift_codegen::timing; use cranelift_frontend::{FunctionBuilder, FunctionBuilderContext, Variable}; -use environ::{FuncEnvironment, WasmError, WasmResult}; +use environ::{FuncEnvironment, ReturnMode, WasmError, WasmResult}; use state::TranslationState; use wasmparser::{self, BinaryReader}; @@ -209,7 +209,10 @@ fn parse_function_body( if state.reachable { debug_assert!(builder.is_pristine()); if !builder.is_unreachable() { - builder.ins().return_(&state.stack); + match environ.return_mode() { + ReturnMode::NormalReturns => builder.ins().return_(&state.stack), + ReturnMode::FallthroughReturn => builder.ins().fallthrough_return(&state.stack), + }; } } diff --git a/lib/wasm/src/lib.rs b/lib/wasm/src/lib.rs index 95667d870d..21b757f9c8 100644 --- a/lib/wasm/src/lib.rs +++ b/lib/wasm/src/lib.rs @@ -59,7 +59,8 @@ mod state; mod translation_utils; pub use environ::{ - DummyEnvironment, FuncEnvironment, GlobalVariable, ModuleEnvironment, WasmError, WasmResult, + DummyEnvironment, FuncEnvironment, GlobalVariable, ModuleEnvironment, ReturnMode, WasmError, + WasmResult, }; pub use func_translator::FuncTranslator; pub use module_translator::translate_module; diff --git a/lib/wasm/tests/wasm_testsuite.rs b/lib/wasm/tests/wasm_testsuite.rs index f7cc0e678e..be9c836463 100644 --- a/lib/wasm/tests/wasm_testsuite.rs +++ b/lib/wasm/tests/wasm_testsuite.rs @@ -6,9 +6,9 @@ extern crate wabt; use cranelift_codegen::isa; use cranelift_codegen::print_errors::pretty_verifier_error; -use cranelift_codegen::settings::{self, Configurable, Flags}; +use cranelift_codegen::settings::{self, Flags}; use cranelift_codegen::verifier; -use cranelift_wasm::{translate_module, DummyEnvironment}; +use cranelift_wasm::{translate_module, DummyEnvironment, ReturnMode}; use std::fs; use std::fs::File; use std::io; @@ -35,16 +35,18 @@ fn testsuite() { let flags = Flags::new(settings::builder()); for path in paths { let path = path.path(); - handle_module(&path, &flags); + handle_module(&path, &flags, ReturnMode::NormalReturns); } } #[test] -fn return_at_end() { - let mut flag_builder = settings::builder(); - flag_builder.enable("return_at_end").unwrap(); - let flags = Flags::new(flag_builder); - handle_module(Path::new("../../wasmtests/return_at_end.wat"), &flags); +fn use_fallthrough_return() { + let flags = Flags::new(settings::builder()); + handle_module( + Path::new("../../wasmtests/use_fallthrough_return.wat"), + &flags, + ReturnMode::FallthroughReturn, + ); } fn read_file(path: &Path) -> io::Result> { @@ -54,7 +56,7 @@ fn read_file(path: &Path) -> io::Result> { Ok(buf) } -fn handle_module(path: &Path, flags: &Flags) { +fn handle_module(path: &Path, flags: &Flags, return_mode: ReturnMode) { let data = match path.extension() { None => { panic!("the file extension is not wasm or wat"); @@ -73,7 +75,9 @@ fn handle_module(path: &Path, flags: &Flags) { None | Some(&_) => panic!("the file extension for {:?} is not wasm or wat", path), }, }; - let mut dummy_environ = DummyEnvironment::with_triple_flags(triple!("riscv64"), flags.clone()); + let mut dummy_environ = + DummyEnvironment::with_triple_flags(triple!("riscv64"), flags.clone(), return_mode); + translate_module(&data, &mut dummy_environ).unwrap(); let isa = isa::lookup(dummy_environ.info.triple)